From 3cc829d7f12f65b7686e2f56e4b651969049691f Mon Sep 17 00:00:00 2001 From: Aaron Lisman Date: Thu, 2 May 2024 19:52:43 -0500 Subject: [PATCH 1/5] skip header rules on asset handler error --- .changeset/stale-books-greet.md | 8 +++++ .../__tests__/asset-server/handler.test.ts | 30 +++++++++++++++++++ packages/pages-shared/asset-server/handler.ts | 6 +++- 3 files changed, 43 insertions(+), 1 deletion(-) create mode 100644 .changeset/stale-books-greet.md diff --git a/.changeset/stale-books-greet.md b/.changeset/stale-books-greet.md new file mode 100644 index 0000000000..d4b1f0a48c --- /dev/null +++ b/.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 500 responses caused by some internal error. +This prevents transient errors from being cached when caching headers are being set by headers rules. diff --git a/packages/pages-shared/__tests__/asset-server/handler.test.ts b/packages/pages-shared/__tests__/asset-server/handler.test.ts index 26fb546431..c489578b27 100644 --- a/packages/pages-shared/__tests__/asset-server/handler.test.ts +++ b/packages/pages-shared/__tests__/asset-server/handler.test.ts @@ -774,6 +774,36 @@ describe("asset-server handler", () => { expect(isPreservationCacheResponseExpiring(res)).toBe(true); }); }); + + test("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: [], + }, + ], + } + }) as Metadata; + + const { response } = await getTestResponse({ + request: "https://foo.com/", + metadata, + fetchAsset: async (...args) => { + throw "uh oh"; + }, + findAssetEntryForPath: async (path: string) => { + return "some page"; + }, + }); + + expect(response.status).toBe(500); + expect(Object.fromEntries(response.headers)).not.toHaveProperty("x-unwanted-header"); + }); }); interface HandlerSpies { diff --git a/packages/pages-shared/asset-server/handler.ts b/packages/pages-shared/asset-server/handler.ts index e8a32dcf95..26621488e2 100644 --- a/packages/pages-shared/asset-server/handler.ts +++ b/packages/pages-shared/asset-server/handler.ts @@ -446,7 +446,11 @@ export async function generateHandler< }; } ); - const matches = headersMatcher({ request }); + + // If the response was an internal error from the asset handler, skip applying header rules. + // This avoids a case where we can cache 500s for a long time, depending on headers set by a user. + const assetError = response.status >= 400 && response.status <= 599 + const matches = assetError ? [] : headersMatcher({ request }); // This keeps track of every header that we've set from _headers // because we want to combine user declared headers but overwrite From 13eb42346c8b5e121559e6b0ae420c9afb7c2ccb Mon Sep 17 00:00:00 2001 From: aaronlisman <83968625+aaronlisman@users.noreply.github.com> Date: Tue, 7 May 2024 10:39:15 -0500 Subject: [PATCH 2/5] Update packages/pages-shared/asset-server/handler.ts Co-authored-by: Daniel Walsh --- packages/pages-shared/asset-server/handler.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pages-shared/asset-server/handler.ts b/packages/pages-shared/asset-server/handler.ts index 26621488e2..fd51348581 100644 --- a/packages/pages-shared/asset-server/handler.ts +++ b/packages/pages-shared/asset-server/handler.ts @@ -448,7 +448,7 @@ export async function generateHandler< ); // If the response was an internal error from the asset handler, skip applying header rules. - // This avoids a case where we can cache 500s for a long time, depending on headers set by a user. + // This avoids a cases where we can cache undesirable files for a long time, depending on headers set by a user. const assetError = response.status >= 400 && response.status <= 599 const matches = assetError ? [] : headersMatcher({ request }); From 2ec2258c85c58d7847011cebf224c66489c5434a Mon Sep 17 00:00:00 2001 From: aaronlisman <83968625+aaronlisman@users.noreply.github.com> Date: Tue, 7 May 2024 10:46:09 -0500 Subject: [PATCH 3/5] Update packages/pages-shared/asset-server/handler.ts Co-authored-by: Daniel Walsh --- packages/pages-shared/asset-server/handler.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/pages-shared/asset-server/handler.ts b/packages/pages-shared/asset-server/handler.ts index fd51348581..2be476c9ae 100644 --- a/packages/pages-shared/asset-server/handler.ts +++ b/packages/pages-shared/asset-server/handler.ts @@ -447,7 +447,7 @@ export async function generateHandler< } ); - // If the response was an internal error from the asset handler, skip applying header rules. + // If the response was an internal error or 404 from the asset handler, skip applying header rules. // This avoids a cases where we can cache undesirable files for a long time, depending on headers set by a user. const assetError = response.status >= 400 && response.status <= 599 const matches = assetError ? [] : headersMatcher({ request }); From 339b66bacabe26979eb8c2796f05ef4d41aaa736 Mon Sep 17 00:00:00 2001 From: Aaron Lisman Date: Tue, 7 May 2024 11:19:56 -0500 Subject: [PATCH 4/5] update test to include 404, 301 --- .../__tests__/asset-server/handler.test.ts | 60 +++++++++++++++---- 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/packages/pages-shared/__tests__/asset-server/handler.test.ts b/packages/pages-shared/__tests__/asset-server/handler.test.ts index c489578b27..021808578a 100644 --- a/packages/pages-shared/__tests__/asset-server/handler.test.ts +++ b/packages/pages-shared/__tests__/asset-server/handler.test.ts @@ -775,7 +775,7 @@ describe("asset-server handler", () => { }); }); - test("internal asset error doesn't set headers", async () => { + describe("internal asset error doesn't set headers", async () => { const metadata = createMetadataObject({ deploymentId: "mock-deployment-id", headers: { @@ -787,22 +787,58 @@ describe("asset-server handler", () => { unsetHeaders: [], }, ], + }, + redirects: { + invalid: [], + rules: [{ + from: "/here", + to: "/there", + status: 301, + lineNumber: 1, + }], } }) as Metadata; - const { response } = await getTestResponse({ - request: "https://foo.com/", - metadata, - fetchAsset: async (...args) => { - throw "uh oh"; - }, - findAssetEntryForPath: async (path: string) => { - return "some page"; - }, + 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 skips headers", async () => { + const { response } = await getTestResponse({ + request: "https://foo.com/404", + metadata, + findAssetEntryForPath: findAssetEntryForPath, + }); + + expect(response.status).toBe(404); + expect(Object.fromEntries(response.headers)).not.toHaveProperty("x-unwanted-header"); }); - expect(response.status).toBe(500); - expect(Object.fromEntries(response.headers)).not.toHaveProperty("x-unwanted-header"); + test("301 still has 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"); + }); }); }); From 65a474f8e6b21777c856f4189bc377a1d8f3d5ab Mon Sep 17 00:00:00 2001 From: Greg Brimble Date: Thu, 9 May 2024 23:35:52 -0400 Subject: [PATCH 5/5] Only prevent attaching headers for 5XX errors --- .changeset/stale-books-greet.md | 2 +- .../__tests__/asset-server/handler.test.ts | 34 ++++++++++++------- packages/pages-shared/asset-server/handler.ts | 12 +++---- 3 files changed, 28 insertions(+), 20 deletions(-) diff --git a/.changeset/stale-books-greet.md b/.changeset/stale-books-greet.md index d4b1f0a48c..e7ca087edd 100644 --- a/.changeset/stale-books-greet.md +++ b/.changeset/stale-books-greet.md @@ -4,5 +4,5 @@ fix: omit headers rules on internal error -The Pages asset handler will no longer apply headers rules on 500 responses caused by some 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. diff --git a/packages/pages-shared/__tests__/asset-server/handler.test.ts b/packages/pages-shared/__tests__/asset-server/handler.test.ts index 021808578a..e3aaa6c397 100644 --- a/packages/pages-shared/__tests__/asset-server/handler.test.ts +++ b/packages/pages-shared/__tests__/asset-server/handler.test.ts @@ -783,20 +783,22 @@ describe("asset-server handler", () => { rules: [ { path: "/*", - headers: {"x-unwanted-header": "foo"}, + headers: { "x-unwanted-header": "foo" }, unsetHeaders: [], }, ], }, redirects: { invalid: [], - rules: [{ - from: "/here", - to: "/there", - status: 301, - lineNumber: 1, - }], - } + rules: [ + { + from: "/here", + to: "/there", + status: 301, + lineNumber: 1, + }, + ], + }, }) as Metadata; const findAssetEntryForPath = async (path: string) => { @@ -815,10 +817,12 @@ describe("asset-server handler", () => { }); expect(response.status).toBe(500); - expect(Object.fromEntries(response.headers)).not.toHaveProperty("x-unwanted-header"); + expect(Object.fromEntries(response.headers)).not.toHaveProperty( + "x-unwanted-header" + ); }); - test("404 skips headers", async () => { + test("404 doesn't skip headers", async () => { const { response } = await getTestResponse({ request: "https://foo.com/404", metadata, @@ -826,10 +830,12 @@ describe("asset-server handler", () => { }); expect(response.status).toBe(404); - expect(Object.fromEntries(response.headers)).not.toHaveProperty("x-unwanted-header"); + expect(Object.fromEntries(response.headers)).toHaveProperty( + "x-unwanted-header" + ); }); - test("301 still has headers", async () => { + test("301 doesn't skip headers", async () => { const { response } = await getTestResponse({ request: "https://foo.com/here", metadata, @@ -837,7 +843,9 @@ describe("asset-server handler", () => { }); expect(response.status).toBe(301); - expect(Object.fromEntries(response.headers)).toHaveProperty("x-unwanted-header"); + expect(Object.fromEntries(response.headers)).toHaveProperty( + "x-unwanted-header" + ); }); }); }); diff --git a/packages/pages-shared/asset-server/handler.ts b/packages/pages-shared/asset-server/handler.ts index 2be476c9ae..45750b6467 100644 --- a/packages/pages-shared/asset-server/handler.ts +++ b/packages/pages-shared/asset-server/handler.ts @@ -446,11 +446,7 @@ export async function generateHandler< }; } ); - - // If the response was an internal error or 404 from the asset handler, skip applying header rules. - // This avoids a cases where we can cache undesirable files for a long time, depending on headers set by a user. - const assetError = response.status >= 400 && response.status <= 599 - const matches = assetError ? [] : headersMatcher({ request }); + const matches = headersMatcher({ request }); // This keeps track of every header that we've set from _headers // because we want to combine user declared headers but overwrite @@ -482,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); /** We have non-standard cache behavior, so strip out all headers but keep the method */ function getCacheKey(): Request {