From 354cd664dfdc96d3a7680b616405dd6f271e71e6 Mon Sep 17 00:00:00 2001 From: GUMBOKIM Date: Sat, 15 Aug 2026 17:58:58 +0900 Subject: [PATCH 1/2] fix(app): stop replaying a failed token refresh to later callers A failed refresh is memoized in `promiseToCachedToken_` and returned to every later caller for up to ~55 minutes, while a valid token sits in `cachedToken_`. `getToken()` now serves the cached token when it is further than TOKEN_EXPIRY_THRESHOLD_MILLIS from expiry, restoring the read path that existed before #2648. The cache read runs before shouldRefresh() because each reads its own Date.now(); in the opposite order a token crossing the threshold between the two reads would satisfy neither. Closes #3234 --- src/app/firebase-app.ts | 11 ++ test/unit/app/firebase-app.spec.ts | 265 +++++++++++++++++++++++++++++ 2 files changed, 276 insertions(+) diff --git a/src/app/firebase-app.ts b/src/app/firebase-app.ts index bc48772f9f..5c0f9fe3ff 100644 --- a/src/app/firebase-app.ts +++ b/src/app/firebase-app.ts @@ -50,6 +50,12 @@ export class FirebaseAppInternals { } public getToken(forceRefresh = false): Promise { + // Prefer the cached token over `promiseToCachedToken_`, which may hold a failed refresh. This + // runs before shouldRefresh() on purpose: each reads its own Date.now(), so in the opposite + // order a token crossing the threshold between the two reads would satisfy neither. + if (!forceRefresh && this.hasUsableCachedToken()) { + return Promise.resolve(this.cachedToken_); + } if (forceRefresh || this.shouldRefresh()) { this.promiseToCachedToken_ = this.refreshToken(); } @@ -126,6 +132,11 @@ export class FirebaseAppInternals { && !this.isRefreshing; } + private hasUsableCachedToken(): boolean { + return !!this.cachedToken_ + && (this.cachedToken_.expirationTime - Date.now()) > TOKEN_EXPIRY_THRESHOLD_MILLIS; + } + /** * Adds a listener that is called each time a token changes. * diff --git a/test/unit/app/firebase-app.spec.ts b/test/unit/app/firebase-app.spec.ts index 711907d238..46cf802b2a 100644 --- a/test/unit/app/firebase-app.spec.ts +++ b/test/unit/app/firebase-app.spec.ts @@ -504,6 +504,271 @@ describe('FirebaseApp', () => { expect(err.cause).to.equal(mockError); } }); + + describe('with a stuck or failing credential', () => { + const TEN_MINUTES_IN_SECONDS = 10 * 60; + + function stubCredential(): sinon.SinonStub { + return sinon.stub<[], Promise>(); + } + + it('serves a valid cached token while a stalled refresh is still in flight', async () => { + const oracle: GoogleOAuthAccessToken = { + access_token: 'Cached before the stall', + expires_in: ONE_HOUR_IN_SECONDS, + }; + const getAccessToken = stubCredential(); + getAccessToken.onCall(0).resolves(oracle); + getAccessToken.onCall(1).returns(new Promise(() => {})); + + const app = utils.createAppWithOptions({ credential: { getAccessToken } }); + expect((await app.INTERNAL.getToken()).accessToken).to.equal(oracle.access_token); + + // This refresh never settles, so joining it would block the caller indefinitely. + app.INTERNAL.getToken(true).catch(() => undefined); + + expect((await app.INTERNAL.getToken()).accessToken).to.equal(oracle.access_token); + expect(getAccessToken).to.have.been.calledTwice; + }); + + it('keeps serving a valid cached token after a forced refresh is rejected', async () => { + // The path RTDB takes after a token revocation, with the rejection swallowed. + const oracle: GoogleOAuthAccessToken = { + access_token: 'Cached before the failure', + expires_in: ONE_HOUR_IN_SECONDS, + }; + const getAccessToken = stubCredential(); + getAccessToken.onCall(0).resolves(oracle); + getAccessToken.onCall(1).rejects(new Error('token endpoint unavailable')); + + const app = utils.createAppWithOptions({ credential: { getAccessToken } }); + expect((await app.INTERNAL.getToken()).accessToken).to.equal(oracle.access_token); + + // The forced caller still sees the failure; only later callers fall back to the cache. + try { + await app.INTERNAL.getToken(true); + expect.fail('Should have failed'); + } catch (err: any) { + expect(err).to.have.property('code', 'app/invalid-credential'); + } + + expect((await app.INTERNAL.getToken()).accessToken).to.equal(oracle.access_token); + expect(app.INTERNAL.getCachedToken()?.accessToken).to.equal(oracle.access_token); + }); + + it('serves a cached token that is only just outside the refresh threshold', async () => { + // The threshold is the only bound on the cache read. A token with barely more than + // TOKEN_EXPIRY_THRESHOLD_MILLIS left must still be served after a refresh fails. + const oracle: GoogleOAuthAccessToken = { + access_token: 'Just outside the threshold', + expires_in: TEN_MINUTES_IN_SECONDS, + }; + const getAccessToken = stubCredential(); + getAccessToken.onCall(0).resolves(oracle); + getAccessToken.onCall(1).rejects(new Error('token endpoint unavailable')); + + const app = utils.createAppWithOptions({ credential: { getAccessToken } }); + await app.INTERNAL.getToken(); + await app.INTERNAL.getToken(true).catch(() => undefined); + + expect((await app.INTERNAL.getToken()).accessToken).to.equal(oracle.access_token); + expect(getAccessToken).to.have.been.calledTwice; + }); + + it('leaves the in-flight refresh reachable after serving the cache', async () => { + // Serving the cache must not overwrite `promiseToCachedToken_`; once the cached token + // reaches the threshold, callers still have to join the refresh already under way. + const oracle: GoogleOAuthAccessToken = { + access_token: 'Cached before the refresh', + expires_in: ONE_HOUR_IN_SECONDS, + }; + const replacement: GoogleOAuthAccessToken = { + access_token: 'Replacement', + expires_in: ONE_HOUR_IN_SECONDS, + }; + let respond: (token: GoogleOAuthAccessToken) => void = () => undefined; + const getAccessToken = stubCredential(); + getAccessToken.onCall(0).resolves(oracle); + getAccessToken.onCall(1).returns(new Promise((resolve) => { + respond = resolve; + })); + + const app = utils.createAppWithOptions({ credential: { getAccessToken } }); + await app.INTERNAL.getToken(); + + const forced = app.INTERNAL.getToken(true); + expect((await app.INTERNAL.getToken()).accessToken).to.equal(oracle.access_token); + + // Age the cached token into the refresh threshold while that refresh is still in flight. + clock.tick((60 - 4) * ONE_MINUTE_IN_MILLISECONDS); + + let settled = false; + const waiter = app.INTERNAL.getToken().then((token) => { + settled = true; + return token; + }); + await Promise.resolve(); + expect(settled).to.be.false; + + respond(replacement); + expect((await waiter).accessToken).to.equal(replacement.access_token); + await forced; + expect(getAccessToken).to.have.been.calledTwice; + }); + + it('replaces the cached token once a later forced refresh succeeds', async () => { + const oracle: GoogleOAuthAccessToken = { + access_token: 'Cached before the failure', + expires_in: ONE_HOUR_IN_SECONDS, + }; + const replacement: GoogleOAuthAccessToken = { + access_token: 'Replacement', + expires_in: ONE_HOUR_IN_SECONDS, + }; + const getAccessToken = stubCredential(); + getAccessToken.onCall(0).resolves(oracle); + getAccessToken.onCall(1).rejects(new Error('token endpoint unavailable')); + getAccessToken.onCall(2).resolves(replacement); + + const app = utils.createAppWithOptions({ credential: { getAccessToken } }); + await app.INTERNAL.getToken(); + await app.INTERNAL.getToken(true).catch(() => undefined); + + // A failed refresh must not stop a later one from replacing the cache. + expect((await app.INTERNAL.getToken(true)).accessToken).to.equal(replacement.access_token); + expect((await app.INTERNAL.getToken()).accessToken).to.equal(replacement.access_token); + expect(getAccessToken).to.have.been.calledThrice; + }); + + it('serves the outgoing token while the refresh that replaces it is in flight', async () => { + const outgoing: GoogleOAuthAccessToken = { + access_token: 'Rejected by the server', + expires_in: ONE_HOUR_IN_SECONDS, + }; + const replacement: GoogleOAuthAccessToken = { + access_token: 'Replacement', + expires_in: ONE_HOUR_IN_SECONDS, + }; + let respond: (token: GoogleOAuthAccessToken) => void = () => undefined; + const getAccessToken = stubCredential(); + getAccessToken.onCall(0).resolves(outgoing); + getAccessToken.onCall(1).returns(new Promise((resolve) => { + respond = resolve; + })); + + const app = utils.createAppWithOptions({ credential: { getAccessToken } }); + await app.INTERNAL.getToken(); + + // A forced refresh means the current token was rejected, so serving it to a concurrent + // caller hands out a token the server may already refuse. + const forced = app.INTERNAL.getToken(true); + expect((await app.INTERNAL.getToken()).accessToken).to.equal(outgoing.access_token); + + respond(replacement); + await forced; + expect((await app.INTERNAL.getToken()).accessToken).to.equal(replacement.access_token); + }); + + it('waits for the in-flight refresh when the cached token is nearly dead', async () => { + const oracle: GoogleOAuthAccessToken = { + access_token: 'Nearly dead', + expires_in: TEN_MINUTES_IN_SECONDS, + }; + const replacement: GoogleOAuthAccessToken = { + access_token: 'Replacement', + expires_in: ONE_HOUR_IN_SECONDS, + }; + let respond: (token: GoogleOAuthAccessToken) => void = () => undefined; + const getAccessToken = stubCredential(); + getAccessToken.onCall(0).resolves(oracle); + getAccessToken.onCall(1).returns(new Promise((resolve) => { + respond = resolve; + })); + + const app = utils.createAppWithOptions({ credential: { getAccessToken } }); + await app.INTERNAL.getToken(); + + // Leave the token less life than the refresh threshold, with a refresh in flight. + clock.tick((10 * ONE_MINUTE_IN_MILLISECONDS) - (20 * 1000)); + const forced = app.INTERNAL.getToken(true); + + let settled = false; + const waiter = app.INTERNAL.getToken().then((token) => { + settled = true; + return token; + }); + await Promise.resolve(); + expect(settled).to.be.false; + + // The waiter must receive the refreshed token, not the nearly dead cached one. + respond(replacement); + expect((await waiter).accessToken).to.equal(replacement.access_token); + await forced; + expect(getAccessToken).to.have.been.calledTwice; + }); + + it('surfaces the credential error once the cached token is inside the refresh threshold', async () => { + const oracle: GoogleOAuthAccessToken = { + access_token: 'Nearly dead', + expires_in: TEN_MINUTES_IN_SECONDS, + }; + const getAccessToken = stubCredential(); + getAccessToken.onCall(0).resolves(oracle); + getAccessToken.onCall(1).rejects(new Error('token endpoint unavailable')); + + const app = utils.createAppWithOptions({ credential: { getAccessToken } }); + await app.INTERNAL.getToken(); + + clock.tick((10 * ONE_MINUTE_IN_MILLISECONDS) - (20 * 1000)); + + try { + await app.INTERNAL.getToken(); + expect.fail('Should have failed'); + } catch (err: any) { + expect(err).to.have.property('code', 'app/invalid-credential'); + expect(err.message).to.include('token endpoint unavailable'); + } + expect(getAccessToken).to.have.been.calledTwice; + }); + + it('does not serve a cached token that is already inside the refresh threshold', async () => { + const oracle: GoogleOAuthAccessToken = { + access_token: 'Valid but nearly due', + expires_in: TEN_MINUTES_IN_SECONDS, + }; + const replacement: GoogleOAuthAccessToken = { + access_token: 'Replacement', + expires_in: ONE_HOUR_IN_SECONDS, + }; + let respond: (token: GoogleOAuthAccessToken) => void = () => undefined; + const getAccessToken = stubCredential(); + getAccessToken.onCall(0).resolves(oracle); + getAccessToken.onCall(1).returns(new Promise((resolve) => { + respond = resolve; + })); + + const app = utils.createAppWithOptions({ credential: { getAccessToken } }); + expect((await app.INTERNAL.getToken()).accessToken).to.equal(oracle.access_token); + + // Advance to exactly where a proactive refresh becomes due, then start one. + clock.tick(5 * ONE_MINUTE_IN_MILLISECONDS); + const forced = app.INTERNAL.getToken(true); + + let settled = false; + const waiter = app.INTERNAL.getToken().then((token) => { + settled = true; + return token; + }); + await Promise.resolve(); + expect(settled).to.be.false; + + // At exactly the threshold the cached token is due, so the waiter gets the new one. + respond(replacement); + expect((await waiter).accessToken).to.equal(replacement.access_token); + await forced; + expect(getAccessToken).to.have.been.calledTwice; + }); + }); }); describe('INTERNAL.addAuthTokenListener()', () => { From b37042c7e0075ac55cf4aafa702a59aa02960bf3 Mon Sep 17 00:00:00 2001 From: GUMBOKIM Date: Sat, 7 Nov 2026 09:23:17 +0900 Subject: [PATCH 2/2] refactor(app): only share an in-flight token refresh Store the refresh promise only while it is in flight and clear it in .finally(), as suggested in review. This drops isRefreshing and hasUsableCachedToken(), removes the Date.now() ordering subtlety, and stops an older refresh from clearing a newer forced one. Also pin the 5m + 1s boundary in the just-outside-threshold test, and add a test for overlapping forced refreshes. --- src/app/firebase-app.ts | 37 +++++++++++--------------- test/unit/app/firebase-app.spec.ts | 42 +++++++++++++++++++++++++++++- 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/src/app/firebase-app.ts b/src/app/firebase-app.ts index 5c0f9fe3ff..91b1b31603 100644 --- a/src/app/firebase-app.ts +++ b/src/app/firebase-app.ts @@ -39,27 +39,30 @@ export interface FirebaseAccessToken { */ export class FirebaseAppInternals { private cachedToken_: FirebaseAccessToken; - private promiseToCachedToken_: Promise; + private refreshTokenPromise_: Promise | undefined; private tokenListeners_: Array<(token: string) => void>; - private isRefreshing: boolean; // eslint-disable-next-line @typescript-eslint/naming-convention constructor(private credential_: Credential) { this.tokenListeners_ = []; - this.isRefreshing = false; } public getToken(forceRefresh = false): Promise { - // Prefer the cached token over `promiseToCachedToken_`, which may hold a failed refresh. This - // runs before shouldRefresh() on purpose: each reads its own Date.now(), so in the opposite - // order a token crossing the threshold between the two reads would satisfy neither. - if (!forceRefresh && this.hasUsableCachedToken()) { + if (!forceRefresh && this.cachedToken_ && !this.shouldRefresh()) { return Promise.resolve(this.cachedToken_); } - if (forceRefresh || this.shouldRefresh()) { - this.promiseToCachedToken_ = this.refreshToken(); + // Only an in-flight refresh is shared. Clearing it once settled keeps a failed refresh from + // being replayed to later callers, and the identity check stops an older refresh from + // clearing a newer forced one. + if (forceRefresh || !this.refreshTokenPromise_) { + const refreshPromise = this.refreshToken().finally(() => { + if (this.refreshTokenPromise_ === refreshPromise) { + this.refreshTokenPromise_ = undefined; + } + }); + this.refreshTokenPromise_ = refreshPromise; } - return this.promiseToCachedToken_ + return this.refreshTokenPromise_; } public getCachedToken(): FirebaseAccessToken | null { @@ -67,7 +70,6 @@ export class FirebaseAppInternals { } private refreshToken(): Promise { - this.isRefreshing = true; return Promise.resolve(this.credential_.getAccessToken()) .then((result) => { // Since the developer can provide the credential implementation, we want to weakly verify @@ -121,20 +123,11 @@ export class FirebaseAppInternals { message: errorMessage, cause: error as Error }); - }) - .finally(() => { - this.isRefreshing = false; - }) + }); } private shouldRefresh(): boolean { - return (!this.cachedToken_ || (this.cachedToken_.expirationTime - Date.now()) <= TOKEN_EXPIRY_THRESHOLD_MILLIS) - && !this.isRefreshing; - } - - private hasUsableCachedToken(): boolean { - return !!this.cachedToken_ - && (this.cachedToken_.expirationTime - Date.now()) > TOKEN_EXPIRY_THRESHOLD_MILLIS; + return !this.cachedToken_ || (this.cachedToken_.expirationTime - Date.now()) <= TOKEN_EXPIRY_THRESHOLD_MILLIS; } /** diff --git a/test/unit/app/firebase-app.spec.ts b/test/unit/app/firebase-app.spec.ts index d6ae058ccd..d32e2b8a31 100644 --- a/test/unit/app/firebase-app.spec.ts +++ b/test/unit/app/firebase-app.spec.ts @@ -570,6 +570,8 @@ describe('FirebaseApp', () => { const app = utils.createAppWithOptions({ credential: { getAccessToken } }); await app.INTERNAL.getToken(); + // Leave five minutes and one second on the token: just outside the threshold. + clock.tick((5 * ONE_MINUTE_IN_MILLISECONDS) - 1000); await app.INTERNAL.getToken(true).catch(() => undefined); expect((await app.INTERNAL.getToken()).accessToken).to.equal(oracle.access_token); @@ -577,7 +579,7 @@ describe('FirebaseApp', () => { }); it('leaves the in-flight refresh reachable after serving the cache', async () => { - // Serving the cache must not overwrite `promiseToCachedToken_`; once the cached token + // Serving the cache must not drop the in-flight refresh; once the cached token // reaches the threshold, callers still have to join the refresh already under way. const oracle: GoogleOAuthAccessToken = { access_token: 'Cached before the refresh', @@ -769,6 +771,44 @@ describe('FirebaseApp', () => { await forced; expect(getAccessToken).to.have.been.calledTwice; }); + + it('keeps a newer forced refresh in flight when an older one settles', async () => { + const oracle: GoogleOAuthAccessToken = { + access_token: 'Nearly dead', + expires_in: TEN_MINUTES_IN_SECONDS, + }; + const replacement: GoogleOAuthAccessToken = { + access_token: 'Replacement', + expires_in: ONE_HOUR_IN_SECONDS, + }; + let failOlder: (err: Error) => void = () => undefined; + let respondNewer: (token: GoogleOAuthAccessToken) => void = () => undefined; + const getAccessToken = stubCredential(); + getAccessToken.onCall(0).resolves(oracle); + getAccessToken.onCall(1).returns(new Promise((_, reject) => { + failOlder = reject; + })); + getAccessToken.onCall(2).returns(new Promise((resolve) => { + respondNewer = resolve; + })); + + const app = utils.createAppWithOptions({ credential: { getAccessToken } }); + await app.INTERNAL.getToken(); + clock.tick((10 * ONE_MINUTE_IN_MILLISECONDS) - (20 * 1000)); + + const older = app.INTERNAL.getToken(true).catch(() => undefined); + const newer = app.INTERNAL.getToken(true); + failOlder(new Error('token endpoint unavailable')); + await older; + + // The older refresh settling must not hide the newer one: the waiter joins it rather + // than starting a refresh of its own. + const waiter = app.INTERNAL.getToken(); + respondNewer(replacement); + expect((await waiter).accessToken).to.equal(replacement.access_token); + await newer; + expect(getAccessToken).to.have.been.calledThrice; + }); }); });