diogo/indieauth
Archived
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Refactored Server to validate params in the correct order

Browse Source 
* Authorization requests start by validating the client_id and redirect_id, and
  if valid, any further errors are reported by redirecting to the redirect_uri
* Exchange requests attempt to exchange an auth code immediately, ensuring that
  auth codes are revoked if the exchange request results in an error (not in the
  spec explicitly, but advised by aaronpk)
This commit is contained in:
Barnaby Walters 2021-06-12 22:54:08 +02:00
parent a0fe1b5f80
commit 3881139b95
4 changed files with 175 additions and 118 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
src/IndieAuthException.php
View File

@ -8,7 +8,7 @@ use Throwable;
class IndieAuthException extends Exception { class IndieAuthException extends Exception {
const INTERNAL_ERROR = 0; const INTERNAL_ERROR = 0;
const REQUEST_MISSING_PARAMETER = 1; const INTERNAL_ERROR_REDIRECT = 1;
const AUTHENTICATION_CALLBACK_MISSING_ME_PARAM = 2; const AUTHENTICATION_CALLBACK_MISSING_ME_PARAM = 2;
const AUTHORIZATION_APPROVAL_REQUEST_MISSING_HASH = 3; const AUTHORIZATION_APPROVAL_REQUEST_MISSING_HASH = 3;
const AUTHORIZATION_APPROVAL_REQUEST_INVALID_HASH = 4; const AUTHORIZATION_APPROVAL_REQUEST_INVALID_HASH = 4;
@ -19,14 +19,13 @@ class IndieAuthException extends Exception {
const INVALID_STATE = 9; const INVALID_STATE = 9;
const INVALID_CODE_CHALLENGE = 10; const INVALID_CODE_CHALLENGE = 10;
const INVALID_SCOPE = 11; const INVALID_SCOPE = 11;
const INTERNAL_ERROR_REDIRECT = 12;
const EXC_INFO = [ const EXC_INFO = [
self::INTERNAL_ERROR => ['statusCode' => 500, 'name' => 'Internal Server Error', 'explanation' => 'An internal server error occurred.'], self::INTERNAL_ERROR => ['statusCode' => 500, 'name' => 'Internal Server Error', 'explanation' => 'An internal server error occurred.'],
self::REQUEST_MISSING_PARAMETER => ['statusCode' => 400, 'name' => 'Request Missing Parameter', 'explanation' => 'The request was missing one or more required query string parameters.'], self::INTERNAL_ERROR_REDIRECT => ['statusCode' => 302, 'name' => 'Internal Server Error', 'error' => 'internal_error'],
self::AUTHENTICATION_CALLBACK_MISSING_ME_PARAM => ['statusCode' => 500, 'name' => 'Internal Server Error', 'explanation' => 'The user data returned from handleAuthenticationRequestCallback was missing a “me” parameter.'], self::AUTHENTICATION_CALLBACK_MISSING_ME_PARAM => ['statusCode' => 302, 'name' => 'Internal Server Error', 'error' => 'internal_error'],
self::AUTHORIZATION_APPROVAL_REQUEST_MISSING_HASH => ['statusCode' => 400, 'name' => 'Request Missing Hash', 'explanation' => 'An authentication form submission request was missing the hash parameter.'], self::AUTHORIZATION_APPROVAL_REQUEST_MISSING_HASH => ['statusCode' => 302, 'name' => 'Request Missing Hash', 'error' => 'internal_error'],
self::AUTHORIZATION_APPROVAL_REQUEST_INVALID_HASH => ['statusCode' => 400, 'name' => 'Request Hash Invalid', 'explanation' => 'The hash protecting the query string parameters from tampering was invalid. Your form submission may have been altered by malicious client-side code.'], self::AUTHORIZATION_APPROVAL_REQUEST_INVALID_HASH => ['statusCode' => 302, 'name' => 'Request Hash Invalid', 'error' => 'internal_error'],
// TODO: should this one be a 500 because it’s an internal server error, or a 400 because the client_id was likely invalid? Is anyone ever going to notice, or care? // TODO: should this one be a 500 because it’s an internal server error, or a 400 because the client_id was likely invalid? Is anyone ever going to notice, or care?
self::HTTP_EXCEPTION_FETCHING_CLIENT_ID => ['statusCode' => 500, 'name' => 'Error Fetching Client App URL', 'explanation' => 'Fetching the client app (client_id) failed.'], self::HTTP_EXCEPTION_FETCHING_CLIENT_ID => ['statusCode' => 500, 'name' => 'Error Fetching Client App URL', 'explanation' => 'Fetching the client app (client_id) failed.'],
self::INTERNAL_EXCEPTION_FETCHING_CLIENT_ID => ['statusCode' => 500, 'name' => 'Internal Error fetching client app URI', 'explanation' => 'Fetching the client app (client_id) failed due to an internal error.'], self::INTERNAL_EXCEPTION_FETCHING_CLIENT_ID => ['statusCode' => 500, 'name' => 'Internal Error fetching client app URI', 'explanation' => 'Fetching the client app (client_id) failed due to an internal error.'],
@ -35,7 +34,6 @@ class IndieAuthException extends Exception {
self::INVALID_STATE => ['statusCode' => 302, 'name' => 'Invalid state Parameter', 'error' => 'invalid_request'], self::INVALID_STATE => ['statusCode' => 302, 'name' => 'Invalid state Parameter', 'error' => 'invalid_request'],
self::INVALID_CODE_CHALLENGE => ['statusCode' => 302, 'name' => 'Invalid code_challenge Parameter', 'error' => 'invalid_request'], self::INVALID_CODE_CHALLENGE => ['statusCode' => 302, 'name' => 'Invalid code_challenge Parameter', 'error' => 'invalid_request'],
self::INVALID_SCOPE => ['statusCode' => 302, 'name' => 'Invalid scope Parameter', 'error' => 'invalid_request'], self::INVALID_SCOPE => ['statusCode' => 302, 'name' => 'Invalid scope Parameter', 'error' => 'invalid_request'],
self::INTERNAL_ERROR_REDIRECT => ['statusCode' => 302, 'name' => 'Internal Server Error', 'error' => 'internal_error'],
]; ];
protected ServerRequestInterface $request; protected ServerRequestInterface $request;

226
src/Server.php
View File

@ -318,12 +318,31 @@ class Server {
$bodyParams = $request->getParsedBody(); $bodyParams = $request->getParsedBody();
// Attempt to internally exchange the provided auth code for an access token.
// We do this before anything else so that the auth code is invalidated as soon as the request starts,
// and the resulting access token is revoked if we encounter an error. This ends up providing a simpler
// and more flexible interface for TokenStorage implementors.
if (array_key_exists('code', $bodyParams)) {
$token = $this->tokenStorage->exchangeAuthCodeForAccessToken($bodyParams['code']);
if (is_null($token)) {
$this->logger->error('Attempting to exchange an auth code for a token resulted in null.', $bodyParams);
return new Response(400, ['content-type' => 'application/json'], json_encode([
'error' => 'invalid_grant',
'error_description' => 'The provided credentials were not valid.'
]));
}
} // The else case is handled by the code below.
// Verify that all required parameters are included. // Verify that all required parameters are included.
$requiredParameters = ['client_id', 'redirect_uri', 'code', 'code_verifier']; $requiredParameters = ['client_id', 'redirect_uri', 'code', 'code_verifier'];
$missingRequiredParameters = array_filter($requiredParameters, function ($p) use ($bodyParams) { $missingRequiredParameters = array_filter($requiredParameters, function ($p) use ($bodyParams) {
return !array_key_exists($p, $bodyParams) || empty($bodyParams[$p]); return !array_key_exists($p, $bodyParams) || empty($bodyParams[$p]);
}); });
if (!empty($missingRequiredParameters)) { if (!empty($missingRequiredParameters)) {
if (isset($token)) {
$this->tokenStorage->revokeAccessToken($token->getKey());
}
$this->logger->warning('The exchange request was missing required parameters. Returning an error response.', ['missing' => $missingRequiredParameters]); $this->logger->warning('The exchange request was missing required parameters. Returning an error response.', ['missing' => $missingRequiredParameters]);
return new Response(400, ['content-type' => 'application/json'], json_encode([ return new Response(400, ['content-type' => 'application/json'], json_encode([
'error' => 'invalid_request', 'error' => 'invalid_request',
@ -331,17 +350,6 @@ class Server {
])); ]));
} }
// Attempt to internally exchange the provided auth code for an access token.
$token = $this->tokenStorage->exchangeAuthCodeForAccessToken($bodyParams['code']);
if (is_null($token)) {
$this->logger->error('Attempting to exchange an auth code for a token resulted in null.', $bodyParams);
return new Response(400, ['content-type' => 'application/json'], json_encode([
'error' => 'invalid_grant',
'error_description' => 'The provided credentials were not valid.'
]));
}
// Verify that it was issued for the same client_id and redirect_uri // Verify that it was issued for the same client_id and redirect_uri
if ($token->getData()['client_id'] !== $bodyParams['client_id'] if ($token->getData()['client_id'] !== $bodyParams['client_id']
|| $token->getData()['redirect_uri'] !== $bodyParams['redirect_uri']) { || $token->getData()['redirect_uri'] !== $bodyParams['redirect_uri']) {
@ -393,36 +401,87 @@ class Server {
// to IndieAuthException if necessary, then passes it to $this->handleException() to be turned into a // to IndieAuthException if necessary, then passes it to $this->handleException() to be turned into a
// response. // response.
try { try {
$queryParams = $request->getQueryParams();
/** @var ResponseInterface|null $clientIdResponse */
/** @var string|null $clientIdEffectiveUrl */
/** @var array|null $clientIdMf2 */
list($clientIdResponse, $clientIdEffectiveUrl, $clientIdMf2) = [null, null, null];
// If this is an authorization or approval request (allowing POST requests as well to accommodate // If this is an authorization or approval request (allowing POST requests as well to accommodate
// approval requests and custom auth form submission. // approval requests and custom auth form submission.
if (isIndieAuthAuthorizationRequest($request, ['get', 'post'])) { if (isIndieAuthAuthorizationRequest($request, ['get', 'post'])) {
$this->logger->info('Handling an authorization request', ['method' => $request->getMethod()]); $this->logger->info('Handling an authorization request', ['method' => $request->getMethod()]);
$queryParams = $request->getQueryParams();
// Return an error if we’re missing required parameters.
$requiredParameters = ['client_id', 'redirect_uri', 'state', 'code_challenge', 'code_challenge_method'];
$missingRequiredParameters = array_filter($requiredParameters, function ($p) use ($queryParams) {
return !array_key_exists($p, $queryParams) || empty($queryParams[$p]);
});
if (!empty($missingRequiredParameters)) {
$this->logger->warning('The authorization request was missing required parameters. Returning an error response.', ['missing' => $missingRequiredParameters]);
// TODO: if the missing parameter isn’t redirect_uri or client_id, this should be a redirect error.
throw IndieAuthException::create(IndieAuthException::REQUEST_MISSING_PARAMETER, $request);
}
// Validate the Client ID. // Validate the Client ID.
if (false === filter_var($queryParams['client_id'], FILTER_VALIDATE_URL) || !isClientIdentifier($queryParams['client_id'])) { if (!isset($queryParams['client_id']) || false === filter_var($queryParams['client_id'], FILTER_VALIDATE_URL) || !isClientIdentifier($queryParams['client_id'])) {
$this->logger->warning("The client_id provided in an authorization request was not valid.", $queryParams); $this->logger->warning("The client_id provided in an authorization request was not valid.", $queryParams);
throw IndieAuthException::create(IndieAuthException::INVALID_CLIENT_ID, $request); throw IndieAuthException::create(IndieAuthException::INVALID_CLIENT_ID, $request);
} }
// Validate the redirect URI — at this stage only superficially, we’ll check it properly later if // Validate the redirect URI.
// things go well. if (!isset($queryParams['redirect_uri']) || false === filter_var($queryParams['redirect_uri'], FILTER_VALIDATE_URL)) {
if (false === filter_var($queryParams['redirect_uri'], FILTER_VALIDATE_URL)) {
$this->logger->warning("The client_id provided in an authorization request was not valid.", $queryParams); $this->logger->warning("The client_id provided in an authorization request was not valid.", $queryParams);
throw IndieAuthException::create(IndieAuthException::INVALID_REDIRECT_URI, $request); throw IndieAuthException::create(IndieAuthException::INVALID_REDIRECT_URI, $request);
} }
// How most errors are handled depends on whether or not the request has a valid redirect_uri. In
// order to know that, we need to also validate, fetch and parse the client_id.
// If the request lacks a hash, or if the provided hash was invalid, perform the validation.
if (!array_key_exists(self::HASH_QUERY_STRING_KEY, $queryParams) || !hash_equals(hashAuthorizationRequestParameters($request, $this->secret), $queryParams[self::HASH_QUERY_STRING_KEY])) {
// All we need to know at this stage is whether the redirect_uri is valid. If it
// sufficiently matches the client_id, we don’t (yet) need to fetch the client_id.
if (!urlComponentsMatch($queryParams['client_id'], $queryParams['redirect_uri'], [PHP_URL_SCHEME, PHP_URL_HOST, PHP_URL_PORT])) {
// If we do need to fetch the client_id, store the response and effective URL in variables
// we defined earlier, so they’re available to the approval request code path, which additionally
// needs to parse client_id for h-app markup.
try {
list($clientIdResponse, $clientIdEffectiveUrl) = call_user_func($this->httpGetWithEffectiveUrl, $queryParams['client_id']);
$clientIdMf2 = Mf2\parse((string) $clientIdResponse->getBody(), $clientIdEffectiveUrl);
} catch (ClientExceptionInterface | RequestExceptionInterface | NetworkExceptionInterface $e) {
$this->logger->error("Caught an HTTP exception while trying to fetch the client_id. Returning an error response.", [
'client_id' => $queryParams['client_id'],
'exception' => $e->__toString()
]);
throw IndieAuthException::create(IndieAuthException::HTTP_EXCEPTION_FETCHING_CLIENT_ID, $request, $e);
} catch (Exception $e) {
$this->logger->error("Caught an unknown exception while trying to fetch the client_id. Returning an error response.", [
'exception' => $e->__toString()
]);
throw IndieAuthException::create(IndieAuthException::INTERNAL_EXCEPTION_FETCHING_CLIENT_ID, $request, $e);
}
// Search for all link@rel=redirect_uri at the client_id.
$clientIdRedirectUris = [];
if (array_key_exists('redirect_uri', $clientIdMf2['rels'])) {
$clientIdRedirectUris = array_merge($clientIdRedirectUris, $clientIdMf2['rels']['redirect_uri']);
}
foreach (HeaderParser::parse($clientIdResponse->getHeader('Link')) as $link) {
if (array_key_exists('rel', $link) && mb_strpos(" {$link['rel']} ", " redirect_uri ") !== false) {
// Strip off the < > which surround the link URL for some reason.
$clientIdRedirectUris[] = substr($link[0], 1, strlen($link[0]) - 2);
}
}
// If the authority of the redirect_uri does not match the client_id, or exactly match one of their redirect URLs, return an error.
if (!in_array($queryParams['redirect_uri'], $clientIdRedirectUris)) {
$this->logger->warning("The provided redirect_uri did not match either the client_id, nor the discovered redirect URIs.", [
'provided_redirect_uri' => $queryParams['redirect_uri'],
'provided_client_id' => $queryParams['client_id'],
'discovered_redirect_uris' => $clientIdRedirectUris
]);
throw IndieAuthException::create(IndieAuthException::INVALID_REDIRECT_URI, $request);
}
}
}
// From now on, we can assume that redirect_uri is valid. Any IndieAuth-related errors should be
// reported by redirecting to redirect_uri with error parameters.
// Validate the state parameter. // Validate the state parameter.
if (!isValidState($queryParams['state'])) { if (!isValidState($queryParams['state'])) {
$this->logger->warning("The state provided in an authorization request was not valid.", $queryParams); $this->logger->warning("The state provided in an authorization request was not valid.", $queryParams);
@ -435,6 +494,10 @@ class Server {
throw IndieAuthException::create(IndieAuthException::INVALID_CODE_CHALLENGE, $request); throw IndieAuthException::create(IndieAuthException::INVALID_CODE_CHALLENGE, $request);
} }
// From now on, any redirect error responses should include the state parameter.
// This is handled automatically in `handleException()` and is only noted here
// for reference.
// Validate the scope parameter, if provided. // Validate the scope parameter, if provided.
if (array_key_exists('scope', $queryParams) && !isValidScope($queryParams['scope'])) { if (array_key_exists('scope', $queryParams) && !isValidScope($queryParams['scope'])) {
$this->logger->warning("The scope provided in an authorization request was not valid.", $queryParams); $this->logger->warning("The scope provided in an authorization request was not valid.", $queryParams);
@ -478,22 +541,12 @@ class Server {
// Authorization approval requests MUST include a hash protecting the sensitive IndieAuth // Authorization approval requests MUST include a hash protecting the sensitive IndieAuth
// authorization request parameters from being changed, e.g. by a malicious script which // authorization request parameters from being changed, e.g. by a malicious script which
// found its way onto the authorization form. // found its way onto the authorization form.
$expectedHash = hashAuthorizationRequestParameters($request, $this->secret);
if (is_null($expectedHash)) {
// In theory this code should never be reached, as we already checked the request for valid parameters.
// However, it’s possible for hashAuthorizationRequestParameters() to return null, and if for whatever
// reason it does, the library should handle that case as elegantly as possible.
// @codeCoverageIgnoreStart
$this->logger->warning("Calculating the expected hash for an authorization approval request failed. This SHOULD NOT happen; if you encounter this error please contact the maintainers of taproot/indieauth.");
throw IndieAuthException::create(IndieAuthException::REQUEST_MISSING_PARAMETER, $request);
// @codeCoverageIgnoreEnd
}
if (!array_key_exists(self::HASH_QUERY_STRING_KEY, $queryParams)) { if (!array_key_exists(self::HASH_QUERY_STRING_KEY, $queryParams)) {
$this->logger->warning("An authorization approval request did not have a " . self::HASH_QUERY_STRING_KEY . " parameter."); $this->logger->warning("An authorization approval request did not have a " . self::HASH_QUERY_STRING_KEY . " parameter.");
throw IndieAuthException::create(IndieAuthException::AUTHORIZATION_APPROVAL_REQUEST_MISSING_HASH, $request); throw IndieAuthException::create(IndieAuthException::AUTHORIZATION_APPROVAL_REQUEST_MISSING_HASH, $request);
} }
$expectedHash = hashAuthorizationRequestParameters($request, $this->secret);
if (!hash_equals($expectedHash, $queryParams[self::HASH_QUERY_STRING_KEY])) { if (!hash_equals($expectedHash, $queryParams[self::HASH_QUERY_STRING_KEY])) {
$this->logger->warning("The hash provided in the URL was invalid!", [ $this->logger->warning("The hash provided in the URL was invalid!", [
'expected' => $expectedHash, 'expected' => $expectedHash,
@ -542,55 +595,32 @@ class Server {
// the spec, errors should only be shown to the user if the client_id and redirect_uri parameters // the spec, errors should only be shown to the user if the client_id and redirect_uri parameters
// are missing or invalid. Otherwise, they should be sent back to the client with an error // are missing or invalid. Otherwise, they should be sent back to the client with an error
// redirect response. // redirect response.
try { if (is_null($clientIdResponse) || is_null($clientIdEffectiveUrl) || is_null($clientIdMf2)) {
/** @var ResponseInterface $clientIdResponse */ try {
list($clientIdResponse, $clientIdEffectiveUrl) = call_user_func($this->httpGetWithEffectiveUrl, $queryParams['client_id']); /** @var ResponseInterface $clientIdResponse */
$clientIdMf2 = Mf2\parse((string) $clientIdResponse->getBody(), $clientIdEffectiveUrl); list($clientIdResponse, $clientIdEffectiveUrl) = call_user_func($this->httpGetWithEffectiveUrl, $queryParams['client_id']);
} catch (ClientExceptionInterface | RequestExceptionInterface | NetworkExceptionInterface $e) { $clientIdMf2 = Mf2\parse((string) $clientIdResponse->getBody(), $clientIdEffectiveUrl);
$this->logger->error("Caught an HTTP exception while trying to fetch the client_id. Returning an error response.", [ } catch (ClientExceptionInterface | RequestExceptionInterface | NetworkExceptionInterface $e) {
'client_id' => $queryParams['client_id'], $this->logger->error("Caught an HTTP exception while trying to fetch the client_id. Returning an error response.", [
'exception' => $e->__toString() 'client_id' => $queryParams['client_id'],
]); 'exception' => $e->__toString()
]);
throw IndieAuthException::create(IndieAuthException::HTTP_EXCEPTION_FETCHING_CLIENT_ID, $request, $e);
} catch (Exception $e) { // At this point in the flow, we’ve already guaranteed that the redirect_uri is valid,
$this->logger->error("Caught an unknown exception while trying to fetch the client_id. Returning an error response.", [ // so in theory we should report these errors by redirecting there.
'exception' => $e->__toString() throw IndieAuthException::create(IndieAuthException::INTERNAL_ERROR_REDIRECT, $request, $e);
]); } catch (Exception $e) {
$this->logger->error("Caught an unknown exception while trying to fetch the client_id. Returning an error response.", [
throw IndieAuthException::create(IndieAuthException::INTERNAL_EXCEPTION_FETCHING_CLIENT_ID, $request, $e); 'exception' => $e->__toString()
]);
throw IndieAuthException::create(IndieAuthException::INTERNAL_ERROR_REDIRECT, $request, $e);
}
} }
// Search for an h-app with u-url matching the client_id. // Search for an h-app with u-url matching the client_id.
$clientHApps = M\findMicroformatsByProperty(M\findMicroformatsByType($clientIdMf2, 'h-app'), 'url', $queryParams['client_id']); $clientHApps = M\findMicroformatsByProperty(M\findMicroformatsByType($clientIdMf2, 'h-app'), 'url', $queryParams['client_id']);
$clientHApp = empty($clientHApps) ? null : $clientHApps[0]; $clientHApp = empty($clientHApps) ? null : $clientHApps[0];
// Search for all link@rel=redirect_uri at the client_id.
$clientIdRedirectUris = [];
if (array_key_exists('redirect_uri', $clientIdMf2['rels'])) {
$clientIdRedirectUris = array_merge($clientIdRedirectUris, $clientIdMf2['rels']['redirect_uri']);
}
foreach (HeaderParser::parse($clientIdResponse->getHeader('Link')) as $link) {
if (array_key_exists('rel', $link) && mb_strpos(" {$link['rel']} ", " redirect_uri ") !== false) {
// Strip off the < > which surround the link URL for some reason.
$clientIdRedirectUris[] = substr($link[0], 1, strlen($link[0]) - 2);
}
}
// If the authority of the redirect_uri does not match the client_id, or exactly match one of their redirect URLs, return an error.
$clientIdMatchesRedirectUri = urlComponentsMatch($queryParams['client_id'], $queryParams['redirect_uri'], [PHP_URL_SCHEME, PHP_URL_HOST, PHP_URL_PORT]);
$redirectUriValid = $clientIdMatchesRedirectUri || in_array($queryParams['redirect_uri'], $clientIdRedirectUris);
if (!$redirectUriValid) {
$this->logger->warning("The provided redirect_uri did not match either the client_id, nor the discovered redirect URIs.", [
'provided_redirect_uri' => $queryParams['redirect_uri'],
'provided_client_id' => $queryParams['client_id'],
'discovered_redirect_uris' => $clientIdRedirectUris
]);
throw IndieAuthException::create(IndieAuthException::INVALID_REDIRECT_URI, $request);
}
// Present the authorization UI. // Present the authorization UI.
return $this->authorizationForm->showForm($request, $authenticationResult, $authenticationRedirect, $clientHApp) return $this->authorizationForm->showForm($request, $authenticationResult, $authenticationRedirect, $clientHApp)
@ -604,6 +634,8 @@ class Server {
if ($nonIndieAuthRequestResult instanceof ResponseInterface) { if ($nonIndieAuthRequestResult instanceof ResponseInterface) {
return $nonIndieAuthRequestResult; return $nonIndieAuthRequestResult;
} else { } else {
// In this code path we have not validated the redirect_uri, so show a regular error page
// rather than returning a redirect error.
throw IndieAuthException::create(IndieAuthException::INTERNAL_ERROR, $request); throw IndieAuthException::create(IndieAuthException::INTERNAL_ERROR, $request);
} }
} catch (IndieAuthException $e) { } catch (IndieAuthException $e) {
@ -641,12 +673,31 @@ class Server {
$bodyParams = $request->getParsedBody(); $bodyParams = $request->getParsedBody();
// Attempt to internally exchange the provided auth code for an access token.
// We do this before anything else so that the auth code is invalidated as soon as the request starts,
// and the resulting access token is revoked if we encounter an error. This ends up providing a simpler
// and more flexible interface for TokenStorage implementors.
if (array_key_exists('code', $bodyParams)) {
$token = $this->tokenStorage->exchangeAuthCodeForAccessToken($bodyParams['code']);
if (is_null($token)) {
$this->logger->error('Attempting to exchange an auth code for a token resulted in null.', $bodyParams);
return new Response(400, ['content-type' => 'application/json'], json_encode([
'error' => 'invalid_grant',
'error_description' => 'The provided credentials were not valid.'
]));
}
}
// Verify that all required parameters are included. // Verify that all required parameters are included.
$requiredParameters = ['client_id', 'redirect_uri', 'code', 'code_verifier']; $requiredParameters = ['client_id', 'redirect_uri', 'code', 'code_verifier'];
$missingRequiredParameters = array_filter($requiredParameters, function ($p) use ($bodyParams) { $missingRequiredParameters = array_filter($requiredParameters, function ($p) use ($bodyParams) {
return !array_key_exists($p, $bodyParams) || empty($bodyParams[$p]); return !array_key_exists($p, $bodyParams) || empty($bodyParams[$p]);
}); });
if (!empty($missingRequiredParameters)) { if (!empty($missingRequiredParameters)) {
if (isset($token)) {
$this->tokenStorage->revokeAccessToken($token->getKey());
}
$this->logger->warning('The exchange request was missing required parameters. Returning an error response.', ['missing' => $missingRequiredParameters]); $this->logger->warning('The exchange request was missing required parameters. Returning an error response.', ['missing' => $missingRequiredParameters]);
return new Response(400, ['content-type' => 'application/json'], json_encode([ return new Response(400, ['content-type' => 'application/json'], json_encode([
'error' => 'invalid_request', 'error' => 'invalid_request',
@ -654,17 +705,6 @@ class Server {
])); ]));
} }
// Attempt to internally exchange the provided auth code for an access token.
$token = $this->tokenStorage->exchangeAuthCodeForAccessToken($bodyParams['code']);
if (is_null($token)) {
$this->logger->error('Attempting to exchange an auth code for a token resulted in null.', $bodyParams);
return new Response(400, ['content-type' => 'application/json'], json_encode([
'error' => 'invalid_grant',
'error_description' => 'The provided credentials were not valid.'
]));
}
// Verify that it was issued for the same client_id and redirect_uri // Verify that it was issued for the same client_id and redirect_uri
if ($token->getData()['client_id'] !== $bodyParams['client_id'] if ($token->getData()['client_id'] !== $bodyParams['client_id']
|| $token->getData()['redirect_uri'] !== $bodyParams['redirect_uri']) { || $token->getData()['redirect_uri'] !== $bodyParams['redirect_uri']) {

9
src/Storage/TokenStorageInterface.php
View File

@ -84,6 +84,15 @@ interface TokenStorageInterface {
* Attempt to exchange an authorization code identified by `$code` for * Attempt to exchange an authorization code identified by `$code` for
* an access token, returning it in a `Token` on success and null on error. * an access token, returning it in a `Token` on success and null on error.
* *
* This method is called at the beginning of a code exchange request, before
* further error checking or validation is applied. On an error, the created
* access token is immediately revoked via `revokeAccessToken()`.
*
* For this reason, the token data in the returned Token object MUST include
* the `client_id` and `redirect_uri` parameters associated with the
* authorization code, as these are used by the IndieAuth Server for further
* validation.
*
* This method is responsible for ensuring that the matched auth code is * This method is responsible for ensuring that the matched auth code is
* not expired. If it is, it should return null, presumably after deleting * not expired. If it is, it should return null, presumably after deleting
* the corresponding authorization code record. * the corresponding authorization code record.

46
tests/ServerTest.php
View File

@ -143,16 +143,6 @@ class ServerTest extends TestCase {
* Authorization Request Tests * Authorization Request Tests
*/ */
public function testAuthorizationRequestMissingParametersReturnsError() {
$s = $this->getDefaultServer();
$req = (new ServerRequest('GET', 'https://example.com/'))->withQueryParams([
'response_type' => 'code' // This param is required to identify the request as an IA authorization request.
]);
$res = $s->handleAuthorizationEndpointRequest($req);
$this->assertEquals((string) IndieAuthException::REQUEST_MISSING_PARAMETER, (string) $res->getBody());
}
public function testAuthorizationRequestWithInvalidClientIdOrRedirectUriShowsErrorToUser() { public function testAuthorizationRequestWithInvalidClientIdOrRedirectUriShowsErrorToUser() {
$testCases = [ $testCases = [
'client_id not a URI' => [ 'client_id not a URI' => [
@ -232,7 +222,7 @@ class ServerTest extends TestCase {
return new Response(200, ['content-type' => 'text/plain'], $expectedResponse); return new Response(200, ['content-type' => 'text/plain'], $expectedResponse);
} }
]); ]);
$res = $s->handleAuthorizationEndpointRequest($this->getIARequest()); $res = $s->handleAuthorizationEndpointRequest($this->getIARequest());
$this->assertEquals(200, $res->getStatusCode()); $this->assertEquals(200, $res->getStatusCode());
@ -246,9 +236,14 @@ class ServerTest extends TestCase {
} }
]); ]);
$res = $s->handleAuthorizationEndpointRequest($this->getIARequest()); $req = $this->getIARequest();
$res = $s->handleAuthorizationEndpointRequest($req);
$this->assertEquals((string) IndieAuthException::AUTHENTICATION_CALLBACK_MISSING_ME_PARAM, (string) $res->getBody()); $this->assertEquals(302, $res->getStatusCode());
$responseLocation = $res->getHeaderLine('Location');
$this->assertTrue(urlComponentsMatch($req->getQueryParams()['redirect_uri'], $responseLocation, [PHP_URL_SCHEME, PHP_URL_HOST, PHP_URL_PATH]));
parse_str(parse_url($responseLocation, PHP_URL_QUERY), $redirectUriQueryParams);
$this->assertEquals('internal_error', $redirectUriQueryParams['error']);
} }
public function testReturnErrorIfFetchingClientIdThrowsException() { public function testReturnErrorIfFetchingClientIdThrowsException() {
@ -268,9 +263,14 @@ class ServerTest extends TestCase {
} }
]); ]);
$res = $s->handleAuthorizationEndpointRequest($this->getIARequest()); $req = $this->getIARequest();
$res = $s->handleAuthorizationEndpointRequest($req);
$this->assertEquals($expectedResponse, (string) $res->getBody()); $this->assertEquals(302, $res->getStatusCode());
$responseLocation = $res->getHeaderLine('Location');
$this->assertTrue(urlComponentsMatch($req->getQueryParams()['redirect_uri'], $responseLocation, [PHP_URL_SCHEME, PHP_URL_HOST, PHP_URL_PATH]));
parse_str(parse_url($responseLocation, PHP_URL_QUERY), $redirectUriQueryParams);
$this->assertEquals('internal_error', $redirectUriQueryParams['error']);
} }
} }
@ -461,9 +461,15 @@ EOT
return ['me' => 'https://example.com']; return ['me' => 'https://example.com'];
} }
]); ]);
$res = $s->handleAuthorizationEndpointRequest($this->getApprovalRequest(true, false));
$req = $this->getApprovalRequest(true, false);
$res = $s->handleAuthorizationEndpointRequest($req);
$this->assertEquals((string) IndieAuthException::AUTHORIZATION_APPROVAL_REQUEST_MISSING_HASH, (string) $res->getBody()); $this->assertEquals(302, $res->getStatusCode());
$responseLocation = $res->getHeaderLine('Location');
$this->assertTrue(urlComponentsMatch($req->getQueryParams()['redirect_uri'], $responseLocation, [PHP_URL_SCHEME, PHP_URL_HOST, PHP_URL_PATH]));
parse_str(parse_url($responseLocation, PHP_URL_QUERY), $redirectUriQueryParams);
$this->assertEquals('internal_error', $redirectUriQueryParams['error']);
} }
public function testReturnsErrorIfApprovalRequestHasInvalidHash() { public function testReturnsErrorIfApprovalRequestHasInvalidHash() {
@ -478,7 +484,11 @@ EOT
])); ]));
$res = $s->handleAuthorizationEndpointRequest($req); $res = $s->handleAuthorizationEndpointRequest($req);
$this->assertEquals((string) IndieAuthException::AUTHORIZATION_APPROVAL_REQUEST_INVALID_HASH, (string) $res->getBody()); $this->assertEquals(302, $res->getStatusCode());
$responseLocation = $res->getHeaderLine('Location');
$this->assertTrue(urlComponentsMatch($req->getQueryParams()['redirect_uri'], $responseLocation, [PHP_URL_SCHEME, PHP_URL_HOST, PHP_URL_PATH]));
parse_str(parse_url($responseLocation, PHP_URL_QUERY), $redirectUriQueryParams);
$this->assertEquals('internal_error', $redirectUriQueryParams['error']);
} }
public function testValidApprovalRequestIsHandledCorrectly() { public function testValidApprovalRequestIsHandledCorrectly() {