hypothesis / h

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

AttributeError: 'UserRoot' object has no attribute 'username' #5281

Closed seanh closed 5 years ago

seanh commented 6 years ago

https://sentry.io/hypothesis/h/issues/657366232/

AttributeError: 'UserRoot' object has no attribute 'username'
(16 additional frame(s) were not displayed)
...
  File "h/viewderivers.py", line 34, in wrapper_view
    resp = view(context, request)
  File "pyramid/viewderivers.py", line 442, in rendered_view
    result = view(context, request)
  File "newrelic/hooks/framework_pyramid.py", line 140, in _wrapper
    return wrapper(context, request)
  File "pyramid/viewderivers.py", line 137, in _class_view
    response = getattr(inst, attr)()
  File "h/views/activity.py", line 332, in search
    result['opts'] = {'search_username': self.user.username}

AttributeError: 'UserRoot' object has no attribute 'username'
hmstepanek commented 6 years ago

I've done some investigating here but I kinda hit a dead end

robertknight commented 5 years ago

I managed to reproduce this. It happens if the server receives a /users/. request. Note the trailing period. Normally if you enter https://hypothes.is/users/. in your browser the path gets normalized into /users/ before the server sees it. However you can reproduce this locally using curl's --path-as-is option to disable normalization:

curl --path-as-is https://hypothes.is/users/.
robertknight commented 5 years ago

A similar issue can be reproduced with:

 curl --path-as-is https://hypothes.is/groups/.

More generally, I suspect this problem will affect any scenario where:

  1. There is a /things/{thing_id}-style route
  2. There is no /things/ route
  3. The server receives a request for /things/. (note trailing period)

In this case the received request is matched against route (1) and the context root is instantiated, but it seems that __getitem__ is never called on the context root (which would raise a KeyError) and what gets passed to the view as the context object is an instance of the context root rather than the result of context_root[thing_id]. In the view code, this is almost certain to trigger an exception due to the context object not being of the expected type.

robertknight commented 5 years ago

I'm guessing the reason that it only happened with a specific browser in production (all reports came from a browser identifying as Opera 12.17, released in 2014, assuming that is the real version of the browser), is because that browser is not doing some normalization that modern browsers do. I couldn't reproduce that behaviour installing Opera 12.17 on a VM locally though under Windows 10.

robertknight commented 5 years ago

See also https://github.com/Pylons/pyramid/issues/3227

Frankly I think we could choose to simply ignore this issue given how few users it affects. That is, tell Sentry to ignore it until it affects an additional X users. All major browsers will canonicalize the URL in links or entered by the user so this will never happen. One remaining question is why the browser tried to navigate to that URL in the first place. Did the user follow an incorrectly generated link on the page? Did they enter this value manually somewhere?

seanh commented 5 years ago

Seems pretty obscure. I'm happy to tell Sentry to ignore this until it affects X users and close it. As long as the issue isn't causing a broken windows affect by showing up in Sentry

seanh commented 5 years ago

Done in Sentry