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

[Ayush7614]

Summary

• AppError::Internal and non-unavailable AppError::Db return generic client messages instead of raw anyhow/sqlx text.
• Real errors are logged server-side (Internal uses {e:#} so context-wrapped chains keep the leaf cause; Db logs the concrete sqlx::Error).
• Closes the unauthenticated DB-error leak path in GET /ipfs/{cid} leaks raw sqlx/DB error text in the 500 body to unauthenticated callers (sibling of #106) #226 (ipfs .map_err(AppError::Internal) and the common bare ? → AppError::Db path on open routes).
• Connection-level failures still map to 503 db_unavailable (not the opaque 500).

Test plan

• cargo test -p gitlawb-node internal_error_body_is_opaque — exact opaque JSON object
• cargo test -p gitlawb-node db_error_body_is_opaque — exact opaque JSON object
• cargo test -p gitlawb-node db_pool_timeout_stays_503_unavailable — PoolTimedOut stays 503
• repo_events_db_error_fails_closed_500 pins the anonymous body to {"error":"db_error","message":DB_ERROR_MESSAGE}
• timeout_maps_to_504 — no status-map regression

Fixes #226

Summary by CodeRabbit

• Bug Fixes
  • Improved error responses for internal and database failures.
  • Sensitive underlying error details are no longer exposed in client-facing responses.
  • Database connection availability failures continue to return a distinct service-unavailable response.
  • Enhanced validation ensures database errors consistently return the expected structured error payload.

[github-actions]

Thanks for the contribution. A couple of things will help us review this faster:

• This changes Rust source but no tests changed. Tests are required for fixes and strongly encouraged for features.

See CONTRIBUTING.md. Update the PR and these notes will clear automatically.

[coderabbitai]

📝 Walkthrough

Walkthrough

The change makes internal and non-unavailable database error responses opaque through shared constants, preserves 503 responses for unavailable databases, and strengthens tests to assert exact JSON payloads.

Changes

Error response opacity

Layer / File(s)	Summary
Opaque error mapping crates/gitlawb-node/src/error.rs	Adds shared client-facing messages and maps internal and database errors to opaque 500 responses while logging underlying details.
Error response validation crates/gitlawb-node/src/error.rs, crates/gitlawb-node/src/api/events.rs	Adds exact JSON assertions for internal, database, unavailable-database, and repository-event error responses.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

• Gitlawb/node\GET /ipfs/{cid} leaks raw sqlx/DB error text in the 500 body to unauthenticated callers (sibling of #106) #226`` — Addresses the same internal error response path that exposed raw database errors.

Possibly related PRs

• Gitlawb/node#170: Both update AppError HTTP mappings and database-unavailable response handling.
• Gitlawb/node#196: Both modify AppError response shapes, status handling, and related tests.

Suggested reviewers: gravirei, kevincodex1

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Description check	⚠️ Warning	The description covers summary and tests, but it omits required template sections like Kind of change, What changed, and reviewer verification.	Add the missing template sections: motivation/context with issue link, kind of change, concrete changes, reviewer verification steps, and relevant checklist/notes.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is concise and accurately describes the main behavior change to opaque HTTP bodies for AppError::Internal and AppError::Db.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[Ayush7614]

@beardthelion — this PR addresses the issue you filed in #226.

Opaque-ifies AppError::Internal client bodies (detail stays in logs). Can follow up on AppError::Git / #106 the same way if you want that in a separate PR.

[beardthelion]

The Internal fix is correct and the test behind it is stronger than it looks: I tried to break it three ways (restoring e.to_string(), keeping the constant message but re-adding the raw text as a separate detail field, and appending just a 12-character prefix of the error) and it went red on all three. The byte-level !rendered.contains(...) scan is doing real work.

The problem is what it does not cover. I do not think this should close #226.

Findings

• [P1] Give AppError::Db the same opaque body, or do not close #226
  crates/gitlawb-node/src/error.rs:154
From<anyhow::Error> downcasts sqlx errors out of the anyhow chain into AppError::Db (error.rs:81-84), so a handler that reaches the database with a bare ? never lands in the arm you fixed. db_unavailable() only diverts connection-level failures, so a server-reported query error takes the 500 db_error arm and renders e.to_string() verbatim. I ran the existing anonymous-caller test repo_events_db_error_fails_closed_500 with the body printed, and the unauthenticated response is {"error":"db_error","message":"error returned from database: column \"is_public\" does not exist"}. That is the same leak #226 describes, one line above your fix, on an open route. GET /api/v1/repos and GET /api/v1/peers reach the database the same way.

• [P2] The arm that leaks is also the arm that logs nothing
  crates/gitlawb-node/src/error.rs:154
Internal now logs server-side and returns nothing useful to the caller, which is right. Db does the opposite: no tracing::error!, full detail to the client. Whatever you do about the body, the Db arm should log the error too, otherwise redacting it later leaves operators with no record at all.

• [P2] Assert the body in the anonymous DB-error test, not just the status
  crates/gitlawb-node/src/api/events.rs:1444
repo_events_db_error_fails_closed_500 already drives a real DB error through a mounted route as an anonymous caller, which is exactly the shape needed here, but it only asserts resp.status(). That is why this class stayed invisible. Reading the body and asserting it carries no schema text turns it into the guard that would have caught the finding above. I confirmed it reproduces with a two-line change to that test.

• [P3] Narrow the comment claim
  crates/gitlawb-node/src/error.rs:155-156
  "GET /ipfs/{cid} and siblings map DB failures here" is true of the ipfs handlers because they write .map_err(AppError::Internal) (ipfs.rs:84, :94, :224), not because DB failures generally route to Internal. As written it reads as a general guarantee, and the Db arm above is the counterexample.

Not your debt, flagged only so the scope of #226 is clear: crates/gitlawb-node/src/api/tasks.rs renders Json(json!({"error": e.to_string()})) at eight sites and bypasses AppError entirely, and GET /api/v1/tasks plus GET /api/v1/tasks/{id} are open routes. That is a separate cleanup and I am not asking you to take it on here.

Everything else checked out. No in-repo client branches on the 500 message text, so this is not a breaking change for gl or the remote helper; no existing test asserts the old raw message; the error code and body shape are unchanged.

[Ayush7614]

@beardthelion — addressed your review findings:

1. [P1] opaque AppError::Db — non-unavailable Db arm now returns DB_ERROR_MESSAGE ("a database error occurred") instead of e.to_string(), so bare ? sqlx paths no longer leak schema text. This should properly close GET /ipfs/{cid} leaks raw sqlx/DB error text in the 500 body to unauthenticated callers (sibling of #106) #226.
2. [P2] log the Db arm — tracing::error!(error = %e, "database error") before the opaque response.
3. [P2] assert body in repo_events_db_error_fails_closed_500 — anonymous caller body must not contain is_public / column.
4. [P3] narrowed comment — Internal arm comment no longer claims all DB failures land there; notes that bare ? usually hits Db, while .map_err(AppError::Internal) (ipfs) hits Internal.

Left the tasks.rs raw Json(json!({"error": e.to_string()})) sites alone as you noted — separate cleanup.

Local: cargo test -p gitlawb-node error_body_is_opaque — both Internal + Db unit tests passed. The sqlx route test needs DATABASE_URL (will run in CI). Ready for another look.

[jatmn]

LGTM

[beardthelion]

All four findings from the last round landed, and the two fixed arms actually bind: restoring e.to_string() in either one turns its unit test red, and the anonymous route test goes red along with the Db arm. The db_unavailable arm still wins the match ordering, so connection failures keep their own 503 and message instead of collapsing into the new opaque 500. I also approved the workflow run, so the suite has now executed on this head rather than sitting queued: 9 jobs green, both test matrices and MSRV included.

Five items worth another pass, all on lines this PR adds. Only the first really needs settling before merge; the rest are test hardening and description.

Findings

• [P2] Log the cause chain with {e:#}, not %e

crates/gitlawb-node/src/error.rs:172

The premise of this change is that detail moves out of the body and into the logs, but error = %e on an anyhow error prints only the outermost context, so for a context-wrapped error that detail now lands nowhere. I ran the pair: Display gives loading repo record, while {:#} gives loading repo record: encountered unexpected or invalid data: error returned from database: column "is_public" does not exist. api/repos.rs:217 already uses format!("{e:#}") with a comment explaining exactly this. The Db arm needs no change, since its e is a concrete sqlx::Error with no chain to walk.

• [P2] Bind the route assertion to the opaque message, not two substrings

crates/gitlawb-node/src/api/events.rs:1471

The two !body.contains(...) checks are keyed to the particular error this test injects, so a differently shaped leak slips past. I mutated the arm to emit only the first 20 characters of the error: db_error_body_is_opaque caught it, this test stayed green. Parsing the body and asserting on the constant closes the gap, and I confirmed this version passes on the current head and fails against that same mutant:

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

Asserting the whole object rather than individual fields also closes a gap the two unit tests share. Their byte scans do catch a new field that repeats the same error text, which I checked, but a new field carrying different sensitive text sails through all three: adding "detail": "constraint repos_owner_did_name_key on table repos" to the shared builder at error.rs:181 left every test green. Pinning the exact object is what makes that impossible rather than unlikely.

• [P3] Replace the assert that cannot fail

crates/gitlawb-node/src/api/events.rs:1479

assert!(body.contains("db_error") || body.contains("internal_error")) is green no matter what the message says, because error.rs:182 always emits "error": code. The three lines above replace it.

• [P3] Pin the 503 arm against a change to db_unavailable

crates/gitlawb-node/src/error.rs:153

Adding a second AppError::Db arm makes the guarded one load-bearing on its predicate. Reordering the arms is safe, since that produces unreachable pattern and the clippy gate fails on it, which I confirmed. Editing the predicate is not: I dropped sqlx::Error::PoolTimedOut from db_unavailable, and clippy stayed clean and every test passed, while a pool timeout would quietly downgrade from a retryable 503 to the new opaque 500. A unit test asserting AppError::Db(sqlx::Error::PoolTimedOut) still yields 503 and DB_UNAVAILABLE_CODE closes it, no database needed.

• [P3] Retitle and update the description to cover the Db arm

The title and Summary still describe only AppError::Internal, and the Test plan omits db_error_body_is_opaque. The Db arm is the half that closes the leak on the common bare ? path, so the record of what actually fixed #226 should say so.

Ignore the needs-tests label and the triage note about missing tests. That check matches only a bare #[test] or #[cfg(test)], so it cannot see the two #[tokio::test] functions you added, and mod tests already existed here so no new #[cfg(test)] line appears in the diff. I ran the detector against this patch to confirm. It is a known false positive with a fix already open in #202, and the label will not clear on its own. Nothing for you to do about it.

On scope: this satisfies what #226 asks for, so closing it here is right. The wider leak class is not closed though, and I would rather that be stated than implied by the merge. AppError::Git still renders caller-supplied text verbatim, and a probe through it returns a repo filesystem path in the body. POST /graphql is reachable unsigned via optional_signature, and graphql/query.rs renders e.to_string() at six sites. Neither is yours to take on here; I will file the GraphQL one separately.

[Ayush7614]

@beardthelion — addressed your review:

1. [P2] {e:#} for Internal — tracing::error!(error = %format!("{e:#}"), ...) so context-wrapped anyhow chains keep the leaf cause in logs.
2. [P2] exact opaque object — unit tests and repo_events_db_error_fails_closed_500 now assert_eq! the full JSON object (catches a new detail field, truncated leaks, etc.).
3. [P3] removed vacuous assert — replaced by the exact-object assert above.
4. [P3] db_unavailable pin — added db_pool_timeout_stays_503_unavailable for PoolTimedOut → 503 + DB_UNAVAILABLE_*.
5. [P3] title/description — updated to cover the Db arm and the new tests.

Noted on scope: AppError::Git path leak and GraphQL e.to_string() sites stay out of this PR as you said.

Local: opaque Internal/Db + PoolTimedOut unit tests passed. Ready for another look.

[beardthelion]

Reasoning behind the approval above, which went up without its body by mistake.

All nine asks from the two earlier rounds landed, and I checked each against the code rather than the summary. The four guards this PR adds are load-bearing. I reverted each protected line in a scratch worktree and every test went red on its own target and nothing else: DB_ERROR_MESSAGE back to e.to_string() reddens db_error_body_is_opaque, the same on the Internal arm reddens internal_error_body_is_opaque, dropping PoolTimedOut from db_unavailable reddens db_pool_timeout_stays_503_unavailable, and adding a third field to the shared body builder reddens all three exact-object asserts, which is the leak shape a substring scan misses.

The route test carries its weight too. repo_events_db_error_fails_closed_500 goes red against a real Postgres with the exact old body, {"error":"db_error","message":"error returned from database: column \"is_public\" does not exist"}, so the anonymous-caller leak #226 describes is now pinned by a test that executes rather than by inspection.

Local run on 948a510: 513 tests pass, fmt clean, clippy clean under -D warnings. Nothing in the repo is coupled to the message text either: db_error and internal_error appear nowhere outside error.rs and this test, and every gl and MCP reader of message prints it rather than branching on it.

That is what the approval rests on. I am holding the merge handoff until jatmn has looked at this head: their approval is on 0548ac5, which predates the round-2 commit, so it does not cover what is here now.

Two things worth recording, neither of which needs another round.

The Summary line "Connection-level failures still map to 503 db_unavailable" is broader than the code. crates/gitlawb-node/src/api/ipfs.rs:84, :94, :224 and crates/gitlawb-node/src/api/arweave.rs:33 use .map_err(AppError::Internal), which skips the From<anyhow::Error> downcast, so a pool timeout on those routes lands on the opaque 500 instead of the 503. I ran both seams: the same sqlx::Error::PoolTimedOut gives 503 through a bare ? and 500 through map_err(AppError::Internal). Those four lines predate this PR and are not yours to fix in it, so I am recording the correction rather than asking for a change, and I will open the follow-up.

The {e:#} change has no test behind it. Reverting it to %e leaves all four error-mapping tests green, which is expected because the repo has no log-capture harness at all. The fix is right and I checked it by hand, but the detail now lives only in the log, so the log has earned the same kind of guard the body just got. That is a repo-level gap rather than something to hold this PR for.

One process note: PR Checks had not run on this head. It was sitting at action_required awaiting approval, which is why the check rollup read empty rather than green. I approved the run and it has since gone green, 9 jobs including both test matrices, MSRV and the release build.