Be more strict about "Location:" redirects
ClosedPublic
Actions

Authored by epriestley on Aug 18 2014, 8:14 PM.

Details

Reviewers

btrahan

Commits

Restricted Diffusion Commit
rPdf361470c1d9: Be more strict about "Location:" redirects

Summary

Via HackerOne. Chrome (at least) interprets backslashes like forward slashes, so a redirect to "/\evil.com" is the same as a redirect to "//evil.com".

Reject local URIs with backslashes (we never generate these).
Fully-qualify all "Location:" redirects.
Require external redirects to be marked explicitly.

Test Plan

Expanded existing test coverage.
Verified that neither Diffusion nor Phriction can generate URIs with backslashes (they are escaped in Diffusion, and removed by slugging in Phriction).
Logged in with Facebook (OAuth2 submits a form to the external site, and isn't affected) and Twitter (OAuth1 redirects, and is affected).
Went through some local redirects (login, save-an-object).
Verified file still work.

Diff Detail

Repository

rP Phabricator

Branch

redirect

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 2267
Build 2271: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley updated this revision to Diff 24779.Aug 18 2014, 8:14 PM

epriestley retitled this revision from to Be more strict about "Location:" redirects.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: btrahan.

Herald added a subscriber: epriestley. · View Herald TranscriptAug 18 2014, 8:14 PM

Harbormaster completed remote builds in B2267: Diff 24779.Aug 18 2014, 8:15 PM

It's possible that I missed an AphrontRedirectResponse somewhere which takes an external URI -- we have a lot of them, and I tried to review them all but there are seriously like a billion.

If I did, users will hit an exception about remote vs local redirects. The fix is to add ->setIsExternal(true) to the AphontRedirectResponse to mark it as external.

(The OAuth server uses a <form /> that users explicitly click nowadays, which is why doesn't get marked in this diff.)

btrahan accepted this revision.Aug 18 2014, 8:31 PM

btrahan edited edge metadata.

This revision is now accepted and ready to land.Aug 18 2014, 8:31 PM