Page MenuHomePhabricator

After writing "next_uri", don't write it again for a while
ClosedPublic

Authored by epriestley on Jan 23 2014, 8:19 PM.
Tags
None
Referenced Files
F13178892: D8047.diff
Wed, May 8, 8:46 PM
Unknown Object (File)
Sat, May 4, 6:26 PM
Unknown Object (File)
Tue, Apr 30, 11:17 PM
Unknown Object (File)
Mon, Apr 22, 10:34 PM
Unknown Object (File)
Fri, Apr 12, 4:59 PM
Unknown Object (File)
Apr 11 2024, 7:16 AM
Unknown Object (File)
Apr 11 2024, 4:30 AM
Unknown Object (File)
Apr 11 2024, 4:30 AM
Subscribers

Details

Reviewers
btrahan
Maniphest Tasks
Restricted Maniphest Task
Commits
Restricted Diffusion Commit
rP5b1d9c935a90: After writing "next_uri", don't write it again for a while
Summary

Fixes T3793. There's a lot of history here, see D4012, T2102. Basically, the problem is that things used to work like this:

  • User is logged out and accesses /xyz/. After they login, we'd like to send them back to /xyz/, so we set a next_uri cookie.
  • User's browser has a bunch of extensions and now makes a ton of requests for stuff that doesn't exist, like humans.txt and apple-touch-icon.png. We can't distinguish between these requests and normal requests in a general way, so we write next_uri cookies, overwriting the user's intent (/xyz/).

To fix this, we made the 404 page not set next_uri, in D4012. So if the browser requests humans.txt, we 404 with no cookie, and the /xyz/ cookie is preserved. However, this is bad because an attacker can determine if objects exist and applications are installed, by visiting, e.g., /T123 and seeing if they get a 404 page (resource really does not exist) or a login page (resource exists). We'd rather not leak this information.

The comment in the body text describes this in more detail.

This diff sort of tries to do the right thing most of the time: we write the cookie only if we haven't written it in the last 2 minutes. Generally, this should mean that the original request to /xyz/ writes it, all the humans.txt requests don't write it, and things work like users expect. This may occasionally do the wrong thing, but it should be very rare, and we stop leaking information about applications and objects.

Test Plan

Logged out, clicked around / logged in, used Charles to verify that cookies were set in the expected way.

Diff Detail

Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley updated this revision to Unknown Object (????).Jan 23 2014, 8:19 PM
  • Minro comment tweak.
epriestley updated this revision to Unknown Object (????).Jan 23 2014, 8:22 PM
  • Another comment.