Fix `security.require-https` by marking redirect as external
ClosedPublic
Actions

Authored by hach-que on Aug 21 2014, 9:58 AM.

Details

Reviewers

epriestley

Group Reviewers

Blessed Reviewers

Maniphest Tasks

T5937: Redirect to HTTPS from HTTP fails because "not marked external"

Commits

Restricted Diffusion Commit
rPa2a0f002f069: Fix `security.require-https` by marking redirect as external

Summary

Resolves T5937. HTTPS redirects caused by security.require-https use a full scheme, domain and port in the URI. Consequently, this causes invocation of the new external redirect logic and prevents redirection from occurring properly when accessing the HTTP version of Phabricator that has security.require-https turned on.

I've also fixed the automatic slash redirection logic to add the external flag where appropriate.

Test Plan

Configured SSL on my local machine and turned on security.require-https. Observed the "Refusing to redirect" exception on master, while the redirect completed successfully with this patch.

Diff Detail

Repository

rP Phabricator

Branch

external-https

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 2305
Build 2309: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

hach-que updated this revision to Diff 24839.Aug 21 2014, 9:58 AM

hach-que retitled this revision from to Fix `security.require-https` by marking redirect as external.

hach-que updated this object.

hach-que edited the test plan for this revision. (Show Details)

hach-que added a reviewer: epriestley.

hach-que added a task: T5937: Redirect to HTTPS from HTTP fails because "not marked external".

Herald added a reviewer: Blessed Reviewers. · View Herald TranscriptAug 21 2014, 9:58 AM

Herald added subscribers: Korvin, epriestley. · View Herald Transcript

Harbormaster completed remote builds in B2303: Diff 24839.Aug 21 2014, 9:59 AM

The automatic slash redirect requires external to be turned off for local redirects

Harbormaster completed remote builds in B2305: Diff 24841.Aug 21 2014, 10:48 AM

hach-que added inline comments.Aug 21 2014, 10:49 AM

src/aphront/configuration/AphrontApplicationConfiguration.php
195–196	I'm not sure whether this redirect is always local, but it's at least not always absolute. In either case, the request URI domain currently hits Phabricator successfully, so it should be safe to redirect regardless of whether this is a relative or absolute URI (because the absolute URI can only ever point at Phabricator).