search

PuzzleServer / mainpuzzleserver

The main repo for the Puzzle Hunt and Puzzleday servers.
MIT License
9 stars 32 forks source link

Site crashes instead of giving 404 when mistyping eventid or role part of the url #997

Closed jbodner09 closed 1 month ago

jbodner09 commented 1 month ago

If the event and/or role parts of the url are mistyped so that they don't correspond to a valid event or role (https://puzzlehunt.azurewebsites.net/event/role/MessingUpAnythingAfterThisJustGivesA404LikeItShould), the site will crash instead of giving a 404 for most pages.

Some of this was introduced with the new shared-resource-supplied homepages in #946, which now require a valid Event object in order to finish displaying certain content on the site (GetFileStoragePrefix in EventSpecificPageModel will crash for some pages, but the new EventSpecific home/rules/faq pages will also crash). However, some of these crashes existed before then (puzzles page, team pages, and anything with an incorrect role, since an invalid role is hard-coded to throw an exception).

At first, we thought we could fix some of these by just validating the event object and role in #993, but it turns out that there are valid cases where the event object is invalid, which was causing new crashes in situations where there shouldn't be any, so that was rolled back in #996. It's also likely more complicated than that, since there is clearly not a just a single failure point depending on which page you're trying to view.

Ideally, any url on puzzlehunt.azurewebsite.net that doesn't explicitly work should give a 404 instead of crashing the site.

jbodner09 commented 1 month ago

999 seems to take care of the event being invalid, but we still need to do something about the role. Just comparing against the object being valid the same way 999 does for the Event object doesn't work. Validating the range with the annotation did work, but it required using the ModelState validation which is what was breaking everything.

I suspect part of the problem is that proper permission checking isn't done (see: TODO in the RoleBinder). At the moment, since Admin has a value of 0, any invalid strings will cause the role to default to Admin and the page to continue to load. Many pages do individual auth checking on load that helps block them, but ideally we could stop somebody from using a role they don't have BEFORE any page even loads. That would then cover the rest of the cases where there's no individual auth check.