openid / AppAuth-JS

JavaScript client SDK for communicating with OAuth 2.0 and OpenID Connect providers.
Apache License 2.0
983 stars 161 forks source link

RedirectRequestHandler using location.hash for the auth code #137

Closed matt-allan closed 4 years ago

matt-allan commented 4 years ago

Expected Behavior

I expected the RedirectRequestHandler to either default to checking location.search instead of location.hash or offer a config option.

[REQUIRED] Describe expected behavior

Describe the problem

The RedirectRequestHandler looks for the code and state parameters in the hash instead of the query string.

Since my OAuth server (Laravel Passport) returns the code and state parameters in the query string the authorization request never completes. I've read the existing issues about this (#114, #92, #98) and I was able to override the BasicQueryStringUtils which fixed the issue.

The reason I'm opening this issue is it isn't clear to me why the RedirectRequestHandler defaults to hash. The OAuth 2.0 spec states:

If the resource owner grants the access request, the authorization server issues an authorization code and delivers it to the client by adding the following parameters to the query component of the redirection URI using the "application/x-www-form-urlencoded" format, per Appendix B: .... For example, the authorization server redirects the user-agent by sending the following HTTP response:

     HTTP/1.1 302 Found
     Location: https://client.example.com/cb?code=SplxlOBeZQQYbYS6WxSbIA
               &state=xyz

If I'm understanding this correctly the OAuth server is confirming to the spec but AppAuth is not.

I realize that the implicit grant uses a hash instead of a query string but this code is looking for a code parameter which only exists with the auth code grant, not the implicit grant.

[REQUIRED] Actual Behavior

The authorization request fails because the code and state parameters are never found. The Potential authorization request log line shows undefined for queryParams, state, and code.

[REQUIRED] Steps to reproduce the behavior

import { RedirectRequestHandler } from '@openid/appauth/built/redirect_based_handler'
import { AuthorizationNotifier } from '@openid/appauth/built/authorization_request_handler'
import { BasicQueryStringUtils } from '@openid/appauth/built/query_string_utils'

const notifier = new AuthorizationNotifier()
notifier.setAuthorizationListener((request, response, error) => {
  console.log('Authorization request complete ', request, response, error)
  if (response) {
    this.code = response.code
    alert(`Authorization Code ${response.code}`)
  }
})

const authorizationHandler = new RedirectRequestHandler()

authorizationHandler.setAuthorizationNotifier(notifier)
authorizationHandler.completeAuthorizationRequestIfPossible()

[REQUIRED] Environment

tikurahul commented 4 years ago

Unfortunately query strings tend to up in Referer headers and as a result in a lot of analytics backends. Hash does not. Therefore we prefer hashes to query strings.

matt-allan commented 4 years ago

Ah interesting, thanks for the info.