Skip to content

Commit 948a510

Browse files
committed
fix(node): harden opaque error logging and tests (#247 review)
Log Internal with {e:#} so context-wrapped anyhow chains keep the leaf cause. Pin opaque bodies to the exact JSON object in unit and route tests, and guard db_unavailable PoolTimedOut as 503.
1 parent 0548ac5 commit 948a510

2 files changed

Lines changed: 47 additions & 34 deletions

File tree

‎crates/gitlawb-node/src/api/events.rs‎

Lines changed: 10 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1440,8 +1440,9 @@ mod ref_updates_feed_tests {
14401440

14411441
// A DB error in the gate fails closed as 500, not swallowed into an empty 200 (the
14421442
// regression the old get_repo().ok().flatten() allowed). Inject by dropping a
1443-
// column get_repo selects so its query errors. Also assert the body is opaque
1444-
// (#226): schema text must not reach an anonymous caller.
1443+
// column get_repo selects so its query errors. Also pin the anonymous body to
1444+
// the exact opaque object (#226): substring checks miss truncated or differently
1445+
// shaped leaks, and a vacuous "db_error || internal_error" assert cannot fail.
14451446
#[sqlx::test]
14461447
async fn repo_events_db_error_fails_closed_500(pool: PgPool) {
14471448
let state = test_state(pool.clone()).await;
@@ -1468,17 +1469,13 @@ mod ref_updates_feed_tests {
14681469
.await
14691470
.expect("read body");
14701471
let body = String::from_utf8_lossy(&bytes);
1471-
assert!(
1472-
!body.contains("is_public"),
1473-
"DB schema detail leaked to anonymous caller: {body}"
1474-
);
1475-
assert!(
1476-
!body.contains("column"),
1477-
"DB schema detail leaked to anonymous caller: {body}"
1478-
);
1479-
assert!(
1480-
body.contains("db_error") || body.contains("internal_error"),
1481-
"expected opaque error code, got: {body}"
1472+
let v: serde_json::Value = serde_json::from_str(&body).expect("json body");
1473+
assert_eq!(
1474+
v,
1475+
serde_json::json!({
1476+
"error": "db_error",
1477+
"message": crate::error::DB_ERROR_MESSAGE,
1478+
})
14821479
);
14831480
}
14841481

‎crates/gitlawb-node/src/error.rs‎

Lines changed: 37 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -168,8 +168,10 @@ impl IntoResponse for AppError {
168168
}
169169
// Opaque body: handlers that map with `.map_err(AppError::Internal)`
170170
// (e.g. GET /ipfs/{cid}) land here; other DB failures usually hit `Db`.
171+
// Log `{e:#}` so context-wrapped anyhow chains keep the leaf cause
172+
// (Display alone is only the outermost layer; see api/repos.rs).
171173
AppError::Internal(e) => {
172-
tracing::error!(error = %e, "internal error");
174+
tracing::error!(error = %format!("{e:#}"), "internal error");
173175
(
174176
StatusCode::INTERNAL_SERVER_ERROR,
175177
"internal_error",
@@ -192,7 +194,7 @@ pub type Result<T> = std::result::Result<T, AppError>;
192194
#[cfg(test)]
193195
mod tests {
194196
use super::*;
195-
use serde_json::Value;
197+
use serde_json::{json, Value};
196198

197199
#[test]
198200
fn timeout_maps_to_504_distinct_from_git_500() {
@@ -218,16 +220,14 @@ mod tests {
218220
.await
219221
.expect("read body");
220222
let v: Value = serde_json::from_slice(&bytes).expect("json body");
221-
assert_eq!(v["error"], "internal_error");
222-
assert_eq!(v["message"], INTERNAL_ERROR_MESSAGE);
223-
let rendered = String::from_utf8_lossy(&bytes);
224-
assert!(
225-
!rendered.contains("relation"),
226-
"DB schema detail leaked into body: {rendered}"
227-
);
228-
assert!(
229-
!rendered.contains(leak),
230-
"raw internal error leaked into body: {rendered}"
223+
// Exact object: a new `detail` field with different sensitive text must
224+
// also fail, not only a repeat of the original error string.
225+
assert_eq!(
226+
v,
227+
json!({
228+
"error": "internal_error",
229+
"message": INTERNAL_ERROR_MESSAGE,
230+
})
231231
);
232232
}
233233

@@ -244,16 +244,32 @@ mod tests {
244244
.await
245245
.expect("read body");
246246
let v: Value = serde_json::from_slice(&bytes).expect("json body");
247-
assert_eq!(v["error"], "db_error");
248-
assert_eq!(v["message"], DB_ERROR_MESSAGE);
249-
let rendered = String::from_utf8_lossy(&bytes);
250-
assert!(
251-
!rendered.contains("is_public"),
252-
"DB schema detail leaked into body: {rendered}"
247+
assert_eq!(
248+
v,
249+
json!({
250+
"error": "db_error",
251+
"message": DB_ERROR_MESSAGE,
252+
})
253253
);
254-
assert!(
255-
!rendered.contains("column"),
256-
"DB schema detail leaked into body: {rendered}"
254+
}
255+
256+
/// Connection-level failures must stay 503 `db_unavailable`, not collapse
257+
/// into the opaque 500 `db_error` arm if `db_unavailable` loses a variant.
258+
#[tokio::test]
259+
async fn db_pool_timeout_stays_503_unavailable() {
260+
let resp = AppError::Db(sqlx::Error::PoolTimedOut).into_response();
261+
assert_eq!(resp.status(), StatusCode::SERVICE_UNAVAILABLE);
262+
263+
let bytes = axum::body::to_bytes(resp.into_body(), usize::MAX)
264+
.await
265+
.expect("read body");
266+
let v: Value = serde_json::from_slice(&bytes).expect("json body");
267+
assert_eq!(
268+
v,
269+
json!({
270+
"error": DB_UNAVAILABLE_CODE,
271+
"message": DB_UNAVAILABLE_MESSAGE,
272+
})
257273
);
258274
}
259275
}

0 commit comments

Comments
 (0)