skip header rules on asset handler error

[aaronlisman]

What this PR solves / how to test

Fixes CUSTESC-40911

Author has addressed the following

• Tests
  • Included
• Changeset (Changeset guidelines)
  • Included
• Public documentation
  • [?] Not necessary because: bugfix, though I could see how it may be worth noting?

[changeset-bot]

🦋 Changeset detected

Latest commit: 65a474f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@cloudflare/pages-shared	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

A wrangler prerelease is available for testing. You can install this latest build in your project with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9026903249/npm-package-wrangler-5750

You can reference the automatically updated head of this PR with:

npm install --save-dev https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/prs/5750/npm-package-wrangler-5750

Or you can use npx with this latest build directly:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9026903249/npm-package-wrangler-5750 dev path/to/script.js

Additional artifacts:

npx https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9026903249/npm-package-create-cloudflare-5750 --no-auto-update

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9026903249/npm-package-cloudflare-kv-asset-handler-5750

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9026903249/npm-package-miniflare-5750

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9026903249/npm-package-cloudflare-pages-shared-5750

npm install https://prerelease-registry.devprod.cloudflare.dev/workers-sdk/runs/9026903249/npm-package-cloudflare-vitest-pool-workers-5750

Note that these links will no longer work once the GitHub Actions artifact expires.

---

wrangler@3.55.0 includes the following runtime dependencies:

Package	Constraint	Resolved
miniflare	workspace:*	3.20240419.1
workerd	1.20240419.0	1.20240419.0
workerd --version	1.20240419.0	2024-04-19

Please ensure constraints are pinned, and miniflare/workerd minor versions match.

[GregBrimble]

I'm a little concerned about omitting some headers even on 404s here. For example, some users might be hardening their site with security headers that will now be dropped on 404s. I had previously considered expanding the _headers and _redirects specs to support matching on request/response properties (incl. status) but never got that over the line. We maybe consider revisiting that idea instead here?

Edit: oops, didn't mean to approve.

[aaronlisman]

I'm a little concerned about omitting some headers even on 404s here. For example, some users might be hardening their site with security headers that will now be dropped on 404s. I had previously considered expanding the _headers and _redirects specs to support matching on request/response properties (incl. status) but never got that over the line. We maybe consider revisiting that idea instead here?

Edit: oops, didn't mean to approve.

instead of matching on the status code, should we catch exceptions from fetchAsset and plumb through a flag that says this was definitely an error? that seems closer to the intent of the bug fix, which is 'something definitely went wrong while fetching the asset, and we don't want to apply user headers'.

or at least just match on 5XX statuses?

[WalshyDev]

I'm a little concerned about omitting some headers even on 404s here. For example, some users might be hardening their site with security headers that will now be dropped on 404s.

True but we also don't really want to potentially cache a 404 for hours/days. If I hit 404 a js file while the site is propagating, I could easily break the site for all users hitting this colo.

We could also just remove cache-control and not the rest?

[GregBrimble]

Yeah, totally understand why we want this — avoiding caching 404s. But we can't add this if it breaks something else (potentially more important to some people).

For example, let's say someone is attaching an X-Frame-Options: DENY with _headers, and the page the attacker is embedding 404s, that doesn't necessarily say that there is nothing of interest on the page. It might still contain buttons with actions which shouldn't be clickjackable.

5XX are definitely less likely to be in this scenario since we'll control the error page, but I'm still a bit concerned about a full-stack site that potentially is then manipulating the asset page in some way, and where users are still reliant on controls they've placed in _headers.

[WalshyDev]

Indeed, my comment didn't disagree and offered an alternative. Fully agree with you, just also think we should solve 404s here too (safely).

[GregBrimble]

@aaronlisman and I chatted and decided that we're okay with not attaching headers on a Cloudflare-owned error page. Our thinking was that, if Workers went down, then we'd be serving a hard error from FL or whatever. @aaronlisman is going to change this PR to check for an actual error from asset serving, and we'll just leave it at that for the moment.

If we need to provide more control over conditionally including/excluding when these headers are applied, we can raise a feature request for that extension as new work and plan it as normal.

He's going to make that change here, and update docs to reflect that headers won't be served in that instance. I'm going to add docs to recommend that any security headers related to Functions code should be attached in that Functions code, rather than relying on pass through from the "upstream" assets with _headers.

[GregBrimble]

I tweaked it, so I kinda invalidate my own review here, but I'm no longer blocking :) @WalshyDev , want to take a quick second look, and then we'll get this merged?

[WalshyDev]

lgtm