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

Avoid + in state token, to fix ClassLink #140

Merged
merged 3 commits into from
Jan 14, 2021
Merged

Avoid + in state token, to fix ClassLink #140

merged 3 commits into from
Jan 14, 2021

Conversation

pbrisbin
Copy link
Member

@pbrisbin pbrisbin commented Jan 5, 2021

When the state token is sent to an OAuth2 provider, it undergoes
%-encoding as a URL parameter. Presumably, the OAuth2 provider decodes
it as part of handling things (because it would take work to prevent
their own web frameworks from doing so), and then re-%-encodes it coming
back to us again as a callback parameter.

For us, and all existing providers, + is not a %-encoded character, so
it's sent as-is and sent back as-is. So far so good.

ClassLink, though, chooses to decode + to space. I'm not aware of the
actual spec or if this is a reasonable thing to do, but they do. This
results in them sending %20 back to us, which doesn't match and we fail.

We can't predict or prescribe what providers do in this area, so our
options are:

  • Look for a match in our Session as-is OR with spaces replaced by +

    This is harder than it sounds: a token could contain +'s or spaces,
    and we'd be getting back only spaces. To succeed, we'd actually have
    to check every permutation of space/+ substitution.

  • Filter + from our tokens

    The only downside is we may generate slightly fewer than 30
    characters, and so produce slightly less secure tokens.

    I chose this option.

  • Generate tokens without + to begin with

    This would be ideal, but I'm just not familiar enough with
    Crypto.Random. I would happily accept a PR to use this option.

@pbrisbin pbrisbin changed the title WIP: ClassLink plugin Avoid + in state token, to fix ClassLink Jan 13, 2021
When the state token is sent to an OAuth2 provider, it undergoes
%-encoding as a URL parameter. Presumably, the OAuth2 provider decodes
it as part of handling things (because it would take work to prevent
their own web frameworks from doing so), and then re-%-encodes it coming
back to us again as a callback parameter.

For us, and all existing providers, + is not a %-encoded character, so
it's sent as-is and sent back as-is. So far so good.

ClassLink, though, chooses to decode + to space. I'm not aware of the
actual spec or if this is a reasonable thing to do, but they do. This
results in them sending %20 back to us, which doesn't match and we fail.

We can't predict or prescribe what providers do in this area, so our
options are:

- Look for a match in our Session as-is OR with spaces replaced by +

  This is harder than it sounds: a token could contain +'s or spaces,
  and we'd be getting back only spaces. To succeed, we'd actually have
  to check every permutation of space/+ substitution.

- Filter + from our tokens

  The only downside is we may generate slightly fewer than 30
  characters, and so produce slightly less secure tokens.

  I chose this option.

- Generate tokens without + to begin with

  This would be ideal, but I'm just not familiar enough with
  Crypto.Random. I would happily accept a PR to use this option.
@pbrisbin pbrisbin merged commit a09528a into master Jan 14, 2021
@pbrisbin pbrisbin deleted the pb/classlink branch January 14, 2021 15:21
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.

1 participant