Return generic error messages to clients for server-side errors#530
Open
ChristianPavilonis wants to merge 4 commits intomainfrom
Open
Return generic error messages to clients for server-side errors#530ChristianPavilonis wants to merge 4 commits intomainfrom
ChristianPavilonis wants to merge 4 commits intomainfrom
Conversation
58c5906 to
820599c
Compare
prk-Jr
reviewed
Mar 20, 2026
Collaborator
prk-Jr
left a comment
There was a problem hiding this comment.
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
requested changes
Mar 20, 2026
Collaborator
aram356
left a comment
There was a problem hiding this comment.
@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
0e8dc86 to
b955ece
Compare
aram356
reviewed
Mar 20, 2026
Collaborator
aram356
left a comment
There was a problem hiding this comment.
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
InvalidUtf8classification: Only produced bysettings_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)GdprConsentmessage 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()anduser_message()can drift apart: Both methods encode client-vs-server classification in separatematchblocks. If a new variant is classified as 4xx instatus_code()but hits the wildcard inuser_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
BadRequestdoc comment precision: Says "message is returned to clients" but the message is formatted viaformat!("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 totrusted-server-corein PR #517. - PR checklist says "Uses
tracingmacros" but per CLAUDE.md the project uses thelogcrate.
CI Status
- fmt: PASS
- clippy: PASS
- rust tests: PASS
- js tests: PASS
- format checks: PASS
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
user_message()passthrough with categorized responsesChanges
crates/trusted-server-core/src/error.rsself.to_string()inuser_message()with amatchthat surfaces safe messages for client errors and a generic message for server errors. Add doc comments onBadRequest(user-visible message warning),GdprConsent(why detail is suppressed), anduser_messagetrait method (updated semantics). Add two unit tests covering all variants.Closes
Closes #437
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 ...")logmacros (notprintln!)