Page MenuHomePhabricator
Files F14058275

D8499.diff
No OneTemporary
Actions

D8499.diff
View Options

diff --git a/src/applications/auth/controller/PhabricatorAuthLoginController.php b/src/applications/auth/controller/PhabricatorAuthLoginController.php
--- a/src/applications/auth/controller/PhabricatorAuthLoginController.php
+++ b/src/applications/auth/controller/PhabricatorAuthLoginController.php
@@ -11,6 +11,15 @@
return false;
}
+ public function shouldAllowRestrictedParameter($parameter_name) {
+ // Whitelist the OAuth 'code' parameter.
+
+ if ($parameter_name == 'code') {
+ return true;
+ }
+ return parent::shouldAllowRestrictedParameter($parameter_name);
+ }
+
public function willProcessRequest(array $data) {
$this->providerKey = $data['pkey'];
$this->extraURIData = idx($data, 'extra');
diff --git a/src/applications/auth/controller/PhabricatorAuthOldOAuthRedirectController.php b/src/applications/auth/controller/PhabricatorAuthOldOAuthRedirectController.php
--- a/src/applications/auth/controller/PhabricatorAuthOldOAuthRedirectController.php
+++ b/src/applications/auth/controller/PhabricatorAuthOldOAuthRedirectController.php
@@ -9,6 +9,13 @@
return false;
}
+ public function shouldAllowRestrictedParameter($parameter_name) {
+ if ($parameter_name == 'code') {
+ return true;
+ }
+ return parent::shouldAllowRestrictedParameter($parameter_name);
+ }
+
public function willProcessRequest(array $data) {
$this->provider = $data['provider'];
}
diff --git a/src/applications/base/controller/PhabricatorController.php b/src/applications/base/controller/PhabricatorController.php
--- a/src/applications/base/controller/PhabricatorController.php
+++ b/src/applications/base/controller/PhabricatorController.php
@@ -24,9 +24,13 @@
return PhabricatorUserEmail::isEmailVerificationRequired();
}
- public function willBeginExecution() {
+ public function shouldAllowRestrictedParameter($parameter_name) {
+ return false;
+ }
+ public function willBeginExecution() {
$request = $this->getRequest();
+
if ($request->getUser()) {
// NOTE: Unit tests can set a user explicitly. Normal requests are not
// permitted to do this.
@@ -85,6 +89,26 @@
}
}
+ // NOTE: We want to set up the user first so we can render a real page
+ // here, but fire this before any real logic.
+ $restricted = array(
+ 'code',
+ );
+ foreach ($restricted as $parameter) {
+ if ($request->getExists($parameter)) {
+ if (!$this->shouldAllowRestrictedParameter($parameter)) {
+ throw new Exception(
+ pht(
+ 'Request includes restricted parameter "%s", but this '.
+ 'controller ("%s") does not whitelist it. Refusing to '.
+ 'serve this request because it might be part of a redirection '.
+ 'attack.',
+ $parameter,
+ get_class($this)));
+ }
+ }
+ }
+
if ($this->shouldRequireEnabledUser()) {
if ($user->isLoggedIn() && !$user->getIsApproved()) {
$controller = new PhabricatorAuthNeedsApprovalController($request);
diff --git a/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php b/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php
--- a/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php
+++ b/src/applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php
@@ -10,6 +10,13 @@
return false;
}
+ public function shouldAllowRestrictedParameter($parameter_name) {
+ if ($parameter_name == 'code') {
+ return true;
+ }
+ return parent::shouldAllowRestrictedParameter($parameter_name);
+ }
+
public function processRequest() {
$request = $this->getRequest();
$grant_type = $request->getStr('grant_type');

File Metadata

Mime Type
text/plain
Expires
Mon, Nov 18, 10:38 AM (1 d, 4 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6716837
Default Alt Text
D8499.diff (3 KB)

Event Timeline

Phabricator · Built by Phacility