fix: oauth redirect params legacy browsers support issue - OKTA-337243 #514
Conversation
Codecov Report
@@ Coverage Diff @@
## master #514 +/- ##
==========================================
+ Coverage 91.02% 91.25% +0.22%
==========================================
Files 32 32
Lines 1839 1840 +1
Branches 407 409 +2
==========================================
+ Hits 1674 1679 +5
+ Misses 165 161 -4
Continue to review full report at Codecov.
|
shuowu
changed the title
lib/token.ts
Outdated
| } | ||
| // Add oauth_params to both cookies and sessionStorage for broader support | ||
| cookies.set(REDIRECT_OAUTH_PARAMS_NAME, tokenParamsStr, null, sdk.options.cookies); | ||
| browserStorage.getSessionStorage().setItem(REDIRECT_OAUTH_PARAMS_NAME, tokenParamsStr); |
There was a problem hiding this comment.
do we still need to check for session storage support (will it throw?)
There was a problem hiding this comment.
I saw the code in 3.x, I think it would be great to use the same logic in both version.
|
@shuowu Based on what we are seeing in this issue: okta/okta-oidc-js#898 Do you think it would make sense to use the same fallback logic for PKCE meta? Prefer session storage, but fall back to cookie if session storage is not available for some reason. |
Agree. We should write to all storages, then read the available value from there. Looks like the read order DO NOT really matter. |
It should not matter, but we did have the case where we had to favor cookie over session storage because the widget was writing to cookie but auth JS library was writing to session storage somewhere before the callback was handled (this was a concurrency issue if I recall correctly). Logically, we should favor session storage over cookie since only the cookie would be affected by cross-tab concurrency, but we may also want to favor whichever option is currently being used by SIW for maximum backward compatibility. If we are writing to both cookie and session storage, then any concurrency in the authJS library will overwrite whatever was set by the SIW. Unfortunately this is something we still have to deal with and we don't have a perfect solution yet. |
|
The problem we had before was that we were only reading from session storage: https://github.com/okta/okta-auth-js/pull/491/files Reading from the cookie first does help avoid a same-tab concurrency issue, but only because the SIW is only setting the cookie and authJS is only setting session storage. Once the SIW and authJS are setting both values, it should not matter, it will break on same-tab concurrency either way. and we should actually favor session storage to avoid cross-tab concurrency issue when a browser is reloading multiple tabs at the same time. |
lib/token.ts
Outdated
| oauthParamsStr = browserStorage.getSessionStorage().getItem(REDIRECT_OAUTH_PARAMS_NAME); | ||
| } | ||
| if (!oauthParamsStr) { | ||
| // fallback to cookies to support legacy browsers, eg: IE/Edge, iphone 8 |
There was a problem hiding this comment.
The iOS issue is actually the other way, the cookies will fail because of of sameSite logic which is incompatible with the logic in iOS 13
There was a problem hiding this comment.
Because the logic was
if (browserHasSessionStorage()) {
// read from session storage
} else {
// read from cookie
}
It was the else that caused the break. Session storage is enabled, but the value was in the cookie. We decided to favor cookie over session storage in that fix to also overcome same-tab concurrency (since authJS was only writing to sessionStorage), however multi-tab concurrency still exists.
With authJS writing to both values, there will be a potential single-tab concurrency issue, but we are fighting this in other ways. I do think it is correct to favor the value read from session storage first, to better defend against multi-tab concurrency.
test/spec/token.js
Outdated
| let sdkMock; | ||
| let setCookieMock; | ||
| let sessionStorageSetItemMock; | ||
| let hasSessionStorage; |
There was a problem hiding this comment.
All local variables defined here should be reset in the beforeEach block. hasSessionStorage is not being cleared/reset in the beforeEach method, so the value from the previous test will remain unless it is set in the new test. This pattern can cause "flaky" tests since the behavior can vary based on what order the tests are run in, or if tests are run with other tests. But since this variable is a boolean it is difficult to reset.
In this case, I think it would be better to have a function like mockSessionStorage(enabled: boolean) which can be called by the test.
Also need to cherry-pick this change to 3.2.x