Page MenuHomePhabricator
Files F14410966

D8318.id19783.diff
No OneTemporary
Actions

D8318.id19783.diff
View Options

Index: src/applications/auth/application/PhabricatorApplicationAuth.php
===================================================================
--- src/applications/auth/application/PhabricatorApplicationAuth.php
+++ src/applications/auth/application/PhabricatorApplicationAuth.php
@@ -76,7 +76,8 @@
'(?P<action>enable|disable)/(?P<id>\d+)/' =>
'PhabricatorAuthDisableController',
),
- 'login/(?P<pkey>[^/]+)/' => 'PhabricatorAuthLoginController',
+ 'login/(?P<pkey>[^/]+)/(?:(?P<extra>[^/]+)/)?' =>
+ 'PhabricatorAuthLoginController',
'register/(?:(?P<akey>[^/]+)/)?' => 'PhabricatorAuthRegisterController',
'start/' => 'PhabricatorAuthStartController',
'validate/' => 'PhabricatorAuthValidateController',
Index: src/applications/auth/controller/PhabricatorAuthLoginController.php
===================================================================
--- src/applications/auth/controller/PhabricatorAuthLoginController.php
+++ src/applications/auth/controller/PhabricatorAuthLoginController.php
@@ -4,6 +4,7 @@
extends PhabricatorAuthController {
private $providerKey;
+ private $extraURIData;
private $provider;
public function shouldRequireLogin() {
@@ -12,6 +13,11 @@
public function willProcessRequest(array $data) {
$this->providerKey = $data['pkey'];
+ $this->extraURIData = idx($data, 'extra');
+ }
+
+ public function getExtraURIData() {
+ return $this->extraURIData;
}
public function processRequest() {
Index: src/applications/auth/provider/PhabricatorAuthProvider.php
===================================================================
--- src/applications/auth/provider/PhabricatorAuthProvider.php
+++ src/applications/auth/provider/PhabricatorAuthProvider.php
@@ -441,4 +441,38 @@
return null;
}
+ protected function getAuthCSRFCode(AphrontRequest $request) {
+ $phcid = $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID);
+ if (!strlen($phcid)) {
+ throw new Exception(
+ pht(
+ 'Your browser did not submit a "%s" cookie with client state '.
+ 'information in the request. Check that cookies are enabled. '.
+ 'If this problem persists, you may need to clear your cookies.',
+ PhabricatorCookies::COOKIE_CLIENTID));
+ }
+
+ return PhabricatorHash::digest($phcid);
+ }
+
+ protected function verifyAuthCSRFCode(AphrontRequest $request, $actual) {
+ $expect = $this->getAuthCSRFCode($request);
+
+ if (!strlen($actual)) {
+ throw new Exception(
+ pht(
+ 'The authentication provider did not return a client state '.
+ 'parameter in its response, but one was expected. If this '.
+ 'problem persists, you may need to clear your cookies.'));
+ }
+
+ if ($actual !== $expect) {
+ throw new Exception(
+ pht(
+ 'The authentication provider did not return the correct client '.
+ 'state parameter in its response. If this problem persists, you may '.
+ 'need to clear your cookies.'));
+ }
+ }
+
}
Index: src/applications/auth/provider/PhabricatorAuthProviderOAuth.php
===================================================================
--- src/applications/auth/provider/PhabricatorAuthProviderOAuth.php
+++ src/applications/auth/provider/PhabricatorAuthProviderOAuth.php
@@ -35,9 +35,7 @@
protected function renderLoginForm(AphrontRequest $request, $mode) {
$adapter = $this->getAdapter();
- $adapter->setState(
- PhabricatorHash::digest(
- $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID)));
+ $adapter->setState($this->getAuthCSRFCode($request));
$scope = $request->getStr('scope');
if ($scope) {
@@ -71,6 +69,8 @@
return array($account, $response);
}
+ $this->verifyAuthCSRFCode($request, $request->getStr('state'));
+
$code = $request->getStr('code');
if (!strlen($code)) {
$response = $controller->buildProviderErrorResponse(
@@ -82,30 +82,6 @@
return array($account, $response);
}
- if ($adapter->supportsStateParameter()) {
- $phcid = $request->getCookie(PhabricatorCookies::COOKIE_CLIENTID);
- if (!strlen($phcid)) {
- $response = $controller->buildProviderErrorResponse(
- $this,
- pht(
- '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.',
- PhabricatorCookies::COOKIE_CLIENTID));
- }
-
- $state = $request->getStr('state');
- $expect = PhabricatorHash::digest($phcid);
- if ($state !== $expect) {
- $response = $controller->buildProviderErrorResponse(
- $this,
- pht(
- 'The OAuth provider did not return the correct "state" parameter '.
- 'in its response. If this problem persists, you may need to clear '.
- 'your cookies.'));
- }
- }
-
$adapter->setCode($code);
// NOTE: As a side effect, this will cause the OAuth adapter to request
Index: src/applications/auth/provider/PhabricatorAuthProviderOAuth1.php
===================================================================
--- src/applications/auth/provider/PhabricatorAuthProviderOAuth1.php
+++ src/applications/auth/provider/PhabricatorAuthProviderOAuth1.php
@@ -55,6 +55,15 @@
$response = null;
if ($request->isHTTPPost()) {
+ // Add a CSRF code to the callback URI, which we'll verify when
+ // performing the login.
+
+ $client_code = $this->getAuthCSRFCode($request);
+
+ $callback_uri = $adapter->getCallbackURI();
+ $callback_uri = $callback_uri.$client_code.'/';
+ $adapter->setCallbackURI($callback_uri);
+
$uri = $adapter->getClientRedirectURI();
$response = id(new AphrontRedirectResponse())->setURI($uri);
return array($account, $response);
@@ -70,6 +79,8 @@
// NOTE: You can get here via GET, this should probably be a bit more
// user friendly.
+ $this->verifyAuthCSRFCode($request, $controller->getExtraURIData());
+
$token = $request->getStr('oauth_token');
$verifier = $request->getStr('oauth_verifier');

File Metadata

Mime Type
text/plain
Expires
Wed, Dec 25, 8:27 AM (5 h, 51 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6925633
Default Alt Text
D8318.id19783.diff (6 KB)

Event Timeline

Phabricator · Built by Phacility