Skip to content

Use with_body instead of with_body_text_plain for JSON responses#529

Open
ChristianPavilonis wants to merge 5 commits intomainfrom
fix/json-content-type
Open

Use with_body instead of with_body_text_plain for JSON responses#529
ChristianPavilonis wants to merge 5 commits intomainfrom
fix/json-content-type

Conversation

@ChristianPavilonis
Copy link
Copy Markdown
Collaborator

@ChristianPavilonis ChristianPavilonis commented Mar 19, 2026 •
edited
Loading

Summary

  • Fix JSON API responses returning Content-Type: text/plain instead of application/json
  • with_body_text_plain() silently overrides the Content-Type header set by with_content_type(APPLICATION_JSON) on the preceding line, breaking automated clients that check Content-Type before parsing
  • Replace with with_body() which sets the body without clobbering headers

Changes

File Change
crates/common/src/request_signing/endpoints.rs Replace with_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 --workspace
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo fmt --all -- --check
  • JS tests: cd crates/js/lib && npx vitest run
  • JS format: cd crates/js/lib && npm run format
  • Docs format: cd docs && npm run format
  • WASM build: cargo build --bin trusted-server-fastly --release --target wasm32-wasip1
  • Manual testing via fastly compute serve

Checklist

  • Changes follow CLAUDE.md conventions
  • No unwrap() in production code — use expect("should ...")
  • Uses tracing macros (not println!)
  • New code has tests
  • No secrets or credentials committed

@ChristianPavilonis ChristianPavilonis marked this pull request as ready for review March 19, 2026 20:59
@aram356 aram356 assigned aram356 and ChristianPavilonis and unassigned aram356 Mar 20, 2026
prk-Jr
prk-Jr approved these changes Mar 20, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@prk-Jr prk-Jr left a comment

Choose a reason for hiding this comment

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

👍 Looks good

aram356
aram356 requested changes Mar 20, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

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

@ChristianPavilonis Please resolve conflicts

@ChristianPavilonis
Use with_body instead of with_body_text_plain for JSON responses
4830434 
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
aram356
aram356 requested changes Mar 20, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

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

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-Type header on responses. The bug this PR fixes could silently regress. At least one test should assert resp.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 tracing macros (not println!)" but the project convention is log macros per CLAUDE.md. Template issue only — the actual code correctly uses log::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) -> Response would 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

aram356
aram356 reviewed Mar 24, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

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

Summary

Clean, well-scoped bug fix. Correctly identifies that with_body_text_plain() clobbers the Content-Type: application/json header and replaces all 6 occurrences with with_body(). All CI passes.

Non-blocking

🤔 thinking

  • Content-Type assertions missing for rotate/deactivate endpoints: The PR adds get_content_type() assertions to verify signature and discovery tests, but not to the rotate/deactivate key tests (test_handle_rotate_key_with_empty_body, test_handle_deactivate_key_request, etc.). The successful Ok(mut resp) arms in those tests could also assert Content-Type for full coverage of the fix across all 6 response sites.

CI Status

  • fmt: PASS
  • clippy: PASS
  • rust tests: PASS
  • js tests: PASS
  • integration tests: PASS
  • browser integration tests: PASS
  • CodeQL: PASS

aram356
aram356 approved these changes Mar 30, 2026
View reviewed changes
Copy link
Copy Markdown
Collaborator

@aram356 aram356 left a comment

Choose a reason for hiding this comment

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

Summary

Clean, focused fix for JSON API responses returning Content-Type: text/plain instead of application/json. The with_body_text_plain() method was silently overriding the content type set by with_content_type(APPLICATION_JSON) on the preceding line. Replacing with with_body() preserves the correct header. All 6 response sites fixed, tests added.

Non-blocking

🤔 thinking

  • Missing Content-Type assertions in rotate/deactivate tests: Content-Type assertions were added to 3 of 6 response sites (verify valid, verify invalid, discovery). The test_handle_rotate_key_* and test_handle_deactivate_key_* tests use a match pattern where both arms just log::debug!, so they pass regardless — the fix isn't verified there. Adding assert_eq!(resp.get_content_type(), Some(fastly::mime::APPLICATION_JSON), ...) inside the Ok arms would close the gap. (endpoints.rs:440-556)

📌 out of scope

  • to_error_response in error.rs still uses with_body_text_plain: Returns error messages as text/plain, inconsistent with the structured JSON error responses in endpoints.rs (lines 221, 330) that return { success: false, error: "..." } on 500s. Separate concern — worth a follow-up for API consistency.

👍 praise

  • Minimal, mechanical fix with clear motivation and good test coverage. Commit messages and PR description are excellent.

CI Status

  • cargo fmt: PASS
  • cargo clippy: PASS
  • cargo test: PASS
  • vitest: PASS
  • integration tests: PASS
  • browser integration tests: PASS
  • CodeQL: PASS

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aram356 aram356 aram356 approved these changes

@prk-Jr prk-Jr prk-Jr approved these changes

Assignees

@ChristianPavilonis ChristianPavilonis

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

with_body_text_plain may override APPLICATION_JSON Content-Type

3 participants

@ChristianPavilonis @aram356 @prk-Jr