-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
fix(auth): Sync CSRF token on form submit for multi-tab scenarios #107389
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -41,10 +41,9 @@ | |
| {{ block.super }} | ||
| {% script %} | ||
| <script> | ||
| // HACK(jferge+claude): inline getCookie function from our utils, | ||
| // update the csrf token value in a rudimentary way (timer based) | ||
| // on auth forms to help out when users have multiple sentry tabs | ||
| // and are logged out | ||
| // HACK(jferge+claude): Sync CSRF tokens from cookie to form fields for multi-tab scenarios. | ||
| // When users have multiple auth tabs open and one tab logs in (rotating the CSRF token), | ||
| // other tabs need to pick up the new token before form submission. | ||
| (function() { | ||
| function getCookie(name) { | ||
| if (document.cookie && document.cookie !== '') { | ||
|
|
@@ -59,7 +58,7 @@ | |
| return null; | ||
| } | ||
|
|
||
| setInterval(function() { | ||
| function syncCsrfTokens() { | ||
| var csrfCookieVal = getCookie(window.csrfCookieName || 'sc'); | ||
| // combine strings for csrf name as some tests grep on the token name :( | ||
| var csrfName = 'csrf' + 'middleware' + 'token'; | ||
|
|
@@ -69,7 +68,19 @@ | |
| csrfEls[i].value = csrfCookieVal; | ||
| } | ||
| } | ||
| }, 200); | ||
| } | ||
|
|
||
| // Periodic sync for visual consistency (user sees correct token in DevTools) | ||
| setInterval(syncCsrfTokens, 200); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can probably remove the setInterval after deployment of this PR and confirming dropoff of remaining errors |
||
|
|
||
| // Sync on form submit to guarantee fresh token even if submit happens | ||
| // within the 200ms polling window. Capture phase ensures this runs | ||
| // before the form's default submit action. | ||
|
Comment on lines
+76
to
+78
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a fast race, 200ms things get out of whack? Is that because of many requests, and the order is the problem? or a few requests that are quick i wonder. Either way, the change in this file can't be worse.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. monitoring here. they still happen a decent amount, even after my change, and some of the logs do look like it's folks with multiple tabs quickly (albiet not 200ms quick) logging in successively. |
||
| document.addEventListener('submit', function(e) { | ||
| if (e.target && e.target.tagName === 'FORM') { | ||
| syncCsrfTokens(); | ||
| } | ||
| }, true); | ||
| })(); | ||
| </script> | ||
| {% endscript %} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -109,10 +109,19 @@ export function WebAuthnAssert({ | |
| // submitted once the response is set. | ||
| const shouldSubmitForm = !onWebAuthn && response !== null; | ||
|
|
||
| useEffect( | ||
| () => void (shouldSubmitForm && inputRef.current?.form?.submit()), | ||
| [shouldSubmitForm] | ||
| ); | ||
| useEffect(() => { | ||
| if (shouldSubmitForm && inputRef.current?.form) { | ||
| const form = inputRef.current.form; | ||
| // Use requestSubmit() to fire the 'submit' event, allowing the global | ||
| // CSRF sync listener in auth.html to update the token for multi-tab scenarios. | ||
| // Falls back to submit() for Safari 15 (requestSubmit added in Safari 16). | ||
| if (form.requestSubmit) { | ||
| form.requestSubmit(); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i've checked and i don't believe this change should cause any problems w/ respect to validation or event listeners.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (also validated that this works locally)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. matches the example: https://developer.mozilla.org/en-US/docs/Web/API/HTMLFormElement/requestSubmit#examples 👍 |
||
| } else { | ||
| form.submit(); | ||
| } | ||
cursor[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| }, [shouldSubmitForm]); | ||
|
|
||
| // Trigger the webAuthn flow immediately | ||
| useEffect(() => { | ||
|
|
||
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.
i should remove this comment, not helpful.