Skip to content

Conversation

pory-gone
Copy link
Contributor

@pory-gone pory-gone commented Sep 21, 2025

Description

fix #2366

Fixed race condition:

  • Added Apollo Client context link that strips authentication headers from requests when window.logoutInProgress flag is true
  • Set logout flag immediately before logout/account switch operations to invalidate auth context for pending requests
  • Created reusable clearAuthCookies() helper function in auth.js using existing cookie constants
  • Added immediate client-side cookie invalidation during logout process using centralized cookie management

Screenshots

NaN

Additional Context

Uses Apollo Client's setContext link for clean request interception, clearAuthCookies() function can be reused for other logout scenarios

Checklist

Are your changes backward compatible? Please answer below:
Yes

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below:
8/10

For frontend changes: Tested on mobile, light and dark mode? Please answer below:
NaN

Did you introduce any new environment variables? If so, call them out explicitly here:
NaN

Did you use AI for this? If so, how much did it assist you?
I used AI to locate interested files and to test the implementations

@pory-gone pory-gone marked this pull request as ready for review September 21, 2025 13:54
Copy link
Member

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you test this?

It does not work, see video:

2025-09-21.16-32-03.mp4

I will add this video to the issue because this is the same test I did to demonstrate that #2406 (review) does not work.

I also added a new possible hint how to fix this in the issue.

lib/auth.js Outdated
Comment on lines 38 to 43
export const clearAuthCookies = () => {
const expiredDate = 'Thu, 01 Jan 1970 00:00:00 GMT'
document.cookie = `${SESSION_COOKIE}=; path=/; expires=${expiredDate}`
document.cookie = `${MULTI_AUTH_POINTER}=; path=/; expires=${expiredDate}`
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think JS can clear httpOnly cookies in production

lib/apollo.js Outdated
function getClient (uri) {
const authContextLink = setContext((_, { headers }) => {
if (SSR) return { headers }
if (typeof window !== 'undefined' && window.logoutInProgress) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

headers is always undefined here

Comment on lines 309 to 310
window.logoutInProgress = true
clearAuthCookies()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might not hit this line during logout because we get switched to another account, see code above

the code above is also prone to the race condition

// prevent navigation
e.preventDefault()

if (typeof window !== 'undefined') window.logoutInProgress = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is the assumption that this variable is reset when the page is reloaded?

@pory-gone
Copy link
Contributor Author

i tested using something like this:
setInterval(() => { fetch('/api/graphql', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ query: '{ me { id name } }' }) }).then(r => r.json()).then(data => { console.log('req complete:', data.data?.me ? 'auth' : 'anon'); }); }, 5000);
and it didn't seem to have any errors, but following the procedure in the video, the problem persists for me too, I'll try to investigate a bit better

@ekzyis
Copy link
Member

ekzyis commented Sep 21, 2025

i tested using something like this: setInterval(() => { fetch('/api/graphql', { method: 'POST', headers: { 'Content-Type': 'application/json' }, body: JSON.stringify({ query: '{ me { id name } }' }) }).then(r => r.json()).then(data => { console.log('req complete:', data.data?.me ? 'auth' : 'anon'); }); }, 5000); and it didn't seem to have any errors, but following the procedure in the video, the problem persists for me too, I'll try to investigate a bit better

This is fetching every 5 seconds, but the bug is about requests that take 5 seconds to finish

@pory-gone
Copy link
Contributor Author

pory-gone commented Sep 21, 2025

After spending some time in the documentations you suggested, I implemented a custom Apollo Link that injects AbortSignal into all GraphQL operations and physically cancels them during logout/account switching. I did some investigations on Apollo's Advanced HTTP networking docs and created an abort link that sits before HttpLink in the chain and adds signal: abortController.signal to fetchOptions via operation.setContext(), then when logout happens we call abortPendingRequests() which triggers controller.abort() to actually cancel pending requests. The abort link handles both logout and account switching scenarios, and I followed the standard AbortController/Fetch API integration patterns from MDN docs combined with Apollo Link request handler patterns. This should completely eliminate the race condition since requests get cancelled mid-flight rather than completing successfully, so no fresh cookies = no automatic re-authentication. Hope i did everything right ahaha

2025-09-21-19-56-11.mp4

@pory-gone pory-gone requested a review from ekzyis September 21, 2025 22:55
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.

cancel all pending requests on logout and account switches
2 participants