Skip to content

WIP cookie auth fixes #9480

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.

Sign up for GitHub

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

Draft
jamesdaniels wants to merge 3 commits into
base: main
Choose a base branch
from
Draft

WIP cookie auth fixes #9480

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dbea83b
WIP cookie auth fixes
jamesdaniels Feb 2, 2026
f843dcd
Add comment
jamesdaniels Feb 2, 2026
fffce7e
Add two more comments
jamesdaniels Feb 2, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 21 additions & 2 deletions packages/auth/src/core/persistence/persistence_user_manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,11 @@
import { getAccountInfo } from '../../api/account_management/account';
import { ApiKey, AppName, AuthInternal } from '../../model/auth';
import { UserInternal } from '../../model/user';
import { PersistedBlob, PersistenceInternal } from '../persistence';
import {
PersistedBlob,
PersistenceInternal,
PersistenceType
} from '../persistence';
import { UserImpl } from '../user/user_impl';
import { _getInstance } from '../util/instantiator';
import { inMemoryPersistence } from './in_memory';
Expand Down Expand Up @@ -80,7 +84,17 @@ export class PersistenceUserManager {
if (!response) {
return null;
}
return UserImpl._fromGetAccountInfoResponse(this.auth, response, blob);
const user = await UserImpl._fromGetAccountInfoResponse(
this.auth,
response,
blob
);
// TODO look into why this is needed, probably something we need to fix in the cookie
// persistence layer
if (this.persistence.type === PersistenceType.COOKIE) {
user.stsTokenManager.refreshToken = 'REDACTED';
}
Comment on lines +94 to +96
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 2, 2026

Choose a reason for hiding this comment

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

critical

Hardcoding the refreshToken to 'REDACTED' is a fragile hack. While the accompanying comment indicates this is a known issue, it's important to highlight that this approach relies on the internal implementation details of StsTokenManager and could break unexpectedly in the future. This could also lead to the 'REDACTED' value being persisted.

A more robust solution would be to make StsTokenManager aware of cookie-based authentication and adjust its token refresh logic accordingly, rather than tricking it with a fake token.

return user;
}
return UserImpl._fromJSON(this.auth, blob);
}
Expand Down Expand Up @@ -170,6 +184,11 @@ export class PersistenceUserManager {
response,
blob
);
// TODO look into why this is needed, probably something we need to fix in the cookie
// persistence layer
if (persistence.type === PersistenceType.COOKIE) {
user.stsTokenManager.refreshToken = 'REDACTED';
}
Comment on lines +189 to +191
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Feb 2, 2026

Choose a reason for hiding this comment

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

critical

This is a duplicate of the same fragile hack of setting a hardcoded refreshToken. Duplicating this logic increases maintenance overhead and the risk of inconsistencies. This logic should be centralized if it's absolutely necessary, but the fundamental approach of using a fake refresh token should be reconsidered as mentioned in the other comment.

} else {
user = UserImpl._fromJSON(auth, blob); // throws for unparsable blob (wrong format)
}
Expand Down
18 changes: 16 additions & 2 deletions packages/auth/src/platform_browser/persistence/cookie_storage.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
PersistenceValue,
StorageEventListener
} from '../../core/persistence';
import { _isSafari } from '../../core/util/browser';

// Pull a cookie value from document.cookie
function getDocumentCookie(name: string): string | null {
Expand All @@ -34,6 +35,9 @@ function getDocumentCookie(name: string): string | null {
return document.cookie.match(matcher)?.[1] ?? null;
}

// 400 days in seconds https://developer.chrome.com/blog/cookie-max-age-expires
const COOKIE_MAX_MAX_AGE = 34560000;

// Produce a sanitized cookie name from the persistence key
function getCookieName(key: string): string {
// __HOST- doesn't work in localhost https://issues.chromium.org/issues/40196122 but it has
Expand Down Expand Up @@ -102,8 +106,18 @@ export class CookiePersistence implements PersistenceInternal {
return;
}
const name = getCookieName(key);
document.cookie = `${name}=;Max-Age=34560000;Partitioned;Secure;SameSite=Strict;Path=/;Priority=High`;
await fetch(`/__cookies__`, { method: 'DELETE' }).catch(() => undefined);
const isDevMode = window.location.protocol === 'http:';
// Safari doesn't consider http://localhost to be secure so we need to set the cookie as
// insecure in this case.
const useInsecureCookie = isDevMode && _isSafari(navigator.userAgent);
// Set a logout sentinel value of "" to indicate that the user is logged out, if this is seen by
// the middleware, it will know to clear the http-only refresh token from the backend as well.
// Along with a long max-age this allows for offline signout.
document.cookie = `${name}=;Max-Age=${COOKIE_MAX_MAX_AGE};${useInsecureCookie ? '' : 'Partitioned;Secure;'}SameSite=Lax;Path=/;Priority=High`;
// Calling DELETE is entirely optional, but it's a nice optimization to clear the http-only
// refresh token from the backend as well. Fire and forget.
// eslint-disable-next-line @typescript-eslint/no-floating-promises
fetch(`/__cookies__`, { method: 'DELETE' }).catch(() => undefined);
}

// Listen for cookie changes, both cookieStore and fallback to polling document.cookie
Expand Down
Loading