Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F13177131
D8041.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
16 KB
Referenced Files
None
Subscribers
None
D8041.diff
View Options
Index: src/__phutil_library_map__.php
===================================================================
--- src/__phutil_library_map__.php
+++ src/__phutil_library_map__.php
@@ -1319,6 +1319,7 @@
'PhabricatorContentSource' => 'applications/metamta/contentsource/PhabricatorContentSource.php',
'PhabricatorContentSourceView' => 'applications/metamta/contentsource/PhabricatorContentSourceView.php',
'PhabricatorController' => 'applications/base/controller/PhabricatorController.php',
+ 'PhabricatorCookies' => 'applications/auth/constants/PhabricatorCookies.php',
'PhabricatorCoreConfigOptions' => 'applications/config/option/PhabricatorCoreConfigOptions.php',
'PhabricatorCountdown' => 'applications/countdown/storage/PhabricatorCountdown.php',
'PhabricatorCountdownCapabilityDefaultView' => 'applications/countdown/capability/PhabricatorCountdownCapabilityDefaultView.php',
@@ -3934,6 +3935,7 @@
'PhabricatorConpherencePHIDTypeThread' => 'PhabricatorPHIDType',
'PhabricatorContentSourceView' => 'AphrontView',
'PhabricatorController' => 'AphrontController',
+ 'PhabricatorCookies' => 'Phobject',
'PhabricatorCoreConfigOptions' => 'PhabricatorApplicationConfigOptions',
'PhabricatorCountdown' =>
array(
Index: src/aphront/console/DarkConsoleDataController.php
===================================================================
--- src/aphront/console/DarkConsoleDataController.php
+++ src/aphront/console/DarkConsoleDataController.php
@@ -58,12 +58,17 @@
$panel = $obj->renderPanel();
- if (!empty($_COOKIE['phsid'])) {
- $panel = PhutilSafeHTML::applyFunction(
- 'str_replace',
- $_COOKIE['phsid'],
- '(session-key)',
- $panel);
+ // Because cookie names can now be prefixed, wipe out any cookie value
+ // with the session cookie name anywhere in its name.
+ $pattern = '('.preg_quote(PhabricatorCookies::COOKIE_SESSION).')';
+ foreach ($_COOKIE as $cookie_name => $cookie_value) {
+ if (preg_match($pattern, $cookie_name)) {
+ $panel = PhutilSafeHTML::applyFunction(
+ 'str_replace',
+ $cookie_value,
+ '(session-key)',
+ $panel);
+ }
}
$output['panel'][$class] = $panel;
Index: src/applications/auth/constants/PhabricatorCookies.php
===================================================================
--- /dev/null
+++ src/applications/auth/constants/PhabricatorCookies.php
@@ -0,0 +1,48 @@
+<?php
+
+/**
+ * Consolidates Phabricator application cookies, including registration
+ * and session management.
+ */
+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';
+
+}
Index: src/applications/auth/controller/PhabricatorAuthController.php
===================================================================
--- src/applications/auth/controller/PhabricatorAuthController.php
+++ src/applications/auth/controller/PhabricatorAuthController.php
@@ -88,8 +88,12 @@
// there's no check for users being disabled here.
$request = $this->getRequest();
- $request->setCookie('phusr', $user->getUsername());
- $request->setCookie('phsid', $session_key);
+ $request->setCookie(
+ PhabricatorCookies::COOKIE_USERNAME,
+ $user->getUsername());
+ $request->setCookie(
+ PhabricatorCookies::COOKIE_SESSION,
+ $session_key);
$this->clearRegistrationCookies();
}
@@ -101,15 +105,15 @@
$request = $this->getRequest();
// Clear the registration key.
- $request->clearCookie('phreg');
+ $request->clearCookie(PhabricatorCookies::COOKIE_REGISTRATION);
// Clear the client ID / OAuth state key.
- $request->clearCookie('phcid');
+ $request->clearCookie(PhabricatorCookies::COOKIE_CLIENTID);
}
private function buildLoginValidateResponse(PhabricatorUser $user) {
$validate_uri = new PhutilURI($this->getApplicationURI('validate/'));
- $validate_uri->setQueryParam('phusr', $user->getUsername());
+ $validate_uri->setQueryParam('expect', $user->getUsername());
return id(new AphrontRedirectResponse())->setURI((string)$validate_uri);
}
@@ -168,7 +172,8 @@
return array($account, $provider, $response);
}
- $registration_key = $request->getCookie('phreg');
+ $registration_key = $request->getCookie(
+ PhabricatorCookies::COOKIE_REGISTRATION);
// NOTE: This registration key check is not strictly necessary, because
// we're only creating new accounts, not linking existing accounts. It
@@ -181,7 +186,7 @@
// since you could have simply completed the process yourself.
if (!$registration_key) {
- $response = $this->renderError(
+ $response = $this->renderError(
pht(
'Your browser did not submit a registration key with the request. '.
'You must use the same browser to begin and complete registration. '.
Index: src/applications/auth/controller/PhabricatorAuthLinkController.php
===================================================================
--- src/applications/auth/controller/PhabricatorAuthLinkController.php
+++ src/applications/auth/controller/PhabricatorAuthLinkController.php
@@ -79,7 +79,10 @@
$panel_uri = '/settings/panel/external/';
- $request->setCookie('phcid', Filesystem::readRandomCharacters(16));
+ $request->setCookie(
+ PhabricatorCookies::COOKIE_CLIENTID,
+ Filesystem::readRandomCharacters(16));
+
switch ($this->action) {
case 'link':
$form = $provider->buildLinkForm($this);
Index: src/applications/auth/controller/PhabricatorAuthLoginController.php
===================================================================
--- src/applications/auth/controller/PhabricatorAuthLoginController.php
+++ src/applications/auth/controller/PhabricatorAuthLoginController.php
@@ -166,7 +166,9 @@
$account->save();
unset($unguarded);
- $this->getRequest()->setCookie('phreg', $registration_key);
+ $this->getRequest()->setCookie(
+ PhabricatorCookies::COOKIE_REGISTRATION,
+ $registration_key);
return id(new AphrontRedirectResponse())->setURI($next_uri);
}
Index: src/applications/auth/controller/PhabricatorAuthStartController.php
===================================================================
--- src/applications/auth/controller/PhabricatorAuthStartController.php
+++ src/applications/auth/controller/PhabricatorAuthStartController.php
@@ -24,10 +24,10 @@
return $this->processConduitRequest();
}
- if ($request->getCookie('phusr') && $request->getCookie('phsid')) {
+ if (strlen($request->getCookie(PhabricatorCookies::COOKIE_SESSION))) {
// The session cookie is invalid, so clear it.
- $request->clearCookie('phusr');
- $request->clearCookie('phsid');
+ $request->clearCookie(PhabricatorCookies::COOKIE_USERNAME);
+ $request->clearCookie(PhabricatorCookies::COOKIE_SESSION);
return $this->renderError(
pht(
@@ -71,8 +71,12 @@
}
if (!$request->isFormPost()) {
- $request->setCookie('next_uri', $next_uri);
- $request->setCookie('phcid', Filesystem::readRandomCharacters(16));
+ $request->setCookie(
+ PhabricatorCookies::COOKIE_NEXTURI,
+ $next_uri);
+ $request->setCookie(
+ PhabricatorCookies::COOKIE_CLIENTID,
+ Filesystem::readRandomCharacters(16));
}
$not_buttons = array();
Index: src/applications/auth/controller/PhabricatorAuthValidateController.php
===================================================================
--- src/applications/auth/controller/PhabricatorAuthValidateController.php
+++ src/applications/auth/controller/PhabricatorAuthValidateController.php
@@ -13,7 +13,7 @@
$failures = array();
- if (!strlen($request->getStr('phusr'))) {
+ if (!strlen($request->getStr('expect'))) {
return $this->renderErrors(
array(
pht(
@@ -21,8 +21,8 @@
'phusr')));
}
- $expect_phusr = $request->getStr('phusr');
- $actual_phusr = $request->getCookie('phusr');
+ $expect_phusr = $request->getStr('expect');
+ $actual_phusr = $request->getCookie(PhabricatorCookies::COOKIE_USERNAME);
if ($actual_phusr != $expect_phusr) {
if ($actual_phusr) {
$failures[] = pht(
@@ -54,8 +54,8 @@
return $this->renderErrors($failures);
}
- $next = $request->getCookie('next_uri');
- $request->clearCookie('next_uri');
+ $next = $request->getCookie(PhabricatorCookies::COOKIE_NEXTURI);
+ $request->clearCookie(PhabricatorCookies::COOKIE_NEXTURI);
if (!PhabricatorEnv::isValidLocalWebResource($next)) {
$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('next_uri', $next);
+ $request->setCookie(PhabricatorCookies::COOKIE_NEXTURI, $next);
return $this->loginUser($target_user);
}
Index: src/applications/auth/controller/PhabricatorLogoutController.php
===================================================================
--- src/applications/auth/controller/PhabricatorLogoutController.php
+++ src/applications/auth/controller/PhabricatorLogoutController.php
@@ -32,8 +32,8 @@
// Destroy the user's session in the database so logout works even if
// their cookies have some issues. We'll detect cookie issues when they
// try to login again and tell them to clear any junk.
- $phsid = $request->getCookie('phsid');
- if ($phsid) {
+ $phsid = $request->getCookie(PhabricatorCookies::COOKIE_SESSION);
+ if (strlen($phsid)) {
$session = id(new PhabricatorAuthSessionQuery())
->setViewer($user)
->withSessionKeys(array($phsid))
@@ -42,7 +42,7 @@
$session->delete();
}
}
- $request->clearCookie('phsid');
+ $request->clearCookie(PhabricatorCookies::COOKIE_SESSION);
return id(new AphrontRedirectResponse())
->setURI('/login/');
@@ -52,8 +52,7 @@
$dialog = id(new AphrontDialogView())
->setUser($user)
->setTitle(pht('Log out of Phabricator?'))
- ->appendChild(phutil_tag('p', array(), pht(
- 'Are you sure you want to log out?')))
+ ->appendChild(pht('Are you sure you want to log out?'))
->addSubmitButton(pht('Logout'))
->addCancelButton('/');
Index: src/applications/auth/provider/PhabricatorAuthProviderOAuth.php
===================================================================
--- src/applications/auth/provider/PhabricatorAuthProviderOAuth.php
+++ src/applications/auth/provider/PhabricatorAuthProviderOAuth.php
@@ -35,9 +35,11 @@
protected function renderLoginForm(AphrontRequest $request, $mode) {
$adapter = $this->getAdapter();
- $adapter->setState(PhabricatorHash::digest($request->getCookie('phcid')));
+ $adapter->setState(
+ PhabricatorHash::digest(
+ $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID)));
- $scope = $request->getStr("scope");
+ $scope = $request->getStr('scope');
if ($scope) {
$adapter->setScope($scope);
}
@@ -81,14 +83,15 @@
}
if ($adapter->supportsStateParameter()) {
- $phcid = $request->getCookie('phcid');
+ $phcid = $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID);
if (!strlen($phcid)) {
$response = $controller->buildProviderErrorResponse(
$this,
pht(
- 'Your browser did not submit a "phcid" cookie with OAuth state '.
+ 'Your browser did not submit a "%s" cookie with OAuth state '.
'information in the request. Check that cookies are enabled. '.
- 'If this problem persists, you may need to clear your cookies.'));
+ 'If this problem persists, you may need to clear your cookies.',
+ PhabricatorCookies::COOKIE_CLIENTID));
}
$state = $request->getStr('state');
Index: src/applications/auth/provider/PhabricatorAuthProviderPassword.php
===================================================================
--- src/applications/auth/provider/PhabricatorAuthProviderPassword.php
+++ src/applications/auth/provider/PhabricatorAuthProviderPassword.php
@@ -87,7 +87,7 @@
$v_user = nonempty(
$request->getStr('username'),
- $request->getCookie('phusr'));
+ $request->getCookie(PhabricatorCookies::COOKIE_USERNAME));
$e_user = null;
$e_pass = null;
@@ -191,8 +191,8 @@
PhabricatorUserLog::ACTION_LOGIN_FAILURE);
$log->save();
- $request->clearCookie('phusr');
- $request->clearCookie('phsid');
+ $request->clearCookie(PhabricatorCookies::COOKIE_USERNAME);
+ $request->clearCookie(PhabricatorCookies::COOKIE_SESSION);
$response = $controller->buildProviderPageResponse(
$this,
Index: src/applications/base/controller/PhabricatorController.php
===================================================================
--- src/applications/base/controller/PhabricatorController.php
+++ src/applications/base/controller/PhabricatorController.php
@@ -35,7 +35,7 @@
} else {
$user = new PhabricatorUser();
- $phsid = $request->getCookie('phsid');
+ $phsid = $request->getCookie(PhabricatorCookies::COOKIE_SESSION);
if ($phsid) {
$session_user = id(new PhabricatorAuthSessionEngine())
->loadUserForSession(PhabricatorAuthSession::TYPE_WEB, $phsid);
Index: src/applications/people/storage/PhabricatorUserLog.php
===================================================================
--- src/applications/people/storage/PhabricatorUserLog.php
+++ src/applications/people/storage/PhabricatorUserLog.php
@@ -66,7 +66,9 @@
$this->remoteAddr = idx($_SERVER, 'REMOTE_ADDR', '');
}
if (!$this->session) {
- $this->setSession(idx($_COOKIE, 'phsid'));
+ // TODO: This is not correct if there's a cookie prefix. This object
+ // should take an AphrontRequest.
+ $this->setSession(idx($_COOKIE, PhabricatorCookies::COOKIE_SESSION));
}
$this->details['host'] = php_uname('n');
$this->details['user_agent'] = AphrontRequest::getHTTPHeader('User-Agent');
Index: src/applications/settings/panel/PhabricatorSettingsPanelSessions.php
===================================================================
--- src/applications/settings/panel/PhabricatorSettingsPanelSessions.php
+++ src/applications/settings/panel/PhabricatorSettingsPanelSessions.php
@@ -40,11 +40,8 @@
->withPHIDs($identity_phids)
->execute();
- // TODO: Once this has a real ID column, use that instead.
- $sessions = msort($sessions, 'getSessionStart');
- $sessions = array_reverse($sessions);
-
- $current_key = PhabricatorHash::digest($request->getCookie('phsid'));
+ $current_key = PhabricatorHash::digest(
+ $request->getCookie(PhabricatorCookies::COOKIE_SESSION));
$rows = array();
$rowc = array();
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Thu, May 9, 7:28 PM (3 w, 4 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6275562
Default Alt Text
D8041.diff (16 KB)
Attached To
Mode
D8041: Consolidate use of magical cookie name strings
Attached
Detach File
Event Timeline
Log In to Comment