Page Menu
Home
Phabricator
Search
Configure Global Search
Log In
Files
F14707837
D10986.diff
No One
Temporary
Actions
View File
Edit File
Delete File
View Transforms
Subscribe
Mute Notifications
Award Token
Flag For Later
Size
12 KB
Referenced Files
None
Subscribers
None
D10986.diff
View Options
diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php
--- a/src/__phutil_library_map__.php
+++ b/src/__phutil_library_map__.php
@@ -1430,6 +1430,7 @@
'PhabricatorConduitMethodQuery' => 'applications/conduit/query/PhabricatorConduitMethodQuery.php',
'PhabricatorConduitSearchEngine' => 'applications/conduit/query/PhabricatorConduitSearchEngine.php',
'PhabricatorConduitSettingsPanel' => 'applications/conduit/settings/PhabricatorConduitSettingsPanel.php',
+ 'PhabricatorConduitTestCase' => '__tests__/PhabricatorConduitTestCase.php',
'PhabricatorConduitToken' => 'applications/conduit/storage/PhabricatorConduitToken.php',
'PhabricatorConduitTokenController' => 'applications/conduit/controller/PhabricatorConduitTokenController.php',
'PhabricatorConduitTokenEditController' => 'applications/conduit/controller/PhabricatorConduitTokenEditController.php',
@@ -4549,6 +4550,7 @@
'PhabricatorConduitMethodQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
'PhabricatorConduitSearchEngine' => 'PhabricatorApplicationSearchEngine',
'PhabricatorConduitSettingsPanel' => 'PhabricatorSettingsPanel',
+ 'PhabricatorConduitTestCase' => 'PhabricatorTestCase',
'PhabricatorConduitToken' => array(
'PhabricatorConduitDAO',
'PhabricatorPolicyInterface',
diff --git a/src/__tests__/PhabricatorConduitTestCase.php b/src/__tests__/PhabricatorConduitTestCase.php
new file mode 100644
--- /dev/null
+++ b/src/__tests__/PhabricatorConduitTestCase.php
@@ -0,0 +1,19 @@
+<?php
+
+final class PhabricatorConduitTestCase extends PhabricatorTestCase {
+
+ public function testConduitMethods() {
+ $methods = id(new PhutilSymbolLoader())
+ ->setAncestorClass('ConduitAPIMethod')
+ ->loadObjects();
+
+ // We're just looking for a side effect of ConduitCall construction
+ // here: it will throw if any methods define reserved parameter names.
+
+ foreach ($methods as $method) {
+ new ConduitCall($method->getAPIMethodName(), array());
+ }
+
+ $this->assertTrue(true);
+ }
+}
diff --git a/src/applications/conduit/call/ConduitCall.php b/src/applications/conduit/call/ConduitCall.php
--- a/src/applications/conduit/call/ConduitCall.php
+++ b/src/applications/conduit/call/ConduitCall.php
@@ -18,13 +18,26 @@
$this->method = $method;
$this->handler = $this->buildMethodHandler($method);
- $invalid_params = array_diff_key(
- $params,
- $this->handler->defineParamTypes());
+ $param_types = $this->handler->defineParamTypes();
+
+ foreach ($param_types as $key => $spec) {
+ if (ConduitAPIMethod::getParameterMetadataKey($key) !== null) {
+ throw new ConduitException(
+ pht(
+ 'API Method "%s" defines a disallowed parameter, "%s". This '.
+ 'parameter name is reserved.',
+ $method,
+ $key));
+ }
+ }
+
+ $invalid_params = array_diff_key($params, $param_types);
if ($invalid_params) {
throw new ConduitException(
- "Method '{$method}' doesn't define these parameters: '".
- implode("', '", array_keys($invalid_params))."'.");
+ pht(
+ 'API Method "%s" does not define these parameters: %s.',
+ $method,
+ "'".implode("', '", array_keys($invalid_params))."'"));
}
$this->request = new ConduitAPIRequest($params);
diff --git a/src/applications/conduit/controller/PhabricatorConduitAPIController.php b/src/applications/conduit/controller/PhabricatorConduitAPIController.php
--- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php
+++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php
@@ -28,12 +28,9 @@
try {
- $params = $this->decodeConduitParams($request, $method);
- $metadata = idx($params, '__conduit__', array());
- unset($params['__conduit__']);
+ list($metadata, $params) = $this->decodeConduitParams($request, $method);
- $call = new ConduitCall(
- $method, $params, idx($metadata, 'isProxied', false));
+ $call = new ConduitCall($method, $params);
$result = null;
@@ -296,9 +293,78 @@
);
}
+ $token_string = idx($metadata, 'token');
+ if (strlen($token_string)) {
+
+ if (strlen($token_string) != 32) {
+ return array(
+ 'ERR-INVALID-AUTH',
+ pht(
+ 'API token "%s" has the wrong length. API tokens should be '.
+ '32 characters long.'),
+ );
+ }
+
+ $type = head(explode('-', $token_string));
+ switch ($type) {
+ case 'api':
+ case 'tmp':
+ break;
+ default:
+ return array(
+ 'ERR-INVALID-AUTH',
+ pht(
+ 'API token "%s" has the wrong format. API tokens should begin '.
+ 'with "api-" or "tmp-" and be 32 characters long.',
+ $token_string),
+ );
+ }
+
+ $token = id(new PhabricatorConduitTokenQuery())
+ ->setViewer(PhabricatorUser::getOmnipotentUser())
+ ->withTokens(array($token_string))
+ ->withExpired(false)
+ ->executeOne();
+ if (!$token) {
+ $token = id(new PhabricatorConduitTokenQuery())
+ ->setViewer(PhabricatorUser::getOmnipotentUser())
+ ->withTokens(array($token_string))
+ ->withExpired(true)
+ ->executeOne();
+ if ($token) {
+ return array(
+ 'ERR-INVALID-AUTH',
+ pht(
+ 'API token "%s" was previously valid, but has expired.',
+ $token_string),
+ );
+ } else {
+ return array(
+ 'ERR-INVALID-AUTH',
+ pht(
+ 'API token "%s" is not valid.',
+ $token_string),
+ );
+ }
+ }
+
+ $user = $token->getObject();
+ if (!($user instanceof PhabricatorUser)) {
+ return array(
+ 'ERR-INVALID-AUTH',
+ pht(
+ 'API token is not associated with a valid user.'),
+ );
+ }
+
+ return $this->validateAuthenticatedUser(
+ $api_request,
+ $user);
+ }
+
// handle oauth
- $access_token = $request->getStr('access_token');
- $method_scope = $metadata['scope'];
+ $access_token = idx($metadata, 'access_token');
+ $method_scope = idx($metadata, 'scope');
if ($access_token &&
$method_scope != PhabricatorOAuthServerScope::SCOPE_NOT_ACCESSIBLE) {
$token = id(new PhabricatorOAuthServerAccessToken())
@@ -337,7 +403,10 @@
$user);
}
- // Handle sessionless auth. TOOD: This is super messy.
+ // Handle sessionless auth.
+ // TODO: This is super messy.
+ // TODO: Remove this in favor of token-based auth.
+
if (isset($metadata['authUser'])) {
$user = id(new PhabricatorUser())->loadOneWhere(
'userName = %s',
@@ -362,6 +431,9 @@
$user);
}
+ // Handle session-based auth.
+ // TODO: Remove this in favor of token-based auth.
+
$session_key = idx($metadata, 'sessionKey');
if (!$session_key) {
return array(
@@ -525,28 +597,16 @@
$params[$key] = $decoded_value;
}
- return $params;
+ $metadata = idx($params, '__conduit__', array());
+ unset($params['__conduit__']);
+
+ return array($metadata, $params);
}
// Otherwise, look for a single parameter called 'params' which has the
- // entire param dictionary JSON encoded. This is the usual case for remote
- // requests.
-
+ // entire param dictionary JSON encoded.
$params_json = $request->getStr('params');
- if (!strlen($params_json)) {
- if ($request->getBool('allowEmptyParams')) {
- // TODO: This is a bit messy, but otherwise you can't call
- // "conduit.ping" from the web console.
- $params = array();
- } else {
- throw new Exception(
- "Request has no 'params' key. This may mean that an extension like ".
- "Suhosin has dropped data from the request. Check the PHP ".
- "configuration on your server. If you are developing a Conduit ".
- "client, you MUST provide a 'params' parameter when making a ".
- "Conduit request, even if the value is empty (e.g., provide '{}').");
- }
- } else {
+ if (strlen($params_json)) {
$params = json_decode($params_json, true);
if (!is_array($params)) {
throw new Exception(
@@ -554,9 +614,27 @@
"'{$method}', could not decode JSON serialization. Data: ".
$params_json);
}
+
+ $metadata = idx($params, '__conduit__', array());
+ unset($params['__conduit__']);
+
+ return array($metadata, $params);
+ }
+
+ // If we do not have `params`, assume this is a simple HTTP request with
+ // HTTP key-value pairs.
+ $params = array();
+ $metadata = array();
+ foreach ($request->getPassthroughRequestData() as $key => $value) {
+ $meta_key = ConduitAPIMethod::getParameterMetadataKey($key);
+ if ($meta_key !== null) {
+ $metadata[$meta_key] = $value;
+ } else {
+ $params[$key] = $value;
+ }
}
- return $params;
+ return array($metadata, $params);
}
}
diff --git a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php
--- a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php
+++ b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php
@@ -67,7 +67,6 @@
$form
->setUser($request->getUser())
->setAction('/api/'.$this->method)
- ->addHiddenInput('allowEmptyParams', 1)
->appendChild(
id(new AphrontFormStaticControl())
->setLabel('Description')
diff --git a/src/applications/conduit/method/ConduitAPIMethod.php b/src/applications/conduit/method/ConduitAPIMethod.php
--- a/src/applications/conduit/method/ConduitAPIMethod.php
+++ b/src/applications/conduit/method/ConduitAPIMethod.php
@@ -149,6 +149,35 @@
return 'string-constant<'.$constants.'>';
}
+ public static function getParameterMetadataKey($key) {
+ if (strncmp($key, 'api.', 4) === 0) {
+ // All keys passed beginning with "api." are always metadata keys.
+ return substr($key, 4);
+ } else {
+ switch ($key) {
+ // These are real keys which always belong to request metadata.
+ case 'access_token':
+ case 'scope':
+ case 'output':
+
+ // This is not a real metadata key; it is included here only to
+ // prevent Conduit methods from defining it.
+ case '__conduit__':
+
+ // This is prevented globally as a blanket defense against OAuth
+ // redirection attacks. It is included here to stop Conduit methods
+ // from defining it.
+ case 'code':
+
+ // This is not a real metadata key, but the presence of this
+ // parameter triggers an alternate request decoding pathway.
+ case 'params':
+ return $key;
+ }
+ }
+
+ return null;
+ }
/* -( Paging Results )----------------------------------------------------- */
diff --git a/src/applications/conduit/query/PhabricatorConduitTokenQuery.php b/src/applications/conduit/query/PhabricatorConduitTokenQuery.php
--- a/src/applications/conduit/query/PhabricatorConduitTokenQuery.php
+++ b/src/applications/conduit/query/PhabricatorConduitTokenQuery.php
@@ -6,6 +6,7 @@
private $ids;
private $objectPHIDs;
private $expired;
+ private $tokens;
public function withExpired($expired) {
$this->expired = $expired;
@@ -22,6 +23,11 @@
return $this;
}
+ public function withTokens(array $tokens) {
+ $this->tokens = $tokens;
+ return $this;
+ }
+
public function loadPage() {
$table = new PhabricatorConduitToken();
$conn_r = $table->establishConnection('r');
@@ -54,6 +60,13 @@
$this->objectPHIDs);
}
+ if ($this->tokens !== null) {
+ $where[] = qsprintf(
+ $conn_r,
+ 'token IN (%Ls)',
+ $this->tokens);
+ }
+
if ($this->expired !== null) {
if ($this->expired) {
$where[] = qsprintf(
File Metadata
Details
Attached
Mime Type
text/plain
Expires
Sat, Jan 18, 6:52 AM (10 h, 53 m)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
7000595
Default Alt Text
D10986.diff (12 KB)
Attached To
Mode
D10986: Accept Conduit tokens as an authentication mechanism
Attached
Detach File
Event Timeline
Log In to Comment