Skip to content

Return generic error messages to clients for server-side errors#530

Open
ChristianPavilonis wants to merge 4 commits intomainfrom
fix/generic-error-messages
Open

Return generic error messages to clients for server-side errors#530
ChristianPavilonis wants to merge 4 commits intomainfrom
fix/generic-error-messages

Conversation

@ChristianPavilonis
Copy link
Collaborator

@ChristianPavilonis ChristianPavilonis commented Mar 19, 2026

Summary

  • Prevent server-side error details (store names, config paths, proxy addresses) from leaking to clients by replacing the user_message() passthrough with categorized responses
  • Client errors (4xx) retain brief, safe descriptions; server/integration errors (5xx/502/503) return a generic "An internal error occurred"
  • Add unit tests asserting the security boundary: all server-error variants must return only the generic message

Changes

File Change
crates/trusted-server-core/src/error.rs Replace self.to_string() in user_message() with a match that surfaces safe messages for client errors and a generic message for server errors. Add doc comments on BadRequest (user-visible message warning), GdprConsent (why detail is suppressed), and user_message trait method (updated semantics). Add two unit tests covering all variants.

Closes

Closes #437

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 log macros (not println!)
  • New code has tests
  • No secrets or credentials committed

@ChristianPavilonis ChristianPavilonis force-pushed the fix/generic-error-messages branch from 58c5906 to 820599c Compare March 19, 2026 21:03
@ChristianPavilonis ChristianPavilonis requested review from aram356 and prk-Jr and removed request for aram356 March 19, 2026 21:22
@ChristianPavilonis ChristianPavilonis marked this pull request as ready for review March 19, 2026 21:24
Copy link
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.

Summary

This hardens client-facing error bodies and the overall direction looks right. GitHub CI is passing, but I found a couple of follow-up improvements around classification drift and regression coverage.

Non-blocking

🤔 thinking

  • InvalidUtf8 is currently surfaced as Invalid request data, but its only producer today is embedded config decoding in settings_data.rs, so it still reads like a client error for a server bootstrap failure.

♻️ refactor

  • status_code() and user_message() now encode the exposure policy in separate matches. Centralizing that mapping behind a small helper or enum would make it harder for classification and response text to drift apart.

Copy link
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

Replace user_message() passthrough of Display output with a match
that returns generic messages for 5xx/502/503 errors while keeping
brief descriptions for 4xx client errors. Full error details are
already logged via log::error! and never lost.

Closes #437
Copy link
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

Well-scoped PR that prevents server-side error details from leaking to clients. The secure-by-default wildcard pattern (new variants automatically get the generic message) is the right approach for a security boundary.

Blocking

❓ question

  • InvalidUtf8 classification: Only produced by settings_data.rs:24 (server bootstrap), yet classified as a 4xx client error returning "Invalid request data". Should this fall through to the generic message? (crates/trusted-server-core/src/error.rs:131)
  • GdprConsent message suppression: The only 4xx error that fully hides its detail message. The rationale (consent strings may contain user data) is sound, but confirming intent and adding a doc comment on the variant would help future maintainers. (crates/trusted-server-core/src/error.rs:127)

Non-blocking

🤔 thinking

  • status_code() and user_message() can drift apart: Both methods encode client-vs-server classification in separate match blocks. If a new variant is classified as 4xx in status_code() but hits the wildcard in user_message(), it would return 400 with "An internal error occurred" — confusing but safe by default. Not a concern for this PR, but worth noting.

⛏ nitpick

  • BadRequest doc comment precision: Says "message is returned to clients" but the message is formatted via format!("Bad request: {message}"), not returned verbatim. (crates/trusted-server-core/src/error.rs:20)

🏕 camp site

  • PR body references old crate path crates/common/src/error.rs — the crate was renamed to trusted-server-core in PR #517.
  • PR checklist says "Uses tracing macros" but per CLAUDE.md the project uses the log crate.

CI Status

  • fmt: PASS
  • clippy: PASS
  • rust tests: PASS
  • js tests: PASS
  • format checks: PASS

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.

Error responses expose internal error context to clients

3 participants