Page MenuHomePhabricator
Files F13178892

D8047.diff
No OneTemporary
Actions

D8047.diff
View Options

Index: src/applications/auth/constants/PhabricatorCookies.php
===================================================================
--- src/applications/auth/constants/PhabricatorCookies.php
+++ src/applications/auth/constants/PhabricatorCookies.php
@@ -3,6 +3,8 @@
/**
* Consolidates Phabricator application cookies, including registration
* and session management.
+ *
+ * @task next Next URI Cookie
*/
final class PhabricatorCookies extends Phobject {
@@ -45,4 +47,84 @@
*/
const COOKIE_NEXTURI = 'next_uri';
+
+/* -( Next URI Cookie )---------------------------------------------------- */
+
+
+ /**
+ * Set the Next URI cookie. We only write the cookie if it wasn't recently
+ * written, to avoid writing over a real URI with a bunch of "humans.txt"
+ * stuff. See T3793 for discussion.
+ *
+ * @param AphrontRequest Request to write to.
+ * @param string URI to write.
+ * @param bool Write this cookie even if we have a fresh
+ * cookie already.
+ * @return void
+ *
+ * @task next
+ */
+ public static function setNextURICookie(
+ AphrontRequest $request,
+ $next_uri,
+ $force = false) {
+
+ if (!$force) {
+ $cookie_value = $request->getCookie(self::COOKIE_NEXTURI);
+ list($set_at, $current_uri) = self::parseNextURICookie($cookie_value);
+
+ // If the cookie was set within the last 2 minutes, don't overwrite it.
+ // Primarily, this prevents browser requests for resources which do not
+ // exist (like "humans.txt" and various icons) from overwriting a normal
+ // URI like "/feed/".
+ if ($set_at > (time() - 120)) {
+ return;
+ }
+ }
+
+ $new_value = time().','.$next_uri;
+ $request->setCookie(self::COOKIE_NEXTURI, $new_value);
+ }
+
+
+ /**
+ * Read the URI out of the Next URI cookie.
+ *
+ * @param AphrontRequest Request to examine.
+ * @return string|null Next URI cookie's URI value.
+ *
+ * @task next
+ */
+ public static function getNextURICookie(AphrontRequest $request) {
+ $cookie_value = $request->getCookie(self::COOKIE_NEXTURI);
+ list($set_at, $next_uri) = self::parseNExtURICookie($cookie_value);
+
+ return $next_uri;
+ }
+
+
+ /**
+ * Parse a Next URI cookie into its components.
+ *
+ * @param string Raw cookie value.
+ * @return list<string> List of timestamp and URI.
+ *
+ * @task next
+ */
+ private static function parseNextURICookie($cookie) {
+ // Old cookies look like: /uri
+ // New cookies look like: timestamp,/uri
+
+ if (!strlen($cookie)) {
+ return null;
+ }
+
+ if (strpos($cookie, ',') !== false) {
+ list($timestamp, $uri) = explode(',', $cookie, 2);
+ return array((int)$timestamp, $uri);
+ }
+
+ return array(0, $cookie);
+ }
+
}
Index: src/applications/auth/controller/PhabricatorAuthStartController.php
===================================================================
--- src/applications/auth/controller/PhabricatorAuthStartController.php
+++ src/applications/auth/controller/PhabricatorAuthStartController.php
@@ -86,9 +86,8 @@
}
if (!$request->isFormPost()) {
- $request->setCookie(
- PhabricatorCookies::COOKIE_NEXTURI,
- $next_uri);
+ PhabricatorCookies::setNextURICookie($request, $next_uri);
+
$request->setCookie(
PhabricatorCookies::COOKIE_CLIENTID,
Filesystem::readRandomCharacters(16));
Index: src/applications/auth/controller/PhabricatorAuthValidateController.php
===================================================================
--- src/applications/auth/controller/PhabricatorAuthValidateController.php
+++ src/applications/auth/controller/PhabricatorAuthValidateController.php
@@ -54,7 +54,7 @@
return $this->renderErrors($failures);
}
- $next = $request->getCookie(PhabricatorCookies::COOKIE_NEXTURI);
+ $next = PhabricatorCookies::getNextURICookie($request);
$request->clearCookie(PhabricatorCookies::COOKIE_NEXTURI);
if (!PhabricatorEnv::isValidLocalWebResource($next)) {
Index: src/applications/auth/controller/PhabricatorEmailTokenController.php
===================================================================
--- src/applications/auth/controller/PhabricatorEmailTokenController.php
+++ src/applications/auth/controller/PhabricatorEmailTokenController.php
@@ -86,7 +86,7 @@
));
}
- $request->setCookie(PhabricatorCookies::COOKIE_NEXTURI, $next);
+ PhabricatorCookies::setNextURICookie($request, $next, $force = true);
return $this->loginUser($target_user);
}
Index: src/applications/base/controller/Phabricator404Controller.php
===================================================================
--- src/applications/base/controller/Phabricator404Controller.php
+++ src/applications/base/controller/Phabricator404Controller.php
@@ -2,35 +2,6 @@
final class Phabricator404Controller extends PhabricatorController {
- public function shouldRequireLogin() {
-
- // NOTE: See T2102 for discussion. When a logged-out user requests a page,
- // we give them a login form and set a `next_uri` cookie so we send them
- // back to the page they requested after they login. However, some browsers
- // or extensions request resources which may not exist (like
- // "apple-touch-icon.png" and "humans.txt") and these requests may overwrite
- // the stored "next_uri" after the login page loads. Our options for dealing
- // with this are all bad:
- //
- // 1. We can't put the URI in the form because some login methods (OAuth2)
- // issue redirects to third-party sites. After T1536 we might be able
- // to.
- // 2. We could set the cookie only if it doesn't exist, but then a user who
- // declines to login will end up in the wrong place if they later do
- // login.
- // 3. We can blacklist all the resources browsers request, but this is a
- // mess.
- // 4. We can just allow users to access the 404 page without login, so
- // requesting bad URIs doesn't set the cookie.
- //
- // This implements (4). The main downside is that an attacker can now detect
- // if a URI is routable (i.e., some application is installed) by testing for
- // 404 vs login. If possible, we should implement T1536 in such a way that
- // we can pass the next URI through the login process.
-
- return false;
- }
-
public function processRequest() {
return new Aphront404Response();
}

File Metadata

Mime Type
text/plain
Expires
Thu, May 9, 8:46 PM (3 w, 6 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6277170
Default Alt Text
D8047.diff (6 KB)

Event Timeline

Phabricator · Built by Phacility