Page MenuHomePhabricator

Generate a 403 page with a nice dialog when a file token is invalid
ClosedPublic

Authored by epriestley on Aug 11 2014, 3:22 PM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Apr 24, 9:48 PM
Unknown Object (File)
Sun, Apr 21, 3:36 PM
Unknown Object (File)
Fri, Apr 19, 2:07 AM
Unknown Object (File)
Fri, Apr 19, 2:07 AM
Unknown Object (File)
Fri, Apr 19, 2:07 AM
Unknown Object (File)
Wed, Apr 17, 2:25 PM
Unknown Object (File)
Mon, Apr 15, 3:27 PM
Unknown Object (File)
Thu, Apr 11, 7:23 PM
Subscribers

Details

Summary

Ref T5685. Currently we just 403 on an invalid token, but we can be a little more helpful.

The issues here are:

  • If we do redirect you on this page and something goes wrong, you might get stuck in a redirect loop.
  • If we don't redirect you, copy/pasting the link to someone (or reloading the page) gives them a pretty confusing result, since the link doesn't work any more. Prior to this diff, they get a 403.

To mitigate this, do a little better than a bare 403: give them a link to auth and generate a new URI for the file.

If this is still confusing, the next best thing I can come up with is something like this:

  • Put some modulous of the timestamp in the URI.
  • If the current time is within 2 seconds of the generation time, show this dialog.
  • Otherwise, redirect.

That seems like it would be okay, but I worry that "2" has to be small (so links you copy/paste -> chat -> click still work) and a small value means that a small amount of clock skew breaks things. We could use the database clock, but ehhh.

Other ideas:

  • Put a hash of the remote IP in the URI, redirect if it doesn't match. Fails for companies behind a NAT gateway but should work in a lot of other cases.
  • Just redirect always, there's no reason it should ever loop and browsers don't really do anything bad when there's a loop (they'll show an error after too many redirects).

I'm leaning toward letting this stabilize in the wild for a bit, then trying "always redirect".

Test Plan

Screen_Shot_2014-08-11_at_8.09.28_AM.png (1×1 px, 128 KB)

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley retitled this revision from to Generate a 403 page with a nice dialog when a file token is invalid.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: 20after4, btrahan.
20after4 edited edge metadata.

I like the dialog, maybe we don't need to do anything more?

This revision is now accepted and ready to land.Aug 11 2014, 4:35 PM

Yeah, this might be good enough. I'm worried users are going to share links to files and might find this workflow confusing, though. We can see how it turns out.

epriestley updated this revision to Diff 24585.

Closed by commit rPa9f2c07345d0 (authored by @epriestley).