Return generic error messages to clients for server-side errors

[ChristianPavilonis]

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

[prk-Jr]

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.

[aram356]

@ChristianPavilonis Please resolve conflicts

[aram356]

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

[aram356]

Summary

Security-focused change that replaces the user_message() passthrough (which exposed internal error details like store names, config paths, proxy addresses) with categorized responses: client errors (4xx) get brief safe descriptions, server errors (5xx/502/503) get a generic message. Well-scoped single-file change with good test coverage.

Non-blocking

🤔 thinking

• Wildcard match may silently suppress future client errors: The _ => catch-all in user_message() defaults new variants to the generic message, even if they're 4xx client errors. Consider matching exhaustively to force explicit decisions. (error.rs:134)

♻️ refactor

• BadRequest format string duplicated: user_message() and #[display] both hardcode "Bad request: {message}" — could diverge silently. (error.rs:128)

🌱 seedling

• Consider a compile-time guarantee that status_code() and user_message() stay in sync: Both methods independently classify variants as client vs. server errors. A future follow-up could extract the classification into a single method (e.g., is_client_error()) or match exhaustively in both to prevent divergence.

📝 note

• InvalidUtf8 status code change is correct: The 400 → 500 change is appropriate — the only usage is for the embedded TOML settings file, which is a server-side resource.

CI Status

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

[aram356]

🤔 thinking — Wildcard match may silently suppress future client errors

The _ => catch-all means any new variant added to TrustedServerError will default to the generic "An internal error occurred" message — even if it's a 4xx client error. This is safe-by-default (no leaks), but it could silently give users unhelpful messages for legitimate client errors.

Consider matching exhaustively (no _) so the compiler forces an explicit decision when new variants are added. The status_code() method already matches exhaustively, so user_message() would stay consistent.

Alternatively, the existing test server_errors_return_generic_message partially mitigates this — but only if someone remembers to add new variants to the test.

[aram356]

♻️ refactor — BadRequest format string duplicated between Display and user_message()

user_message() returns format!("Bad request: {message}") which duplicates the #[display("Bad request: {message}")] attribute. If one changes, the other could silently diverge.

Suggestion: Reuse Display for the BadRequest arm:

Self::BadRequest { .. } => self.to_string(),

[aram356]

📝 note — InvalidUtf8 status code change from 400 → 500 is correct

The only usage is for the embedded trusted-server.toml file (settings_data.rs:24), which is a server-side resource. Invalid UTF-8 there is a server error, not a client error.