From 1ac6490c51953810180cf434409d5c5dc646e624 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Wed, 12 Mar 2025 15:46:56 -0700 Subject: [PATCH 1/3] Fix surfacing of public getToken errors --- packages/app-check/src/api.ts | 3 +++ packages/app-check/src/errors.ts | 7 +++++-- packages/app-check/src/internal-api.test.ts | 17 ----------------- packages/app-check/src/internal-api.ts | 6 ------ packages/app-check/src/providers.ts | 4 ++-- 5 files changed, 10 insertions(+), 27 deletions(-) diff --git a/packages/app-check/src/api.ts b/packages/app-check/src/api.ts index e6897320be1..a4dd87a4e77 100644 --- a/packages/app-check/src/api.ts +++ b/packages/app-check/src/api.ts @@ -209,6 +209,9 @@ export async function getToken( if (result.error) { throw result.error; } + if (result.internalError) { + throw result.internalError; + } return { token: result.token }; } diff --git a/packages/app-check/src/errors.ts b/packages/app-check/src/errors.ts index c6f088b371b..7ccdadc38ab 100644 --- a/packages/app-check/src/errors.ts +++ b/packages/app-check/src/errors.ts @@ -27,7 +27,8 @@ export const enum AppCheckError { STORAGE_GET = 'storage-get', STORAGE_WRITE = 'storage-set', RECAPTCHA_ERROR = 'recaptcha-error', - THROTTLED = 'throttled' + THROTTLED = 'throttled', + INITIAL_THROTTLE = 'initial-throttle' } const ERRORS: ErrorMap = { @@ -54,7 +55,8 @@ const ERRORS: ErrorMap = { [AppCheckError.STORAGE_WRITE]: 'Error thrown when writing to storage. Original error: {$originalErrorMessage}.', [AppCheckError.RECAPTCHA_ERROR]: 'ReCAPTCHA error.', - [AppCheckError.THROTTLED]: `Requests throttled due to {$httpStatus} error. Attempts allowed again after {$time}` + [AppCheckError.INITIAL_THROTTLE]: `{$httpStatus} error. Attempts allowed again after {$time}`, + [AppCheckError.THROTTLED]: `Requests throttled due to previous {$httpStatus} error. Attempts allowed again after {$time}` }; interface ErrorParams { @@ -67,6 +69,7 @@ interface ErrorParams { [AppCheckError.STORAGE_GET]: { originalErrorMessage?: string }; [AppCheckError.STORAGE_WRITE]: { originalErrorMessage?: string }; [AppCheckError.THROTTLED]: { time: string; httpStatus: number }; + [AppCheckError.INITIAL_THROTTLE]: { time: string; httpStatus: number }; } export const ERROR_FACTORY = new ErrorFactory( diff --git a/packages/app-check/src/internal-api.test.ts b/packages/app-check/src/internal-api.test.ts index 1e43a5e7e21..0de6f052a59 100644 --- a/packages/app-check/src/internal-api.test.ts +++ b/packages/app-check/src/internal-api.test.ts @@ -153,7 +153,6 @@ describe('internal api', () => { }); it('resolves with a dummy token and an error if failed to get a token', async () => { - const errorStub = stub(console, 'error'); const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) }); @@ -170,14 +169,9 @@ describe('internal api', () => { token: formatDummyToken(defaultTokenErrorData), error }); - expect(errorStub.args[0][1].message).to.include( - 'oops, something went wrong' - ); - errorStub.restore(); }); it('resolves with a dummy token and an error if failed to get a token in debug mode', async () => { - const errorStub = stub(console, 'error'); window.FIREBASE_APPCHECK_DEBUG_TOKEN = true; const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) @@ -192,15 +186,10 @@ describe('internal api', () => { token: formatDummyToken(defaultTokenErrorData), error }); - expect(errorStub.args[0][1].message).to.include( - 'oops, something went wrong' - ); delete window.FIREBASE_APPCHECK_DEBUG_TOKEN; - errorStub.restore(); }); it('resolves with a dummy token and an error if recaptcha failed', async () => { - const errorStub = stub(console, 'error'); const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) }); @@ -213,10 +202,6 @@ describe('internal api', () => { expect(reCAPTCHASpy).to.be.called; expect(exchangeTokenStub).to.not.be.called; expect(token.token).to.equal(formatDummyToken(defaultTokenErrorData)); - expect(errorStub.args[0][1].message).to.include( - AppCheckError.RECAPTCHA_ERROR - ); - errorStub.restore(); }); it('notifies listeners using cached token', async () => { @@ -290,7 +275,6 @@ describe('internal api', () => { }); it('calls 3P error handler if there is an error getting a token', async () => { - stub(console, 'error'); const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), isTokenAutoRefreshEnabled: true @@ -314,7 +298,6 @@ describe('internal api', () => { }); it('ignores listeners that throw', async () => { - stub(console, 'error'); const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY), isTokenAutoRefreshEnabled: true diff --git a/packages/app-check/src/internal-api.ts b/packages/app-check/src/internal-api.ts index 4eb3953614a..3800b91d5e1 100644 --- a/packages/app-check/src/internal-api.ts +++ b/packages/app-check/src/internal-api.ts @@ -139,9 +139,6 @@ export async function getToken( if ((e as FirebaseError).code === `appCheck/${AppCheckError.THROTTLED}`) { // Warn if throttled, but do not treat it as an error. logger.warn((e as FirebaseError).message); - } else { - // `getToken()` should never throw, but logging error text to console will aid debugging. - logger.error(e); } // Return dummy token and error return makeDummyTokenResult(e as FirebaseError); @@ -170,9 +167,6 @@ export async function getToken( if ((e as FirebaseError).code === `appCheck/${AppCheckError.THROTTLED}`) { // Warn if throttled, but do not treat it as an error. logger.warn((e as FirebaseError).message); - } else { - // `getToken()` should never throw, but logging error text to console will aid debugging. - logger.error(e); } // Always save error to be added to dummy token. error = e as FirebaseError; diff --git a/packages/app-check/src/providers.ts b/packages/app-check/src/providers.ts index 55ab598b5e9..e8d2eb5af5f 100644 --- a/packages/app-check/src/providers.ts +++ b/packages/app-check/src/providers.ts @@ -92,7 +92,7 @@ export class ReCaptchaV3Provider implements AppCheckProvider { Number((e as FirebaseError).customData?.httpStatus), this._throttleData ); - throw ERROR_FACTORY.create(AppCheckError.THROTTLED, { + throw ERROR_FACTORY.create(AppCheckError.INITIAL_THROTTLE, { time: getDurationString( this._throttleData.allowRequestsAfter - Date.now() ), @@ -185,7 +185,7 @@ export class ReCaptchaEnterpriseProvider implements AppCheckProvider { Number((e as FirebaseError).customData?.httpStatus), this._throttleData ); - throw ERROR_FACTORY.create(AppCheckError.THROTTLED, { + throw ERROR_FACTORY.create(AppCheckError.INITIAL_THROTTLE, { time: getDurationString( this._throttleData.allowRequestsAfter - Date.now() ), From 47460e71521b1c1cc35add0aca02168bd422de66 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Wed, 12 Mar 2025 16:15:14 -0700 Subject: [PATCH 2/3] Fix tests and clean up --- .changeset/fluffy-otters-whisper.md | 5 +++++ packages/app-check/src/internal-api.test.ts | 21 ++++++++++++++++++--- packages/app-check/src/internal-api.ts | 18 +++++++++++++++--- 3 files changed, 38 insertions(+), 6 deletions(-) create mode 100644 .changeset/fluffy-otters-whisper.md diff --git a/.changeset/fluffy-otters-whisper.md b/.changeset/fluffy-otters-whisper.md new file mode 100644 index 00000000000..863b608875b --- /dev/null +++ b/.changeset/fluffy-otters-whisper.md @@ -0,0 +1,5 @@ +--- +'@firebase/app-check': patch +--- + +Improve error handling in AppCheck. The publicly-exported `getToken()` will now throw `internalError` strings it was previously ignoring. diff --git a/packages/app-check/src/internal-api.test.ts b/packages/app-check/src/internal-api.test.ts index 0de6f052a59..5d6b88f1c32 100644 --- a/packages/app-check/src/internal-api.test.ts +++ b/packages/app-check/src/internal-api.test.ts @@ -153,6 +153,7 @@ describe('internal api', () => { }); it('resolves with a dummy token and an error if failed to get a token', async () => { + const errorStub = stub(console, 'error'); const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) }); @@ -162,16 +163,21 @@ describe('internal api', () => { const error = new Error('oops, something went wrong'); stub(client, 'exchangeToken').returns(Promise.reject(error)); - const token = await getToken(appCheck as AppCheckService); + const token = await getToken(appCheck as AppCheckService, false, true); expect(reCAPTCHASpy).to.be.called; expect(token).to.deep.equal({ token: formatDummyToken(defaultTokenErrorData), error }); + expect(errorStub.args[0][1].message).to.include( + 'oops, something went wrong' + ); + errorStub.restore(); }); it('resolves with a dummy token and an error if failed to get a token in debug mode', async () => { + const errorStub = stub(console, 'error'); window.FIREBASE_APPCHECK_DEBUG_TOKEN = true; const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) @@ -180,16 +186,21 @@ describe('internal api', () => { const error = new Error('oops, something went wrong'); stub(client, 'exchangeToken').returns(Promise.reject(error)); - const token = await getToken(appCheck as AppCheckService); + const token = await getToken(appCheck as AppCheckService, false, true); expect(token).to.deep.equal({ token: formatDummyToken(defaultTokenErrorData), error }); + expect(errorStub.args[0][1].message).to.include( + 'oops, something went wrong' + ); delete window.FIREBASE_APPCHECK_DEBUG_TOKEN; + errorStub.restore(); }); it('resolves with a dummy token and an error if recaptcha failed', async () => { + const errorStub = stub(console, 'error'); const appCheck = initializeAppCheck(app, { provider: new ReCaptchaV3Provider(FAKE_SITE_KEY) }); @@ -197,11 +208,15 @@ describe('internal api', () => { const reCAPTCHASpy = stubGetRecaptchaToken('', false); const exchangeTokenStub = stub(client, 'exchangeToken'); - const token = await getToken(appCheck as AppCheckService); + const token = await getToken(appCheck as AppCheckService, false, true); expect(reCAPTCHASpy).to.be.called; expect(exchangeTokenStub).to.not.be.called; expect(token.token).to.equal(formatDummyToken(defaultTokenErrorData)); + expect(errorStub.args[0][1].message).to.include( + AppCheckError.RECAPTCHA_ERROR + ); + errorStub.restore(); }); it('notifies listeners using cached token', async () => { diff --git a/packages/app-check/src/internal-api.ts b/packages/app-check/src/internal-api.ts index 3800b91d5e1..eddf043c843 100644 --- a/packages/app-check/src/internal-api.ts +++ b/packages/app-check/src/internal-api.ts @@ -60,7 +60,8 @@ export function formatDummyToken( */ export async function getToken( appCheck: AppCheckService, - forceRefresh = false + forceRefresh = false, + shouldLogErrors = false ): Promise { const app = appCheck.app; ensureActivated(app); @@ -136,9 +137,15 @@ export async function getToken( state.token = tokenFromDebugExchange; return { token: tokenFromDebugExchange.token }; } catch (e) { - if ((e as FirebaseError).code === `appCheck/${AppCheckError.THROTTLED}`) { + if ( + (e as FirebaseError).code === `appCheck/${AppCheckError.THROTTLED}` || + (e as FirebaseError).code === + `appCheck/${AppCheckError.INITIAL_THROTTLE}` + ) { // Warn if throttled, but do not treat it as an error. logger.warn((e as FirebaseError).message); + } else if (shouldLogErrors) { + logger.error(e); } // Return dummy token and error return makeDummyTokenResult(e as FirebaseError); @@ -164,9 +171,14 @@ export async function getToken( } token = await getStateReference(app).exchangeTokenPromise; } catch (e) { - if ((e as FirebaseError).code === `appCheck/${AppCheckError.THROTTLED}`) { + if ( + (e as FirebaseError).code === `appCheck/${AppCheckError.THROTTLED}` || + (e as FirebaseError).code === `appCheck/${AppCheckError.INITIAL_THROTTLE}` + ) { // Warn if throttled, but do not treat it as an error. logger.warn((e as FirebaseError).message); + } else if (shouldLogErrors) { + logger.error(e); } // Always save error to be added to dummy token. error = e as FirebaseError; From f5a1e4d43107185decb330bb9aabfe216590afc2 Mon Sep 17 00:00:00 2001 From: Christina Holland Date: Mon, 17 Mar 2025 15:37:39 -0700 Subject: [PATCH 3/3] reorder error properties --- packages/app-check/src/errors.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/app-check/src/errors.ts b/packages/app-check/src/errors.ts index 7ccdadc38ab..ca5a60aed6b 100644 --- a/packages/app-check/src/errors.ts +++ b/packages/app-check/src/errors.ts @@ -27,8 +27,8 @@ export const enum AppCheckError { STORAGE_GET = 'storage-get', STORAGE_WRITE = 'storage-set', RECAPTCHA_ERROR = 'recaptcha-error', - THROTTLED = 'throttled', - INITIAL_THROTTLE = 'initial-throttle' + INITIAL_THROTTLE = 'initial-throttle', + THROTTLED = 'throttled' } const ERRORS: ErrorMap = { @@ -68,8 +68,8 @@ interface ErrorParams { [AppCheckError.STORAGE_OPEN]: { originalErrorMessage?: string }; [AppCheckError.STORAGE_GET]: { originalErrorMessage?: string }; [AppCheckError.STORAGE_WRITE]: { originalErrorMessage?: string }; - [AppCheckError.THROTTLED]: { time: string; httpStatus: number }; [AppCheckError.INITIAL_THROTTLE]: { time: string; httpStatus: number }; + [AppCheckError.THROTTLED]: { time: string; httpStatus: number }; } export const ERROR_FACTORY = new ErrorFactory(