diff --git a/crates/gitlawb-node/src/api/events.rs b/crates/gitlawb-node/src/api/events.rs index 875e6261..30a69e5d 100644 --- a/crates/gitlawb-node/src/api/events.rs +++ b/crates/gitlawb-node/src/api/events.rs @@ -1440,7 +1440,9 @@ mod ref_updates_feed_tests { // A DB error in the gate fails closed as 500, not swallowed into an empty 200 (the // regression the old get_repo().ok().flatten() allowed). Inject by dropping a - // column get_repo selects so its query errors. + // column get_repo selects so its query errors. Also pin the anonymous body to + // the exact opaque object (#226): substring checks miss truncated or differently + // shaped leaks, and a vacuous "db_error || internal_error" assert cannot fail. #[sqlx::test] async fn repo_events_db_error_fails_closed_500(pool: PgPool) { let state = test_state(pool.clone()).await; @@ -1463,6 +1465,18 @@ mod ref_updates_feed_tests { StatusCode::INTERNAL_SERVER_ERROR, "a DB error must fail closed (500), never serve an empty 200" ); + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .expect("read body"); + let body = String::from_utf8_lossy(&bytes); + let v: serde_json::Value = serde_json::from_str(&body).expect("json body"); + assert_eq!( + v, + serde_json::json!({ + "error": "db_error", + "message": crate::error::DB_ERROR_MESSAGE, + }) + ); } // Symmetric to the gate DB-error test: a DB error in the CERT fetch (after the gate diff --git a/crates/gitlawb-node/src/error.rs b/crates/gitlawb-node/src/error.rs index bdd5aec9..e7fcf7e8 100644 --- a/crates/gitlawb-node/src/error.rs +++ b/crates/gitlawb-node/src/error.rs @@ -85,6 +85,14 @@ impl From for AppError { } } +/// Generic client-facing message for `AppError::Internal`. The real error is +/// logged server-side; never put sqlx/anyhow detail in the HTTP body (#226). +pub const INTERNAL_ERROR_MESSAGE: &str = "an internal error occurred"; + +/// Generic client-facing message for non-unavailable `AppError::Db`. Query / +/// schema errors stay in logs; the HTTP body must not leak them (#226). +pub const DB_ERROR_MESSAGE: &str = "a database error occurred"; + impl IntoResponse for AppError { fn into_response(self) -> Response { // iCaptcha challenges carry structured discovery so clients don't have to @@ -147,12 +155,29 @@ impl IntoResponse for AppError { DB_UNAVAILABLE_CODE, DB_UNAVAILABLE_MESSAGE.into(), ), - AppError::Db(e) => (StatusCode::INTERNAL_SERVER_ERROR, "db_error", e.to_string()), - AppError::Internal(e) => ( - StatusCode::INTERNAL_SERVER_ERROR, - "internal_error", - e.to_string(), - ), + // Opaque body + server log: bare `?` on sqlx paths becomes `AppError::Db` + // via `From`, so this arm (not `Internal`) is the common leak for open + // routes like GET /api/v1/repos and GET /api/v1/peers (#226). + AppError::Db(e) => { + tracing::error!(error = %e, "database error"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + "db_error", + DB_ERROR_MESSAGE.into(), + ) + } + // Opaque body: handlers that map with `.map_err(AppError::Internal)` + // (e.g. GET /ipfs/{cid}) land here; other DB failures usually hit `Db`. + // Log `{e:#}` so context-wrapped anyhow chains keep the leaf cause + // (Display alone is only the outermost layer; see api/repos.rs). + AppError::Internal(e) => { + tracing::error!(error = %format!("{e:#}"), "internal error"); + ( + StatusCode::INTERNAL_SERVER_ERROR, + "internal_error", + INTERNAL_ERROR_MESSAGE.into(), + ) + } }; let body = Json(json!({ @@ -169,6 +194,7 @@ pub type Result = std::result::Result; #[cfg(test)] mod tests { use super::*; + use serde_json::{json, Value}; #[test] fn timeout_maps_to_504_distinct_from_git_500() { @@ -182,4 +208,68 @@ mod tests { StatusCode::INTERNAL_SERVER_ERROR ); } + + /// #226: raw sqlx/DB detail must never appear in the Internal 500 body. + #[tokio::test] + async fn internal_error_body_is_opaque() { + let leak = "error returned from database: relation \"repos\" does not exist"; + let resp = AppError::Internal(anyhow::anyhow!("{leak}")).into_response(); + assert_eq!(resp.status(), StatusCode::INTERNAL_SERVER_ERROR); + + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .expect("read body"); + let v: Value = serde_json::from_slice(&bytes).expect("json body"); + // Exact object: a new `detail` field with different sensitive text must + // also fail, not only a repeat of the original error string. + assert_eq!( + v, + json!({ + "error": "internal_error", + "message": INTERNAL_ERROR_MESSAGE, + }) + ); + } + + /// #226: `AppError::Db` query errors (the common `?` path) must also be opaque. + #[tokio::test] + async fn db_error_body_is_opaque() { + let resp = AppError::Db(sqlx::Error::Protocol( + "error returned from database: column \"is_public\" does not exist".into(), + )) + .into_response(); + assert_eq!(resp.status(), StatusCode::INTERNAL_SERVER_ERROR); + + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .expect("read body"); + let v: Value = serde_json::from_slice(&bytes).expect("json body"); + assert_eq!( + v, + json!({ + "error": "db_error", + "message": DB_ERROR_MESSAGE, + }) + ); + } + + /// Connection-level failures must stay 503 `db_unavailable`, not collapse + /// into the opaque 500 `db_error` arm if `db_unavailable` loses a variant. + #[tokio::test] + async fn db_pool_timeout_stays_503_unavailable() { + let resp = AppError::Db(sqlx::Error::PoolTimedOut).into_response(); + assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE); + + let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .expect("read body"); + let v: Value = serde_json::from_slice(&bytes).expect("json body"); + assert_eq!( + v, + json!({ + "error": DB_UNAVAILABLE_CODE, + "message": DB_UNAVAILABLE_MESSAGE, + }) + ); + } }