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

handleRedirectCallback still requires nonce #807

Closed
hle-skillz opened this issue Sep 29, 2021 · 3 comments
Closed

handleRedirectCallback still requires nonce #807

hle-skillz opened this issue Sep 29, 2021 · 3 comments
Labels
bug report This issue reports a suspect bug or issue with the SDK itself

Comments

@hle-skillz
Copy link

hle-skillz commented Sep 29, 2021

Describe the problem

#678 attempts to make the nonce optional as it is possibly redundant when a PKCE code_verififer is present. However, the nonce is still checked if it's present in the TransactionManager:

const decodedToken = await this._verifyIdToken(
authResult.id_token,
transaction.nonce,
transaction.organizationId
);

and the authorize flow unconditionally generates the PKCE AND the nonce:

public async buildAuthorizeUrl(
options: RedirectLoginOptions = {}
): Promise<string> {
const { redirect_uri, appState, ...authorizeOptions } = options;
const stateIn = encode(createRandomString());
const nonceIn = encode(createRandomString());
const code_verifier = createRandomString();

this.transactionManager.create({
nonce: nonceIn,

This prevents operability with a PKCE-compliant issuer like Doorkeeper without also bringing in OpenID Connect.

Reproduction

Use any issuer that does not provide OpenID Connect.

Environment

  • Version of auth0-spa-js used: 1.18.0
  • Which browsers have you tested in? Chrome
  • Which framework are you using, if applicable (Angular, React, etc): React
  • Other modules/plugins/libraries that might be involved: Doorkeeper
@hle-skillz hle-skillz added the bug report This issue reports a suspect bug or issue with the SDK itself label Sep 29, 2021
@stevehobbsdev
Copy link
Contributor

Hi @hle-skillz,

The SDK makes no claim to work with just any authentication provider and as the name suggests, is mostly designed to work with Auth0. Also, #678 is not an attempt to make nonce optional, it was just removing a check that was not necessary and causing issues in some circumstances. As you say, the SDK continues to always generate a nonce, and is validated when it returns in the ID token.

Right now we don't have plans to remove the check as we expect Auth0 as the IdP but we also enforce the OpenID Connect flow. Even if we removed the nonce check, there are a few other things here that may prevent another IDP from being used successfully, such as our non-standard logout endpoint.

Will close for now but happy to discuss further.

@hle-skillz
Copy link
Author

@stevehobbsdev any chance we could get a backwards-compatible prop / option useNonce=true that can be overridden for use with a non-Auth0 IdP?

@stevehobbsdev
Copy link
Contributor

As long as the primary use case is to support another IdP, it's highly unlikely to be prioritised. As it is at the moment, it's causing no issues for Auth0 users and at best is a potentially redundant but innocuous check.

As I said though, even without this check you may run into issues depending on the IdP being used, and we'd likely need to add other affordances to work better with other IdPs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug report This issue reports a suspect bug or issue with the SDK itself
Projects
None yet
Development

No branches or pull requests

2 participants