Merge pull request #247 from Ayush7614/fix/226-opaque-internal-errors

fix(node): opaque AppError::Internal and AppError::Db HTTP bodies (#226)

Author: kevincodex1

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

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

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,
+            })
+        );
+    }
 }