Skip to content

fix: respect wasm-set Cache-Control and CORS response headers#131

Closed
bazilek wants to merge 1 commit into
mainfrom
fix/respect-wasm-response-headers
Closed

fix: respect wasm-set Cache-Control and CORS response headers#131
bazilek wants to merge 1 commit into
mainfrom
fix/respect-wasm-response-headers

Conversation

@bazilek
Copy link
Copy Markdown

@bazilek bazilek commented May 9, 2026

Summary

crates/http-service/src/lib.rs was using HeaderMap::extend to merge runtime defaults onto the wasm component's response. extend appends, so a JS handler returning Cache-Control: public, max-age=60 ended up with both that value and the runtime's Cache-Control: no-store on the wire — and per RFC 9111 the more restrictive directive wins, silently overriding the developer's intent. Same hazard for Access-Control-Allow-Origin: *.

This PR turns the runtime's * and no-store from forced-overrides into actual defaults via HeaderMap::entry().or_insert_with(), applied in-place on the response headers. Per-app static rsp_headers config still takes precedence over both.

New precedence

app_cfg.rsp_headers (operator) > wasm response (developer) > runtime default

Why the defaults exist at all

Both Access-Control-Allow-Origin: * and Cache-Control: no-store shipped with the very first commit (2fbda12, 2024-05-07) — no commit-message rationale, no in-source comment, untouched in 4 follow-up refactors. Best inference: * is the *.fastedge.app product default for browseable origins; no-store is "safe by default" for dynamic edge compute. Both defensible as defaults, never re-evaluated as forced overrides. This PR keeps them as defaults.

Error-path reserved header

Reordered the error-response builder so X_CDN_INTERNAL_STATUS is forcibly inserted after apply_default_and_app_headers. Previously, an app config that mapped rsp_headers["x-cdn-internal-status"] would replace the genuine FastEdge error code (timeout/OOM/execute-error) in the helper's loop. The reserved internal status now always wins on error responses.

Tests

  • 4 pre-existing tests (test_success, test_timeout, test_insufficient_memory, test_success_with_fastedge_app_id) pass unchanged — their wasm fixtures don't set Cache-Control / Access-Control-Allow-Origin, so the defaults still kick in.
  • 7 new unit tests for apply_default_and_app_headers covering: defaults fill when wasm silent, wasm-set values are preserved (no append-duplicates), per-app config replaces wasm value, custom app headers, empty-value no-op, reserved internal status cannot be spoofed by rsp_headers.
$ cargo test -p http-service
test result: ok. 34 passed; 0 failed

Migration note

Apps that already set Cache-Control or Access-Control-Allow-Origin from JS will start having those values honored. Apps that don't set them still get the same no-store / * defaults as before.

Test plan

  • cargo test -p http-service
  • cargo test -p http-service --features metrics
  • cargo clippy -p http-service --all-targets — no new warnings (one pre-existing let_and_return at executor/http.rs:259 unrelated to this PR)
  • cargo fmt -p http-service
  • Manual smoke against a deployed app post-merge: re-add Cache-Control: public, max-age=60 to a JS handler, confirm value lands in the response headers without no-store siblings.

🤖 Generated with Claude Code

Replace HeaderMap::extend with entry().or_insert_with() so the runtime
defaults (Access-Control-Allow-Origin: *, Cache-Control: no-store) fill
only when the wasm component does not set them. Per-app `rsp_headers`
config still wins over both wasm and defaults.

Previously the defaults were unconditionally appended, producing duplicate
Cache-Control values; per RFC 9111 the more restrictive directive (no-store)
then silently overrode any cache-control the JS handler returned.

Adds 6 unit tests covering the new precedence:
  default fills when wasm silent, wasm-set values are preserved,
  per-app config replaces wasm value, custom app headers, empty-value
  no-op. Existing executor::http tests pass unchanged — their wasm
  fixtures don't set those headers, so defaults still kick in.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copy link
Copy Markdown
Collaborator

@qrdl qrdl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we need to address it differently.
We just shouldn't set these two response headers in our engine (so no need for merging two sets of headers), instead we have to add them in *.fastedge.app resource so traditional request (through the resource) have these headers in the response, but direct responses (from FastEdge app using fetch()) don't.

@bazilek
Copy link
Copy Markdown
Author

bazilek commented May 11, 2026

could be an option
cons:

  • changes the initial behaviour for origin path
  • customer won't be possible to remove headers for traditional path

@ruslanti ruslanti closed this May 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants