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

fix: clear PKCE meta before save. favor localSession when reading meta. #441

Merged
merged 2 commits into from
Jul 31, 2020

Conversation

aarongranick-okta
Copy link
Contributor

This PR fixes an issue involving PKCE metadata:

  • current version of the widget stores PKCE code in localStorage
  • current version of authJS looks for code in sessionStorage, but falls back to localStorage if code is not present
  • token renew failure does not cleanup but leaves a code in sessionStorage. token renew failure can occur if the session is expired OR if 3rd party cookies are blocked
  • the error occurs because the code from token renew (in sessionStorage) is passed to the /token endpoint instead of the code saved from the widget (in localStorage)

This PR fixes the issue by favoring localStorage over sessionStorage in loadMeta. If meta exists in both locations, we will use the value in localStorage because it is assumed the meta was put there by an older version of the signin widget, and the data in sessionStorage may be left over from a failed token call that did not clean up after itself.

Additionally, we add extra protections:

  • Adding catch block on getToken to clear PKCE storage. This is where the meta was being left behind by failed token renew
  • Before saving, storage is read for existing values. If meta is found in either location a warning message is printed. This may help developers detect a concurrency issue and provide better bug reports
  • All PKCE storage is cleared before saving, in both local and session storage. This should prevent the wrong code from being sent to a token call. For concurrency issues, we will see an error that the PKCE code is not found in storage.

This PR also adds a "login using signin widget" option to test app. This was used to manually reproduce the issue.

@aarongranick-okta aarongranick-okta merged commit 929bb27 into master Jul 31, 2020
@aarongranick-okta aarongranick-okta deleted the ag-pkce-storage-OKTA-318184 branch July 31, 2020 23:53
aarongranick-okta added a commit that referenced this pull request Sep 17, 2020
…a. (#441)

* fix: clear PKCE meta before save. favor localSession when reading meta.

- Adds signin widget to test app for manual testing of compatibility
aarongranick-okta added a commit that referenced this pull request Sep 18, 2020
…a. (#441)

* fix: clear PKCE meta before save. favor localSession when reading meta.

- Adds signin widget to test app for manual testing of compatibility
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.

2 participants