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

disableNonceCheck always leads to an error #1210

Closed
ssaip opened this issue Mar 14, 2022 · 5 comments
Closed

disableNonceCheck always leads to an error #1210

ssaip opened this issue Mar 14, 2022 · 5 comments
Labels
bug For tagging faulty or unexpected behavior.

Comments

@ssaip
Copy link
Contributor

ssaip commented Mar 14, 2022

on version 13.0.1

Current behavior
Passing { disableNonceCheck: true } to tryLoginCodeFlow (or other calling methods) will always result in Promise.reject()

The method is as following:

if (!options.disableNonceCheck) { ... }
return Promise.reject();

therefore, it will always result in an error.

Expected behavior
code should probably be something like:

        if (!options.disableNonceCheck) {
            if (!nonceInState) {
                this.saveRequestedRoute();
                return Promise.resolve();
            }
            if (!options.disableOAuth2StateCheck) {
                const success = this.validateNonce(nonceInState);
                if (!success) {
                    const event = new OAuthErrorEvent('invalid_nonce_in_state', null);
                    this.eventsSubject.next(event);
                    return Promise.reject(event);
                }
            }
        }
        this.storeSessionState(sessionState);
        if (code) {
            await this.getTokenFromCode(code, options);
            this.restoreRequestedRoute();
            return Promise.resolve();
        }
        else {
            return Promise.resolve();
        }
ssaip added a commit to ssaip/angular-oauth2-oidc that referenced this issue Mar 14, 2022
@jeroenheijmans jeroenheijmans added bug For tagging faulty or unexpected behavior. investigation-needed Indication that the maintainer or involved community members may need to investigate more. labels Mar 14, 2022
@thierryve
Copy link

Running into the same problem. @ssaip do you have any workaround for this issue?

@ssaip
Copy link
Contributor Author

ssaip commented Apr 23, 2022

Running into the same problem. @ssaip do you have any workaround for this issue?

We forked the project, fixed it there and build it ourselves.

There's also an open pull request with the fix: #1211

@youett
Copy link

youett commented Aug 3, 2022

@jeroenheijmans any update on this? I think it is a clear bug because setting "disableNonceCheck" will skip the session-state-storage and code-to-token-exchange and always Promise.reject

@jeroenheijmans
Copy link
Collaborator

Nope, no update from me personally. I currently mostly moderate issues and answer questions when I can do so from my knowledge.

Judging (at a glance) based on the above comments, folks have found fixes and workarounds, so at first sight I can remove the "needs investigation" tag to label things a bit more appropriately.

Note that in general the library's development is not super active, so rolling with a fork that has a fix might be an option depending on your case.

Hope that helps!

@jeroenheijmans jeroenheijmans removed the investigation-needed Indication that the maintainer or involved community members may need to investigate more. label Aug 3, 2022
manfredsteyer added a commit that referenced this issue Nov 18, 2022
disableNonceCheck always leads to an error #1210
@manfredsteyer
Copy link
Owner

Thanks for pointing this out. will be fixed in the next version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug For tagging faulty or unexpected behavior.
Projects
None yet
Development

No branches or pull requests

5 participants