-
Notifications
You must be signed in to change notification settings - Fork 8
Return generic error messages to clients for server-side errors #530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
b955ece
5936a4e
763ee4a
2d1e798
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,10 @@ use http::StatusCode; | |
| #[derive(Debug, Display)] | ||
| pub enum TrustedServerError { | ||
| /// Client-side input/validation error resulting in a 400 Bad Request. | ||
| /// | ||
| /// **Note:** The `message` field is included in client-facing HTTP responses | ||
| /// via [`IntoHttpResponse::user_message()`]. Keep it free of internal | ||
| /// implementation details. | ||
| #[display("Bad request: {message}")] | ||
| BadRequest { message: String }, | ||
| /// Configuration errors that prevent the server from starting. | ||
|
|
@@ -30,6 +34,10 @@ pub enum TrustedServerError { | |
| #[display("GAM error: {message}")] | ||
| Gam { message: String }, | ||
| /// GDPR consent handling error. | ||
| /// | ||
| /// **Note:** Unlike [`BadRequest`](Self::BadRequest), the detail `message` | ||
| /// is intentionally suppressed in client-facing responses because consent | ||
| /// strings may contain user data. Only the category name is returned. | ||
| #[display("GDPR consent error: {message}")] | ||
| GdprConsent { message: String }, | ||
|
|
||
|
|
@@ -87,7 +95,10 @@ pub trait IntoHttpResponse { | |
| /// Convert the error into an HTTP status code. | ||
| fn status_code(&self) -> StatusCode; | ||
|
|
||
| /// Get the error message to show to users (uses the Display implementation). | ||
| /// Get a safe, user-facing error message. | ||
| /// | ||
| /// Client errors (4xx) return a brief description; server/integration errors | ||
| /// return a generic message. Full error details are preserved in server logs. | ||
| fn user_message(&self) -> String; | ||
| } | ||
|
|
||
|
|
@@ -101,7 +112,7 @@ impl IntoHttpResponse for TrustedServerError { | |
| Self::GdprConsent { .. } => StatusCode::BAD_REQUEST, | ||
| Self::InsecureDefault { .. } => StatusCode::INTERNAL_SERVER_ERROR, | ||
| Self::InvalidHeaderValue { .. } => StatusCode::BAD_REQUEST, | ||
| Self::InvalidUtf8 { .. } => StatusCode::BAD_REQUEST, | ||
| Self::InvalidUtf8 { .. } => StatusCode::INTERNAL_SERVER_ERROR, | ||
| Self::KvStore { .. } => StatusCode::SERVICE_UNAVAILABLE, | ||
| Self::Prebid { .. } => StatusCode::BAD_GATEWAY, | ||
| Self::Integration { .. } => StatusCode::BAD_GATEWAY, | ||
|
|
@@ -112,7 +123,89 @@ impl IntoHttpResponse for TrustedServerError { | |
| } | ||
|
|
||
| fn user_message(&self) -> String { | ||
| // Use the Display implementation which already has the specific error message | ||
| self.to_string() | ||
| match self { | ||
| // Client errors (4xx) — safe to surface a brief description | ||
| Self::BadRequest { message } => format!("Bad request: {message}"), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ♻️ refactor —
Suggestion: Reuse Self::BadRequest { .. } => self.to_string(), |
||
| // Consent strings may contain user data; return category only. | ||
| Self::GdprConsent { .. } => "GDPR consent error".to_string(), | ||
| Self::InvalidHeaderValue { .. } => "Invalid header value".to_string(), | ||
| // Server/integration errors (5xx/502/503) — generic message only. | ||
| // Full details are already logged via log::error! in to_error_response. | ||
| _ => "An internal error occurred".to_string(), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 thinking — Wildcard match may silently suppress future client errors The Consider matching exhaustively (no Alternatively, the existing test |
||
| } | ||
ChristianPavilonis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| #[test] | ||
ChristianPavilonis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| fn server_errors_return_generic_message() { | ||
| let cases = [ | ||
| TrustedServerError::Configuration { | ||
| message: "secret db path".into(), | ||
| }, | ||
| TrustedServerError::KvStore { | ||
| store_name: "users".into(), | ||
| message: "timeout".into(), | ||
| }, | ||
| TrustedServerError::Proxy { | ||
| message: "upstream 10.0.0.1 refused".into(), | ||
| }, | ||
| TrustedServerError::SyntheticId { | ||
| message: "seed file missing".into(), | ||
| }, | ||
| TrustedServerError::Template { | ||
| message: "render failed".into(), | ||
| }, | ||
| TrustedServerError::Auction { | ||
| message: "bid timeout".into(), | ||
| }, | ||
| TrustedServerError::Gam { | ||
| message: "api key invalid".into(), | ||
| }, | ||
| TrustedServerError::Prebid { | ||
| message: "adapter error".into(), | ||
| }, | ||
| TrustedServerError::Integration { | ||
| integration: "foo".into(), | ||
| message: "connection refused".into(), | ||
| }, | ||
| TrustedServerError::Settings { | ||
| message: "parse failed".into(), | ||
| }, | ||
| TrustedServerError::InsecureDefault { | ||
| field: "api_key".into(), | ||
| }, | ||
| TrustedServerError::InvalidUtf8 { | ||
| message: "byte 0xff".into(), | ||
| }, | ||
| ]; | ||
| for error in &cases { | ||
| assert_eq!( | ||
| error.user_message(), | ||
| "An internal error occurred", | ||
| "should not leak details for {error:?}", | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn client_errors_return_safe_descriptions() { | ||
| let error = TrustedServerError::BadRequest { | ||
| message: "missing field".into(), | ||
| }; | ||
| assert_eq!(error.user_message(), "Bad request: missing field"); | ||
|
|
||
| let error = TrustedServerError::GdprConsent { | ||
| message: "no consent string".into(), | ||
| }; | ||
| assert_eq!(error.user_message(), "GDPR consent error"); | ||
|
|
||
| let error = TrustedServerError::InvalidHeaderValue { | ||
| message: "non-ascii".into(), | ||
| }; | ||
| assert_eq!(error.user_message(), "Invalid header value"); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 note —
InvalidUtf8status code change from 400 → 500 is correctThe only usage is for the embedded
trusted-server.tomlfile (settings_data.rs:24), which is a server-side resource. Invalid UTF-8 there is a server error, not a client error.