Skip to content

Better handling for GFE errors #10313

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

Open
joehan wants to merge 1 commit into
base: main
Choose a base branch
from
+20 −0
Open

Better handling for GFE errors #10313

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
13 changes: 13 additions & 0 deletions src/apiv2.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
import { expect } from "chai";
import * as nock from "nock";
import AbortController from "abort-controller";
const proxySetup = require("proxy");

Check warning on line 5 in src/apiv2.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Require statement not part of import statement

Check warning on line 5 in src/apiv2.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe assignment of an `any` value

import { Client, CLI_OAUTH_PROJECT_NUMBER } from "./apiv2";
import { FirebaseError } from "./error";
Expand Down Expand Up @@ -162,6 +162,19 @@
expect(nock.isDone()).to.be.true;
});

it("should error with a specific message if response is a GFE error", async () => {
const gfeError = `<!DOCTYPE html>\n<html lang=en>\n<title>Error 404 (Not Found)!!1</title>\n<p><b>404.</b> <ins>That’s an error.</ins>\n<p>The requested URL /v1beta/projects/... was not found on this server. <ins>That’s all we know.</ins>\n`;
nock("https://example.com").get("/path/to/foo").reply(404, gfeError);

const c = new Client({ urlPrefix: "https://example.com" });
const r = c.request({
method: "GET",
path: "/path/to/foo",
});
await expect(r).to.eventually.be.rejectedWith(FirebaseError, "the service you are calling doesnt exist or is misconfigured");

Check failure on line 174 in src/apiv2.spec.ts

View workflow job for this annotation

GitHub Actions / unit (24) 

Replace `FirebaseError,·"the·service·you·are·calling·doesnt·exist·or·is·misconfigured"` with `⏎········FirebaseError,⏎········"the·service·you·are·calling·doesnt·exist·or·is·misconfigured",⏎······`

Check failure on line 174 in src/apiv2.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Replace `FirebaseError,·"the·service·you·are·calling·doesnt·exist·or·is·misconfigured"` with `⏎········FirebaseError,⏎········"the·service·you·are·calling·doesnt·exist·or·is·misconfigured",⏎······`

Check failure on line 174 in src/apiv2.spec.ts

View workflow job for this annotation

GitHub Actions / unit (24) 

Replace `FirebaseError,·"the·service·you·are·calling·doesnt·exist·or·is·misconfigured"` with `⏎········FirebaseError,⏎········"the·service·you·are·calling·doesnt·exist·or·is·misconfigured",⏎······`
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 10, 2026

Choose a reason for hiding this comment

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

medium

Updating the test expectation to match the corrected error message.

Suggested change
await expect(r).to.eventually.be.rejectedWith(FirebaseError, "the service you are calling doesnt exist or is misconfigured");
await expect(r).to.eventually.be.rejectedWith(FirebaseError, "The service you are calling doesn't exist or is misconfigured.");

expect(nock.isDone()).to.be.true;
});

it("should error with a FirebaseError if an error happens", async () => {
nock("https://example.com").get("/path/to/foo").replyWithError("boom");

Expand Down Expand Up @@ -488,7 +501,7 @@
let targetServer: Server;
let oldProxy: string | undefined;
before(async () => {
proxyServer = proxySetup(createServer());

Check warning on line 504 in src/apiv2.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe call of an `any` typed value

Check warning on line 504 in src/apiv2.spec.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe assignment of an `any` value
targetServer = createServer((req, res) => {
res.writeHead(200, { "content-type": "application/json" });
res.end(JSON.stringify({ proxied: true }));
Expand Down
7 changes: 7 additions & 0 deletions src/apiv2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@

// Using import would require resolveJsonModule, which seems to break the
// build/output format.
const pkg = require("../package.json");

Check warning on line 19 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Require statement not part of import statement

Check warning on line 19 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe assignment of an `any` value
const CLI_VERSION: string = pkg.version;

Check warning on line 20 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe member access .version on an `any` value

Check warning on line 20 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Unsafe assignment of an `any` value

export const standardHeaders: () => Record<string, string> = () => {
const agent = detectAIAgent();
Expand Down Expand Up @@ -134,7 +134,7 @@

/**
* Gets a singleton access token
* @returns An access token

Check warning on line 137 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Invalid JSDoc tag (preference). Replace "returns" JSDoc tag with "return"
*/
export async function getAccessToken(): Promise<string> {
const valid = auth.haveValidTokens(refreshToken, []);
Expand Down Expand Up @@ -239,7 +239,7 @@
* Makes a request as specified by the options.
* By default, this will:
* - use content-type: application/json
* - assume the HTTP GET method

Check warning on line 242 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Expected only 0 line after block description
*
* @example
* const res = apiv2.request<ResourceType>({
Expand Down Expand Up @@ -466,6 +466,9 @@

if (options.responseType === "json") {
const text = await res.text();
if (isGfeError(text)) {
throw new FirebaseError("the service you are calling doesnt exist or is misconfigured", { status: res.status });

Check failure on line 470 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / unit (24) 

Replace `"the·service·you·are·calling·doesnt·exist·or·is·misconfigured",·{·status:·res.status·}` with `⏎················"the·service·you·are·calling·doesnt·exist·or·is·misconfigured",⏎················{·status:·res.status·},⏎··············`

Check failure on line 470 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / lint (20) 

Replace `"the·service·you·are·calling·doesnt·exist·or·is·misconfigured",·{·status:·res.status·}` with `⏎················"the·service·you·are·calling·doesnt·exist·or·is·misconfigured",⏎················{·status:·res.status·},⏎··············`

Check failure on line 470 in src/apiv2.ts

View workflow job for this annotation

GitHub Actions / unit (24) 

Replace `"the·service·you·are·calling·doesnt·exist·or·is·misconfigured",·{·status:·res.status·}` with `⏎················"the·service·you·are·calling·doesnt·exist·or·is·misconfigured",⏎················{·status:·res.status·},⏎··············`
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 10, 2026

Choose a reason for hiding this comment

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

medium

The error message contains a typo ("doesnt") and should be properly capitalized for consistency with other user-facing errors in the codebase.

Suggested change
throw new FirebaseError("the service you are calling doesnt exist or is misconfigured", { status: res.status });
throw new FirebaseError("The service you are calling doesn't exist or is misconfigured.", { status: res.status });
References
  1. The repository style guide recommends using FirebaseError for user-facing errors; ensuring these messages are clear and correctly formatted maintains professional quality. (link)

}
// Some responses, such as 204 and occasionally 202s don't have
// any content. We can't just rely on response code (202 may have conent)
// and unfortuantely res.length is unreliable (many requests return zero).
Expand Down Expand Up @@ -580,6 +583,10 @@
return u.protocol === "http:";
}

function isGfeError(text: string): boolean {
return text.includes("That’s all we know.") && text.includes("<!DOCTYPE html>");
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist bot Apr 10, 2026

Choose a reason for hiding this comment

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

medium

Using a specific smart quote (’) in text.includes makes the detection brittle, as the response might use a standard apostrophe or different casing. A regular expression is more robust for identifying these error pages.

Suggested change
return text.includes("That’s all we know.") && text.includes("<!DOCTYPE html>");
return /That['’]s all we know/i.test(text) && /<!DOCTYPE html>/i.test(text);

}

function bodyToString(body: unknown): string {
if (isStream(body)) {
// Don't attempt to read any stream type, in case the caller needs it.
Expand Down
Loading