diff --git a/.changeset/fix-format-errors-missing-await.md b/.changeset/fix-format-errors-missing-await.md new file mode 100644 index 000000000000..7e50f2f92d0a --- /dev/null +++ b/.changeset/fix-format-errors-missing-await.md @@ -0,0 +1,7 @@ +--- +"@cloudflare/format-errors": patch +--- + +Fix error formatting to reliably return fallback responses on failure + +Previously, if something went wrong while formatting a pretty error page, the failure could go unhandled, resulting in no response being returned to the user. Now, errors during formatting are properly caught, ensuring users always receive a 500 JSON fallback response. diff --git a/packages/format-errors/package.json b/packages/format-errors/package.json index bdd00ef462dd..9301efefd59a 100644 --- a/packages/format-errors/package.json +++ b/packages/format-errors/package.json @@ -7,7 +7,8 @@ "check:lint": "eslint . --max-warnings=0 --cache", "check:type": "tsc", "deploy": "wrangler deploy", - "start": "wrangler dev" + "start": "wrangler dev", + "test:ci": "vitest run" }, "devDependencies": { "@cloudflare/eslint-config-shared": "workspace:*", @@ -17,6 +18,7 @@ "promjs": "^0.4.2", "toucan-js": "4.0.0", "tsconfig": "*", + "vitest": "catalog:default", "wrangler": "workspace:*", "zod": "^3.22.3" }, diff --git a/packages/format-errors/src/__tests__/index.test.ts b/packages/format-errors/src/__tests__/index.test.ts new file mode 100644 index 000000000000..136eb4d256b3 --- /dev/null +++ b/packages/format-errors/src/__tests__/index.test.ts @@ -0,0 +1,107 @@ +import { describe, expect, it, vi } from "vitest"; +import type { Payload } from "../index"; + +// Mock external dependencies that can't be resolved by Vite in test env +vi.mock("promjs", () => { + const mockCounter = { inc: vi.fn() }; + const mockRegistry = { + create: vi.fn(() => mockCounter), + metrics: vi.fn(() => ""), + }; + return { default: () => mockRegistry }; +}); + +vi.mock("toucan-js", () => { + return { + Toucan: vi.fn().mockImplementation(() => ({ + captureException: vi.fn(), + })), + }; +}); + +describe("handlePrettyErrorRequest", () => { + it("should propagate async rejections to callers", async () => { + // Mock Youch to throw asynchronously + vi.doMock("../Youch", () => { + return { + default: vi.fn().mockImplementation(() => ({ + addLink: vi.fn(), + toHTML: vi.fn().mockRejectedValue(new Error("Youch async error")), + })), + }; + }); + + // handlePrettyErrorRequest is async — if it's not awaited, async + // rejections (e.g. from Youch.toHTML()) won't be caught by a + // surrounding try/catch, leading to unhandled promise rejections. + // This test verifies that the function properly rejects so that + // callers who `await` it can catch the error. + const { handlePrettyErrorRequest } = await import("../index"); + + const payload: Payload = { + url: "https://example.com", + method: "GET", + headers: { "content-type": "text/html" }, + error: { + message: "Test error", + name: "Error", + stack: "Error: Test error\n at test:1:1", + }, + }; + + await expect(handlePrettyErrorRequest(payload)).rejects.toThrow( + "Youch async error" + ); + }); +}); + +describe("reviveError", () => { + it("should revive a plain Error", async () => { + const { reviveError } = await import("../index"); + const error = reviveError({ + message: "test", + name: "Error", + stack: "Error: test\n at foo:1:1", + }); + expect(error).toBeInstanceOf(Error); + expect(error.message).toBe("test"); + expect(error.name).toBe("Error"); + expect(error.stack).toBe("Error: test\n at foo:1:1"); + }); + + it("should revive a TypeError", async () => { + const { reviveError } = await import("../index"); + const error = reviveError({ + message: "x is not a function", + name: "TypeError", + }); + expect(error).toBeInstanceOf(TypeError); + expect(error.message).toBe("x is not a function"); + }); + + it("should revive an error with a cause", async () => { + const { reviveError } = await import("../index"); + const error = reviveError({ + message: "outer", + name: "Error", + cause: { + message: "inner", + name: "RangeError", + }, + }); + expect(error).toBeInstanceOf(Error); + expect(error.message).toBe("outer"); + expect(error.cause).toBeInstanceOf(RangeError); + expect((error.cause as Error).message).toBe("inner"); + }); + + it("should fall back to Error for unknown error names", async () => { + const { reviveError } = await import("../index"); + const error = reviveError({ + message: "custom", + name: "CustomError", + }); + expect(error).toBeInstanceOf(Error); + expect(error.name).toBe("CustomError"); + }); +}); diff --git a/packages/format-errors/src/index.ts b/packages/format-errors/src/index.ts index e372a106b0f0..f29859feefe9 100644 --- a/packages/format-errors/src/index.ts +++ b/packages/format-errors/src/index.ts @@ -167,7 +167,7 @@ export default { } try { - return handlePrettyErrorRequest(payload); + return await handlePrettyErrorRequest(payload); } catch (e) { sentry.captureException(e); const errorCounter = registry.create( diff --git a/packages/format-errors/vitest.config.mts b/packages/format-errors/vitest.config.mts new file mode 100644 index 000000000000..89b8af32fa12 --- /dev/null +++ b/packages/format-errors/vitest.config.mts @@ -0,0 +1,23 @@ +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { defineProject, mergeConfig } from "vitest/config"; +import configShared from "../../vitest.shared"; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); + +export default mergeConfig( + configShared, + defineProject({ + resolve: { + alias: { + // promjs has a broken package.json (main points to lib/index.js + // which doesn't exist in the installed package). Alias it to the + // actual entry point so Vite can resolve it in tests. + promjs: path.resolve(__dirname, "node_modules/promjs/index.js"), + }, + }, + test: { + include: ["src/__tests__/**/*.{test,spec}.{ts,js}"], + }, + }) +); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index d5ba71b66337..1359f0f39c1d 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -2069,6 +2069,9 @@ importers: tsconfig: specifier: '*' version: 7.0.0 + vitest: + specifier: catalog:default + version: 3.2.3(@types/debug@4.1.12)(@types/node@20.19.9)(@vitest/ui@3.2.3)(jiti@2.6.1)(lightningcss@1.30.2)(msw@2.12.0(@types/node@20.19.9)(typescript@5.9.3))(tsx@4.21.0)(yaml@2.8.1) wrangler: specifier: workspace:* version: link:../wrangler @@ -10415,9 +10418,6 @@ packages: resolution: {integrity: sha512-TLz+x/vEXm/Y7P7wn1EJFNLxYpUD4TgMosxY6fAVJUnJMbupHBOncxyWUG9OpTaH9EBD7uFI5LfEgmMOc54DsA==} engines: {node: '>=8'} - devalue@5.3.2: - resolution: {integrity: sha512-UDsjUbpQn9kvm68slnrs+mfxwFkIflOhkanmyabZ8zOYk8SMEIbJ3TK+88g70hSIeytu4y18f0z/hYHMTrXIWw==} - devalue@5.6.3: resolution: {integrity: sha512-nc7XjUU/2Lb+SvEFVGcWLiKkzfw8+qHI7zn8WYXKkLMgfGSHbgCEaR6bJpev8Cm6Rmrb19Gfd/tZvGqx9is3wg==} @@ -11083,9 +11083,6 @@ packages: resolution: {integrity: sha512-f7ccFPK3SXFHpx15UIGyRJ/FJQctuKZ0zVuN3frBo4HnK3cay9VEW0R6yPYFHC0AgqhukPzKjq22t5DmAyqGyw==} engines: {node: '>=16'} - flatted@3.3.3: - resolution: {integrity: sha512-GX+ysw4PBCz0PzosHDepZGANEuFCMLrnRTiEy9McGjmkCQYwRq4A/X786G/fjM/+OjsWSU1ZrY5qyARZmO/uwg==} - flatted@3.3.4: resolution: {integrity: sha512-3+mMldrTAPdta5kjX2G2J7iX4zxtnwpdA8Tr2ZSjkyPSanvbZAcy6flmtnXbEybHrDcU9641lxrMfFuUxVz9vA==} @@ -11273,12 +11270,12 @@ packages: glob@7.2.3: resolution: {integrity: sha512-nFR0zLpU2YCaRxwoCJvL6UvCH2JFyFVIvwTLsIf21AuHlMskA1hhTdk+LlYJtOlYt9v6dvszD2BGRqBL+iQK9Q==} - deprecated: Old versions of glob are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting i@izs.me + deprecated: Glob versions prior to v9 are no longer supported glob@8.1.0: resolution: {integrity: sha512-r8hpEjiQEYlF2QU0df3dS+nxxSIreXQS1qRhMJM0Q5NDdR386C7jb7Hwwod8Fgiuex+k0GFjgft18yvxm5XoCQ==} engines: {node: '>=12'} - deprecated: Old versions of glob are not supported, and contain widely publicized security vulnerabilities, which have been fixed in the current version. Please update. Support for old versions may be purchased (at exorbitant rates) by contacting i@izs.me + deprecated: Glob versions prior to v9 are no longer supported globals@11.12.0: resolution: {integrity: sha512-WOBp/EEGUiIsJSp7wcv/y6MO+lV9UoncWqxuFfm8eBwzWNgyfBd6Gz+IeKQ9jCmyhoH99g15M3T+QaVHFjizVA==} @@ -16797,7 +16794,7 @@ snapshots: '@vitest/snapshot': 3.2.3 birpc: 0.2.14 cjs-module-lexer: 1.2.3 - devalue: 5.3.2 + devalue: 5.6.3 miniflare: 4.20251210.0 semver: 7.7.3 vitest: 2.1.9(@types/node@20.19.9)(@vitest/ui@2.1.9)(lightningcss@1.30.2)(msw@2.12.0(@types/node@20.19.9)(typescript@5.9.3)) @@ -16814,7 +16811,7 @@ snapshots: '@vitest/snapshot': 3.2.3 birpc: 0.2.14 cjs-module-lexer: 1.2.3 - devalue: 5.3.2 + devalue: 5.6.3 miniflare: 4.20251210.0 semver: 7.7.3 vitest: 3.2.3(@types/debug@4.1.12)(@types/node@20.19.9)(@vitest/ui@3.2.3)(jiti@2.6.1)(lightningcss@1.30.2)(msw@2.12.0(@types/node@20.19.9)(typescript@5.9.3))(tsx@4.21.0)(yaml@2.8.1) @@ -20537,7 +20534,7 @@ snapshots: dependencies: '@vitest/utils': 3.2.3 fflate: 0.8.2 - flatted: 3.3.3 + flatted: 3.3.4 pathe: 2.0.3 sirv: 3.0.2 tinyglobby: 0.2.15 @@ -21748,8 +21745,6 @@ snapshots: detect-newline@3.1.0: {} - devalue@5.3.2: {} - devalue@5.6.3: {} devtools-protocol@0.0.1182435: {} @@ -22860,13 +22855,10 @@ snapshots: flat-cache@4.0.1: dependencies: - flatted: 3.3.3 + flatted: 3.3.4 keyv: 4.5.4 - flatted@3.3.3: {} - - flatted@3.3.4: - optional: true + flatted@3.3.4: {} for-each@0.3.5: dependencies: @@ -25135,7 +25127,7 @@ snapshots: proxy-agent@6.4.0: dependencies: agent-base: 7.1.3 - debug: 4.3.4 + debug: 4.4.1(supports-color@9.2.2) http-proxy-agent: 7.0.2 https-proxy-agent: 7.0.6 lru-cache: 7.18.3