diff --git a/CHANGELOG.md b/CHANGELOG.md index 27324d997ee..53158af08ff 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,2 +1,3 @@ - Added support for a two-phase non-interactive login flow. Initiate this by running `firebase login --non-interactive`, navigate to the printed link to get an authorization code, and complete the login by running `firebase login `. - Fixed `apps:init` writing `google-services.json` to an `app/app` path when the Android module has no `src` directory, by detecting the module from the directory basename instead of the first path segment (#10863). +- Fixed `firestore.get()`/`firestore.exists()` in Storage rules reporting a failed request as "document not found", which could cause rules to deny access on the strength of a transport failure. Only a confirmed 404 is now treated as a real answer; other failures are retried. diff --git a/src/emulator/storage/rules/runtime.spec.ts b/src/emulator/storage/rules/runtime.spec.ts index 6b854a7acc7..e5c5c2cb0b8 100644 --- a/src/emulator/storage/rules/runtime.spec.ts +++ b/src/emulator/storage/rules/runtime.spec.ts @@ -1,6 +1,8 @@ import { expect } from "chai"; -import { createAuthExpressionValue, StorageRulesRuntime } from "./runtime"; -import { RulesetOperationMethod, RuntimeActionResponse } from "./types"; +import * as sinon from "sinon"; +import { createAuthExpressionValue, fetchFirestoreDocument, StorageRulesRuntime } from "./runtime"; +import { DataLoadStatus, RulesetOperationMethod, RuntimeActionResponse } from "./types"; +import { EmulatorRegistry } from "../../registry"; // Reaches the private stdout handler and pending-request map so we can drive the // framing logic directly, without spawning the Java rules runtime. @@ -102,4 +104,95 @@ describe("Storage Rules Runtime", () => { expect(received).to.deep.equal([1, 2]); }); }); + + describe("fetchFirestoreDocument", () => { + let sandbox: sinon.SinonSandbox; + + beforeEach(() => { + sandbox = sinon.createSandbox(); + }); + + afterEach(() => { + sandbox.restore(); + }); + + function stubFirestoreClient(get: sinon.SinonStub) { + sandbox.stub(EmulatorRegistry, "client").returns({ get } as any); + } + + function fakeRequest(path = "/documents/jobs/job1") { + return { + action: "fetch_firestore_document" as const, + context: { path }, + warnings: [], + errors: [], + }; + } + + it("returns the document immediately on a successful first attempt", async () => { + const get = sandbox.stub().resolves({ body: { name: "jobs/job1", fields: {} } }); + stubFirestoreClient(get); + + const response = await fetchFirestoreDocument("proj", fakeRequest()); + + expect(response.status).to.equal(DataLoadStatus.OK); + expect(get.callCount).to.equal(1); + }); + + it("returns not_found immediately on a confirmed 404, without retrying", async () => { + // A 404 from a server that's actually up is a real answer — it must + // not be retried, both to keep the common "does this exist yet" + // check fast and to prove the fix doesn't change that fast path. + const notFound = Object.assign(new Error("Not Found"), { status: 404 }); + const get = sandbox.stub().rejects(notFound); + stubFirestoreClient(get); + + const response = await fetchFirestoreDocument("proj", fakeRequest()); + + expect(response.status).to.equal(DataLoadStatus.NOT_FOUND); + expect(get.callCount).to.equal(1); + }); + + it("retries past a transient connection failure and succeeds once the server is reachable", async () => { + // A request that fails to reach the server says nothing about whether + // the document exists; previously any such failure was reported as an + // immediate, permanent "not found". + const connectionRefused = Object.assign(new Error("connect ECONNREFUSED 127.0.0.1:8080"), { + code: "ECONNREFUSED", + }); + const get = sandbox.stub(); + get.onCall(0).rejects(connectionRefused); + get.onCall(1).rejects(connectionRefused); + get.onCall(2).resolves({ body: { name: "jobs/job1", fields: {} } }); + stubFirestoreClient(get); + + const response = await fetchFirestoreDocument("proj", fakeRequest()); + + expect(response.status).to.equal(DataLoadStatus.OK); + expect(get.callCount).to.equal(3); + }); + + it("returns not_found without retrying when the response body is malformed", async () => { + // A bad payload is a schema/programming problem, not a transient network + // one — retrying it would waste time and disguise the real cause. + const get = sandbox.stub().resolves({ body: undefined }); + stubFirestoreClient(get); + + const response = await fetchFirestoreDocument("proj", fakeRequest()); + + expect(response.status).to.equal(DataLoadStatus.NOT_FOUND); + expect(get.callCount).to.equal(1); + }); + + it("gives up and returns not_found after exhausting retries on a persistent non-404 failure", async () => { + const timeout = new Error("timeout"); + const get = sandbox.stub().rejects(timeout); + stubFirestoreClient(get); + + const response = await fetchFirestoreDocument("proj", fakeRequest()); + + expect(response.status).to.equal(DataLoadStatus.NOT_FOUND); + expect(get.callCount).to.equal(3); + }); + }); }); diff --git a/src/emulator/storage/rules/runtime.ts b/src/emulator/storage/rules/runtime.ts index 73c7bd6b35a..c31f24cadd8 100644 --- a/src/emulator/storage/rules/runtime.ts +++ b/src/emulator/storage/rules/runtime.ts @@ -488,22 +488,65 @@ function toExpressionValue(obj: any): ExpressionValue { ); } -async function fetchFirestoreDocument( +// A request that never completed says nothing about whether the document +// exists, so it isn't reported as "not found" on the first failure. Retried +// a small, bounded number of times instead — enough to ride out a brief +// blip without noticeably delaying a genuine failure. +const FETCH_FIRESTORE_DOCUMENT_MAX_ATTEMPTS = 3; +const FETCH_FIRESTORE_DOCUMENT_RETRY_DELAY_MS = 150; + +/** Narrows a thrown value to one carrying an HTTP status code (e.g. FirebaseError). */ +function hasHttpStatus(e: unknown): e is { status: number } { + return ( + typeof e === "object" && + e !== null && + "status" in e && + typeof (e as Record).status === "number" + ); +} + +/** + * Fetches a Firestore document for a `firestore.get()`/`firestore.exists()` + * call made from within a Storage rules evaluation. Exported for unit + * testing only; not part of this module's public surface. + * @internal + */ +export async function fetchFirestoreDocument( projectId: string, request: RuntimeActionFirestoreDataRequest, ): Promise { const pathname = `projects/${projectId}${request.context.path}`; const client = EmulatorRegistry.client(Emulators.FIRESTORE, { apiVersion: "v1", auth: true }); - try { - const doc = await client.get(pathname); - const { name, fields } = doc.body as { name: string; fields: string }; - const result = { name, fields }; + for (let attempt = 1; attempt <= FETCH_FIRESTORE_DOCUMENT_MAX_ATTEMPTS; attempt++) { + let doc; + // Only the request itself is guarded here — parsing the response below is + // deliberately left outside, so a malformed payload surfaces as the + // programming/schema error it is rather than being retried as though it + // were a transient network failure. + try { + doc = await client.get(pathname); + } catch (e: unknown) { + // A confirmed 404 is a real answer about the document, so it's returned + // straight away — only failures that leave the answer unknown are retried. + const isConfirmedNotFound = hasHttpStatus(e) && e.status === 404; + const isLastAttempt = attempt === FETCH_FIRESTORE_DOCUMENT_MAX_ATTEMPTS; + if (isConfirmedNotFound || isLastAttempt) { + return { status: DataLoadStatus.NOT_FOUND, warnings: [], errors: [] }; + } + await utils.sleep(FETCH_FIRESTORE_DOCUMENT_RETRY_DELAY_MS); + continue; + } + + const body = doc.body as { name?: string; fields?: unknown } | undefined; + if (!body || typeof body.name !== "string") { + return { status: DataLoadStatus.NOT_FOUND, warnings: [], errors: [] }; + } + const result = { name: body.name, fields: body.fields }; return { result, status: DataLoadStatus.OK, warnings: [], errors: [] }; - } catch (e) { - // Don't care what the error is, just return not_found - return { status: DataLoadStatus.NOT_FOUND, warnings: [], errors: [] }; } + // Unreachable: the loop above always returns by its last iteration. + return { status: DataLoadStatus.NOT_FOUND, warnings: [], errors: [] }; } /**