Skip to content

Only require 'redirect_uri' in token/authorization_code when one was provided in authorize/code #709

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

scagood
Copy link

@scagood scagood commented Aug 21, 2021

This PR is to prevent redirect_uri being saved using saveAuthorizationCode when its not directly requested, this is so that AuthorizationCodeGrantType#validateRedirectUri does not try to validate against the fallback of client.redirectUris[0]

This is the comment that made me think this makes sense:
https://github.com/oauthjs/node-oauth2-server/blob/master/lib/grant-types/authorization-code-grant-type.js#L125-L134

@scagood
Copy link
Author

scagood commented Aug 21, 2021

For reference this is an example of what I think is incorrect:

First request a token:

[~/github/saml-oauth2 (master)] $ curl -v 'http://localhost:8080/oauth/authorize?client_id=123&response_type=code&scope=read&state=31232' 
> GET /oauth/authorize?client_id=123&response_type=code&scope=read&state=31232 HTTP/1.1
> Host: localhost:8080
> User-Agent: curl/7.68.0
> Accept: */*
> 
< HTTP/1.1 302 Found
< X-Powered-By: Express
< location: http://localhost:8080/success?code=7abc490fda5133d7d401b41e911ad1736e8ae1fe&state=31232
< Content-Type: application/json; charset=utf-8
< Content-Length: 2
< ETag: W/"2-vyGp6PvFo4RvsFtPoIWeCReyIC8"
< Date: Sat, 21 Aug 2021 02:25:51 GMT
< Connection: keep-alive
< Keep-Alive: timeout=5
< 
{}

The request that I think is incorrect because I never sent a redirect_uri

[~/github/saml-oauth2 (master)] $ curl --request POST --url 'http://localhost:8080/oauth/token' \
  --header 'content-type: application/x-www-form-urlencoded' \
  --data grant_type=authorization_code \
  --data client_id=123 \
  --data scope='read write' \
  --data code=7abc490fda5133d7d401b41e911ad1736e8ae1fe
invalid_request: Invalid request: `redirect_uri` is not a valid URI
    at new InvalidRequest (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/lib/errors/invalid-request-error.js:26:14)
    at AuthorizationCodeGrantType.validateRedirectUri (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/lib/grant-types/authorization-code-grant-type.js:144:12)
    at AuthorizationCodeGrantType.<anonymous> (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/lib/grant-types/authorization-code-grant-type.js:69:19)
    at PassThroughHandlerContext.finallyHandler (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/node_modules/bluebird/js/release/finally.js:57:23)
    at PassThroughHandlerContext.tryCatcher (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/node_modules/bluebird/js/release/promise.js:547:31)
    at Promise._settlePromise (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/node_modules/bluebird/js/release/promise.js:604:18)
    at Promise._settlePromise0 (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/node_modules/bluebird/js/release/promise.js:649:10)
    at Promise._settlePromises (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/node_modules/bluebird/js/release/promise.js:729:18)
    at _drainQueueStep (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/node_modules/bluebird/js/release/async.js:93:12)
    at _drainQueue (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/node_modules/bluebird/js/release/async.js:86:9)
    at Async._drainQueues (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/node_modules/bluebird/js/release/async.js:102:5)
    at Immediate.Async.drainQueues [as _onImmediate] (/home/scagood/github/saml-oauth2/node_modules/oauth2-server/node_modules/bluebird/js/release/async.js:15:14)
    at processImmediate (node:internal/timers:464:21)

The request that actually works as it contains redirect_uri:

[~/github/saml-oauth2 (master)] $ curl --request POST --url 'http://localhost:8080/oauth/token' \
  --header 'content-type: application/x-www-form-urlencoded' \
  --data grant_type=authorization_code \
  --data client_id=123 \
  --data scope='read write' \
  --data code=7abc490fda5133d7d401b41e911ad1736e8ae1fe \
  --data redirect_uri=http://localhost:8080/success
{
  "access_token": "5c67a9a07a2b8c6d18bbe592e7f5f215a57e4b1e",
  "token_type": "Bearer",
  "expires_in": 3599,
  "refresh_token": "acd822f2c8b2f3bbfd2baf9ef7131a84359e4fc9",
  "scope": "read",
  "_id": "612064638b41291f191d2c33",
  "clientId": "123",
  "userId": null,
  "expiresAt": "2021-09-04T02:26:43.252Z"
}

@whatwewant
Copy link

Maybe you are wrong.
You should read RFC 6749 The OAuth 2.0 Authorization Framework - Authorization Code Grant

And check your model on getClient and

if (redirectUri && !_.includes(client.redirectUris, redirectUri)) {

@scagood
Copy link
Author

scagood commented Sep 6, 2021

@whatwewant I referenced the relevent comment to the relevent part of the spec in my description.

To quote:
[redirect_uri] REQUIRED, if the "redirect_uri" parameter was included in the authorization request

The redirect_uri is only REQUIRED IF it was included in the authorization request, not if it is in the client.

Full quote:

redirect_uri
         REQUIRED, if the "redirect_uri" parameter was included in the
         authorization request as described in Section 4.1.1, and their
         values MUST be identical.

https://tools.ietf.org/html/rfc6749#section-4.1.3

@HappyZombies
Copy link

Hello, due to this project appearing to be dead and no maintainers responding, I went ahead and forked the project under a new organization, and will continue the work over there. https://github.com/node-oauth/node-oauth2-server

Feel free to move over there to further the discussion

@scagood scagood closed this by deleting the head repository Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants