Keep the token pair as a fallback after switching to cookie auth (#23755)
`CookieSessionBootEffect` cleared the token pair the moment it switched a client onto cookie auth. That leaves the client with a single credential, and a server that still has `AUTH_COOKIE_SESSIONS_ENABLED=false` ignores the session cookie entirely — `extractSessionTokenFromRequest` early-returns when the flag is off. A cookie-only client is therefore unauthenticated against such a server, `handleTokenRenewal` finds no refresh token, and `onUnauthenticatedError` signs the user out. That is not a hypothetical state. It is every request routed to a not-yet-rolled pod while the flag is being enabled, and every request after the flag is rolled back. Requests are load-balanced per request, so a migrated client hits an old pod almost immediately and gets signed out; signing back in can migrate it again and repeat for the length of the rollout. It also means rollback was not free, contrary to how it was described: flipping the flag back to `false` signed out everyone who had already migrated, because the pair they were supposed to fall back to had been deleted. ## Approach Keep the token pair as a dormant fallback, and stop *sending* it while cookie auth is active. Both halves are needed. Retaining it without suppressing the header would be worse than the bug: `validateTokenByRequest` checks the Bearer token first and only falls back to the session cookie when there is none, so a client that keeps sending Bearer would never exercise the cookie at all, and `CookieSessionCsrfMiddleware` bypasses on any Bearer-carrying request. Cookie sessions would silently become a no-op. So: - `switchToCookieAuth` no longer nulls the token pair - the auth link omits `authorization` while cookie auth is active, leaving the cookie as the credential in use - on an unauthenticated error while cookie auth is active, the client deactivates cookie auth once per operation and falls through to the existing renewal path, which replays with a fresh Bearer The fallback deliberately goes through renewal rather than replaying immediately: access tokens live 10 minutes, so the retained one has usually expired while the client was authenticating by cookie, and an immediate replay would just fail again. `isCookieAuthActive` is read and written through `localStorage` from the link because the links run per request and must agree with the atom synchronously — a React state update lands a render too late to affect the request being built. ## Follow-up This trades the immediate removal of the token pair from `localStorage` for rollout safety, so the XSS-exfiltration surface that cookie sessions close stays open a while longer. Once cookie sessions are stable across every environment, the retained pair should be dropped — reverting to a clear on `switchToCookieAuth` is a one-line change. ## Test Three cases added to `apollo.factory.test.ts`: no Bearer header while cookie auth is active; an unauthenticated response falls back and replays with the token pair rather than calling `onUnauthenticatedError`; and the fallback is attempted only once before going through renewal. The existing `CookieSessionBootEffect` assertion that the pair is cleared is inverted to assert it is retained. <!-- This is an auto-generated description by cubic. --> <a href="https://cubic.dev/pr/twentyhq/twenty/pull/23755?utm_source=github" target="_blank" rel="noopener noreferrer" data-no-image-dialog="true"><picture><source media="(prefers-color-scheme: dark)" srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img alt="Review in cubic" src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a> <!-- End of auto-generated description by cubic. -->
This commit is contained in:
@@ -4,6 +4,8 @@ import fetchMock, { enableFetchMocks } from 'jest-fetch-mock';
|
||||
|
||||
import { ApolloFactory, type Options } from '@/apollo/services/apollo.factory';
|
||||
import { getTokenPair } from '@/apollo/utils/getTokenPair';
|
||||
import { isCookieAuthActiveState } from '@/auth/states/isCookieAuthActiveState';
|
||||
import { jotaiStore } from '@/ui/utilities/state/jotai/jotaiStore';
|
||||
import { renewToken } from '@/auth/services/AuthService';
|
||||
import { CUSTOM_WORKSPACE_APPLICATION_MOCK } from '@/object-metadata/hooks/__tests__/constants/CustomWorkspaceApplicationMock.test.constant';
|
||||
import {
|
||||
@@ -154,6 +156,7 @@ describe('ApolloFactory', () => {
|
||||
fetchMock.resetMocks();
|
||||
jest.mocked(renewToken).mockReset().mockResolvedValue(RENEWED_TOKEN_PAIR);
|
||||
jest.mocked(getTokenPair).mockReset().mockReturnValue(CURRENT_TOKEN_PAIR);
|
||||
jotaiStore.set(isCookieAuthActiveState.atom, false);
|
||||
});
|
||||
|
||||
it('should create an instance of ApolloFactory', () => {
|
||||
@@ -366,4 +369,69 @@ describe('ApolloFactory', () => {
|
||||
expect(renewToken).not.toHaveBeenCalled();
|
||||
expect(mockOnUnauthenticatedError).toHaveBeenCalledTimes(1);
|
||||
});
|
||||
describe('cookie auth fallback during a mixed-version rollout', () => {
|
||||
const setCookieAuthActive = () =>
|
||||
jotaiStore.set(isCookieAuthActiveState.atom, true);
|
||||
|
||||
// fetch normalises header names, so assert case-insensitively rather than
|
||||
// depending on the casing the mock happens to expose.
|
||||
const readHeader = (
|
||||
headers: Record<string, string>,
|
||||
name: string,
|
||||
): string | undefined =>
|
||||
Object.entries(headers).find(
|
||||
([key]) => key.toLowerCase() === name.toLowerCase(),
|
||||
)?.[1];
|
||||
|
||||
it('should not attach the Bearer header while cookie auth is active', async () => {
|
||||
setCookieAuthActive();
|
||||
fetchMock.mockResponse(() =>
|
||||
Promise.resolve({ body: JSON.stringify({ data: {} }) }),
|
||||
);
|
||||
|
||||
await makeRequest();
|
||||
|
||||
const headers = fetchMock.mock.calls[0]?.[1]?.headers as Record<
|
||||
string,
|
||||
string
|
||||
>;
|
||||
|
||||
expect(readHeader(headers, 'authorization')).toBeUndefined();
|
||||
// Version-mismatch detection must keep working in cookie-auth mode.
|
||||
expect(readHeader(headers, 'X-App-Version')).toBe('1.0.0');
|
||||
});
|
||||
|
||||
it('should fall back to the token pair instead of signing out when a server ignores the session cookie', async () => {
|
||||
setCookieAuthActive();
|
||||
fetchMock
|
||||
.mockResponseOnce(UNAUTHENTICATED_RESPONSE)
|
||||
.mockResponseOnce(JSON.stringify({ data: { trackAnalytics: null } }));
|
||||
|
||||
await makeRequest();
|
||||
|
||||
expect(mockOnUnauthenticatedError).not.toHaveBeenCalled();
|
||||
expect(jotaiStore.get(isCookieAuthActiveState.atom)).toBe(false);
|
||||
|
||||
const retryHeaders = fetchMock.mock.calls[1]?.[1]?.headers as Record<
|
||||
string,
|
||||
string
|
||||
>;
|
||||
|
||||
expect(readHeader(retryHeaders, 'authorization')).toBe(
|
||||
`Bearer ${CURRENT_TOKEN_PAIR.accessOrWorkspaceAgnosticToken.token}`,
|
||||
);
|
||||
});
|
||||
|
||||
it('should only attempt the cookie fallback once, then go through renewal', async () => {
|
||||
setCookieAuthActive();
|
||||
fetchMock.mockResponse(UNAUTHENTICATED_RESPONSE);
|
||||
|
||||
try {
|
||||
await makeRequest();
|
||||
} catch {}
|
||||
|
||||
expect(jotaiStore.get(isCookieAuthActiveState.atom)).toBe(false);
|
||||
expect(renewToken).toHaveBeenCalled();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -20,7 +20,9 @@ import { retryWithBackoff } from '~/utils/retryWithBackoff';
|
||||
|
||||
import { REST_API_BASE_URL } from '@/apollo/constant/rest-api-base-url';
|
||||
import { type ApolloManager } from '@/apollo/types/apolloManager.interface';
|
||||
import { getIsCookieAuthActive } from '@/apollo/utils/getIsCookieAuthActive';
|
||||
import { getTokenPair } from '@/apollo/utils/getTokenPair';
|
||||
import { setIsCookieAuthActive } from '@/apollo/utils/setIsCookieAuthActive';
|
||||
import { isUnauthenticatedGraphQLError } from '@/apollo/utils/isUnauthenticatedGraphQLError';
|
||||
import { loggerLink } from '@/apollo/utils/loggerLink';
|
||||
import { StreamingRestLink } from '@/apollo/utils/streamingRestLink';
|
||||
@@ -133,12 +135,23 @@ export class ApolloFactory implements ApolloManager {
|
||||
|
||||
const locale = this.currentWorkspaceMember?.locale ?? i18n.locale;
|
||||
|
||||
if (isUndefinedOrNull(tokenPair) || skipAuthToken === true) {
|
||||
// The token pair is kept as a dormant fallback once cookie auth is
|
||||
// active, but must not be sent: Bearer takes precedence over the
|
||||
// session cookie server-side, so attaching it would keep the cookie
|
||||
// unused and bypass the CSRF origin check.
|
||||
if (
|
||||
isUndefinedOrNull(tokenPair) ||
|
||||
skipAuthToken === true ||
|
||||
getIsCookieAuthActive()
|
||||
) {
|
||||
return {
|
||||
headers: {
|
||||
...headers,
|
||||
...optionHeaders,
|
||||
'x-locale': locale,
|
||||
...(isDefined(this.appVersion) && {
|
||||
'X-App-Version': this.appVersion,
|
||||
}),
|
||||
},
|
||||
};
|
||||
}
|
||||
@@ -151,7 +164,9 @@ export class ApolloFactory implements ApolloManager {
|
||||
...optionHeaders,
|
||||
authorization: token ? `Bearer ${token}` : '',
|
||||
'x-locale': locale,
|
||||
...(this.appVersion && { 'X-App-Version': this.appVersion }),
|
||||
...(isDefined(this.appVersion) && {
|
||||
'X-App-Version': this.appVersion,
|
||||
}),
|
||||
},
|
||||
};
|
||||
});
|
||||
@@ -205,6 +220,31 @@ export class ApolloFactory implements ApolloManager {
|
||||
return throwError(() => error);
|
||||
}
|
||||
|
||||
// A server that still has cookie sessions disabled ignores the session
|
||||
// cookie, so a cookie-only client reads as unauthenticated there. That
|
||||
// happens on every request routed to a not-yet-rolled pod, and after a
|
||||
// rollback. Fall back to the retained token pair instead of signing the
|
||||
// user out. Attempted once per operation so a genuinely expired token
|
||||
// still reaches the renewal path below.
|
||||
if (
|
||||
getIsCookieAuthActive() &&
|
||||
operation.getContext().hasAttemptedCookieAuthFallback !== true &&
|
||||
isDefined(getTokenPair()?.refreshToken?.token)
|
||||
) {
|
||||
setIsCookieAuthActive(false);
|
||||
operation.setContext({ hasAttemptedCookieAuthFallback: true });
|
||||
// Deactivation is sticky for the rest of the mount by design. Both
|
||||
// credentials stay valid, so re-probing after every fallback would
|
||||
// thrash between them for the whole rollout: the probe succeeds on a
|
||||
// rolled pod, the next request lands on an old one and falls back
|
||||
// again. CookieSessionBootEffect re-probes on the next mount, which
|
||||
// restores cookie auth once the fleet is uniform.
|
||||
// Deliberately falls through to the renewal below rather than
|
||||
// replaying immediately: the retained access token is likely to have
|
||||
// expired while the client was authenticating by cookie, so the
|
||||
// replay needs a fresh one to succeed on the first try.
|
||||
}
|
||||
|
||||
if (!getTokenPair()?.refreshToken?.token) {
|
||||
onUnauthenticatedError?.();
|
||||
|
||||
|
||||
@@ -0,0 +1,13 @@
|
||||
import { isCookieAuthActiveState } from '@/auth/states/isCookieAuthActiveState';
|
||||
import { jotaiStore } from '@/ui/utilities/state/jotai/jotaiStore';
|
||||
|
||||
// Read straight off the store rather than through a subscription because the
|
||||
// Apollo links run per request and must see the value synchronously, before
|
||||
// the render a React subscription would wait for.
|
||||
export const getIsCookieAuthActive = (): boolean => {
|
||||
try {
|
||||
return jotaiStore.get(isCookieAuthActiveState.atom) === true;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
};
|
||||
@@ -0,0 +1,14 @@
|
||||
import { isCookieAuthActiveState } from '@/auth/states/isCookieAuthActiveState';
|
||||
import { jotaiStore } from '@/ui/utilities/state/jotai/jotaiStore';
|
||||
|
||||
// Goes through the atom so the persisted key keeps a single writer, and so
|
||||
// React subscribers follow without the caller mirroring this into component
|
||||
// state. Guarded because it runs inside unauthenticated error handling, where
|
||||
// blocked storage would otherwise throw out of the error path.
|
||||
export const setIsCookieAuthActive = (isActive: boolean): void => {
|
||||
try {
|
||||
jotaiStore.set(isCookieAuthActiveState.atom, isActive);
|
||||
} catch {
|
||||
// noop
|
||||
}
|
||||
};
|
||||
+8
-3
@@ -47,7 +47,7 @@ export const CookieSessionBootEffect = () => {
|
||||
const [isCookieAuthActive, setIsCookieAuthActive] = useAtomState(
|
||||
isCookieAuthActiveState,
|
||||
);
|
||||
const [tokenPair, setTokenPair] = useAtomState(tokenPairState);
|
||||
const tokenPair = useAtomStateValue(tokenPairState);
|
||||
// oxlint-disable-next-line twenty/no-state-useref
|
||||
const hasProbeRunRef = useRef(false);
|
||||
|
||||
@@ -68,9 +68,15 @@ export const CookieSessionBootEffect = () => {
|
||||
}
|
||||
};
|
||||
|
||||
// The token pair is deliberately retained rather than cleared. A server
|
||||
// that still has cookie sessions disabled ignores the session cookie, so a
|
||||
// cookie-only client is unauthenticated against it — which is every request
|
||||
// routed to a not-yet-rolled pod during a deploy, and every request after a
|
||||
// rollback. Keeping the pair lets those fall back instead of signing the
|
||||
// user out. It stops being sent while cookie auth is active, so the cookie
|
||||
// is still the credential in use.
|
||||
const switchToCookieAuth = () => {
|
||||
setIsCookieAuthActive(true);
|
||||
setTokenPair(null);
|
||||
};
|
||||
|
||||
const attemptCookieSessionBoot = async (): Promise<boolean> => {
|
||||
@@ -145,7 +151,6 @@ export const CookieSessionBootEffect = () => {
|
||||
isCookieSessionEnabled,
|
||||
isLoadedOnce,
|
||||
setIsCookieAuthActive,
|
||||
setTokenPair,
|
||||
store,
|
||||
tokenPair,
|
||||
]);
|
||||
|
||||
+3
-1
@@ -89,7 +89,9 @@ describe('CookieSessionBootEffect', () => {
|
||||
expect(store.get(isCookieAuthActiveState.atom)).toBe(true);
|
||||
});
|
||||
|
||||
expect(store.get(tokenPairState.atom)).toBeNull();
|
||||
// Retained as a fallback for servers that still have cookie sessions
|
||||
// disabled, which would otherwise sign the user out mid-rollout.
|
||||
expect(store.get(tokenPairState.atom)).not.toBeNull();
|
||||
});
|
||||
|
||||
it('should stay retryable when the probe fails for an unrelated reason', async () => {
|
||||
|
||||
@@ -1,7 +1,10 @@
|
||||
import { createAtomState } from '@/ui/utilities/state/jotai/utils/createAtomState';
|
||||
|
||||
export const IS_COOKIE_AUTH_ACTIVE_LOCAL_STORAGE_KEY =
|
||||
'isCookieAuthActiveState';
|
||||
|
||||
export const isCookieAuthActiveState = createAtomState<boolean>({
|
||||
key: 'isCookieAuthActiveState',
|
||||
key: IS_COOKIE_AUTH_ACTIVE_LOCAL_STORAGE_KEY,
|
||||
defaultValue: false,
|
||||
useLocalStorage: true,
|
||||
localStorageOptions: { getOnInit: true },
|
||||
|
||||
Reference in New Issue
Block a user