diff --git a/crates/gitlawb-node/src/api/events.rs b/crates/gitlawb-node/src/api/events.rs index 875e6261..1158f47e 100644 --- a/crates/gitlawb-node/src/api/events.rs +++ b/crates/gitlawb-node/src/api/events.rs @@ -1440,7 +1440,8 @@ 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). #[sqlx::test] async fn repo_events_db_error_fails_closed_500(pool: PgPool) { let state = test_state(pool.clone()).await; @@ -1463,6 +1464,18 @@ mod ref_updates_feed_tests { StatusCode::INTERNAL_SERVER_ERROR, "a DB error must fail closed (500), never serve an empty 200" ); + let body = axum::body::to_bytes(resp.into_body(), usize::MAX) + .await + .expect("read body"); + let body = String::from_utf8_lossy(&body); + 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/db/mod.rs b/crates/gitlawb-node/src/db/mod.rs index 6354f17a..99c5d8c6 100644 --- a/crates/gitlawb-node/src/db/mod.rs +++ b/crates/gitlawb-node/src/db/mod.rs @@ -6945,6 +6945,8 @@ mod peer_authority_tests { /// | `prune_non_public_peers` (db/mod.rs) | a delete keyed on a computed bad-DID array; cannot repoint; boot-only caller in main.rs | /// | `seed_local_peer` (sync.rs) | excluded by test-module location: a deliberate `upsert_peer` bypass for `file://` fixtures, which the public-URL gate rejects | /// | `a_legacy_row_can_still_refresh_its_liveness` (db/mod.rs) | test-only. Seeds a PRE-GATE row by raw SQL on purpose: `upsert_peer` cannot create one, since the gate it is testing refuses exactly that DID. The fixture models what a deployed table already holds | +/// | `gossip_ping_round_requires_two_failures_before_persisting_unreachable` (main.rs) | test-only. Seeds a peer row by raw SQL so the gossip ping round can probe readiness hysteresis without going through `upsert_peer` | +/// | `manual_ping_uses_readiness_without_mutating_federation_gate` (api/peers.rs) | test-only. Seeds a peer row by raw SQL so the manual ping route can assert readiness probing without mutating federation gate state | /// /// And the `upsert_peer` CALL-SITE authority table, which the ledger above /// structurally cannot hold, because the bootstrap site issues no SQL of its own @@ -7030,6 +7032,14 @@ mod peers_table_writer_guard { /// listed function that no longer has one. const LEDGER: &[(&str, usize)] = &[ ("a_legacy_row_can_still_refresh_its_liveness", 1), + ( + "gossip_ping_round_requires_two_failures_before_persisting_unreachable", + 1, + ), + ( + "manual_ping_uses_readiness_without_mutating_federation_gate", + 1, + ), ("mark_peer_ping", 1), ("prune_non_public_peers", 1), ("prune_self_peers", 1), diff --git a/crates/gitlawb-node/src/error.rs b/crates/gitlawb-node/src/error.rs index ee890c50..91c7891c 100644 --- a/crates/gitlawb-node/src/error.rs +++ b/crates/gitlawb-node/src/error.rs @@ -72,6 +72,14 @@ pub enum AppError { pub const DB_UNAVAILABLE_CODE: &str = "db_unavailable"; pub const DB_UNAVAILABLE_MESSAGE: &str = "database is temporarily unavailable"; +/// 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"; + /// Connection-level sqlx failures that mean the database is unreachable right /// now (retryable, 503), as opposed to server-reported query errors. fn db_unavailable(e: &sqlx::Error) -> bool { @@ -168,12 +176,29 @@ impl IntoResponse for AppError { AppError::Overloaded(msg) => { (StatusCode::SERVICE_UNAVAILABLE, "overloaded", msg.clone()) } - 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!({ @@ -223,4 +248,74 @@ mod tests { "1" ); } + + /// #226: raw sqlx/DB detail must never appear in the Internal 500 body. + #[tokio::test] + async fn internal_error_body_is_opaque() { + use serde_json::{json, Value}; + + 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() { + use serde_json::{json, Value}; + + 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() { + use serde_json::{json, Value}; + + 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, + }) + ); + } }