EC-24102 refresh token to cookie

[kkignatov]

EC-24102: Move refresh tokens to secure HttpOnly cookies (BFF pattern)

Description generated with Claude Code

Summary

Implements the Backend-for-Frontend (BFF) pattern to move refresh tokens out of localStorage and into server-side HttpOnly cookies, eliminating XSS exposure of long-lived credentials.

Changes

New optional callbacks on IAuthenticationOptions:

• storeRefreshToken(token) — persist token server-side and remove from localStorage
• refreshAccessToken() — obtain new access tokens via HttpOnly cookies
• revokeRefreshToken() — revoke server-side sessions on sign-out

All three must be provided together or not at all. Consumers without these callbacks continue using the existing localStorage flow unchanged.

Internal improvements:

• Lazy initialization via cached promises replaces polling — eliminates race conditions in concurrent requests
• Token migration on first load moves any existing localStorage tokens server-side automatically
• ES2015-compatible Promise handling (removes Promise.allSettled dependency)
• OAuth param sanitization for both query string and fragment modes

Test coverage

Expanded from 3 → 51 tests covering new BFF flows, error recovery, token migration, and concurrent request deduplication.

Breaking changes

None — existing localStorage flow is fully preserved for consumers that don't provide the new callbacks.

[johannesheesterman]

Code Review: Refresh token to cookie

+354 / -110 across 6 files

Overview

This PR removes the refresh token from localStorage in favor of secure server-side storage (e.g., HttpOnly cookies). It introduces three new options on IAuthenticationOptions:

• refreshAccessToken — custom token refresh via backend endpoint
• storeRefreshToken — securely persist refresh token server-side
• fetchServiceConfiguration — injectable OIDC config (useful for testing)

It also cleans up significant architectural debt: lazy initialization via ensureInitialized() (replacing polling with sleep/waitFor), fully awaited async flows, and removal of dead code (signedInResolvers, isInitialized, etc.).

Positives

• Security improvement: Moving the refresh token out of localStorage is the right call — localStorage is vulnerable to XSS.
• Lazy initialization: ensureInitialized() with a cached promise is much cleaner than the polling-based waitFor/sleep approach.
• Fully awaited flows: Replacing .then() fire-and-forget chains with await in initialize() and signOut() eliminates subtle race conditions.
• Good test coverage: 200+ lines of new tests covering the new code paths, migration, error handling, and edge cases.
• Optional chaining on regex: The ?. on regex matches in onAuthorization fixes potential null-reference crashes.

Issues & Suggestions

1. signOut ordering change — potential behavioral regression (Medium)

// Before: revoke → endSession → deleteTokens (delete after redirect)
// After:  revoke → deleteTokens → endSession

endSession sets window.location.href, which triggers a redirect. In the old code, deleteTokens ran after endSession but before navigation completed. In the new code, tokens are deleted before calling endSession. This is likely fine (and arguably more correct), but worth confirming that no consumer depends on tokens being available during the redirect.

2. onSignIn() race condition (Medium)

public onSignIn(): Promise<void> {
    this.ensureInitialized();  // fire-and-forget — not awaited
    if (this.validateAccessTokenResponse()) {
        return Promise.resolve();
    }
    // ...
}

ensureInitialized() is called but not awaited. If it's the first call, initialization starts but the validateAccessTokenResponse() check runs immediately (before init completes). This means the early-return path will almost never fire on the first call — the caller always gets a pending promise. This isn't necessarily a bug (the promise resolves when callSignInResolvers() fires), but it's inconsistent with how other methods (isSignedIn, getAccessToken, getIdToken) await ensureInitialized(). Consider awaiting it here too for consistency.

3. refreshAccessToken option: no idToken returned (Low-Medium)

When using the custom refreshAccessToken callback, the constructed TokenResponse has no idToken. This means getIdToken() will return undefined after a custom refresh. If consumers use getIdToken(), this could be a problem. Consider either:

• Adding idToken to the callback return type
• Documenting this limitation

4. storeRefreshToken without refreshAccessToken (Low)

If a consumer provides storeRefreshToken but not refreshAccessToken, the refresh token gets stored server-side and removed from localStorage, but the built-in refreshAccessToken() still reads from TokenStore (localStorage). This means the next refresh attempt would fail since the token was migrated away. The options should be documented as a pair, or the code should guard against this combination.

5. performAuthorizationRequest override is a no-op (Nit)

The new override in AuthorizationHandler just calls super without adding behavior. If it's for type visibility, a comment would help. Otherwise, it can be removed.

Summary

Solid PR that addresses a real security concern and significantly improves the initialization architecture. Main areas to address:

1. Verify the signOut ordering change is intentional
2. Decide whether onSignIn should await ensureInitialized
3. Consider the storeRefreshToken without refreshAccessToken edge case
4. Clarify that getIdToken() won't work with custom refreshAccessToken

🤖 Generated with Claude Code

[johannesheesterman]

1. src/authentication/authenticationContext.ts:442-485 moves the refresh token out of localStorage, but signOut()/revokeRefreshToken() still only know how to revoke tokens that are in TokenStore. In the new HttpOnly-cookie-backed flow, nothing here clears the server-side refresh cookie/session, so after logout the next initialize() can call options.refreshAccessToken() and silently mint a new access token again. That means signOut() no longer signs the user out in the primary flow this PR introduces unless every consumer adds an out-of-band cookie-clearing step.

2. src/authentication/authenticationOptions.ts:25-34 and src/authentication/authenticationContext.ts:259-345 make the custom refresh path access-token-only. refreshAccessToken() synthesizes a new TokenResponse without an id_token, overwrites the previous response, and persists that stripped-down response. After the first custom refresh, getIdToken() returns undefined, and signOut() loses its id_token_hint for end-session requests. This is a regression in the public getIdToken() contract for the new secure-refresh mode. The callback needs to preserve or return the ID token instead of dropping it.

[kkignatov]

Solid comments by the AIs. Issue 1 should not be a problem because I changed the order of deleting the token and finishing the session on purpose. Before that, the browser got redirected and deleteTokens was never called, leaving the tokens in the storage.

I will carefully consider the solutions to the other issues 2, 3, 4 as well as the comments requiring changes.

[Copilot]

Pull request overview

This PR updates the authentication library to avoid storing refresh tokens in localStorage, enabling secure refresh flows (e.g., HttpOnly-cookie backed) while keeping the public API behavior largely consistent. It also refactors initialization to be lazy/promise-cached and updates Jest setup to provide a proper localStorage mock.

Changes:

• Add optional callbacks to proxy refresh-token storage/refresh/revocation away from localStorage, plus an optional service configuration factory.
• Refactor AuthenticationContext initialization to be lazy and fully awaited (removing polling / resolver plumbing), and migrate legacy refresh tokens out of localStorage when configured.
• Fix Jest test environment by adding an in-memory localStorage mock and expanding/adjusting unit tests.

Reviewed changes

Copilot reviewed 4 out of 6 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
tsconfig.json	Adds DOM/ES2018 libs to support browser globals in TS compilation.
src/authentication/authenticationOptions.ts	Introduces new optional hooks for secure refresh-token handling and config fetching.
src/authentication/authenticationContext.ts	Implements lazy initialization, custom refresh flow support, and refresh-token migration/removal.
src/authentication/authenticationContext.spec.ts	Adds/updates unit tests for the new options and initialization behavior.
jest.setup.ts	Adds a shared in-memory localStorage mock for node-based Jest runs.
jest.config.js	Switches Jest globals mocking to setupFiles.

Comments suppressed due to low confidence (1)

src/authentication/authenticationContext.ts:438

• If response.code is missing and the regex/URL parsing doesn’t find a code parameter, code becomes undefined and a token request is still attempted. Please add a guard that rejects/throws with a clear error when no authorization code can be determined, instead of sending an invalid token request.

        let code = response.code;
        if (!code){
            code = this.options.responseMode == 'fragment'
                ? new RegExp('#code=(.*?)&').exec(locationVariable)?.[1]
                : new URL(location.href).searchParams.get('code');
        }

        let tokenRequest = new TokenRequest({
            client_id: this.options.clientId,
            redirect_uri: this.options.redirectUri,
            grant_type: GRANT_TYPE_AUTHORIZATION_CODE,
            code: code,
            refresh_token: undefined,
        });

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

src/authentication/authenticationContext.ts:449

• onAuthorization(): if the authorization code cannot be extracted (e.g. fragment URL where code is last param and there’s no trailing &), the regex returns undefined and the code proceeds to create a TokenRequest with an undefined code. Add an explicit check that code is non-empty and reject the sign-in promise with a clear error when it’s missing; also consider using a pattern that matches end-of-string.

        let code = response.code;
        if (!code){
            code = this.options.responseMode == 'fragment'
                ? new RegExp('#code=(.*?)&').exec(locationVariable)?.[1]
                : new URL(location.href).searchParams.get('code');
        }

        let tokenRequest = new TokenRequest({
            client_id: this.options.clientId,
            redirect_uri: this.options.redirectUri,
            grant_type: GRANT_TYPE_AUTHORIZATION_CODE,
            code: code,
            refresh_token: undefined,
        });

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated 5 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated 2 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 6 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[johannesheesterman]

1. src/authentication/authenticationContext.ts:92 blocks onSignIn() on ensureInitialized() before registering any rejector. When initialize() processes a callback error path itself (completeAuthorizationRequest() -> onAuthorization() -> callSignInRejectors()), there are no listeners yet, no error is thrown, and onSignIn() then returns a promise that never settles. I reproduced this with a callback response missing state: Promise.race([ctx.onSignIn(), timeout]) times out on this branch instead of rejecting. That regresses error handling on the redirect page.

2. src/authentication/authenticationContext.ts:279 calls refreshAccessToken() directly, bypassing the shared _refreshTokenPromise gate added in getAccessToken() at src/authentication/authenticationContext.ts:250. With an expired token, await Promise.all([ctx.getAccessToken(), ctx.getIdToken()]) invokes the custom refreshAccessToken callback twice on this branch. That defeats the new deduplication logic and can trigger duplicate BFF refresh requests / races.

[johannesheesterman]

1. src/authentication/authenticationContext.ts:480 stores this.accessTokenResponse before awaiting storeRefreshToken(), and the error path never rolls it back. If storeRefreshToken() rejects, onAuthorization() rejects as expected, but the instance still holds a valid in-memory access token. On the next public call, initialize() retries migration from accessTokenResponse?.refreshToken at src/authentication/authenticationContext.ts:507, logs the error, then validateAccessTokenResponse() at src/authentication/authenticationContext.ts:523 returns true and the user is treated as signed in even though sign-in just failed and no server-side refresh session was established. I reproduced this with a rejecting storeRefreshToken: onAuthorization() throws, then await ctx.isSignedIn() returns true. The token assignment needs to be deferred until persistence succeeds, or cleared on failure.

2. src/authentication/authenticationContext.ts:545 only strips OAuth params from the fragment when responseMode === "fragment". In query mode, a callback URL like https://app.example.com/callback#code=C&state=S is left untouched, so the OAuth params remain in browser history/referrers despite the PR description claiming sanitization in both query and fragment for all response modes. I reproduced this by calling sanitizeRedirectUrl() with responseMode: "query"; it leaves #code=C&state=S intact. This needs the same fragment sanitization pass in query mode, while still preserving non-auth hash routes.

[johannesheesterman]

src/authentication/authenticationContext.ts:159 now relies on Promise.allSettled() for sign-out revocation. This package still targets es2015 in tsconfig.json, and the built artifact leaves that call intact in dist/index.js, so environments without ES2020 Promise helpers will crash on sign-out with TypeError: Promise.allSettled is not a function. I reproduced it by temporarily deleting Promise.allSettled and calling signOut(); it throws before token cleanup or redirect. Please either avoid allSettled() here (for example by catching each revocation separately) or explicitly raise/polyfill the runtime requirement.