fix(storage emulator): don't report request failures as "document not found" in firestore.get()

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 <auth_code>`.
 - 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.

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);
+    });
+  });
 });

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<string, unknown>).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<RuntimeActionFirestoreDataResponse> {
   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: [] };
 }
 
 /**