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.

@ChristianPavilonis Please resolve conflicts

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

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

Summary

Clean, focused security fix that correctly addresses #437. The user_message() method previously exposed internal details (store names, IP addresses, config paths) to clients. Now server/integration errors (5xx/502/503) return a generic string while client errors (4xx) retain safe descriptions.

Non-blocking

🌱 seedling

• status_code / user_message consistency test: The wildcard _ arm in user_message() catches everything with the generic message. If someone adds a new 4xx variant to status_code but forgets to update user_message, it would silently get the 500-style generic message. A test that asserts all 4xx variants have a specific (non-generic) user_message would catch this.

📝 note

• Tests use .into() for String construction: CLAUDE.md prefers Type::from(value) over .into() for clarity. Common Rust idiom in tests though — not worth blocking on.

👍 praise

• Correct reclassification of InvalidUtf8 from 400 to 500
• Thoughtful suppression of GdprConsent details (consent strings may contain user data)
• Comprehensive test coverage of all error variants

CI Status

All 13 checks: PASS