Page MenuHomePhabricator
Files F14009149

D10291.id24779.diff
No OneTemporary
Actions

D10291.id24779.diff
View Options

diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -76,6 +76,7 @@
'AphrontProgressBarView' => 'view/widget/bars/AphrontProgressBarView.php',
'AphrontProxyResponse' => 'aphront/response/AphrontProxyResponse.php',
'AphrontRedirectResponse' => 'aphront/response/AphrontRedirectResponse.php',
+ 'AphrontRedirectResponseTestCase' => 'aphront/response/__tests__/AphrontRedirectResponseTestCase.php',
'AphrontReloadResponse' => 'aphront/response/AphrontReloadResponse.php',
'AphrontRequest' => 'aphront/AphrontRequest.php',
'AphrontRequestFailureView' => 'view/page/AphrontRequestFailureView.php',
@@ -2835,6 +2836,7 @@
'AphrontProgressBarView' => 'AphrontBarView',
'AphrontProxyResponse' => 'AphrontResponse',
'AphrontRedirectResponse' => 'AphrontResponse',
+ 'AphrontRedirectResponseTestCase' => 'PhabricatorTestCase',
'AphrontReloadResponse' => 'AphrontRedirectResponse',
'AphrontRequestFailureView' => 'AphrontView',
'AphrontRequestTestCase' => 'PhabricatorTestCase',
diff --git a/src/aphront/response/AphrontRedirectResponse.php b/src/aphront/response/AphrontRedirectResponse.php
--- a/src/aphront/response/AphrontRedirectResponse.php
+++ b/src/aphront/response/AphrontRedirectResponse.php
@@ -7,6 +7,12 @@
private $uri;
private $stackWhenCreated;
+ private $isExternal;
+
+ public function setIsExternal($external) {
+ $this->isExternal = $external;
+ return $this;
+ }
public function __construct() {
if ($this->shouldStopForDebugging()) {
@@ -21,7 +27,10 @@
}
public function getURI() {
- return (string)$this->uri;
+ // NOTE: When we convert a RedirectResponse into an AjaxResponse, we pull
+ // the URI through this method. Make sure it passes checks before we
+ // hand it over to callers.
+ return self::getURIForRedirect($this->uri, $this->isExternal);
}
public function shouldStopForDebugging() {
@@ -31,7 +40,8 @@
public function getHeaders() {
$headers = array();
if (!$this->shouldStopForDebugging()) {
- $headers[] = array('Location', $this->uri);
+ $uri = self::getURIForRedirect($this->uri, $this->isExternal);
+ $headers[] = array('Location', $uri);
}
$headers = array_merge(parent::getHeaders(), $headers);
return $headers;
@@ -85,4 +95,72 @@
return '';
}
+
+ /**
+ * Format a URI for use in a "Location:" header.
+ *
+ * Verifies that a URI redirects to the expected type of resource (local or
+ * remote) and formats it for use in a "Location:" header.
+ *
+ * The HTTP spec says "Location:" headers must use absolute URIs. Although
+ * browsers work with relative URIs, we return absolute URIs to avoid
+ * ambiguity. For example, Chrome interprets "Location: /\evil.com" to mean
+ * "perform a protocol-relative redirect to evil.com".
+ *
+ * @param string URI to redirect to.
+ * @param bool True if this URI identifies a remote resource.
+ * @return string URI for use in a "Location:" header.
+ */
+ public static function getURIForRedirect($uri, $is_external) {
+ $uri_object = new PhutilURI($uri);
+ if ($is_external) {
+ // If this is a remote resource it must have a domain set. This
+ // would also be caught below, but testing for it explicitly first allows
+ // us to raise a better error message.
+ if (!strlen($uri_object->getDomain())) {
+ throw new Exception(
+ pht(
+ 'Refusing to redirect to external URI "%s". This URI '.
+ 'is not fully qualified, and is missing a domain name. To '.
+ 'redirect to a local resource, remove the external flag.',
+ (string)$uri));
+ }
+
+ // Check that it's a valid remote resource.
+ if (!PhabricatorEnv::isValidRemoteWebResource($uri)) {
+ throw new Exception(
+ pht(
+ 'Refusing to redirect to external URI "%s". This URI '.
+ 'is not a valid remote web resource.',
+ (string)$uri));
+ }
+ } else {
+ // If this is a local resource, it must not have a domain set. This allows
+ // us to raise a better error message than the check below can.
+ if (strlen($uri_object->getDomain())) {
+ throw new Exception(
+ pht(
+ 'Refusing to redirect to local resource "%s". The URI has a '.
+ 'domain, but the redirect is not marked external. Mark '.
+ 'redirects as external to allow redirection off the local '.
+ 'domain.',
+ (string)$uri));
+ }
+
+ // If this is a local resource, it must be a valid local resource.
+ if (!PhabricatorEnv::isValidLocalWebResource($uri)) {
+ throw new Exception(
+ pht(
+ 'Refusing to redirect to local resource "%s". This URI is not '.
+ 'formatted in a recognizable way.',
+ (string)$uri));
+ }
+
+ // Fully qualify the result URI.
+ $uri = PhabricatorEnv::getURI((string)$uri);
+ }
+
+ return (string)$uri;
+ }
+
}
diff --git a/src/aphront/response/__tests__/AphrontRedirectResponseTestCase.php b/src/aphront/response/__tests__/AphrontRedirectResponseTestCase.php
new file mode 100644
--- /dev/null
+++ b/src/aphront/response/__tests__/AphrontRedirectResponseTestCase.php
@@ -0,0 +1,63 @@
+<?php
+
+final class AphrontRedirectResponseTestCase extends PhabricatorTestCase {
+
+ public function testLocalRedirectURIs() {
+ // Format a bunch of URIs for local and remote redirection, making sure
+ // we get the expected results.
+
+ $uris = array(
+ '/a' => array(
+ 'http://phabricator.example.com/a',
+ false,
+ ),
+ 'a' => array(
+ false,
+ false,
+ ),
+ '/\\evil.com' => array(
+ false,
+ false,
+ ),
+ 'http://www.evil.com/' => array(
+ false,
+ 'http://www.evil.com/',
+ ),
+ '//evil.com' => array(
+ false,
+ false,
+ ),
+ '//' => array(
+ false,
+ false,
+ ),
+ '' => array(
+ false,
+ false,
+ ),
+ );
+
+ foreach ($uris as $input => $cases) {
+ foreach (array(false, true) as $idx => $is_external) {
+ $expect = $cases[$idx];
+
+ $caught = null;
+ try {
+ $result = AphrontRedirectResponse::getURIForRedirect(
+ $input,
+ $is_external);
+ } catch (Exception $ex) {
+ $caught = $ex;
+ }
+
+ if ($expect === false) {
+ $this->assertTrue(($caught instanceof Exception), $input);
+ } else {
+ $this->assertEqual(null, $caught, $input);
+ $this->assertEqual($expect, $result, $input);
+ }
+ }
+ }
+ }
+
+}
diff --git a/src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php b/src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php
--- a/src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php
+++ b/src/applications/auth/provider/PhabricatorOAuth1AuthProvider.php
@@ -62,7 +62,9 @@
$client_code,
$adapter->getTokenSecret());
- $response = id(new AphrontRedirectResponse())->setURI($uri);
+ $response = id(new AphrontRedirectResponse())
+ ->setIsExternal(true)
+ ->setURI($uri);
return array($account, $response);
}
diff --git a/src/applications/files/controller/PhabricatorFileDataController.php b/src/applications/files/controller/PhabricatorFileDataController.php
--- a/src/applications/files/controller/PhabricatorFileDataController.php
+++ b/src/applications/files/controller/PhabricatorFileDataController.php
@@ -74,6 +74,7 @@
// if the user can see the file, generate a token;
// redirect to the alt domain with the token;
return id(new AphrontRedirectResponse())
+ ->setIsExternal(true)
->setURI($file->getCDNURIWithToken());
} else {
@@ -128,7 +129,9 @@
// file cannot be served via cdn, and no token given
// redirect to the main domain to aquire a token
+ // This is marked as an "external" URI because it is fully qualified.
return id(new AphrontRedirectResponse())
+ ->setIsExternal(true)
->setURI($acquire_token_uri);
}
}
@@ -171,7 +174,10 @@
// authenticate users on the file domain. This should blunt any
// attacks based on iframes, script tags, applet tags, etc., at least.
// Send the user to the "info" page if they're using some other method.
+
+ // This is marked as "external" because it is fully qualified.
return id(new AphrontRedirectResponse())
+ ->setIsExternal(true)
->setURI(PhabricatorEnv::getProductionURI($file->getBestURI()));
}
$response->setMimeType($file->getMimeType());
diff --git a/src/applications/phortune/provider/PhortunePaypalPaymentProvider.php b/src/applications/phortune/provider/PhortunePaypalPaymentProvider.php
--- a/src/applications/phortune/provider/PhortunePaypalPaymentProvider.php
+++ b/src/applications/phortune/provider/PhortunePaypalPaymentProvider.php
@@ -116,7 +116,9 @@
'token' => $result['TOKEN'],
));
- return id(new AphrontRedirectResponse())->setURI($uri);
+ return id(new AphrontRedirectResponse())
+ ->setIsExternal(true)
+ ->setURI($uri);
case 'charge':
var_dump($_REQUEST);
break;
diff --git a/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php b/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php
--- a/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php
+++ b/src/applications/phortune/provider/PhortuneWePayPaymentProvider.php
@@ -149,7 +149,9 @@
// user might not end up back here. Really this needs a bunch of junk.
$uri = new PhutilURI($result->checkout_uri);
- return id(new AphrontRedirectResponse())->setURI($uri);
+ return id(new AphrontRedirectResponse())
+ ->setIsExternal(true)
+ ->setURI($uri);
case 'charge':
$checkout_id = $request->getInt('checkout_id');
$params = array(
@@ -195,7 +197,9 @@
unset($unguarded);
- return id(new AphrontRedirectResponse())->setURI($cart_uri);
+ return id(new AphrontRedirectResponse())
+ ->setIsExternal(true)
+ ->setURI($cart_uri);
case 'cancel':
var_dump($_REQUEST);
break;
diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php
--- a/src/infrastructure/env/PhabricatorEnv.php
+++ b/src/infrastructure/env/PhabricatorEnv.php
@@ -462,6 +462,21 @@
return false;
}
+ // Chrome (at a minimum) interprets backslashes in Location headers and the
+ // URL bar as forward slashes. This is probably intended to reduce user
+ // error caused by confusion over which key is "forward slash" vs "back
+ // slash".
+ //
+ // However, it means a URI like "/\evil.com" is interpreted like
+ // "//evil.com", which is a protocol relative remote URI.
+ //
+ // Since we currently never generate URIs with backslashes in them, reject
+ // these unconditionally rather than trying to figure out how browsers will
+ // interpret them.
+ if (preg_match('/\\\\/', $uri)) {
+ return false;
+ }
+
// Valid URIs must begin with '/', followed by the end of the string or some
// other non-'/' character. This rejects protocol-relative URIs like
// "//evil.com/evil_stuff/".
diff --git a/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php
--- a/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php
+++ b/src/infrastructure/env/__tests__/PhabricatorEnvTestCase.php
@@ -13,6 +13,7 @@
'javascript:lol' => false,
'' => false,
null => false,
+ '/\\evil.com' => false,
);
foreach ($map as $uri => $expect) {
diff --git a/src/infrastructure/testing/PhabricatorTestCase.php b/src/infrastructure/testing/PhabricatorTestCase.php
--- a/src/infrastructure/testing/PhabricatorTestCase.php
+++ b/src/infrastructure/testing/PhabricatorTestCase.php
@@ -118,6 +118,10 @@
// TODO: Remove this when we remove "releeph.installed".
$this->env->overrideEnvConfig('releeph.installed', true);
+
+ $this->env->overrideEnvConfig(
+ 'phabricator.base-uri',
+ 'http://phabricator.example.com');
}
protected function didRunTests() {

File Metadata

Mime Type
text/plain
Expires
Thu, Oct 31, 11:00 AM (2 w, 5 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6719345
Default Alt Text
D10291.id24779.diff (12 KB)

Event Timeline

Phabricator · Built by Phacility