diff --git a/.changeset/selfish-worms-glow.md b/.changeset/selfish-worms-glow.md new file mode 100644 index 00000000000..af25eed8fbb --- /dev/null +++ b/.changeset/selfish-worms-glow.md @@ -0,0 +1,5 @@ +--- +'@firebase/app-check': patch +--- + +Fix timer issues in App Check that caused the token to fail to refresh after the token expired, or caused rapid repeated requests attempting to do so. diff --git a/packages/app-check/src/indexeddb.ts b/packages/app-check/src/indexeddb.ts index 4041a535904..4da7165c8fa 100644 --- a/packages/app-check/src/indexeddb.ts +++ b/packages/app-check/src/indexeddb.ts @@ -80,7 +80,7 @@ export function readTokenFromIndexedDB( export function writeTokenToIndexedDB( app: FirebaseApp, - token: AppCheckTokenInternal + token?: AppCheckTokenInternal ): Promise { return write(computeKey(app), token); } diff --git a/packages/app-check/src/internal-api.test.ts b/packages/app-check/src/internal-api.test.ts index 09b81f5ee24..03c690fb42e 100644 --- a/packages/app-check/src/internal-api.test.ts +++ b/packages/app-check/src/internal-api.test.ts @@ -368,6 +368,95 @@ describe('internal api', () => { }); }); + it('ignores in-memory token if it is invalid and continues to exchange request', async () => { + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + setState(app, { + ...getState(app), + token: { + token: 'something', + expireTimeMillis: Date.now() - 1000, + issuedAtTimeMillis: 0 + } + }); + + stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); + stub(client, 'exchangeToken').returns( + Promise.resolve({ + token: 'new-recaptcha-app-check-token', + expireTimeMillis: Date.now() + 60000, + issuedAtTimeMillis: 0 + }) + ); + + expect(await getToken(appCheck as AppCheckService)).to.deep.equal({ + token: 'new-recaptcha-app-check-token' + }); + }); + + it('returns the valid token in storage without making a network request', async () => { + const clock = useFakeTimers(); + + storageReadStub.resolves(fakeCachedAppCheckToken); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + + const clientStub = stub(client, 'exchangeToken'); + expect(await getToken(appCheck as AppCheckService)).to.deep.equal({ + token: fakeCachedAppCheckToken.token + }); + expect(clientStub).to.not.have.been.called; + + clock.restore(); + }); + + it('deletes cached token if it is invalid and continues to exchange request', async () => { + storageReadStub.resolves({ + token: 'something', + expireTimeMillis: Date.now() - 1000, + issuedAtTimeMillis: 0 + }); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + + const freshToken = { + token: 'new-recaptcha-app-check-token', + expireTimeMillis: Date.now() + 60000, + issuedAtTimeMillis: 0 + }; + + stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); + stub(client, 'exchangeToken').returns(Promise.resolve(freshToken)); + + expect(await getToken(appCheck as AppCheckService)).to.deep.equal({ + token: 'new-recaptcha-app-check-token' + }); + + // When it wiped the invalid token. + expect(storageWriteStub).has.been.calledWith(app, undefined); + + // When it wrote the new token fetched from the exchange endpoint. + expect(storageWriteStub).has.been.calledWith(app, freshToken); + }); + + it('returns the actual token and an internalError if a token is valid but the request fails', async () => { + stub(logger, 'error'); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) + }); + setState(app, { ...getState(app), token: fakeRecaptchaAppCheckToken }); + + stub(reCAPTCHA, 'getToken').returns(Promise.resolve(fakeRecaptchaToken)); + stub(client, 'exchangeToken').returns(Promise.reject(new Error('blah'))); + + const tokenResult = await getToken(appCheck as AppCheckService, true); + expect(tokenResult.internalError?.message).to.equal('blah'); + expect(tokenResult.token).to.equal('fake-recaptcha-app-check-token'); + }); + it('exchanges debug token if in debug mode and there is no cached token', async () => { const exchangeTokenStub: SinonStub = stub( client, @@ -534,6 +623,205 @@ describe('internal api', () => { fakeListener ); }); + + it('does not make rapid requests within proactive refresh window', async () => { + const clock = useFakeTimers(); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), + isTokenAutoRefreshEnabled: true + }); + setState(app, { + ...getState(app), + token: { + token: `fake-cached-app-check-token`, + // within refresh window + expireTimeMillis: 10000, + issuedAtTimeMillis: 0 + } + }); + + const fakeListener: AppCheckTokenListener = stub(); + + const fakeExchange = stub(client, 'exchangeToken').returns( + Promise.resolve({ + token: 'new-recaptcha-app-check-token', + expireTimeMillis: 10 * 60 * 1000, + issuedAtTimeMillis: 0 + }) + ); + + addTokenListener( + appCheck as AppCheckService, + ListenerType.INTERNAL, + fakeListener + ); + // Tick 10s, make sure nothing is called repeatedly in that time. + await clock.tickAsync(10000); + expect(fakeListener).to.be.calledWith({ + token: 'fake-cached-app-check-token' + }); + expect(fakeListener).to.be.calledWith({ + token: 'new-recaptcha-app-check-token' + }); + expect(fakeExchange).to.be.calledOnce; + clock.restore(); + }); + + it('proactive refresh window test - exchange request fails - wait 10s', async () => { + stub(logger, 'error'); + const clock = useFakeTimers(); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), + isTokenAutoRefreshEnabled: true + }); + setState(app, { + ...getState(app), + token: { + token: `fake-cached-app-check-token`, + // not expired but within refresh window + expireTimeMillis: 10000, + issuedAtTimeMillis: 0 + } + }); + + const fakeListener: AppCheckTokenListener = stub(); + + const fakeExchange = stub(client, 'exchangeToken').returns( + Promise.reject(new Error('fetch failed or something')) + ); + + addTokenListener( + appCheck as AppCheckService, + ListenerType.EXTERNAL, + fakeListener + ); + // Tick 10s, make sure nothing is called repeatedly in that time. + await clock.tickAsync(10000); + expect(fakeListener).to.be.calledWith({ + token: 'fake-cached-app-check-token' + }); + // once on init and once invoked directly in this test + expect(fakeListener).to.be.calledTwice; + expect(fakeExchange).to.be.calledOnce; + clock.restore(); + }); + + it('proactive refresh window test - exchange request fails - wait 40s', async () => { + stub(logger, 'error'); + const clock = useFakeTimers(); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), + isTokenAutoRefreshEnabled: true + }); + setState(app, { + ...getState(app), + token: { + token: `fake-cached-app-check-token`, + // not expired but within refresh window + expireTimeMillis: 10000, + issuedAtTimeMillis: 0 + } + }); + + const fakeListener: AppCheckTokenListener = stub(); + + const fakeExchange = stub(client, 'exchangeToken').returns( + Promise.reject(new Error('fetch failed or something')) + ); + + addTokenListener( + appCheck as AppCheckService, + ListenerType.EXTERNAL, + fakeListener + ); + // Tick 40s, expect one initial exchange request and one retry. + // (First backoff is 30s). + await clock.tickAsync(40000); + expect(fakeListener).to.be.calledTwice; + expect(fakeExchange).to.be.calledTwice; + clock.restore(); + }); + + it('expired token - exchange request fails - wait 10s', async () => { + stub(logger, 'error'); + const clock = useFakeTimers(); + clock.tick(1); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), + isTokenAutoRefreshEnabled: true + }); + setState(app, { + ...getState(app), + token: { + token: `fake-cached-app-check-token`, + // expired + expireTimeMillis: 0, + issuedAtTimeMillis: 0 + } + }); + + const fakeListener = stub(); + const errorHandler = stub(); + const fakeNetworkError = new Error('fetch failed or something'); + + const fakeExchange = stub(client, 'exchangeToken').returns( + Promise.reject(fakeNetworkError) + ); + + addTokenListener( + appCheck as AppCheckService, + ListenerType.EXTERNAL, + fakeListener, + errorHandler + ); + // Tick 10s, make sure nothing is called repeatedly in that time. + await clock.tickAsync(10000); + expect(fakeListener).not.to.be.called; + expect(fakeExchange).to.be.calledOnce; + expect(errorHandler).to.be.calledWith(fakeNetworkError); + clock.restore(); + }); + + it('expired token - exchange request fails - wait 40s', async () => { + stub(logger, 'error'); + const clock = useFakeTimers(); + clock.tick(1); + const appCheck = initializeAppCheck(app, { + provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), + isTokenAutoRefreshEnabled: true + }); + setState(app, { + ...getState(app), + token: { + token: `fake-cached-app-check-token`, + // expired + expireTimeMillis: 0, + issuedAtTimeMillis: 0 + } + }); + + const fakeListener = stub(); + const errorHandler = stub(); + const fakeNetworkError = new Error('fetch failed or something'); + + const fakeExchange = stub(client, 'exchangeToken').returns( + Promise.reject(fakeNetworkError) + ); + + addTokenListener( + appCheck as AppCheckService, + ListenerType.EXTERNAL, + fakeListener, + errorHandler + ); + // Tick 40s, expect one initial exchange request and one retry. + // (First backoff is 30s). + await clock.tickAsync(40000); + expect(fakeListener).not.to.be.called; + expect(fakeExchange).to.be.calledTwice; + expect(errorHandler).to.be.calledTwice; + clock.restore(); + }); }); describe('removeTokenListener', () => { diff --git a/packages/app-check/src/internal-api.ts b/packages/app-check/src/internal-api.ts index 6b086c675cd..99c0f5d2c00 100644 --- a/packages/app-check/src/internal-api.ts +++ b/packages/app-check/src/internal-api.ts @@ -74,13 +74,27 @@ export async function getToken( let error: Error | undefined = undefined; /** - * If there is no token in memory, try to load token from indexedDB. + * If an invalid token was found in memory, clear token from + * memory and unset the local variable `token`. + */ + if (token && !isValid(token)) { + setState(app, { ...state, token: undefined }); + token = undefined; + } + + /** + * If there is no valid token in memory, try to load token from indexedDB. */ if (!token) { // cachedTokenPromise contains the token found in IndexedDB or undefined if not found. const cachedToken = await state.cachedTokenPromise; - if (cachedToken && isValid(cachedToken)) { - token = cachedToken; + if (cachedToken) { + if (isValid(cachedToken)) { + token = cachedToken; + } else { + // If there was an invalid token in the indexedDB cache, clear it. + await writeTokenToStorage(app, undefined); + } } } @@ -107,9 +121,9 @@ export async function getToken( state.exchangeTokenPromise = exchangeToken( getExchangeDebugTokenRequest(app, await getDebugToken()), appCheck.heartbeatServiceProvider - ).then(token => { + ).finally(() => { + // Clear promise when settled - either resolved or rejected. state.exchangeTokenPromise = undefined; - return token; }); shouldCallListeners = true; } @@ -123,7 +137,9 @@ export async function getToken( } /** - * request a new token + * There are no valid tokens in memory or indexedDB and we are not in + * debug mode. + * Request a new token from the exchange endpoint. */ try { // Avoid making another call to the exchange endpoint if one is in flight. @@ -131,9 +147,9 @@ export async function getToken( // state.provider is populated in initializeAppCheck() // ensureActivated() at the top of this function checks that // initializeAppCheck() has been called. - state.exchangeTokenPromise = state.provider!.getToken().then(token => { + state.exchangeTokenPromise = state.provider!.getToken().finally(() => { + // Clear promise when settled - either resolved or rejected. state.exchangeTokenPromise = undefined; - return token; }); shouldCallListeners = true; } @@ -152,9 +168,27 @@ export async function getToken( let interopTokenResult: AppCheckTokenResult | undefined; if (!token) { - // if token is undefined, there must be an error. - // we return a dummy token along with the error + // If token is undefined, there must be an error. + // Return a dummy token along with the error. interopTokenResult = makeDummyTokenResult(error!); + } else if (error) { + if (isValid(token)) { + // It's also possible a valid token exists, but there's also an error. + // (Such as if the token is almost expired, tries to refresh, and + // the exchange request fails.) + // We add a special error property here so that the refresher will + // count this as a failed attempt and use the backoff instead of + // retrying repeatedly with no delay, but any 3P listeners will not + // be hindered in getting the still-valid token. + interopTokenResult = { + token: token.token, + internalError: error + }; + } else { + // No invalid tokens should make it to this step. Memory and cached tokens + // are checked. Other tokens are from fresh exchanges. But just in case. + interopTokenResult = makeDummyTokenResult(error!); + } } else { interopTokenResult = { token: token.token @@ -274,10 +308,24 @@ function createTokenRefresher(appCheck: AppCheckService): Refresher { result = await getToken(appCheck, true); } - // getToken() always resolves. In case the result has an error field defined, it means the operation failed, and we should retry. + /** + * getToken() always resolves. In case the result has an error field defined, it means + * the operation failed, and we should retry. + */ if (result.error) { throw result.error; } + /** + * A special `internalError` field reflects that there was an error + * getting a new token from the exchange endpoint, but there's still a + * previous token that's valid for now and this should be passed to 2P/3P + * requests for a token. But we want this callback (`this.operation` in + * `Refresher`) to throw in order to kick off the Refresher's retry + * backoff. (Setting `hasSucceeded` to false.) + */ + if (result.internalError) { + throw result.internalError; + } }, () => { return true; diff --git a/packages/app-check/src/storage.ts b/packages/app-check/src/storage.ts index 0acc5ddc218..5c680a08393 100644 --- a/packages/app-check/src/storage.ts +++ b/packages/app-check/src/storage.ts @@ -51,7 +51,7 @@ export async function readTokenFromStorage( */ export function writeTokenToStorage( app: FirebaseApp, - token: AppCheckTokenInternal + token?: AppCheckTokenInternal ): Promise { if (isIndexedDBAvailable()) { return writeTokenToIndexedDB(app, token).catch(e => { diff --git a/packages/app-check/src/types.ts b/packages/app-check/src/types.ts index 534509d04d4..e4292f2d51a 100644 --- a/packages/app-check/src/types.ts +++ b/packages/app-check/src/types.ts @@ -52,6 +52,10 @@ export const enum ListenerType { export interface AppCheckTokenResult { readonly token: string; readonly error?: Error; + // Error that should not be propagated to 3P listeners, e.g. exchange + // request errors during proactive refresh period, while the token + // is still valid. + readonly internalError?: Error; } export interface AppCheckTokenInternal extends AppCheckToken {