Skip to content

Commit

Permalink
Exclude + from state tokens
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
pbrisbin committed Jan 13, 2021
1 parent 424b71d commit 73a7e98
Showing 1 changed file with 21 additions and 4 deletions.
25 changes: 21 additions & 4 deletions src/Yesod/Auth/OAuth2/Dispatch.hs
Original file line number Diff line number Diff line change
Expand Up @@ -156,14 +156,25 @@ withCallbackAndState name oauth2 csrf = do
getParentUrlRender :: MonadHandler m => m (Route (SubHandlerSite m) -> Text)
getParentUrlRender = (.) <$> getUrlRender <*> getRouteToParent

-- | Set a random, 30-character value in the session
-- | Set a random, ~30-character value in the session
--
-- Some (but not all) providers decode a @+@ in the state token as a space when
-- sending it back to us. We don't expect this and fail. And if we did code for
-- it, we'd then fail on the providers that /don't/ do that.
--
-- Therefore, we just exclude @+@ in our tokens, which means this function may
-- return slightly less than 30 characters.
--
setSessionCSRF :: MonadHandler m => Text -> m Text
setSessionCSRF sessionKey = do
csrfToken <- liftIO randomToken
csrfToken <$ setSession sessionKey csrfToken
where
randomToken =
decodeUtf8 . convertToBase @ByteString Base64 <$> getRandomBytes 64
T.filter (/= '+')
. decodeUtf8
. convertToBase @ByteString Base64
<$> getRandomBytes 64

-- | Verify the callback provided the same CSRF token as in our session
verifySessionCSRF :: MonadHandler m => Text -> m Text
Expand All @@ -172,8 +183,14 @@ verifySessionCSRF sessionKey = do
sessionToken <- lookupSession sessionKey
deleteSession sessionKey

unless (sessionToken == Just token)
$ permissionDenied "Invalid OAuth2 state token"
unless (sessionToken == Just token) $ do
$(logError)
$ "state token does not match. "
<> "Param: "
<> tshow token
<> "State: "
<> tshow sessionToken
permissionDenied "Invalid OAuth2 state token"

return token

Expand Down

0 comments on commit 73a7e98

Please sign in to comment.