Use with_body instead of with_body_text_plain for JSON responses#529
Use with_body instead of with_body_text_plain for JSON responses#529ChristianPavilonis wants to merge 2 commits intomainfrom
Conversation
aram356
left a comment
There was a problem hiding this comment.
@ChristianPavilonis Please resolve conflicts
The with_body_text_plain method sets Content-Type to text/plain, overriding the APPLICATION_JSON type set by with_content_type. Replace all 6 occurrences in request signing endpoints with with_body which preserves the previously set Content-Type. Closes #424
0fde405 to
4830434
Compare
aram356
left a comment
There was a problem hiding this comment.
Summary
Clean fix for with_body_text_plain() clobbering the Content-Type: application/json header at all 6 JSON response sites. The switch to with_body() is correct. However, the bug remains undetectable by the test suite — a regression test for the Content-Type header is needed.
Blocking
🔧 wrench
- Missing Content-Type assertion in tests: None of the existing tests check the
Content-Typeheader on responses. The bug this PR fixes could silently regress. At least one test should assertresp.get_content_type() == Some(APPLICATION_JSON). (crates/trusted-server-core/src/request_signing/endpoints.rs)
Non-blocking
⛏ nitpick
- PR checklist template mentions "tracing macros": The checklist says "Uses
tracingmacros (notprintln!)" but the project convention islogmacros per CLAUDE.md. Template issue only — the actual code correctly useslog::debug!.
🌱 seedling
- Consider a helper for JSON responses: There are 6 nearly-identical response construction patterns in this file. A small helper like
fn json_response(status: u16, body: &str) -> Responsewould eliminate this class of bug entirely. Not for this PR, but worth considering.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS
- integration tests: PASS
- browser integration tests: PASS
- CodeQL: PASS
| Ok(Response::from_status(200) | ||
| .with_content_type(fastly::mime::APPLICATION_JSON) | ||
| .with_body_text_plain(&json)) | ||
| .with_body(json)) |
There was a problem hiding this comment.
🔧 wrench — No test asserts the Content-Type header.
This exact bug existed because none of the existing tests check Content-Type on responses — they only parse the body. After this fix, the bug is still undetectable by the test suite and could silently regress.
At minimum, one test (e.g., test_handle_verify_signature_valid or test_handle_trusted_server_discovery) should assert:
assert_eq!(
resp.get_content_type(),
Some(fastly::mime::APPLICATION_JSON),
"should return application/json content type"
);
Summary
Content-Type: text/plaininstead ofapplication/jsonwith_body_text_plain()silently overrides theContent-Typeheader set bywith_content_type(APPLICATION_JSON)on the preceding line, breaking automated clients that check Content-Type before parsingwith_body()which sets the body without clobbering headersChanges
crates/common/src/request_signing/endpoints.rswith_body_text_plain()→with_body()across all 6 response sites (discovery, verify signature, rotate key success/error, deactivate key success/error)Closes
Closes #424
Test plan
cargo test --workspacecargo clippy --all-targets --all-features -- -D warningscargo fmt --all -- --checkcd crates/js/lib && npx vitest runcd crates/js/lib && npm run formatcd docs && npm run formatcargo build --bin trusted-server-fastly --release --target wasm32-wasip1fastly compute serveChecklist
unwrap()in production code — useexpect("should ...")tracingmacros (notprintln!)