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

[SDK-4670] Improved state handling errors #140

Merged
merged 7 commits into from
Dec 18, 2023

Conversation

jimmyjames
Copy link
Contributor

@jimmyjames jimmyjames commented Dec 13, 2023

Changes

This PR provides additional information to the error message received when a state validation error occurs. To minimize any potential breaking changes, it:

  • Does not introduce any new exception classes
  • Does not modify the existing exception messaging (in case clients pattern match on the message), but does add additional messaging to indicate the source of the error (missing auth state param, missing state value, or state mismatch between session/cookie value and auth response param)

Note that the SDK currently needs to support the deprecated methods that use the session for auth state param storage (both for building the auth URL as well as processing the callback). In v2, we should:

  • Remove deprecated methods that store auth params in the session
  • Provide specific exception types for various state validation errors (e.g., state param missing from the auth response, state cookie not present, or state value mismatch), as well as provided specific and improved exception messages. See the nextjs-auth0 SDK for an example.

Note that this change also includes additional tests for functionality that was not changed, in addition to tests for the changes. The first commit adds tests to cover all paths prior to the change, as well as updates tests to the new expected behavor. This commit provides the functional change.

@jimmyjames jimmyjames requested a review from a team as a code owner December 13, 2023 03:28
@evansims
Copy link
Member

Failures look OK. Just a dupe Snyk webhook causing that one failure; fixed that. And Codecov is weird, as usual.

evansims
evansims previously approved these changes Dec 13, 2023
poovamraj
poovamraj previously approved these changes Dec 13, 2023
@jimmyjames jimmyjames closed this Dec 15, 2023
@jimmyjames jimmyjames reopened this Dec 15, 2023
@jimmyjames jimmyjames dismissed stale reviews from poovamraj and evansims via f598974 December 15, 2023 16:21
@jimmyjames
Copy link
Contributor Author

Codecov was failing because there was a check in RandomStorage#checkSessionState for a null session value but non-null request state value. This is now being handled in RequestProcessor#assertValidState so there was a drop in coverage. Rather than updating checkSessionState, I've kept that logic but added a unit test for that specific case, just to ensure the changes are minimized. Codecov is now fixed.

@evansims I see the security/snyk (auth0-sdks) check is still expected and not reporting (security/snyk (DX) is passing). Did the webhook change you make intend to remove the duplicated check, security/snyk (auth0-sdks)?

@jimmyjames jimmyjames merged commit fbfe27e into master Dec 18, 2023
7 checks passed
@jimmyjames jimmyjames deleted the improved-state-handling-errors branch December 18, 2023 16:01
@jimmyjames jimmyjames mentioned this pull request Dec 19, 2023
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.

3 participants