2014-01-23 23:01:35 +01:00
|
|
|
<?php
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Consolidates Phabricator application cookies, including registration
|
|
|
|
* and session management.
|
After writing "next_uri", don't write it again for a while
Summary:
Fixes T3793. There's a lot of history here, see D4012, T2102. Basically, the problem is that things used to work like this:
- User is logged out and accesses `/xyz/`. After they login, we'd like to send them back to `/xyz/`, so we set a `next_uri` cookie.
- User's browser has a bunch of extensions and now makes a ton of requests for stuff that doesn't exist, like `humans.txt` and `apple-touch-icon.png`. We can't distinguish between these requests and normal requests in a general way, so we write `next_uri` cookies, overwriting the user's intent (`/xyz/`).
To fix this, we made the 404 page not set `next_uri`, in D4012. So if the browser requests `humans.txt`, we 404 with no cookie, and the `/xyz/` cookie is preserved. However, this is bad because an attacker can determine if objects exist and applications are installed, by visiting, e.g., `/T123` and seeing if they get a 404 page (resource really does not exist) or a login page (resource exists). We'd rather not leak this information.
The comment in the body text describes this in more detail.
This diff sort of tries to do the right thing most of the time: we write the cookie only if we haven't written it in the last 2 minutes. Generally, this should mean that the original request to `/xyz/` writes it, all the `humans.txt` requests don't write it, and things work like users expect. This may occasionally do the wrong thing, but it should be very rare, and we stop leaking information about applications and objects.
Test Plan: Logged out, clicked around / logged in, used Charles to verify that cookies were set in the expected way.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3793
Differential Revision: https://secure.phabricator.com/D8047
2014-01-23 23:16:08 +01:00
|
|
|
*
|
2014-03-14 22:33:31 +01:00
|
|
|
* @task clientid Client ID Cookie
|
|
|
|
* @task next Next URI Cookie
|
2014-01-23 23:01:35 +01:00
|
|
|
*/
|
|
|
|
final class PhabricatorCookies extends Phobject {
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Stores the login username for password authentication. This is just a
|
|
|
|
* display value for convenience, used to prefill the login form. It is not
|
|
|
|
* authoritative.
|
|
|
|
*/
|
|
|
|
const COOKIE_USERNAME = 'phusr';
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Stores the user's current session ID. This is authoritative and establishes
|
|
|
|
* the user's identity.
|
|
|
|
*/
|
|
|
|
const COOKIE_SESSION = 'phsid';
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Stores a secret used during new account registration to prevent an attacker
|
|
|
|
* from tricking a victim into registering an account which is linked to
|
|
|
|
* credentials the attacker controls.
|
|
|
|
*/
|
|
|
|
const COOKIE_REGISTRATION = 'phreg';
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Stores a secret used during OAuth2 handshakes to prevent various attacks
|
|
|
|
* where an attacker hands a victim a URI corresponding to the middle of an
|
|
|
|
* OAuth2 workflow and we might otherwise do something sketchy. Particularly,
|
|
|
|
* this corresponds to the OAuth2 "code".
|
|
|
|
*/
|
|
|
|
const COOKIE_CLIENTID = 'phcid';
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Stores the URI to redirect the user to after login. This allows users to
|
|
|
|
* visit a path like `/feed/`, be prompted to login, and then be redirected
|
|
|
|
* back to `/feed/` after the workflow completes.
|
|
|
|
*/
|
|
|
|
const COOKIE_NEXTURI = 'next_uri';
|
|
|
|
|
After writing "next_uri", don't write it again for a while
Summary:
Fixes T3793. There's a lot of history here, see D4012, T2102. Basically, the problem is that things used to work like this:
- User is logged out and accesses `/xyz/`. After they login, we'd like to send them back to `/xyz/`, so we set a `next_uri` cookie.
- User's browser has a bunch of extensions and now makes a ton of requests for stuff that doesn't exist, like `humans.txt` and `apple-touch-icon.png`. We can't distinguish between these requests and normal requests in a general way, so we write `next_uri` cookies, overwriting the user's intent (`/xyz/`).
To fix this, we made the 404 page not set `next_uri`, in D4012. So if the browser requests `humans.txt`, we 404 with no cookie, and the `/xyz/` cookie is preserved. However, this is bad because an attacker can determine if objects exist and applications are installed, by visiting, e.g., `/T123` and seeing if they get a 404 page (resource really does not exist) or a login page (resource exists). We'd rather not leak this information.
The comment in the body text describes this in more detail.
This diff sort of tries to do the right thing most of the time: we write the cookie only if we haven't written it in the last 2 minutes. Generally, this should mean that the original request to `/xyz/` writes it, all the `humans.txt` requests don't write it, and things work like users expect. This may occasionally do the wrong thing, but it should be very rare, and we stop leaking information about applications and objects.
Test Plan: Logged out, clicked around / logged in, used Charles to verify that cookies were set in the expected way.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3793
Differential Revision: https://secure.phabricator.com/D8047
2014-01-23 23:16:08 +01:00
|
|
|
|
2014-03-14 22:33:31 +01:00
|
|
|
/* -( Client ID Cookie )--------------------------------------------------- */
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* Set the client ID cookie. This is a random cookie used like a CSRF value
|
|
|
|
* during authentication workflows.
|
|
|
|
*
|
|
|
|
* @param AphrontRequest Request to modify.
|
|
|
|
* @return void
|
|
|
|
* @task clientid
|
|
|
|
*/
|
|
|
|
public static function setClientIDCookie(AphrontRequest $request) {
|
|
|
|
|
|
|
|
// NOTE: See T3471 for some discussion. Some browsers and browser extensions
|
|
|
|
// can make duplicate requests, so we overwrite this cookie only if it is
|
|
|
|
// not present in the request. The cookie lifetime is limited by making it
|
|
|
|
// temporary and clearing it when users log out.
|
|
|
|
|
|
|
|
$value = $request->getCookie(self::COOKIE_CLIENTID);
|
|
|
|
if (!strlen($value)) {
|
|
|
|
$request->setTemporaryCookie(
|
|
|
|
self::COOKIE_CLIENTID,
|
|
|
|
Filesystem::readRandomCharacters(16));
|
|
|
|
}
|
|
|
|
}
|
|
|
|
|
|
|
|
|
After writing "next_uri", don't write it again for a while
Summary:
Fixes T3793. There's a lot of history here, see D4012, T2102. Basically, the problem is that things used to work like this:
- User is logged out and accesses `/xyz/`. After they login, we'd like to send them back to `/xyz/`, so we set a `next_uri` cookie.
- User's browser has a bunch of extensions and now makes a ton of requests for stuff that doesn't exist, like `humans.txt` and `apple-touch-icon.png`. We can't distinguish between these requests and normal requests in a general way, so we write `next_uri` cookies, overwriting the user's intent (`/xyz/`).
To fix this, we made the 404 page not set `next_uri`, in D4012. So if the browser requests `humans.txt`, we 404 with no cookie, and the `/xyz/` cookie is preserved. However, this is bad because an attacker can determine if objects exist and applications are installed, by visiting, e.g., `/T123` and seeing if they get a 404 page (resource really does not exist) or a login page (resource exists). We'd rather not leak this information.
The comment in the body text describes this in more detail.
This diff sort of tries to do the right thing most of the time: we write the cookie only if we haven't written it in the last 2 minutes. Generally, this should mean that the original request to `/xyz/` writes it, all the `humans.txt` requests don't write it, and things work like users expect. This may occasionally do the wrong thing, but it should be very rare, and we stop leaking information about applications and objects.
Test Plan: Logged out, clicked around / logged in, used Charles to verify that cookies were set in the expected way.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3793
Differential Revision: https://secure.phabricator.com/D8047
2014-01-23 23:16:08 +01:00
|
|
|
/* -( 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;
|
2014-03-14 22:33:31 +01:00
|
|
|
$request->setTemporaryCookie(self::COOKIE_NEXTURI, $new_value);
|
After writing "next_uri", don't write it again for a while
Summary:
Fixes T3793. There's a lot of history here, see D4012, T2102. Basically, the problem is that things used to work like this:
- User is logged out and accesses `/xyz/`. After they login, we'd like to send them back to `/xyz/`, so we set a `next_uri` cookie.
- User's browser has a bunch of extensions and now makes a ton of requests for stuff that doesn't exist, like `humans.txt` and `apple-touch-icon.png`. We can't distinguish between these requests and normal requests in a general way, so we write `next_uri` cookies, overwriting the user's intent (`/xyz/`).
To fix this, we made the 404 page not set `next_uri`, in D4012. So if the browser requests `humans.txt`, we 404 with no cookie, and the `/xyz/` cookie is preserved. However, this is bad because an attacker can determine if objects exist and applications are installed, by visiting, e.g., `/T123` and seeing if they get a 404 page (resource really does not exist) or a login page (resource exists). We'd rather not leak this information.
The comment in the body text describes this in more detail.
This diff sort of tries to do the right thing most of the time: we write the cookie only if we haven't written it in the last 2 minutes. Generally, this should mean that the original request to `/xyz/` writes it, all the `humans.txt` requests don't write it, and things work like users expect. This may occasionally do the wrong thing, but it should be very rare, and we stop leaking information about applications and objects.
Test Plan: Logged out, clicked around / logged in, used Charles to verify that cookies were set in the expected way.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T3793
Differential Revision: https://secure.phabricator.com/D8047
2014-01-23 23:16:08 +01:00
|
|
|
}
|
|
|
|
|
|
|
|
|
|
|
|
/**
|
|
|
|
* 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);
|
|
|
|
}
|
|
|
|
|
2014-01-23 23:01:35 +01:00
|
|
|
}
|