compserv / hkn-rails

The website for HKN Mu Chapter
https://hkn.eecs.berkeley.edu
Other
5 stars 10 forks source link

Fixing url parse errors #173

Open jameslzhu opened 6 years ago

jameslzhu commented 6 years ago

Bug reproduction: navigate to any url of the form:

These are available on Rollbar as error 68, 93, 95, 99, 110.

These all stem from poor url parsing, especially assumed Integer conversion successes (errors are uncaught).

Ex: error 68.

Traceback: models/exam.rb.

Root cause: department (decoded from url) assumed to be found (no nil checks), causing dept.name to throw 'undefined method 'name''.

jameslzhu commented 6 years ago

It appears that, for many invalid routes below the root, the 404 page isn't used; instead handlers frequently assume that the url is valid, and throw some sort of error when this isn't the case.

I don't think it's feasible to handle 404 errors at the top-level router (as response handling is delegated), but perhaps for each subpage's routing it may be.

jameslzhu commented 5 years ago

Update: https://rollbar.com/compserv/hkn-rails/items/93/?person_page=0&#ip-addresses This contained the following error traceback:

ArgumentError: invalid value for Integer(): "/../../../../../../../windows/win.ini............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................\u0000."

File /home/h/hk/hkn/hkn-rails/migrate/releases/20190107053052/app/controllers/events_controller.rb, line 62 in index

The root of this is bad url parsing: assuming a cast to Integer will work, and not handling any errors.