From 6244cd4179bbe772aa9b2af178f100532778d7bf Mon Sep 17 00:00:00 2001 From: Joe Hanley Date: Fri, 23 Oct 2026 01:40:24 +0000 Subject: [PATCH 1/4] fix/refactor: [Task] Fix no-misused-promises violations (Express 4 async handlers, listeners, streams) and remove its warn override (b/576834648) --- .eslintrc.js | 1 - CHANGELOG.md | 2 + src/auth.ts | 12 +- src/database/import.ts | 12 +- .../functions/runtimes/discovery/index.ts | 7 +- src/deploy/functions/services/index.ts | 23 +- src/emulator/commandUtils.ts | 7 +- src/emulator/databaseEmulator.ts | 43 ++-- src/emulator/dataconnect/pgliteServer.ts | 80 ++++--- src/emulator/downloadableEmulators.ts | 5 +- src/emulator/firestoreEmulator.ts | 42 ++-- src/emulator/functionsEmulator.ts | 24 +- src/emulator/functionsEmulatorRuntime.ts | 7 +- src/emulator/hub.ts | 165 +++++++------- src/emulator/shared/express.ts | 14 ++ src/emulator/storage/apis/firebase.ts | 214 +++++++++--------- src/emulator/storage/apis/gcloud.ts | 168 +++++++------- src/emulator/storage/rules/manager.ts | 61 ++--- src/emulator/storage/server.ts | 122 +++++----- src/frameworks/utils.spec.ts | 15 +- src/functionsShellCommandAction.ts | 2 +- src/hosting/proxy.ts | 11 +- src/mcp/apps/init/mcp-app.ts | 7 +- src/mcp/index.ts | 14 +- src/utils.ts | 6 +- 25 files changed, 616 insertions(+), 448 deletions(-) create mode 100644 src/emulator/shared/express.ts diff --git a/.eslintrc.js b/.eslintrc.js index ecc336961d9..ca79e022618 100644 --- a/.eslintrc.js +++ b/.eslintrc.js @@ -55,7 +55,6 @@ module.exports = { "@typescript-eslint/no-extra-non-null-assertion": "warn", // TODO(bkendall): remove, allow to error. "@typescript-eslint/no-floating-promises": "warn", // TODO(bkendall): remove, allow to error. "@typescript-eslint/no-inferrable-types": "warn", // TODO(bkendall): remove, allow to error. - "@typescript-eslint/no-misused-promises": "warn", // TODO(bkendall): remove, allow to error. "@typescript-eslint/no-unnecessary-type-assertion": "warn", // TODO(bkendall): remove, allow to error. "@typescript-eslint/no-unsafe-argument": "warn", // TODO(bkendall): remove, allow to error. "@typescript-eslint/no-unsafe-assignment": "warn", // TODO(bkendall): remove, allow to error. diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b4aa487898..c9efbe2210a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,2 +1,4 @@ - Updated chokidar dependency to ^4.0.3 to remove vulnerable braces dependency (#11308). - Fixed an issue where the Firebase MCP server referred to `update_firebase_environment` instead of `firebase_update_environment`. +- Fixed `database:import` hanging when a batch write fails. +- Fixed emulator crashes when a Firestore rules reload or emulator HTTP handler throws. diff --git a/src/auth.ts b/src/auth.ts index 2adbcc79910..aa964958cca 100644 --- a/src/auth.ts +++ b/src/auth.ts @@ -605,7 +605,10 @@ async function loginWithLocalhost( getTokens: (queryCode: string, callbackUrl: string) => Promise, ): Promise { return new Promise((resolve, reject) => { - const server = http.createServer(async (req, res) => { + async function handleCallback( + req: http.IncomingMessage, + res: http.ServerResponse, + ): Promise { let searchParams: URLSearchParams; try { searchParams = new URL(req.url || "", callbackUrl).searchParams; @@ -634,6 +637,13 @@ async function loginWithLocalhost( } server.close(); return; + } + + const server = http.createServer((req, res) => { + handleCallback(req, res).catch((err: unknown) => { + reject(err); + server.close(); + }); }); server.listen(port, () => { diff --git a/src/database/import.ts b/src/database/import.ts index 01d848a9e3d..f6284624734 100644 --- a/src/database/import.ts +++ b/src/database/import.ts @@ -186,10 +186,14 @@ export default class DatabaseImporter { }; const writeBatch = new stream.Transform({ objectMode: true }); - writeBatch._transform = async function (batch: SizedData, _, done) { - const res = await doWriteBatch(batch); - this.push(res); - done(); + writeBatch._transform = function (batch: SizedData, _, done) { + void doWriteBatch(batch).then( + (res) => { + this.push(res); + done(); + }, + (err: Error) => done(err), + ); }; return new Promise((resolve, reject) => { diff --git a/src/deploy/functions/runtimes/discovery/index.ts b/src/deploy/functions/runtimes/discovery/index.ts index 28f1380b83a..9d2e665383b 100644 --- a/src/deploy/functions/runtimes/discovery/index.ts +++ b/src/deploy/functions/runtimes/discovery/index.ts @@ -162,7 +162,7 @@ export async function detectFromOutputPath( stderrBuffer += chunk.toString(); }); - childProcess.on("exit", async (code: number | null) => { + const onExit = async (code: number | null): Promise => { if (!resolved) { clearTimeout(timer); resolved = true; @@ -192,6 +192,11 @@ export async function detectFromOutputPath( } } } + }; + + // onExit catches and routes all outcomes and errors to resolve or reject + childProcess.on("exit", (code: number | null) => { + void onExit(code); }); childProcess.on("error", (err: Error) => { diff --git a/src/deploy/functions/services/index.ts b/src/deploy/functions/services/index.ts index c5ba4ff2f92..fa16427f11e 100644 --- a/src/deploy/functions/services/index.ts +++ b/src/deploy/functions/services/index.ts @@ -29,6 +29,9 @@ import { ensureAuthEventarcTriggerRegion } from "./authEventarc"; /** A standard void No Op */ export const noop = (): Promise => Promise.resolve(); +/** A synchronous No Op, for hooks such as validateTrigger that return void */ +export const syncNoop = (): void => undefined; + /** A No Op that's useful for Services that don't have specific bindings but should still try to set default bindings */ export const noopProjectBindings = (): Promise> => Promise.resolve([]); @@ -73,7 +76,7 @@ const noOpService: Service = { name: "noop", api: "", ensureTriggerRegion: noop, - validateTrigger: noop, + validateTrigger: syncNoop, registerTrigger: noop, unregisterTrigger: noop, getDefaultRegion: () => Promise.resolve(FALLBACK_DEPLOYMENT_REGION), @@ -85,7 +88,7 @@ const pubSubService: Service = { api: "pubsub.googleapis.com", requiredProjectBindings: noopProjectBindings, ensureTriggerRegion: noop, - validateTrigger: noop, + validateTrigger: syncNoop, registerTrigger: noop, unregisterTrigger: noop, getDefaultRegion: () => Promise.resolve(DEFAULT_GLOBAL_TRIGGER_REGION), @@ -97,7 +100,7 @@ const storageService: Service = { api: "storage.googleapis.com", requiredProjectBindings: obtainStorageBindings, ensureTriggerRegion: ensureStorageTriggerRegion, - validateTrigger: noop, + validateTrigger: syncNoop, registerTrigger: noop, unregisterTrigger: noop, getDefaultRegion: ensureStorageDefaultRegion, @@ -109,7 +112,7 @@ const firebaseAlertsService: Service = { api: "firebasealerts.googleapis.com", requiredProjectBindings: noopProjectBindings, ensureTriggerRegion: ensureFirebaseAlertsTriggerRegion, - validateTrigger: noop, + validateTrigger: syncNoop, registerTrigger: noop, unregisterTrigger: noop, getDefaultRegion: () => Promise.resolve(DEFAULT_GLOBAL_TRIGGER_REGION), @@ -125,7 +128,7 @@ const databaseService: Service = { api: "firebasedatabase.googleapis.com", requiredProjectBindings: noopProjectBindings, ensureTriggerRegion: ensureDatabaseTriggerRegion, - validateTrigger: noop, + validateTrigger: syncNoop, registerTrigger: noop, unregisterTrigger: noop, getDefaultRegion: ensureDatabaseDefaultRegion, @@ -137,7 +140,7 @@ const remoteConfigService: Service = { api: "firebaseremoteconfig.googleapis.com", requiredProjectBindings: noopProjectBindings, ensureTriggerRegion: ensureRemoteConfigTriggerRegion, - validateTrigger: noop, + validateTrigger: syncNoop, registerTrigger: noop, unregisterTrigger: noop, getDefaultRegion: () => Promise.resolve(DEFAULT_GLOBAL_TRIGGER_REGION), @@ -149,7 +152,7 @@ const testLabService: Service = { api: "testing.googleapis.com", requiredProjectBindings: noopProjectBindings, ensureTriggerRegion: ensureTestLabTriggerRegion, - validateTrigger: noop, + validateTrigger: syncNoop, registerTrigger: noop, unregisterTrigger: noop, getDefaultRegion: () => Promise.resolve(DEFAULT_GLOBAL_TRIGGER_REGION), @@ -161,7 +164,7 @@ const firestoreService: Service = { api: "firestore.googleapis.com", requiredProjectBindings: noopProjectBindings, ensureTriggerRegion: ensureFirestoreTriggerRegion, - validateTrigger: noop, + validateTrigger: syncNoop, registerTrigger: noop, unregisterTrigger: noop, getDefaultRegion: ensureFirestoreDefaultRegion, @@ -173,7 +176,7 @@ const dataconnectService: Service = { api: "firebasedataconnect.googleapis.com", requiredProjectBindings: noopProjectBindings, ensureTriggerRegion: ensureDataConnectTriggerRegion, - validateTrigger: noop, + validateTrigger: syncNoop, registerTrigger: noop, unregisterTrigger: noop, getDefaultRegion: ensureDataConnectDefaultRegion, @@ -185,7 +188,7 @@ const authEventarcService: Service = { api: "eventarc.googleapis.com", requiredProjectBindings: noopProjectBindings, ensureTriggerRegion: ensureAuthEventarcTriggerRegion, - validateTrigger: noop, + validateTrigger: syncNoop, registerTrigger: noop, unregisterTrigger: noop, getDefaultRegion: () => Promise.resolve(DEFAULT_GLOBAL_TRIGGER_REGION), diff --git a/src/emulator/commandUtils.ts b/src/emulator/commandUtils.ts index 06019ec27f1..c992b4cd8ce 100644 --- a/src/emulator/commandUtils.ts +++ b/src/emulator/commandUtils.ts @@ -265,7 +265,7 @@ function processKillSignal( ): SignalsListener { let lastSignal = new Date().getTime(); let signalCount = 0; - return async () => { + const handleSignal = async (): Promise => { try { const now = new Date().getTime(); const diff = now - lastSignal; @@ -339,6 +339,11 @@ function processKillSignal( rej(getError(e)); } }; + + // handleSignal catches all errors and routes them to res/rej + return () => { + void handleSignal(); + }; } /** diff --git a/src/emulator/databaseEmulator.ts b/src/emulator/databaseEmulator.ts index 31641a14fcc..cf5ec8e1a53 100644 --- a/src/emulator/databaseEmulator.ts +++ b/src/emulator/databaseEmulator.ts @@ -9,7 +9,7 @@ import { EmulatorInfo, EmulatorInstance, Emulators } from "../emulator/types"; import { Constants } from "./constants"; import { EmulatorRegistry } from "./registry"; import { EmulatorLogger } from "./emulatorLogger"; -import { FirebaseError } from "../error"; +import { FirebaseError, getErrMsg } from "../error"; import { connectableHostname } from "../utils"; export interface DatabaseEmulatorArgs { @@ -50,25 +50,30 @@ export class DatabaseEmulator implements EmulatorInstance { const watcher = chokidar.watch(c.rules, { persistent: true, ignoreInitial: true }); this.rulesWatchers.push(watcher); - watcher.on("change", async () => { - // There have been some race conditions reported (on Windows) where reading the - // file too quickly after the watcher fires results in an empty file being read. - // Adding a small delay prevents that at very little cost. - await new Promise((res) => setTimeout(res, 5)); + watcher.on("change", () => { + const update = async (): Promise => { + // There have been some race conditions reported (on Windows) where reading the + // file too quickly after the watcher fires results in an empty file being read. + // Adding a small delay prevents that at very little cost. + await new Promise((res) => setTimeout(res, 5)); + + this.logger.logLabeled( + "BULLET", + "database", + `Change detected, updating rules for ${c.instance}...`, + ); - this.logger.logLabeled( - "BULLET", - "database", - `Change detected, updating rules for ${c.instance}...`, - ); - - try { - await this.updateRules(c.instance, c.rules); - this.logger.logLabeled("SUCCESS", "database", "Rules updated."); - } catch (e: any) { - this.logger.logLabeled("WARN", "database", this.prettyPrintRulesError(c.rules, e)); - this.logger.logLabeled("WARN", "database", "Failed to update rules"); - } + try { + await this.updateRules(c.instance, c.rules); + this.logger.logLabeled("SUCCESS", "database", "Rules updated."); + } catch (e: any) { + this.logger.logLabeled("WARN", "database", this.prettyPrintRulesError(c.rules, e)); + this.logger.logLabeled("WARN", "database", "Failed to update rules"); + } + }; + update().catch((err: unknown) => { + this.logger.logLabeled("WARN", "database", `Failed to update rules: ${getErrMsg(err)}`); + }); }); } } diff --git a/src/emulator/dataconnect/pgliteServer.ts b/src/emulator/dataconnect/pgliteServer.ts index 1e8daf0a0fc..ff3a1b8c44e 100644 --- a/src/emulator/dataconnect/pgliteServer.ts +++ b/src/emulator/dataconnect/pgliteServer.ts @@ -49,43 +49,49 @@ export class PostgresServer { public async createPGServer(host: string = "127.0.0.1", port: number): Promise { const getDb = this.getDb.bind(this); - const server = net.createServer(async (socket) => { - const connection = await fromNodeSocket(socket, { - serverVersion: "17.4 (PGlite 0.3.3)", - auth: { method: "trust" }, - - async *onMessage(data: Uint8Array, { isAuthenticated }: { isAuthenticated: boolean }) { - // Only forward messages to PGlite after authentication - if (!isAuthenticated) { - return; - } - const db = await getDb(); - if (data[0] === FrontendMessageCode.Terminate) { - // When the frontend terminates a connection, throw out all prepared statements - // because the next client won't know about them (and may create overlapping statements) - await db.query("DEALLOCATE ALL"); - } - const response = await db.execProtocolRaw(data); - for await (const message of extendedQueryPatch.getMessages(data, response)) { - yield message; - } - - // Extended query patch removes the extra Ready for Query messages that - // pglite wrongly sends. - }, - }); - - const extendedQueryPatch: PGliteLoggerPatch = new PGliteLoggerPatch(connection); - socket.on("end", () => { - logger.debug("Postgres client disconnected"); - }); - // Log and destroy individual client socket errors instead of escalating them - // to server-level errors. - socket.on("error", (err) => { - logger.debug( - "Postgres client socket error (if this happened during request cancellation, it is expected and harmless):", - err, - ); + const server = net.createServer((socket) => { + const handleConnection = async (): Promise => { + const connection = await fromNodeSocket(socket, { + serverVersion: "17.4 (PGlite 0.3.3)", + auth: { method: "trust" }, + + async *onMessage(data: Uint8Array, { isAuthenticated }: { isAuthenticated: boolean }) { + // Only forward messages to PGlite after authentication + if (!isAuthenticated) { + return; + } + const db = await getDb(); + if (data[0] === FrontendMessageCode.Terminate) { + // When the frontend terminates a connection, throw out all prepared statements + // because the next client won't know about them (and may create overlapping statements) + await db.query("DEALLOCATE ALL"); + } + const response = await db.execProtocolRaw(data); + for await (const message of extendedQueryPatch.getMessages(data, response)) { + yield message; + } + + // Extended query patch removes the extra Ready for Query messages that + // pglite wrongly sends. + }, + }); + + const extendedQueryPatch: PGliteLoggerPatch = new PGliteLoggerPatch(connection); + socket.on("end", () => { + logger.debug("Postgres client disconnected"); + }); + // Log and destroy individual client socket errors instead of escalating them + // to server-level errors. + socket.on("error", (err) => { + logger.debug( + "Postgres client socket error (if this happened during request cancellation, it is expected and harmless):", + err, + ); + socket.destroy(); + }); + }; + handleConnection().catch((err: unknown) => { + logger.debug("Postgres client connection setup failed:", err); socket.destroy(); }); }); diff --git a/src/emulator/downloadableEmulators.ts b/src/emulator/downloadableEmulators.ts index 5fc9ae5db41..b33fa9ddd5b 100755 --- a/src/emulator/downloadableEmulators.ts +++ b/src/emulator/downloadableEmulators.ts @@ -518,11 +518,12 @@ async function _runBinary( void handleEmulatorProcessError(emulator.name, err, command.port); }); - emulator.instance.once("exit", async (code, signal) => { + emulator.instance.once("exit", (code, signal) => { if (signal) { utils.logWarning(`${description} has exited upon receiving signal: ${signal}`); } else if (code && code !== 0 && code !== /* SIGINT */ 130) { - await _fatal(emulator.name, `${description} has exited with code: ${code}`); + // _fatal never rejects because its finally block calls process.exit(1) + void _fatal(emulator.name, `${description} has exited with code: ${code}`); } }); resolve(); diff --git a/src/emulator/firestoreEmulator.ts b/src/emulator/firestoreEmulator.ts index cfd2c5e909a..15590e6e8bb 100644 --- a/src/emulator/firestoreEmulator.ts +++ b/src/emulator/firestoreEmulator.ts @@ -4,6 +4,7 @@ import * as clc from "colorette"; import * as path from "path"; import * as utils from "../utils"; +import { getErrMsg } from "../error"; import * as downloadableEmulators from "./downloadableEmulators"; import { EmulatorInfo, EmulatorInstance, Emulators, Severity } from "../emulator/types"; import { EmulatorRegistry } from "./registry"; @@ -45,25 +46,30 @@ export class FirestoreEmulator implements EmulatorInstance { if (this.args.rules && this.args.project_id) { const rulesPath = this.args.rules; this.rulesWatcher = chokidar.watch(rulesPath, { persistent: true, ignoreInitial: true }); - this.rulesWatcher.on("change", async () => { - // There have been some race conditions reported (on Windows) where reading the - // file too quickly after the watcher fires results in an empty file being read. - // Adding a small delay prevents that at very little cost. - await new Promise((res) => setTimeout(res, 5)); - - utils.logLabeledBullet("firestore", "Change detected, updating rules..."); - const newContent = fs.readFileSync(rulesPath, "utf8").toString(); - const issues = await this.updateRules(newContent); - if (issues) { - for (const issue of issues) { - utils.logWarning(this.prettyPrintRulesIssue(rulesPath, issue)); + this.rulesWatcher.on("change", () => { + const update = async (): Promise => { + // There have been some race conditions reported (on Windows) where reading the + // file too quickly after the watcher fires results in an empty file being read. + // Adding a small delay prevents that at very little cost. + await new Promise((res) => setTimeout(res, 5)); + + utils.logLabeledBullet("firestore", "Change detected, updating rules..."); + const newContent = fs.readFileSync(rulesPath, "utf8").toString(); + const issues = await this.updateRules(newContent); + if (issues) { + for (const issue of issues) { + utils.logWarning(this.prettyPrintRulesIssue(rulesPath, issue)); + } } - } - if (issues.some((issue) => issue.severity === Severity.ERROR)) { - utils.logWarning("Failed to update rules"); - } else { - utils.logLabeledSuccess("firestore", "Rules updated."); - } + if (issues.some((issue) => issue.severity === Severity.ERROR)) { + utils.logWarning("Failed to update rules"); + } else { + utils.logLabeledSuccess("firestore", "Rules updated."); + } + }; + update().catch((err: unknown) => { + utils.logWarning(`Failed to update rules: ${getErrMsg(err)}`); + }); }); } diff --git a/src/emulator/functionsEmulator.ts b/src/emulator/functionsEmulator.ts index 9c26fde1cdc..661f561da02 100644 --- a/src/emulator/functionsEmulator.ts +++ b/src/emulator/functionsEmulator.ts @@ -42,7 +42,7 @@ import { EmulatorRegistry } from "./registry"; import { EmulatorLogger, Verbosity } from "./emulatorLogger"; import { RuntimeWorker, RuntimeWorkerPool } from "./functionsRuntimeWorker"; import { PubsubEmulator } from "./pubsubEmulator"; -import { FirebaseError } from "../error"; +import { FirebaseError, getErrMsg } from "../error"; import { WorkQueue, Work } from "./workQueue"; import { allSettled, connectableHostname, createDestroyer, debounce, randomInt } from "../utils"; import { @@ -534,7 +534,15 @@ export class FunctionsEmulator implements EmulatorInstance { "functions", `Starting build_runner watch for Dart functions...`, ); - const debouncedLoadTriggers = debounce(() => this.loadTriggers(backend), 1000); + const debouncedLoadTriggers = debounce(() => { + this.loadTriggers(backend).catch((err: unknown) => + this.logger.logLabeled( + "WARN", + "functions", + `Failed to reload triggers: ${getErrMsg(err)}`, + ), + ); + }, 1000); const cleanup = await delegate.watch(() => { this.logger.log("DEBUG", "build_runner rebuilt, reloading triggers"); debouncedLoadTriggers(); @@ -549,10 +557,18 @@ export class FunctionsEmulator implements EmulatorInstance { this.watchers.push(watcher); - const debouncedLoadTriggers = debounce(() => this.loadTriggers(backend), 1000); + const debouncedLoadTriggers = debounce(() => { + this.loadTriggers(backend).catch((err: unknown) => + this.logger.logLabeled( + "WARN", + "functions", + `Failed to reload triggers: ${getErrMsg(err)}`, + ), + ); + }, 1000); watcher.on("change", (filePath) => { this.logger.log("DEBUG", `File ${filePath} changed, reloading triggers`); - return debouncedLoadTriggers(); + debouncedLoadTriggers(); }); } } diff --git a/src/emulator/functionsEmulatorRuntime.ts b/src/emulator/functionsEmulatorRuntime.ts index c563937cd68..72f9e2450f3 100644 --- a/src/emulator/functionsEmulatorRuntime.ts +++ b/src/emulator/functionsEmulatorRuntime.ts @@ -869,7 +869,7 @@ async function main(): Promise { app.all("/favicon.ico|/robots.txt", (req, res) => { res.status(404).send(); }); - app.all(`/*`, async (req: express.Request, res: express.Response) => { + async function handleRequest(req: express.Request, res: express.Response): Promise { try { const trigger = FUNCTION_TARGET_NAME.split(".").reduce((mod, functionTargetPart) => { return mod?.[functionTargetPart]; @@ -900,6 +900,11 @@ async function main(): Promise { new EmulatorLog("FATAL", "runtime-error", getErrStack(err)).log(); res.status(500).send(getErrMsg(err)); } + } + + // handleRequest catches all errors and responds with status 500 + app.all(`/*`, (req: express.Request, res: express.Response) => { + void handleRequest(req, res); }); app.listen(process.env.PORT, () => { logDebug(`Listening to port: ${process.env.PORT}`); diff --git a/src/emulator/hub.ts b/src/emulator/hub.ts index 43260048e06..fbba683a95a 100644 --- a/src/emulator/hub.ts +++ b/src/emulator/hub.ts @@ -12,6 +12,7 @@ import { FunctionsEmulator } from "./functionsEmulator"; import { ExpressBasedEmulator } from "./ExpressBasedEmulator"; import { PortName } from "./portUtils"; import { DataConnectEmulator } from "./dataconnectEmulator"; +import { asyncHandler } from "./shared/express"; // We use the CLI version from package.json const pkg = require("../../package.json"); @@ -104,86 +105,98 @@ export class EmulatorHub extends ExpressBasedEmulator { res.json(this.getRunningEmulatorsMapping()); }); - app.post(EmulatorHub.PATH_EXPORT, async (req, res) => { - if (req.headers.origin) { - res.status(403).json({ - message: `Export cannot be triggered by external callers.`, - }); - } - const path: string = req.body.path; - const initiatedBy: string = req.body.initiatedBy || "unknown"; - const targets: string[] = req.body.targets; - utils.logLabeledBullet("emulators", `Received export request. Exporting data to ${path}.`); - try { - await new HubExport(this.args.projectId, { - path, - initiatedBy, - targets, - }).exportAll(); - utils.logLabeledSuccess("emulators", "Export complete."); - res.status(200).send({ - message: "OK", - }); - } catch (e: any) { - const errorString = e.message || JSON.stringify(e); - utils.logLabeledWarning("emulators", `Export failed: ${errorString}`); - res.status(500).json({ - message: errorString, - }); - } - }); - - app.put(EmulatorHub.PATH_DISABLE_FUNCTIONS, async (req, res) => { - utils.logLabeledBullet( - "emulators", - `Disabling Cloud Functions triggers, non-HTTP functions will not execute.`, - ); - - const instance = EmulatorRegistry.get(Emulators.FUNCTIONS); - if (!instance) { - res.status(400).json({ error: "The Cloud Functions emulator is not running." }); - return; - } - - const emu = instance as FunctionsEmulator; - await emu.disableBackgroundTriggers(); - res.status(200).json({ enabled: false }); - }); - - app.put(EmulatorHub.PATH_ENABLE_FUNCTIONS, async (req, res) => { - utils.logLabeledBullet( - "emulators", - `Enabling Cloud Functions triggers, non-HTTP functions will execute.`, - ); - - const instance = EmulatorRegistry.get(Emulators.FUNCTIONS); - if (!instance) { - res.status(400).send("The Cloud Functions emulator is not running."); - return; - } + app.post( + EmulatorHub.PATH_EXPORT, + asyncHandler(async (req, res) => { + if (req.headers.origin) { + res.status(403).json({ + message: `Export cannot be triggered by external callers.`, + }); + } + const path: string = req.body.path; + const initiatedBy: string = req.body.initiatedBy || "unknown"; + const targets: string[] = req.body.targets; + utils.logLabeledBullet("emulators", `Received export request. Exporting data to ${path}.`); + try { + await new HubExport(this.args.projectId, { + path, + initiatedBy, + targets, + }).exportAll(); + utils.logLabeledSuccess("emulators", "Export complete."); + res.status(200).send({ + message: "OK", + }); + } catch (e: any) { + const errorString = e.message || JSON.stringify(e); + utils.logLabeledWarning("emulators", `Export failed: ${errorString}`); + res.status(500).json({ + message: errorString, + }); + } + }), + ); + + app.put( + EmulatorHub.PATH_DISABLE_FUNCTIONS, + asyncHandler(async (req, res) => { + utils.logLabeledBullet( + "emulators", + `Disabling Cloud Functions triggers, non-HTTP functions will not execute.`, + ); + + const instance = EmulatorRegistry.get(Emulators.FUNCTIONS); + if (!instance) { + res.status(400).json({ error: "The Cloud Functions emulator is not running." }); + return; + } - const emu = instance as FunctionsEmulator; - await emu.reloadTriggers(); - res.status(200).json({ enabled: true }); - }); + const emu = instance as FunctionsEmulator; + await emu.disableBackgroundTriggers(); + res.status(200).json({ enabled: false }); + }), + ); + + app.put( + EmulatorHub.PATH_ENABLE_FUNCTIONS, + asyncHandler(async (req, res) => { + utils.logLabeledBullet( + "emulators", + `Enabling Cloud Functions triggers, non-HTTP functions will execute.`, + ); + + const instance = EmulatorRegistry.get(Emulators.FUNCTIONS); + if (!instance) { + res.status(400).send("The Cloud Functions emulator is not running."); + return; + } - app.post(EmulatorHub.PATH_CLEAR_DATA_CONNECT, async (req, res) => { - if (req.headers.origin) { - res.status(403).json({ - message: `Clear SQL Connect cannot be triggered by external callers.`, - }); - } - utils.logLabeledBullet("emulators", `Clearing data from SQL Connect data sources.`); + const emu = instance as FunctionsEmulator; + await emu.reloadTriggers(); + res.status(200).json({ enabled: true }); + }), + ); + + app.post( + EmulatorHub.PATH_CLEAR_DATA_CONNECT, + asyncHandler(async (req, res) => { + if (req.headers.origin) { + res.status(403).json({ + message: `Clear SQL Connect cannot be triggered by external callers.`, + }); + } + utils.logLabeledBullet("emulators", `Clearing data from SQL Connect data sources.`); - const instance = EmulatorRegistry.get(Emulators.DATACONNECT) as DataConnectEmulator; - if (!instance) { - res.status(400).json({ error: "The SQL Connect emulator is not running." }); - return; - } + const instance = EmulatorRegistry.get(Emulators.DATACONNECT) as DataConnectEmulator; + if (!instance) { + res.status(400).json({ error: "The SQL Connect emulator is not running." }); + return; + } - await instance.clearData(); - res.status(200).json({ success: true }); - }); + await instance.clearData(); + res.status(200).json({ success: true }); + }), + ); return app; } diff --git a/src/emulator/shared/express.ts b/src/emulator/shared/express.ts new file mode 100644 index 00000000000..d1264190573 --- /dev/null +++ b/src/emulator/shared/express.ts @@ -0,0 +1,14 @@ +import type { NextFunction, Request, RequestHandler, Response } from "express"; + +/** + * Adapts an async route handler for Express 4, which ignores the promise a handler returns. + * A rejection is passed to `next(err)` (Express's error handling) instead of becoming an + * unhandled promise rejection, which by default terminates the Node.js process. + */ +export function asyncHandler( + fn: (req: Request, res: Response, next: NextFunction) => Promise, +): RequestHandler { + return (req, res, next) => { + void fn(req, res, next).catch(next); + }; +} diff --git a/src/emulator/storage/apis/firebase.ts b/src/emulator/storage/apis/firebase.ts index 419a30d7250..03cf6f73b50 100644 --- a/src/emulator/storage/apis/firebase.ts +++ b/src/emulator/storage/apis/firebase.ts @@ -16,6 +16,7 @@ import { UploadPreviouslyFinalizedError, } from "../upload"; import { reqBodyToBuffer } from "../../shared/request"; +import { asyncHandler } from "../../shared/express"; import { ListObjectsResponse } from "../files"; /** @@ -111,92 +112,98 @@ export function createFirebaseEndpoints(emulator: StorageEmulator): Router { next(); }); - firebaseStorageAPI.get("/b/:bucketId/o/:objectId", async (req, res) => { - let metadata: StoredFileMetadata; - let data: Buffer; - try { - // Both object data and metadata get can use the same handler since they share auth logic. - ({ metadata, data } = await storageLayer.getObject({ - bucketId: req.params.bucketId, - decodedObjectId: decodeURIComponent(req.params.objectId), - authorization: req.header("authorization"), - downloadToken: req.query.token?.toString(), - })); - } catch (err) { - if (err instanceof NotFoundError) { - return res.sendStatus(404); - } else if (err instanceof ForbiddenError) { - return res.status(403).json({ - error: { - code: 403, - message: `Permission denied. No READ permission.`, - }, - }); + firebaseStorageAPI.get( + "/b/:bucketId/o/:objectId", + asyncHandler(async (req, res) => { + let metadata: StoredFileMetadata; + let data: Buffer; + try { + // Both object data and metadata get can use the same handler since they share auth logic. + ({ metadata, data } = await storageLayer.getObject({ + bucketId: req.params.bucketId, + decodedObjectId: decodeURIComponent(req.params.objectId), + authorization: req.header("authorization"), + downloadToken: req.query.token?.toString(), + })); + } catch (err) { + if (err instanceof NotFoundError) { + return res.sendStatus(404); + } else if (err instanceof ForbiddenError) { + return res.status(403).json({ + error: { + code: 403, + message: `Permission denied. No READ permission.`, + }, + }); + } + throw err; } - throw err; - } - if (metadata.downloadTokens.length === 0) { - metadata.addDownloadToken(/* shouldTrigger = */ true); - } + if (metadata.downloadTokens.length === 0) { + metadata.addDownloadToken(/* shouldTrigger = */ true); + } - // Object data request - if (req.query.alt === "media") { - return sendFileBytes(metadata, data, req, res); - } + // Object data request + if (req.query.alt === "media") { + return sendFileBytes(metadata, data, req, res); + } - // Object metadata request - return res.json(new OutgoingFirebaseMetadata(metadata)); - }); + // Object metadata request + return res.json(new OutgoingFirebaseMetadata(metadata)); + }), + ); // list object handler - firebaseStorageAPI.get("/b/:bucketId/o", async (req, res) => { - const maxResults = req.query.maxResults?.toString(); - let listResponse: ListObjectsResponse; - // The prefix query param must be empty or end in a "/" - let prefix = ""; - if (req.query.prefix) { - prefix = req.query.prefix.toString(); - if (!prefix.endsWith("/")) { - return res.status(400).json({ - error: { - code: 400, - message: - "The prefix parameter is required to be empty or ends with a single / character.", - }, - }); + firebaseStorageAPI.get( + "/b/:bucketId/o", + asyncHandler(async (req, res) => { + const maxResults = req.query.maxResults?.toString(); + let listResponse: ListObjectsResponse; + // The prefix query param must be empty or end in a "/" + let prefix = ""; + if (req.query.prefix) { + prefix = req.query.prefix.toString(); + if (!prefix.endsWith("/")) { + return res.status(400).json({ + error: { + code: 400, + message: + "The prefix parameter is required to be empty or ends with a single / character.", + }, + }); + } } - } - try { - listResponse = await storageLayer.listObjects({ - bucketId: req.params.bucketId, - prefix: prefix, - delimiter: req.query.delimiter ? req.query.delimiter.toString() : "", - pageToken: req.query.pageToken?.toString(), - maxResults: maxResults ? +maxResults : undefined, - authorization: req.header("authorization"), - }); - } catch (err) { - if (err instanceof ForbiddenError) { - return res.status(403).json({ - error: { - code: 403, - message: `Permission denied. No LIST permission.`, - }, + try { + listResponse = await storageLayer.listObjects({ + bucketId: req.params.bucketId, + prefix: prefix, + delimiter: req.query.delimiter ? req.query.delimiter.toString() : "", + pageToken: req.query.pageToken?.toString(), + maxResults: maxResults ? +maxResults : undefined, + authorization: req.header("authorization"), }); + } catch (err) { + if (err instanceof ForbiddenError) { + return res.status(403).json({ + error: { + code: 403, + message: `Permission denied. No LIST permission.`, + }, + }); + } + throw err; } - throw err; - } - return res.status(200).json({ - nextPageToken: listResponse.nextPageToken, - prefixes: (listResponse.prefixes ?? []).filter(isValidPrefix), - items: (listResponse.items ?? []) - .filter((item) => isValidNonEncodedPathString(item.name)) - .map((item) => { - return { name: item.name, bucket: item.bucket }; - }), - }); - }); + return res.status(200).json({ + nextPageToken: listResponse.nextPageToken, + prefixes: (listResponse.prefixes ?? []).filter(isValidPrefix), + items: (listResponse.items ?? []) + .filter((item) => isValidNonEncodedPathString(item.name)) + .map((item) => { + return { name: item.name, bucket: item.bucket }; + }), + }); + }), + ); const handleUpload = async (req: Request, res: Response) => { const bucketId = req.params.bucketId; @@ -492,50 +499,53 @@ export function createFirebaseEndpoints(emulator: StorageEmulator): Router { firebaseStorageAPI.patch( "/b/:bucketId/o/:objectId", json({ limit: "130mb" }), - handleMetadataUpdate, + asyncHandler(handleMetadataUpdate), ); firebaseStorageAPI.put( "/b/:bucketId/o/:objectId?", firebasePostUploadMiddleware, - async (req, res) => { + asyncHandler(async (req, res) => { switch (req.header("x-http-method-override")?.toLowerCase()) { case "patch": return handleMetadataUpdate(req, res); default: return handleObjectPostRequest(req, res); } - }, + }), ); firebaseStorageAPI.post( "/b/:bucketId/o/:objectId?", firebasePostUploadMiddleware, - handleObjectPostRequest, + asyncHandler(handleObjectPostRequest), ); - firebaseStorageAPI.delete("/b/:bucketId/o/:objectId", async (req, res) => { - try { - await storageLayer.deleteObject({ - bucketId: req.params.bucketId, - decodedObjectId: decodeURIComponent(req.params.objectId), - authorization: req.header("authorization"), - }); - } catch (err) { - if (err instanceof ForbiddenError) { - return res.status(403).json({ - error: { - code: 403, - message: `Permission denied. No WRITE permission.`, - }, + firebaseStorageAPI.delete( + "/b/:bucketId/o/:objectId", + asyncHandler(async (req, res) => { + try { + await storageLayer.deleteObject({ + bucketId: req.params.bucketId, + decodedObjectId: decodeURIComponent(req.params.objectId), + authorization: req.header("authorization"), }); + } catch (err) { + if (err instanceof ForbiddenError) { + return res.status(403).json({ + error: { + code: 403, + message: `Permission denied. No WRITE permission.`, + }, + }); + } + if (err instanceof NotFoundError) { + return res.sendStatus(404); + } + throw err; } - if (err instanceof NotFoundError) { - return res.sendStatus(404); - } - throw err; - } - res.sendStatus(204); - }); + res.sendStatus(204); + }), + ); firebaseStorageAPI.get("/", (req, res) => { res.json({ emulator: "storage" }); diff --git a/src/emulator/storage/apis/gcloud.ts b/src/emulator/storage/apis/gcloud.ts index 30a868baace..775229a3e2e 100644 --- a/src/emulator/storage/apis/gcloud.ts +++ b/src/emulator/storage/apis/gcloud.ts @@ -17,6 +17,7 @@ import { parseFormDataMultipartRequest, parseObjectUploadMultipartRequest } from import { Upload, UploadNotActiveError } from "../upload"; import { ForbiddenError, NotFoundError } from "../errors"; import { reqBodyToBuffer } from "../../shared/request"; +import { asyncHandler } from "../../shared/express"; import type { Query } from "express-serve-static-core"; export function createCloudEndpoints(emulator: StorageEmulator): Router { @@ -87,12 +88,15 @@ export function createCloudEndpoints(emulator: StorageEmulator): Router { next(); }); - gcloudStorageAPI.get("/b", async (req, res) => { - res.json({ - kind: "storage#buckets", - items: await adminStorageLayer.listBuckets(), - }); - }); + gcloudStorageAPI.get( + "/b", + asyncHandler(async (req, res) => { + res.json({ + kind: "storage#buckets", + items: await adminStorageLayer.listBuckets(), + }); + }), + ); gcloudStorageAPI.get( [ @@ -100,7 +104,7 @@ export function createCloudEndpoints(emulator: StorageEmulator): Router { "/download/storage/v1/b/:bucketId/o/:objectId", "/storage/v1/b/:bucketId/o/:objectId", ], - async (req, res) => { + asyncHandler(async (req, res) => { let getObjectResponse: GetObjectResponse; try { getObjectResponse = await adminStorageLayer.getObject({ @@ -121,58 +125,65 @@ export function createCloudEndpoints(emulator: StorageEmulator): Router { return sendFileBytes(getObjectResponse.metadata, getObjectResponse.data, req, res); } return res.json(new CloudStorageObjectMetadata(getObjectResponse.metadata)); - }, + }), ); - gcloudStorageAPI.patch("/b/:bucketId/o/:objectId", json({ limit: "130mb" }), async (req, res) => { - let updatedMetadata: StoredFileMetadata; - try { - updatedMetadata = await adminStorageLayer.updateObjectMetadata({ - bucketId: req.params.bucketId, - decodedObjectId: req.params.objectId, - metadata: req.body as IncomingMetadata, - }); - } catch (err) { - if (err instanceof NotFoundError) { - return sendObjectNotFound(req, res); - } - if (err instanceof ForbiddenError) { - return res.sendStatus(403); + gcloudStorageAPI.patch( + "/b/:bucketId/o/:objectId", + json({ limit: "130mb" }), + asyncHandler(async (req, res) => { + let updatedMetadata: StoredFileMetadata; + try { + updatedMetadata = await adminStorageLayer.updateObjectMetadata({ + bucketId: req.params.bucketId, + decodedObjectId: req.params.objectId, + metadata: req.body as IncomingMetadata, + }); + } catch (err) { + if (err instanceof NotFoundError) { + return sendObjectNotFound(req, res); + } + if (err instanceof ForbiddenError) { + return res.sendStatus(403); + } + throw err; } - throw err; - } - return res.json(new CloudStorageObjectMetadata(updatedMetadata)); - }); + return res.json(new CloudStorageObjectMetadata(updatedMetadata)); + }), + ); - gcloudStorageAPI.get(["/b/:bucketId/o", "/storage/v1/b/:bucketId/o"], async (req, res) => { - let listResponse: ListObjectsResponse; - // TODO validate that all query params are single strings and are not repeated. - try { - listResponse = await adminStorageLayer.listObjects({ - bucketId: req.params.bucketId, - prefix: req.query.prefix ? req.query.prefix.toString() : "", - delimiter: req.query.delimiter ? req.query.delimiter.toString() : "", - pageToken: req.query.pageToken ? req.query.pageToken.toString() : undefined, - maxResults: req.query.maxResults ? +req.query.maxResults.toString() : undefined, - authorization: req.header("authorization"), - }); - } catch (err) { - if (err instanceof ForbiddenError) { - return res.sendStatus(403); + gcloudStorageAPI.get( + ["/b/:bucketId/o", "/storage/v1/b/:bucketId/o"], + asyncHandler(async (req, res) => { + let listResponse: ListObjectsResponse; + // TODO validate that all query params are single strings and are not repeated. + try { + listResponse = await adminStorageLayer.listObjects({ + bucketId: req.params.bucketId, + prefix: req.query.prefix ? req.query.prefix.toString() : "", + delimiter: req.query.delimiter ? req.query.delimiter.toString() : "", + pageToken: req.query.pageToken ? req.query.pageToken.toString() : undefined, + maxResults: req.query.maxResults ? +req.query.maxResults.toString() : undefined, + authorization: req.header("authorization"), + }); + } catch (err) { + if (err instanceof ForbiddenError) { + return res.sendStatus(403); + } + throw err; } - throw err; - } - return res.status(200).json({ - kind: "storage#objects", - nextPageToken: listResponse.nextPageToken, - prefixes: listResponse.prefixes, - items: listResponse.items?.map((item) => new CloudStorageObjectMetadata(item)), - }); - }); + return res.status(200).json({ + kind: "storage#objects", + nextPageToken: listResponse.nextPageToken, + prefixes: listResponse.prefixes, + items: listResponse.items?.map((item) => new CloudStorageObjectMetadata(item)), + }); + }), + ); gcloudStorageAPI.delete( ["/b/:bucketId/o/:objectId", "/storage/v1/b/:bucketId/o/:objectId"], - async (req, res) => { + asyncHandler(async (req, res) => { try { await adminStorageLayer.deleteObject({ bucketId: req.params.bucketId, @@ -188,13 +199,13 @@ export function createCloudEndpoints(emulator: StorageEmulator): Router { throw err; } return res.sendStatus(204); - }, + }), ); gcloudStorageAPI.put( "/upload/storage/v1/b/:bucketId/o", bodyParser.raw({ type: "*/*", limit: "130mb" }), - async (req, res) => { + asyncHandler(async (req, res) => { if (!req.query.upload_id) { res.sendStatus(400); return; @@ -224,13 +235,13 @@ export function createCloudEndpoints(emulator: StorageEmulator): Router { throw err; } return res.json(new CloudStorageObjectMetadata(metadata)); - }, + }), ); gcloudStorageAPI.post( "/b/:bucketId/o/:objectId/acl", json({ limit: "130mb" }), - async (req, res) => { + asyncHandler(async (req, res) => { EmulatorLogger.forEmulator(Emulators.STORAGE).log( "WARN_ONCE", "Cloud Storage ACLs are not supported in the Storage Emulator. All related methods will succeed, but have no effect.", @@ -268,13 +279,13 @@ export function createCloudEndpoints(emulator: StorageEmulator): Router { etag: "someEtag", generation: metadata.generation.toString(), } as CloudStorageObjectAccessControlMetadata); - }, + }), ); gcloudStorageAPI.post( "/upload/storage/v1/b/:bucketId/o", postUploadMiddleware, - async (req, res) => { + asyncHandler(async (req, res) => { const uploadType = req.query.uploadType || req.header("X-Goog-Upload-Protocol"); // Resumable upload protocol. @@ -368,27 +379,30 @@ export function createCloudEndpoints(emulator: StorageEmulator): Router { authorization: req.header("authorization"), }); return await finalizeOneShotUpload(upload); - }, + }), ); - gcloudStorageAPI.get("/:bucketId/:objectId(**)", async (req, res) => { - let getObjectResponse: GetObjectResponse; - try { - getObjectResponse = await adminStorageLayer.getObject({ - bucketId: req.params.bucketId, - decodedObjectId: req.params.objectId, - }); - } catch (err) { - if (err instanceof NotFoundError) { - return sendObjectNotFound(req, res); - } - if (err instanceof ForbiddenError) { - return res.sendStatus(403); + gcloudStorageAPI.get( + "/:bucketId/:objectId(**)", + asyncHandler(async (req, res) => { + let getObjectResponse: GetObjectResponse; + try { + getObjectResponse = await adminStorageLayer.getObject({ + bucketId: req.params.bucketId, + decodedObjectId: req.params.objectId, + }); + } catch (err) { + if (err instanceof NotFoundError) { + return sendObjectNotFound(req, res); + } + if (err instanceof ForbiddenError) { + return res.sendStatus(403); + } + throw err; } - throw err; - } - return sendFileBytes(getObjectResponse.metadata, getObjectResponse.data, req, res); - }); + return sendFileBytes(getObjectResponse.metadata, getObjectResponse.data, req, res); + }), + ); gcloudStorageAPI.post( "/:bucketId", @@ -396,7 +410,7 @@ export function createCloudEndpoints(emulator: StorageEmulator): Router { adminStorageLayer.createBucket(req.params.bucketId); next(); }, - async (req, res) => { + asyncHandler(async (req, res) => { const contentTypeHeader = req.header("content-type"); if (!contentTypeHeader?.includes("multipart/form-data")) { return res.status(400).send("Content-Type must be multipart/form-data"); @@ -458,7 +472,7 @@ export function createCloudEndpoints(emulator: StorageEmulator): Router { } throw err; } - }, + }), ); gcloudStorageAPI.post( diff --git a/src/emulator/storage/rules/manager.ts b/src/emulator/storage/rules/manager.ts index 1b26e0b72ef..ee0a77aaadd 100644 --- a/src/emulator/storage/rules/manager.ts +++ b/src/emulator/storage/rules/manager.ts @@ -77,36 +77,41 @@ class DefaultStorageRulesManager implements StorageRulesManager { } private updateWatcher(rulesFile: string): void { + const handleFileChange = async (): Promise => { + // There have been some race conditions reported (on Windows) where reading the + // file too quickly after the watcher fires results in an empty file being read. + // Adding a small delay prevents that at very little cost. + await new Promise((res) => setTimeout(res, 5)); + + // In some OSes and CI environments, chokidar can emit a 'change' event + // when a file is deleted or if it is a non-existent mock file (e.g. /dev/null/storage.rules). + // Because we are inside an async callback that is not awaited by the event emitter, + // any synchronous errors thrown by `readFile` will result in an UnhandledPromiseRejection, + // which crashes the process. We catch these errors to prevent the emulator and tests from crashing. + try { + const content = readFile(rulesFile); + this._rules.content = content; + this._logger.logLabeled( + "BULLET", + "storage", + "Change detected, updating rules for Cloud Storage...", + ); + await this.loadRuleset(); + } catch (e) { + const message = e instanceof Error ? e.message : String(e); + this._logger.logLabeled( + "DEBUG", + "storage", + `A rule file change was detected, but there was an error reading it: ${message}`, + ); + } + }; + + // handleFileChange catches all errors and logs via logger this._watcher = chokidar .watch(rulesFile, { persistent: true, ignoreInitial: true }) - .on("change", async () => { - // There have been some race conditions reported (on Windows) where reading the - // file too quickly after the watcher fires results in an empty file being read. - // Adding a small delay prevents that at very little cost. - await new Promise((res) => setTimeout(res, 5)); - - // In some OSes and CI environments, chokidar can emit a 'change' event - // when a file is deleted or if it is a non-existent mock file (e.g. /dev/null/storage.rules). - // Because we are inside an async callback that is not awaited by the event emitter, - // any synchronous errors thrown by `readFile` will result in an UnhandledPromiseRejection, - // which crashes the process. We catch these errors to prevent the emulator and tests from crashing. - try { - const content = readFile(rulesFile); - this._rules.content = content; - this._logger.logLabeled( - "BULLET", - "storage", - "Change detected, updating rules for Cloud Storage...", - ); - await this.loadRuleset(); - } catch (e) { - const message = e instanceof Error ? e.message : String(e); - this._logger.logLabeled( - "DEBUG", - "storage", - `A rule file change was detected, but there was an error reading it: ${message}`, - ); - } + .on("change", () => { + void handleFileChange(); }); } diff --git a/src/emulator/storage/server.ts b/src/emulator/storage/server.ts index e2b8ccc923e..2c08c9e5c3f 100644 --- a/src/emulator/storage/server.ts +++ b/src/emulator/storage/server.ts @@ -8,6 +8,7 @@ import { RulesConfig, StorageEmulator } from "./index"; import { createFirebaseEndpoints } from "./apis/firebase"; import { InvalidArgumentError } from "../auth/errors"; import { SourceFile } from "./rules/types"; +import { asyncHandler } from "../shared/express"; /** * @param defaultProjectId @@ -61,17 +62,21 @@ export function createApp( app.use(bodyParser.raw({ limit: "130mb", type: "application/x-www-form-urlencoded" })); app.use(bodyParser.raw({ limit: "130mb", type: "multipart/related" })); - app.post("/internal/export", express.json({ limit: "130mb" }), async (req, res) => { - const initiatedBy: string = req.body.initiatedBy || "unknown"; - const path: string = req.body.path; - if (!path) { - res.status(400).send("Export request body must include 'path'."); - return; - } + app.post( + "/internal/export", + express.json({ limit: "130mb" }), + asyncHandler(async (req, res) => { + const initiatedBy: string = req.body.initiatedBy || "unknown"; + const path: string = req.body.path; + if (!path) { + res.status(400).send("Export request body must include 'path'."); + return; + } - await storageLayer.export(path, { initiatedBy }); - res.sendStatus(200); - }); + await storageLayer.export(path, { initiatedBy }); + res.sendStatus(200); + }), + ); /** * Internal endpoint to overwrite current rules. Callers provide either a single set of rules to @@ -100,63 +105,70 @@ export function createApp( * } * ``` */ - app.put("/internal/setRules", express.json({ limit: "130mb" }), async (req, res) => { - const rulesRaw = req.body.rules; - if (!(rulesRaw && Array.isArray(rulesRaw.files) && rulesRaw.files.length > 0)) { - res.status(400).json({ - message: "Request body must include 'rules.files' array", - }); - return; - } + app.put( + "/internal/setRules", + express.json({ limit: "130mb" }), + asyncHandler(async (req, res) => { + const rulesRaw = req.body.rules; + if (!(rulesRaw && Array.isArray(rulesRaw.files) && rulesRaw.files.length > 0)) { + res.status(400).json({ + message: "Request body must include 'rules.files' array", + }); + return; + } - const { files } = rulesRaw; + const { files } = rulesRaw; + + function parseRulesFromFiles(files: Array): SourceFile | RulesConfig[] { + if (files.length === 1) { + const file = files[0]; + if (!isRulesFile(file)) { + throw new InvalidArgumentError( + "Each member of 'rules.files' array must contain 'name' and 'content'", + ); + } + return { name: file.name, content: file.content }; + } - function parseRulesFromFiles(files: Array): SourceFile | RulesConfig[] { - if (files.length === 1) { - const file = files[0]; - if (!isRulesFile(file)) { - throw new InvalidArgumentError( - "Each member of 'rules.files' array must contain 'name' and 'content'", - ); + const rules: RulesConfig[] = []; + for (const file of files) { + if (!isRulesFile(file) || !file.resource) { + throw new InvalidArgumentError( + "Each member of 'rules.files' array must contain 'name', 'content', and 'resource'", + ); + } + rules.push({ + resource: file.resource, + rules: { name: file.name, content: file.content }, + }); } - return { name: file.name, content: file.content }; + return rules; } - const rules: RulesConfig[] = []; - for (const file of files) { - if (!isRulesFile(file) || !file.resource) { - throw new InvalidArgumentError( - "Each member of 'rules.files' array must contain 'name', 'content', and 'resource'", - ); + let rules: SourceFile | RulesConfig[]; + try { + rules = parseRulesFromFiles(files); + } catch (err) { + if (err instanceof InvalidArgumentError) { + res.status(400).json({ message: err.message }); + return; } - rules.push({ resource: file.resource, rules: { name: file.name, content: file.content } }); + throw err; } - return rules; - } - let rules: SourceFile | RulesConfig[]; - try { - rules = parseRulesFromFiles(files); - } catch (err) { - if (err instanceof InvalidArgumentError) { - res.status(400).json({ message: err.message }); + const issues = await emulator.replaceRules(rules); + if (issues.errors.length > 0) { + res.status(400).json({ + message: "There was an error updating rules, see logs for more details", + }); return; } - throw err; - } - const issues = await emulator.replaceRules(rules); - if (issues.errors.length > 0) { - res.status(400).json({ - message: "There was an error updating rules, see logs for more details", + res.status(200).json({ + message: "Rules updated successfully", }); - return; - } - - res.status(200).json({ - message: "Rules updated successfully", - }); - }); + }), + ); app.post("/internal/reset", (req, res) => { emulator.reset(); diff --git a/src/frameworks/utils.spec.ts b/src/frameworks/utils.spec.ts index f694dbcf5a6..af9f5ecfedc 100644 --- a/src/frameworks/utils.spec.ts +++ b/src/frameworks/utils.spec.ts @@ -254,7 +254,10 @@ describe("Frameworks utils", () => { nock("http://localhost:8999").post("/api", "payload").reply(404); const app = express(); - app.use(simpleProxy("http://localhost:8999")); + const proxy = simpleProxy("http://localhost:8999"); + app.use((req, res, next) => { + void proxy(req, res, next).catch(next); + }); app.use((req, res) => { res.json({ rawBody: (req as any).rawBody?.toString() }); }); @@ -266,7 +269,10 @@ describe("Frameworks utils", () => { nock("http://localhost:8999").post("/api", "payload").reply(200, "handled"); const app = express(); - app.use(simpleProxy("http://localhost:8999")); + const proxy = simpleProxy("http://localhost:8999"); + app.use((req, res, next) => { + void proxy(req, res, next).catch(next); + }); app.use((_req, res) => { res.status(500).send("should not cascade"); }); @@ -284,7 +290,10 @@ describe("Frameworks utils", () => { .reply(404); const app = express(); - app.use(simpleProxy("http://localhost:8999")); + const proxy = simpleProxy("http://localhost:8999"); + app.use((req, res, next) => { + void proxy(req, res, next).catch(next); + }); app.use((req, res) => { res.json({ rawBody: (req as any).rawBody?.toString() }); }); diff --git a/src/functionsShellCommandAction.ts b/src/functionsShellCommandAction.ts index fb6cefbba08..3ac62b52bf5 100644 --- a/src/functionsShellCommandAction.ts +++ b/src/functionsShellCommandAction.ts @@ -143,7 +143,7 @@ export const actionFunction = async (options: Options) => { return new Promise((resolve) => { replServer.on("exit", () => { - return serveFunctions.stop().then(resolve).catch(resolve); + serveFunctions.stop().then(resolve).catch(resolve); }); }); }); diff --git a/src/hosting/proxy.ts b/src/hosting/proxy.ts index e3f90ddaffe..51fad8e336a 100644 --- a/src/hosting/proxy.ts +++ b/src/hosting/proxy.ts @@ -1,7 +1,7 @@ import { capitalize, includes } from "lodash"; import { IncomingMessage, ServerResponse } from "http"; import { PassThrough } from "stream"; -import { Request, RequestHandler, Response } from "express"; +import { NextFunction, Request, RequestHandler, Response } from "express"; import { URL } from "url"; import { Client, HttpMethod } from "../apiv2"; @@ -55,7 +55,11 @@ export function proxyRequestHandler( rewriteIdentifier: string, options: { forceCascade?: boolean } = {}, ): RequestHandler { - return async (req: IncomingMessage, res: ServerResponse, next: () => void): Promise => { + const handle = async ( + req: IncomingMessage, + res: ServerResponse, + next: NextFunction, + ): Promise => { logger.info(`[hosting] Rewriting ${req.url} to ${url} for ${rewriteIdentifier}`); // Extract the __session cookie from headers to forward it to the // functions cookie is not a string[]. @@ -229,6 +233,9 @@ export function proxyRequestHandler( res.statusCode = proxyRes.status; proxyRes.body.pipe(res); }; + return (req, res, next) => { + void handle(req, res, next).catch(next); + }; } /** diff --git a/src/mcp/apps/init/mcp-app.ts b/src/mcp/apps/init/mcp-app.ts index b509973e814..761b4bd8fec 100644 --- a/src/mcp/apps/init/mcp-app.ts +++ b/src/mcp/apps/init/mcp-app.ts @@ -111,7 +111,7 @@ googleCheckbox.addEventListener("change", (e) => { } }); -initBtn.addEventListener("click", async () => { +async function onInitClick(): Promise { const selectedProduct = Array.from(productRadios).find((r) => r.checked)?.value; if (!selectedProjectId) { @@ -205,6 +205,11 @@ initBtn.addEventListener("click", async () => { initBtn.disabled = false; initBtn.textContent = "Initialize"; } +} + +// onInitClick catches all errors and reports status via setStatus +initBtn.addEventListener("click", () => { + void onInitClick(); }); app.onhostcontextchanged = (ctx) => { diff --git a/src/mcp/index.ts b/src/mcp/index.ts index 4b3516d3358..51b4b949ad4 100644 --- a/src/mcp/index.ts +++ b/src/mcp/index.ts @@ -537,7 +537,7 @@ export class FirebaseMcpServer { const port = options.port || 3000; const transports: Record = {}; // session ID to transport - app.get("/sse", async (req: express.Request, res: express.Response) => { + const handleSse = async (req: express.Request, res: express.Response): Promise => { this.logger.debug(`[SSE] GET /sse connection attempt from ${req.ip}`); try { const transport = new SSEServerTransport("/message", res); @@ -564,9 +564,14 @@ export class FirebaseMcpServer { } catch (err) { this.logger.error(`[SSE] Connection error: ${err}`); } + }; + + // handleSse catches all errors and logs via this.logger.error + app.get("/sse", (req: express.Request, res: express.Response) => { + void handleSse(req, res); }); - app.post("/message", async (req: express.Request, res: express.Response) => { + const handleMessage = async (req: express.Request, res: express.Response): Promise => { const sessionId = req.query.sessionId as string; this.logger.debug(`[SSE] POST /message attempt for session ${sessionId}`); @@ -585,6 +590,11 @@ export class FirebaseMcpServer { ); res.status(400).send("No active SSE transport connection found for this session"); } + }; + + // handleMessage catches all errors and logs via this.logger.error + app.post("/message", (req: express.Request, res: express.Response) => { + void handleMessage(req, res); }); app.listen(port, "127.0.0.1", () => { diff --git a/src/utils.ts b/src/utils.ts index 3e2f61d7025..6c4c13db56b 100644 --- a/src/utils.ts +++ b/src/utils.ts @@ -404,12 +404,14 @@ export async function promiseWhile( if (check(res)) { return resolve(res); } - setTimeout(run, interval); + // run catches all errors and routes them to promiseReject + setTimeout(() => void run(), interval); } catch (err: unknown) { return promiseReject(err); } }; - run(); + // run catches all errors and routes them to promiseReject + void run(); }); } From 700ba91d4a6b11eee88d2573e9d19988baaeca59 Mon Sep 17 00:00:00 2001 From: Joe Hanley Date: Fri, 23 Oct 2026 04:34:13 +0000 Subject: [PATCH 2/4] fix: address PR review feedback on PR #11347 --- src/emulator/hub.spec.ts | 45 ++++++++++++++++++++++++++++++++++++++++ src/emulator/hub.ts | 2 ++ 2 files changed, 47 insertions(+) create mode 100644 src/emulator/hub.spec.ts diff --git a/src/emulator/hub.spec.ts b/src/emulator/hub.spec.ts new file mode 100644 index 00000000000..591e010f2c3 --- /dev/null +++ b/src/emulator/hub.spec.ts @@ -0,0 +1,45 @@ +import { expect } from "chai"; +import * as supertest from "supertest"; +import { EmulatorHub } from "./hub"; + +describe("EmulatorHub", () => { + let hub: EmulatorHub; + let app: any; + + before(async () => { + hub = new EmulatorHub({ + projectId: "test-project", + listen: [{ address: "127.0.0.1", port: 4400, family: "IPv4" }], + listenForEmulator: {} as any, + }); + // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment + app = await (hub as any).createExpressApp(); + }); + + describe("PATH_EXPORT", () => { + it("should reject external requests with 403 when origin header is present", async () => { + const res = await supertest(app) + .post(EmulatorHub.PATH_EXPORT) + .set("origin", "http://malicious.example.com") + .send({ path: "/tmp/export" }); + + expect(res.status).to.equal(403); + expect(res.body).to.deep.equal({ + message: "Export cannot be triggered by external callers.", + }); + }); + }); + + describe("PATH_CLEAR_DATA_CONNECT", () => { + it("should reject external requests with 403 when origin header is present", async () => { + const res = await supertest(app) + .post(EmulatorHub.PATH_CLEAR_DATA_CONNECT) + .set("origin", "http://malicious.example.com"); + + expect(res.status).to.equal(403); + expect(res.body).to.deep.equal({ + message: "Clear SQL Connect cannot be triggered by external callers.", + }); + }); + }); +}); diff --git a/src/emulator/hub.ts b/src/emulator/hub.ts index fbba683a95a..4e65b1b896d 100644 --- a/src/emulator/hub.ts +++ b/src/emulator/hub.ts @@ -112,6 +112,7 @@ export class EmulatorHub extends ExpressBasedEmulator { res.status(403).json({ message: `Export cannot be triggered by external callers.`, }); + return; } const path: string = req.body.path; const initiatedBy: string = req.body.initiatedBy || "unknown"; @@ -184,6 +185,7 @@ export class EmulatorHub extends ExpressBasedEmulator { res.status(403).json({ message: `Clear SQL Connect cannot be triggered by external callers.`, }); + return; } utils.logLabeledBullet("emulators", `Clearing data from SQL Connect data sources.`); From 837a6d1383a632aa694dea4418471bbf43ef6883 Mon Sep 17 00:00:00 2001 From: Joe Hanley Date: Fri, 23 Oct 2026 05:11:45 +0000 Subject: [PATCH 3/4] fix: address adversarial review feedback on PR #11347 --- CHANGELOG.md | 2 +- src/auth.ts | 14 +++++------ src/database/import.spec.ts | 7 ++++++ src/emulator/functionsEmulatorRuntime.ts | 4 ++- src/emulator/hub.spec.ts | 32 ++++++++++++++++-------- src/emulator/shared/express.ts | 4 ++- 6 files changed, 43 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c9efbe2210a..2f9759dbe00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,4 +1,4 @@ - Updated chokidar dependency to ^4.0.3 to remove vulnerable braces dependency (#11308). - Fixed an issue where the Firebase MCP server referred to `update_firebase_environment` instead of `firebase_update_environment`. -- Fixed `database:import` hanging when a batch write fails. +- Fixed `database:import` failing with an unhandled promise rejection when a batch write fails. - Fixed emulator crashes when a Firestore rules reload or emulator HTTP handler throws. diff --git a/src/auth.ts b/src/auth.ts index aa964958cca..73351e6bb19 100644 --- a/src/auth.ts +++ b/src/auth.ts @@ -605,6 +605,13 @@ async function loginWithLocalhost( getTokens: (queryCode: string, callbackUrl: string) => Promise, ): Promise { return new Promise((resolve, reject) => { + const server = http.createServer((req, res) => { + handleCallback(req, res).catch((err: unknown) => { + reject(err); + server.close(); + }); + }); + async function handleCallback( req: http.IncomingMessage, res: http.ServerResponse, @@ -639,13 +646,6 @@ async function loginWithLocalhost( return; } - const server = http.createServer((req, res) => { - handleCallback(req, res).catch((err: unknown) => { - reject(err); - server.close(); - }); - }); - server.listen(port, () => { logger.info(); logger.info("Visit this URL on this device to log in:"); diff --git a/src/database/import.spec.ts b/src/database/import.spec.ts index b0534dbabad..6a14fdb56e7 100644 --- a/src/database/import.spec.ts +++ b/src/database/import.spec.ts @@ -217,4 +217,11 @@ describe("DatabaseImporter", () => { expect(responses).to.have.length(1); expect(nock.isDone()).to.be.true; }); + + it("rejects with the write error when a batch write fails", async () => { + nock("https://test-db.firebaseio.com").get("/foo.json?shallow=true").reply(200); + nock("https://test-db.firebaseio.com").patch("/foo.json").reply(400, { error: "bad" }); + const importer = new DatabaseImporter(dbUrl, DATA_STREAM, "/", payloadSize, concurrencyLimit); + await expect(importer.execute()).to.be.rejectedWith(FirebaseError, /400/); + }); }); diff --git a/src/emulator/functionsEmulatorRuntime.ts b/src/emulator/functionsEmulatorRuntime.ts index 72f9e2450f3..a70b2b5b61a 100644 --- a/src/emulator/functionsEmulatorRuntime.ts +++ b/src/emulator/functionsEmulatorRuntime.ts @@ -898,7 +898,9 @@ async function main(): Promise { } } catch (err: unknown) { new EmulatorLog("FATAL", "runtime-error", getErrStack(err)).log(); - res.status(500).send(getErrMsg(err)); + if (!res.headersSent) { + res.status(500).send(getErrMsg(err)); + } } } diff --git a/src/emulator/hub.spec.ts b/src/emulator/hub.spec.ts index 591e010f2c3..493a05df176 100644 --- a/src/emulator/hub.spec.ts +++ b/src/emulator/hub.spec.ts @@ -1,23 +1,33 @@ import { expect } from "chai"; +import type { Express } from "express"; +import * as sinon from "sinon"; import * as supertest from "supertest"; -import { EmulatorHub } from "./hub"; +import { EmulatorHub, EmulatorHubArgs } from "./hub"; +import { HubExport } from "./hubExport"; +import { EmulatorRegistry } from "./registry"; describe("EmulatorHub", () => { - let hub: EmulatorHub; - let app: any; + let app: Express; + let exportAll: sinon.SinonStub; + let registryGet: sinon.SinonStub; - before(async () => { - hub = new EmulatorHub({ + beforeEach(async () => { + exportAll = sinon.stub(HubExport.prototype, "exportAll").resolves(); + registryGet = sinon.stub(EmulatorRegistry, "get").returns(undefined); + const hub = new EmulatorHub({ projectId: "test-project", listen: [{ address: "127.0.0.1", port: 4400, family: "IPv4" }], - listenForEmulator: {} as any, + listenForEmulator: {} as EmulatorHubArgs["listenForEmulator"], }); - // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment - app = await (hub as any).createExpressApp(); + app = await hub["createExpressApp"](); + }); + + afterEach(() => { + sinon.restore(); }); describe("PATH_EXPORT", () => { - it("should reject external requests with 403 when origin header is present", async () => { + it("should reject external requests with 403 without exporting", async () => { const res = await supertest(app) .post(EmulatorHub.PATH_EXPORT) .set("origin", "http://malicious.example.com") @@ -27,11 +37,12 @@ describe("EmulatorHub", () => { expect(res.body).to.deep.equal({ message: "Export cannot be triggered by external callers.", }); + expect(exportAll).to.not.have.been.called; }); }); describe("PATH_CLEAR_DATA_CONNECT", () => { - it("should reject external requests with 403 when origin header is present", async () => { + it("should reject external requests with 403 without clearing data", async () => { const res = await supertest(app) .post(EmulatorHub.PATH_CLEAR_DATA_CONNECT) .set("origin", "http://malicious.example.com"); @@ -40,6 +51,7 @@ describe("EmulatorHub", () => { expect(res.body).to.deep.equal({ message: "Clear SQL Connect cannot be triggered by external callers.", }); + expect(registryGet).to.not.have.been.called; }); }); }); diff --git a/src/emulator/shared/express.ts b/src/emulator/shared/express.ts index d1264190573..3f7242e5bec 100644 --- a/src/emulator/shared/express.ts +++ b/src/emulator/shared/express.ts @@ -9,6 +9,8 @@ export function asyncHandler( fn: (req: Request, res: Response, next: NextFunction) => Promise, ): RequestHandler { return (req, res, next) => { - void fn(req, res, next).catch(next); + void fn(req, res, next).catch((err: unknown) => { + next(err ?? new Error("Async route handler rejected without a reason")); + }); }; } From b2206b1e1232d1e53715194775847c30ba16fe3f Mon Sep 17 00:00:00 2001 From: Joe Hanley Date: Fri, 6 Nov 2026 22:24:34 +0000 Subject: [PATCH 4/4] fix(emulator): return JSON errors from the Emulator Hub and Storage emulator Adds jsonErrorHandler to src/emulator/shared/express.ts and registers it after all routes in the Emulator Hub and the Storage emulator. Errors that reach Express's error handling, such as rejections that asyncHandler forwards and malformed JSON bodies, now return a JSON body instead of Express's default HTML error page. 5xx responses use a generic message, and the error with its stack is logged through EmulatorLogger. 4xx errors keep their status and message. Adds src/emulator/shared/express.spec.ts. --- src/emulator/hub.ts | 3 +- src/emulator/shared/express.spec.ts | 106 ++++++++++++++++++++++++++++ src/emulator/shared/express.ts | 31 +++++++- src/emulator/storage/server.ts | 3 +- 4 files changed, 140 insertions(+), 3 deletions(-) create mode 100644 src/emulator/shared/express.spec.ts diff --git a/src/emulator/hub.ts b/src/emulator/hub.ts index 4e65b1b896d..135faff3611 100644 --- a/src/emulator/hub.ts +++ b/src/emulator/hub.ts @@ -12,7 +12,7 @@ import { FunctionsEmulator } from "./functionsEmulator"; import { ExpressBasedEmulator } from "./ExpressBasedEmulator"; import { PortName } from "./portUtils"; import { DataConnectEmulator } from "./dataconnectEmulator"; -import { asyncHandler } from "./shared/express"; +import { asyncHandler, jsonErrorHandler } from "./shared/express"; // We use the CLI version from package.json const pkg = require("../../package.json"); @@ -200,6 +200,7 @@ export class EmulatorHub extends ExpressBasedEmulator { }), ); + app.use(jsonErrorHandler(Emulators.HUB)); return app; } diff --git a/src/emulator/shared/express.spec.ts b/src/emulator/shared/express.spec.ts new file mode 100644 index 00000000000..750298fa84f --- /dev/null +++ b/src/emulator/shared/express.spec.ts @@ -0,0 +1,106 @@ +import { expect } from "chai"; +import * as express from "express"; +import * as sinon from "sinon"; +import * as supertest from "supertest"; +import { EmulatorLogger } from "../emulatorLogger"; +import { Emulators } from "../types"; +import { asyncHandler, jsonErrorHandler } from "./express"; + +describe("shared/express", () => { + let logStub: sinon.SinonStub; + + beforeEach(() => { + logStub = sinon.stub(EmulatorLogger.prototype, "log"); + }); + + afterEach(() => { + sinon.restore(); + }); + + function appWithRoute( + handler: (req: express.Request, res: express.Response) => Promise, + ): express.Express { + const app = express(); + // Keeps Express from printing errors it handles itself (after a response was sent) to stderr. + app.set("env", "test"); + app.get("/route", asyncHandler(handler)); + app.use(jsonErrorHandler(Emulators.HUB)); + return app; + } + + describe("jsonErrorHandler", () => { + it("returns a generic JSON 500 without the error details when a handler rejects", async () => { + const app = appWithRoute(() => Promise.reject(new Error("boom at /home/someone/project"))); + + const res = await supertest(app).get("/route"); + + expect(res.status).to.equal(500); + expect(res.type).to.equal("application/json"); + expect(res.body).to.deep.equal({ error: { code: 500, message: "Internal error" } }); + expect(res.text).to.not.include("boom"); + expect(res.text).to.not.include("/home/someone"); + }); + + it("logs server errors through the emulator logger", async () => { + const app = appWithRoute(() => Promise.reject(new Error("boom"))); + + await supertest(app).get("/route"); + + expect(logStub).to.have.been.calledOnce; + const [type, text] = logStub.firstCall.args as [string, string]; + expect(type).to.equal("WARN"); + expect(text).to.include("GET /route failed"); + expect(text).to.include("boom"); + }); + + it("keeps a client error's status and message", async () => { + const app = appWithRoute(() => + Promise.reject(Object.assign(new Error("Bad request body"), { status: 400 })), + ); + + const res = await supertest(app).get("/route"); + + expect(res.status).to.equal(400); + expect(res.body).to.deep.equal({ error: { code: 400, message: "Bad request body" } }); + expect(logStub).to.not.have.been.called; + }); + + it("answers malformed JSON bodies with a JSON 400", async () => { + const app = express(); + app.post("/route", express.json(), (req, res) => { + res.json({ ok: true }); + }); + app.use(jsonErrorHandler(Emulators.STORAGE)); + + const res = await supertest(app) + .post("/route") + .set("Content-Type", "application/json") + .send("{"); + + expect(res.status).to.equal(400); + expect(res.type).to.equal("application/json"); + expect(res.body).to.have.nested.property("error.code", 400); + expect(res.text).to.not.include(" at "); + }); + + it("leaves a response that was already sent alone", async () => { + let handlerDone: () => void = () => undefined; + const handlerSettled = new Promise((resolve) => { + handlerDone = resolve; + }); + const app = appWithRoute(async (req, res) => { + res.status(200).json({ ok: true }); + await new Promise((resolve) => res.once("finish", resolve)); + setImmediate(handlerDone); + throw new Error("late failure"); + }); + + const res = await supertest(app).get("/route"); + await handlerSettled; + + expect(res.status).to.equal(200); + expect(res.body).to.deep.equal({ ok: true }); + expect(logStub).to.not.have.been.called; + }); + }); +}); diff --git a/src/emulator/shared/express.ts b/src/emulator/shared/express.ts index 3f7242e5bec..15dddb96232 100644 --- a/src/emulator/shared/express.ts +++ b/src/emulator/shared/express.ts @@ -1,4 +1,7 @@ -import type { NextFunction, Request, RequestHandler, Response } from "express"; +import type { ErrorRequestHandler, NextFunction, Request, RequestHandler, Response } from "express"; +import { getErrMsg, getErrStack } from "../../error"; +import { EmulatorLogger } from "../emulatorLogger"; +import { Emulators } from "../types"; /** * Adapts an async route handler for Express 4, which ignores the promise a handler returns. @@ -14,3 +17,29 @@ export function asyncHandler( }); }; } + +/** + * Last-resort error handler for an emulator's Express app. Register it after all routes. + * Replies with JSON instead of Express's default HTML error page, which includes the stack + * trace unless NODE_ENV is "production", and logs server errors through the emulator logger. + */ +export function jsonErrorHandler(emulator: Emulators): ErrorRequestHandler { + return (err: unknown, req, res, next) => { + if (res.headersSent) { + next(err); + return; + } + const e = err as { status?: unknown; statusCode?: unknown } | undefined; + const status = e?.status ?? e?.statusCode; + const code = typeof status === "number" && status >= 400 && status < 600 ? status : 500; + if (code >= 500) { + EmulatorLogger.forEmulator(emulator).log( + "WARN", + `${req.method} ${req.path} failed: ${getErrStack(err)}`, + ); + } + res + .status(code) + .json({ error: { code, message: code < 500 ? getErrMsg(err) : "Internal error" } }); + }; +} diff --git a/src/emulator/storage/server.ts b/src/emulator/storage/server.ts index 2c08c9e5c3f..56601dc2de1 100644 --- a/src/emulator/storage/server.ts +++ b/src/emulator/storage/server.ts @@ -8,7 +8,7 @@ import { RulesConfig, StorageEmulator } from "./index"; import { createFirebaseEndpoints } from "./apis/firebase"; import { InvalidArgumentError } from "../auth/errors"; import { SourceFile } from "./rules/types"; -import { asyncHandler } from "../shared/express"; +import { asyncHandler, jsonErrorHandler } from "../shared/express"; /** * @param defaultProjectId @@ -177,6 +177,7 @@ export function createApp( app.use("/v0", createFirebaseEndpoints(emulator)); app.use("/", createCloudEndpoints(emulator)); + app.use(jsonErrorHandler(Emulators.STORAGE)); return Promise.resolve(app); }