Skip to content

Commit a2a0f00

Browse files
committed
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. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley, Korvin Maniphest Tasks: T5937 Differential Revision: https://secure.phabricator.com/D10318
1 parent 1ffa16a commit a2a0f00

File tree

3 files changed

+15
-5
lines changed

3 files changed

+15
-5
lines changed

src/aphront/configuration/AphrontApplicationConfiguration.php

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@ abstract public function getApplicationName();
1414
abstract public function getURIMap();
1515
abstract public function buildRequest();
1616
abstract public function build404Controller();
17-
abstract public function buildRedirectController($uri);
17+
abstract public function buildRedirectController($uri, $external);
1818

1919
final public function setRequest(AphrontRequest $request) {
2020
$this->request = $request;
@@ -96,7 +96,10 @@ final public function buildController() {
9696
$https_uri = $request->getRequestURI();
9797
$https_uri->setDomain($request->getHost());
9898
$https_uri->setProtocol('https');
99-
return $this->buildRedirectController($https_uri);
99+
100+
// In this scenario, we'll be redirecting to HTTPS using an absolute
101+
// URI, so we need to permit an external redirect.
102+
return $this->buildRedirectController($https_uri, true);
100103
}
101104
}
102105

@@ -188,7 +191,9 @@ final public function buildController() {
188191

189192
if ($controller && !$request->isHTTPPost()) {
190193
$slash_uri = $request->getRequestURI()->setPath($path.'/');
191-
return $this->buildRedirectController($slash_uri);
194+
195+
$external = strlen($request->getRequestURI()->getDomain());
196+
return $this->buildRedirectController($slash_uri, $external);
192197
}
193198
}
194199
return $this->build404Controller();

src/aphront/configuration/AphrontDefaultApplicationConfiguration.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,11 +300,12 @@ public function build404Controller() {
300300
return array(new Phabricator404Controller($this->getRequest()), array());
301301
}
302302

303-
public function buildRedirectController($uri) {
303+
public function buildRedirectController($uri, $external) {
304304
return array(
305305
new PhabricatorRedirectController($this->getRequest()),
306306
array(
307307
'uri' => $uri,
308+
'external' => $external,
308309
));
309310
}
310311

src/applications/base/controller/PhabricatorRedirectController.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
final class PhabricatorRedirectController extends PhabricatorController {
44

55
private $uri;
6+
private $allowExternal;
67

78
public function shouldRequireLogin() {
89
return false;
@@ -14,10 +15,13 @@ public function shouldRequireEnabledUser() {
1415

1516
public function willProcessRequest(array $data) {
1617
$this->uri = $data['uri'];
18+
$this->allowExternal = idx($data, 'external', false);
1719
}
1820

1921
public function processRequest() {
20-
return id(new AphrontRedirectResponse())->setURI($this->uri);
22+
return id(new AphrontRedirectResponse())
23+
->setURI($this->uri)
24+
->setIsExternal($this->allowExternal);
2125
}
2226

2327
}

0 commit comments

Comments
 (0)