Skip to content

Surface all App Check errors in public getToken() method #8842

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

Merged
merged 3 commits into from
Mar 17, 2025
Merged

Surface all App Check errors in public getToken() method #8842

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
1ac6490
Fix surfacing of public getToken errors
hsubox76 Mar 12, 2025
47460e7
Fix tests and clean up
hsubox76 Mar 12, 2025
f5a1e4d
reorder error properties
hsubox76 Mar 17, 2025
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
5 changes: 5 additions & 0 deletions .changeset/fluffy-otters-whisper.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.
3 changes: 3 additions & 0 deletions packages/app-check/src/api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,9 @@ export async function getToken(
if (result.error) {
throw result.error;
}
if (result.internalError) {
throw result.internalError;
}
return { token: result.token };
}

Expand Down
5 changes: 4 additions & 1 deletion packages/app-check/src/errors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const enum AppCheckError {
STORAGE_GET = 'storage-get',
STORAGE_WRITE = 'storage-set',
RECAPTCHA_ERROR = 'recaptcha-error',
INITIAL_THROTTLE = 'initial-throttle',
THROTTLED = 'throttled'
}

Expand Down Expand Up @@ -54,7 +55,8 @@ const ERRORS: ErrorMap<AppCheckError> = {
[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}`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, these should be transposed to match the order in ErrorParams and AppCheckError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Flipped it around the other way on the other two since logically I guess the "initial throttle" should come first.

};

interface ErrorParams {
Expand All @@ -66,6 +68,7 @@ interface ErrorParams {
[AppCheckError.STORAGE_OPEN]: { originalErrorMessage?: string };
[AppCheckError.STORAGE_GET]: { originalErrorMessage?: string };
[AppCheckError.STORAGE_WRITE]: { originalErrorMessage?: string };
[AppCheckError.INITIAL_THROTTLE]: { time: string; httpStatus: number };
[AppCheckError.THROTTLED]: { time: string; httpStatus: number };
}

Expand Down
8 changes: 3 additions & 5 deletions packages/app-check/src/internal-api.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ 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({
Expand All @@ -186,7 +186,7 @@ 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),
Expand All @@ -208,7 +208,7 @@ 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;
Expand Down Expand Up @@ -290,7 +290,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
Expand All @@ -314,7 +313,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
Expand Down
20 changes: 13 additions & 7 deletions packages/app-check/src/internal-api.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,8 @@ export function formatDummyToken(
*/
export async function getToken(
appCheck: AppCheckService,
forceRefresh = false
forceRefresh = false,
shouldLogErrors = false
): Promise<AppCheckTokenResult> {
const app = appCheck.app;
ensureActivated(app);
Expand Down Expand Up @@ -136,11 +137,14 @@ 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 {
// `getToken()` should never throw, but logging error text to console will aid debugging.
} else if (shouldLogErrors) {
logger.error(e);
}
// Return dummy token and error
Expand All @@ -167,11 +171,13 @@ 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 {
// `getToken()` should never throw, but logging error text to console will aid debugging.
} else if (shouldLogErrors) {
logger.error(e);
}
// Always save error to be added to dummy token.
Expand Down
4 changes: 2 additions & 2 deletions packages/app-check/src/providers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
),
Expand Down Expand Up @@ -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()
),
Expand Down
Loading