hypothesis / h

Annotate with anyone, anywhere.
https://hypothes.is/
BSD 2-Clause "Simplified" License
2.96k stars 427 forks source link

Update API's forbidden view config to not always return 404s #8605

Open lyzadanger opened 6 years ago

lyzadanger commented 6 years ago

At present, h.views.api.exceptions defines a @forbidden_view_config (i.e. handler for 403 responses) for use by API endpoints. However, this view always changes request.response.status_code to 404—de facto changing the response to an HTTPNotFound.

In some cases, this is what we should do, so as not to leak information about whether a protected resource—which can be identified by part of the request path—exists or not. But in other cases, a 404 is misleading and kind of wrong. An example is POST /api/groups. That should return a 403 if auth fails, not a 404.

There are a couple of ways to approach "fixing" this; the obvious mechanism being a view predicate that can apply a different handler depending on the context (sometimes a 404, sometimes a 403). There are some patterns in the routes and traversal that may be useful "hooks" for determining when to do what.

Note: The default/main forbidden view handler—not just the API's—does this always-return-a-404 thing as well. This issue pertains to the API variant.

lyzadanger commented 6 years ago

Much work has been done to protect various API endpoints with permissions (authz) instead of such things as security.Authenticated and then some view-level logic. We're close. You can see the status as of this writing in this Google Doc (this spreadsheet is globally readable):

https://docs.google.com/spreadsheets/d/1TJWLSaf1wjjHzmZxi4OHcaaUYrIZdbxiQAtxU2hnUjc/edit?usp=sharing

There are a couple of API resources that are not tracked in this spreadsheet (yet):

  1. auth endpoints (in h.views.api.auth). These do not have paths within /api and thus are not covered by the API view exceptions. This module is large and complex and I'd like to leave it be for the moment. Changes to API exceptions will not affect this module's views.
  2. badge. This view has the inverse situation of the auth views. It is served under the /api path but its module lives outside of api. It uses a @json_view decorator instead of an @api_config decorator. badge doesn't have any authorization constraints, however, so changes to view exceptions for auth'z failures won't affect it.

(Note that both of the above modules should be rectified; it's just outside of the scope of the authorization-failure responses).

Right now, all of the views served under /api/* will intercept 403 errors and convert them to 404s. This is sometimes appropriate, but sometimes not. The pattern to raising the right response appears to be:

  1. If the matchdict is empty (i.e. there are no parameters in the path), raise a 403
  2. If the matchdict has entries (i.e. parameters within the path are used for traversal), raise a 404 so as not to leak information about the existence of off-limits resources

Following this model, authorization failure on the following endpoints would result in a 403:

While these would (continue to) result in a 404: