-
Notifications
You must be signed in to change notification settings - Fork 356
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
[CORL-1048] Cookie Deprecation #2944
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
works!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Putting a merge block for now, needs discussion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionlist from discussion with @wyattjoh :
- Restore Relay Storage as source of truth
- Remove
Auth
,JWTManager
,cleanupCallbacks
and simplify into functions (or a class if it really needs to^^) - clear session storage during
clearSession
- No need to deal with about-to-expire-tokens for now
- Remove automatic pause/resume in
createManagedSubscriptionClient
6e49a6a
to
40d6e24
Compare
src/core/client/framework/lib/network/createManagedSubscriptionClient.ts
Show resolved
Hide resolved
// Return the parsed access token. | ||
return parseAccessToken(accessToken); | ||
} catch (err) { | ||
if (process.env.NODE_ENV !== "production") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a question: What harm is there to output these errors in production?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a users perspective, it's not likely that this would be used to debug issues. If we're concerned about tracking these errors, we should consider these types of guards as indicators to include error reporters.
What does this PR do?
New third party cookie policies created by Safari in their recent 13.1 release have been causing issues for organizations using Coral with our non-SSO based (email/password) login methods.
This is closely related to the issue described privacycg/storage-access#2 (comment). Our hopes is that the Storage Access API takes these issues into account during the development to provide guidelines for first-party SaaS social integrations while maintaining the security boundary of the iframe.
Other comment platforms render directly into the DOM of the top level frame which provide inferior security boundaries when compared to the isolation granted by the iframe.
This PR removes cookies entirely as they are transparent to the JS and therefore causes issues described in the below scenario. This PR instead tries to save access token details to
localStorage
whenever possible to ensure admin logins persist after login.Problem Scenario
organization.com
, where Coral is embedded fromorganization.coral.coralproject.net
.postMessage
to the iframe, and the user can proceed logged in.organization.com
), and scrolls back to the comment section.Because the user navigated, the storage access is removed, and the iframe no longer has storage access, and therefore does not show that the user is signed in. The issue we're experiencing now is when the user again clicks "Sign In":
Because we don't have access to the access token after the refresh (because of the
HttpOnly
flag on the cookie), we can't send it back to the iframe after the second popup. This means that the user is stuck, the iframe does not have permission to access or send cookies along with the request, and we don't want to expose the cookie to the JS.What changes to the GraphQL/Database Schema does this PR introduce?
None.
How do I test this PR?
Try to use in Safari where the Coral domain differs from the embedder domain.