skip header rules on asset handler error (#5750)

* skip header rules on asset handler error

* Update packages/pages-shared/asset-server/handler.ts

Co-authored-by: Daniel Walsh <walshydev@gmail.com>

* Update packages/pages-shared/asset-server/handler.ts

Co-authored-by: Daniel Walsh <walshydev@gmail.com>

* update test to include 404, 301

* Only prevent attaching headers for 5XX errors

---------

Co-authored-by: Daniel Walsh <walshydev@gmail.com>
Co-authored-by: Greg Brimble <gbrimble@cloudflare.com>

.changeset/stale-books-greet.md

@@ -0,0 +1,8 @@
+---
+"@cloudflare/pages-shared": patch
+---
+
+fix: omit headers rules on internal error
+
+The Pages asset handler will no longer apply headers rules on 5XX responses caused by some internal error.
+This prevents transient errors from being cached when caching headers are being set by headers rules.

packages/pages-shared/__tests__/asset-server/handler.test.ts

@@ -774,6 +774,80 @@ describe("asset-server handler", () => {
 			expect(isPreservationCacheResponseExpiring(res)).toBe(true);
 		});
 	});
+
+	describe("internal asset error doesn't set headers", async () => {
+		const metadata = createMetadataObject({
+			deploymentId: "mock-deployment-id",
+			headers: {
+				invalid: [],
+				rules: [
+					{
+						path: "/*",
+						headers: { "x-unwanted-header": "foo" },
+						unsetHeaders: [],
+					},
+				],
+			},
+			redirects: {
+				invalid: [],
+				rules: [
+					{
+						from: "/here",
+						to: "/there",
+						status: 301,
+						lineNumber: 1,
+					},
+				],
+			},
+		}) as Metadata;
+
+		const findAssetEntryForPath = async (path: string) => {
+			if (path.startsWith("/asset")) return "some-asset";
+			return null;
+		};
+
+		test("500 skips headers", async () => {
+			const { response } = await getTestResponse({
+				request: "https://foo.com/asset",
+				metadata,
+				fetchAsset: async (...args) => {
+					throw "uh oh";
+				},
+				findAssetEntryForPath: findAssetEntryForPath,
+			});
+
+			expect(response.status).toBe(500);
+			expect(Object.fromEntries(response.headers)).not.toHaveProperty(
+				"x-unwanted-header"
+			);
+		});
+
+		test("404 doesn't skip headers", async () => {
+			const { response } = await getTestResponse({
+				request: "https://foo.com/404",
+				metadata,
+				findAssetEntryForPath: findAssetEntryForPath,
+			});
+
+			expect(response.status).toBe(404);
+			expect(Object.fromEntries(response.headers)).toHaveProperty(
+				"x-unwanted-header"
+			);
+		});
+
+		test("301 doesn't skip headers", async () => {
+			const { response } = await getTestResponse({
+				request: "https://foo.com/here",
+				metadata,
+				findAssetEntryForPath: findAssetEntryForPath,
+			});
+
+			expect(response.status).toBe(301);
+			expect(Object.fromEntries(response.headers)).toHaveProperty(
+				"x-unwanted-header"
+			);
+		});
+	});
 });
 
 interface HandlerSpies {

packages/pages-shared/asset-server/handler.ts

@@ -478,7 +478,11 @@ export async function generateHandler<
 		);
 	}
 
-	return await attachHeaders(await generateResponse());
+	const responseWithoutHeaders = await generateResponse();
+	if (responseWithoutHeaders.status >= 500) {
+		return responseWithoutHeaders;
+	}
+	return await attachHeaders(responseWithoutHeaders);
 
 	async function serveAsset(
 		servingAssetEntry: AssetEntry,