Skip to content
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

fosite.Request: Update Request ID in request.Merge() #386

Closed

Conversation

silverspace
Copy link

Updating Request.Merge() to also merge the request ID from the passed in
request, since in all cases where Merge() is called, the receiver object
has an uninitialized ID value. This fixes an issue where
introspectAccessToken() is returning the wrong request ID:

  1. Fosite.IntrospectToken() creates a new ar object with no initialized ID:
    https://github.com/ory/fosite/blob/master/introspect.go#L59
  2. Fosite.IntrospectToken calls CoreValidator.IntrospectToken():
    https://github.com/ory/fosite/blob/master/introspect.go#L61
  3. CoreValidator.IntrospectToken() calls CoreValidator.introspectAccessToken():
    https://github.com/ory/fosite/blob/master/handler/oauth2/introspector.go#L58
  4. introspectAccessToken reads the access token session from storage. The returned or object has the correct RequestID:
    https://github.com/ory/fosite/blob/master/handler/oauth2/introspector.go#L83
  5. introspectAccessToken merges accessRequest with or, but the call to Merge() does not merge the RequestID from or.GetID().
    https://github.com/ory/fosite/blob/master/handler/oauth2/introspector.go#L112
  6. The returned accessRequest object has an uninitialized ID, so the next call to GetID() will return a random UUID.

Steps to reproduce:

  1. Create a new access token for RequestID 'requestId`.
  2. Introspect the token via
tt, ar, err := oauth2Provider.IntrospectToken(...)
  1. Observe that ar.GetID() is not the original requestId value.

Related issue

Proposed changes

Set the RequestID from the passed in request object in Request.Merge():
https://github.com/ory/fosite/blob/master/request.go#L168

Checklist

  • I have read the contributing guidelines
  • I have read the security policy
  • I confirm that this pull request does not address a security vulnerability. If this pull request addresses a security
    vulnerability, I confirm that I got green light (please contact security@ory.sh) from the maintainers to push the changes.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation within the code base (if appropriate)

Further comments

Updating Request.Merge() to also merge the request ID from the passed in
request, since in all cases where Merge() is called, the receiver object
has an uninitialized RequestID value. This fixes an issue where
introspectAccessToken() is returning the wrong request ID, when it
shoudl be returning the value returned by the
storage.AccessTokenSession().
@CLAassistant
Copy link

CLAassistant commented Oct 2, 2019

CLA assistant check
All committers have signed the CLA.

@aeneasr
Copy link
Member

aeneasr commented Oct 3, 2019

This looks good to me! Could you also add a test so this doesn't break in a future patch?

aeneasr added a commit that referenced this pull request Mar 2, 2020
@aeneasr aeneasr closed this in #398 Mar 2, 2020
aeneasr added a commit that referenced this pull request Mar 2, 2020
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.

None yet

3 participants