diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 2441c55351..a8282dcef3 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1430,6 +1430,7 @@ phutil_register_library_map(array( '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 @@ phutil_register_library_map(array( '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 index 0000000000..71d6b744fc --- /dev/null +++ b/src/__tests__/PhabricatorConduitTestCase.php @@ -0,0 +1,19 @@ +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 index bbeeaab071..0d3b52d4ea 100644 --- a/src/applications/conduit/call/ConduitCall.php +++ b/src/applications/conduit/call/ConduitCall.php @@ -18,13 +18,26 @@ final class ConduitCall { $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 index dfacbb8058..835f52448d 100644 --- a/src/applications/conduit/controller/PhabricatorConduitAPIController.php +++ b/src/applications/conduit/controller/PhabricatorConduitAPIController.php @@ -28,12 +28,9 @@ final class PhabricatorConduitAPIController 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 @@ final class PhabricatorConduitAPIController ); } + $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 @@ final class PhabricatorConduitAPIController $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 @@ final class PhabricatorConduitAPIController $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 @@ final class PhabricatorConduitAPIController $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 @@ final class PhabricatorConduitAPIController "'{$method}', could not decode JSON serialization. Data: ". $params_json); } + + $metadata = idx($params, '__conduit__', array()); + unset($params['__conduit__']); + + return array($metadata, $params); } - return $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 array($metadata, $params); } } diff --git a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php index 41076752bb..7da3f4851f 100644 --- a/src/applications/conduit/controller/PhabricatorConduitConsoleController.php +++ b/src/applications/conduit/controller/PhabricatorConduitConsoleController.php @@ -67,7 +67,6 @@ final class PhabricatorConduitConsoleController $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 index 28af74dcec..3e28d6899e 100644 --- a/src/applications/conduit/method/ConduitAPIMethod.php +++ b/src/applications/conduit/method/ConduitAPIMethod.php @@ -149,6 +149,35 @@ abstract class ConduitAPIMethod 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 index d6e151bb0a..e0d0886273 100644 --- a/src/applications/conduit/query/PhabricatorConduitTokenQuery.php +++ b/src/applications/conduit/query/PhabricatorConduitTokenQuery.php @@ -6,6 +6,7 @@ final class PhabricatorConduitTokenQuery private $ids; private $objectPHIDs; private $expired; + private $tokens; public function withExpired($expired) { $this->expired = $expired; @@ -22,6 +23,11 @@ final class PhabricatorConduitTokenQuery 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 @@ final class PhabricatorConduitTokenQuery $this->objectPHIDs); } + if ($this->tokens !== null) { + $where[] = qsprintf( + $conn_r, + 'token IN (%Ls)', + $this->tokens); + } + if ($this->expired !== null) { if ($this->expired) { $where[] = qsprintf(