Skip to content
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 97 additions & 4 deletions crates/trusted-server-core/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -30,6 +34,10 @@ pub enum TrustedServerError {
#[display("GAM error: {message}")]
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 praise — Thoughtful handling of GdprConsent

Suppressing detail for consent errors (consent strings may contain user data) while still returning the category name is a well-reasoned middle ground. Good doc comment explaining the rationale.

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 },

Expand Down Expand Up @@ -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;
}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 praise — Correct reclassification of InvalidUtf8 to 500

The only usage is in settings_data.rs for the embedded TOML config — a server-side data integrity issue, not client input. Changing from BAD_REQUEST to INTERNAL_SERVER_ERROR is the right call.

Expand All @@ -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,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 noteInvalidUtf8 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.

Self::KvStore { .. } => StatusCode::SERVICE_UNAVAILABLE,
Self::Prebid { .. } => StatusCode::BAD_GATEWAY,
Self::Integration { .. } => StatusCode::BAD_GATEWAY,
Expand All @@ -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 {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 thinkingBadRequest passes caller-provided strings directly to clients

The BadRequest variant echoes its message field verbatim to clients. While the doc comment warns callers to keep it free of internal details, there's no compile-time or runtime enforcement. Current call sites are safe, but as more are added a caller could accidentally pass sensitive details. A future refinement (e.g., a newtype ClientMessage(String) or a fixed enum of safe messages) could prevent this.

// Client errors (4xx) — safe to surface a brief description
Self::BadRequest { message } => format!("Bad request: {message}"),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ refactorBadRequest 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(),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🏕 camp siteInvalidHeaderValue message could include the header name

Since this is a 400 (client error), including which header was invalid would help debugging. Current usage in cookies.rs has "Cookie header contains invalid UTF-8" in the message field, but that detail is now suppressed by the match. Safe and correct as-is — just a campsite opportunity.

// 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(),
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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.

}
}
}

#[cfg(test)]
mod tests {
use super::*;

#[test]
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");
}
}
Loading