fix(node): opaque GraphQL DB error messages (#250) - #255
Conversation
Route query and mutation DB failures through a shared helper that logs the cause chain server-side and returns a generic client message, so unauthenticated POST /graphql cannot leak sqlx text.
|
Warning Review limit reached
Next review available in: 46 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughGraphQL error mapping is centralized to redact SQLx/database details while preserving business errors. Mutation and query resolvers adopt the helpers, and task queries clamp limits to ChangesGraphQL error handling and query validation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GraphQLResolver
participant ErrorMapper
participant Database
Client->>GraphQLResolver: Execute mutation or query
GraphQLResolver->>Database: Perform database operation
Database-->>GraphQLResolver: Return success or error
GraphQLResolver->>ErrorMapper: Convert failure
ErrorMapper-->>Client: Opaque DB or preserved business error
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
@beardthelion — this PR addresses #250 that you filed. GraphQL never went through |
beardthelion
added
crate:node
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
The approach is right and the suppression is genuinely complete inside the module. I checked the part that usually goes wrong: Error::new sets source: None, and both source fields carry #[serde(skip)], so nothing can reach the wire even by accident, and {e:#} does render the full anyhow chain into the log rather than trading a client leak for an operator blindspot. I also checked the sink a grep for the old map_err shape cannot see, since async-graphql auto-converts any Display into an error message on a bare ?. No resolver uses one today, so the sweep holds.
Two things block it, and one of them is not style.
Findings
-
[P1] Drop the redundant closures, CI cannot pass as written
crates/gitlawb-node/src/graphql/query.rs:17
All twelve sites are written.map_err(|e| crate::graphql::graphql_db_err(e)), which tripsclippy::redundant_closure, and.github/workflows/pr-checks.yml:49runscargo clippy --workspace --all-targets -- -D warnings. I ran clippy on this head and got twelve of them..map_err(crate::graphql::graphql_db_err)is enough: I applied it to the six in query.rs and the count went to six, withcargo checkclean. -
[P1] Only send real DB faults through the opaque helper
crates/gitlawb-node/src/graphql/mutation.rs:79
claim_taskreturnsanyhow!("task not claimable: not found or already claimed")(db/mod.rs:2619) andfinish_taskreturnsanyhow!("task not found or not in claimed state")(db/mod.rs:2641). Neither ever contained sqlx text, so neither was leaking. With this change a signed agent that loses a claim race is tolda database error occurred, which is wrong and gives it nothing to act on, and losing a claim race is the ordinary path in a task queue rather than an edge case. Downcasting tosqlx::Errorat the call site, or returning a typed variant from the db layer, keeps those two messages while still closing the leak. -
[P2] Cover the other eleven sites, mutations first
crates/gitlawb-node/src/graphql/mutation.rs:57
The#[sqlx::test]makes exactly one line load-bearing.list_all_repos_dedupedselectsd.is_public(db/mod.rs:1215), so dropping that column fails the first call and?returns beforequery.rs:30ever runs; I confirmed by reverting each line separately, and onlyquery.rs:17goes red. The unit test in mod.rs calls the helper directly, so it stays green even with every call site reverted.mutation.rs:175already has a test module usingtest_support::test_stateandRequest::new(q).data(AuthenticatedDid(...)), so the mutation-side test has both a home and a pattern to copy. -
[P2] Clamp the tasks limit before it reaches the query
crates/gitlawb-node/src/graphql/query.rs:112
limit: i64goes unclamped intoLIMIT $3(db/mod.rs:2572), and Postgres rejects a negative limit outright (2201W: LIMIT must not be negative). So an anonymous{ tasks(limit: -1) { id } }is a deterministic failure that now writes an error-level log line on every request, where before it only returned one. The REST feed already clamps atapi/events.rs:129-134. Worth consideringwarn!rather thanerror!for client-caused failures generally. -
[P3] Share one constant with #247 instead of adding a second
crates/gitlawb-node/src/graphql/mod.rs:20
#247 definesDB_ERROR_MESSAGE: &str = "a database error occurred"aterror.rs:94, the same string, and this adds a second constant with its own test and a different log event name for the same concept. Whichever lands second should fold into the first. The#226reference in the comment above also does not hold on this base:error.rs:150still returnse.to_string()forAppError::Db, so a reader would conclude the leak class closes when this merges, and it does not.
Not asks, just so they are on the record. api/tasks.rs:138-143 still puts e.to_string() into the REST body on an anonymous route, which is the same class as #250 but untouched here, so it wants its own issue rather than scope creep into this one. And async-graphql has a custom-error-conversion feature that removes the blanket Display conversion, which would make a future bare ? in a resolver a compile error instead of a silent re-leak. That is the durable version of this fix if it appeals later, not something to do now.
One thing that is not yours: PR Checks has never run on this head, so the green you see is a triage check rather than the suite. I have approved the run, which means it will now go red on the clippy item above rather than staying silent about it.
Use function-pointer map_err to clear redundant_closure. Opaque only when a sqlx::Error is in the anyhow chain so claim/finish business messages survive. Clamp GraphQL tasks(limit); add mutation coverage.
|
@beardthelion — addressed your review (and the clippy red):
Ready for another look. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/gitlawb-node/src/graphql/mod.rs (1)
71-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winGood coverage for
graphql_db_err; consider adding tests forgraphql_app_errtoo.The two tests here pin down
graphql_db_err's sqlx-vs-business split well, butgraphql_app_err(Lines 47-58) has no direct unit test verifying its per-variant behavior — which would have caught the message-collapsing issue flagged above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/graphql/mod.rs` around lines 71 - 91, The test module covers graphql_db_err but lacks direct coverage for graphql_app_err. Add unit tests targeting graphql_app_err that exercise each error variant and assert the expected variant-specific messages, including preservation of the original business message and the configured generic message where applicable.crates/gitlawb-node/src/graphql/query.rs (1)
108-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the clamp in the GraphQL schema.
limitis silently clamped to0..=200, but unlikeref_updates'limitparam (Line 52-55), there's no#[graphql(desc = ...)]telling API consumers that requests above 200 are truncated without error. Worth adding for schema-introspection consistency.🔧 Proposed fix
- #[graphql(default = 50)] limit: i64, + #[graphql(default = 50, desc = "Max 200; larger values are clamped to 200.")] limit: i64,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/gitlawb-node/src/graphql/query.rs` around lines 108 - 125, The GraphQL schema for the tasks query does not document the `limit` clamp. Update the `limit` argument in `tasks` with a `#[graphql(desc = ...)]` description stating that values are clamped to 0–200 and values above 200 are truncated without error, matching the documentation style used by `ref_updates`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/gitlawb-node/src/graphql/mod.rs`:
- Around line 47-58: Update graphql_app_err to preserve and return the safe
messages for non-Db, non-Internal AppError variants, while keeping Db and
Internal responses opaque. Adjust logging so expected client/business errors use
warn-level logging rather than error-level logging, and retain error-level
logging for genuinely opaque failures. Use the existing AppError variants and
graphql_db_err behavior as the reference.
---
Nitpick comments:
In `@crates/gitlawb-node/src/graphql/mod.rs`:
- Around line 71-91: The test module covers graphql_db_err but lacks direct
coverage for graphql_app_err. Add unit tests targeting graphql_app_err that
exercise each error variant and assert the expected variant-specific messages,
including preservation of the original business message and the configured
generic message where applicable.
In `@crates/gitlawb-node/src/graphql/query.rs`:
- Around line 108-125: The GraphQL schema for the tasks query does not document
the `limit` clamp. Update the `limit` argument in `tasks` with a `#[graphql(desc
= ...)]` description stating that values are clamped to 0–200 and values above
200 are truncated without error, matching the documentation style used by
`ref_updates`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: bad9390a-53e0-4639-a1ee-27ac3c675684
📒 Files selected for processing (3)
crates/gitlawb-node/src/graphql/mod.rscrates/gitlawb-node/src/graphql/mutation.rscrates/gitlawb-node/src/graphql/query.rs
Keep Db/Internal opaque; pass through NotFound/BadRequest/etc. and document the tasks limit clamp in the schema.
|
@coderabbitai thanks — addressed in 88a436a:
@beardthelion FYI — this was the remaining CodeRabbit major on #255. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 ✅ If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
2 participants
Summary
graphql_db_errhelper that logs{e:#}server-side and returns a generic"a database error occurred"client message.query.rsand 6mutation.rsDB.map_errsites through it.POST /graphqlsqlx/Postgres leak described in Unauthenticated POST /graphql leaks raw sqlx/Postgres error text in query resolver errors (sibling of #226/#106) #250 (bypassesAppError, so GET /ipfs/{cid} leaks raw sqlx/DB error text in the 500 body to unauthenticated callers (sibling of #106) #226/fix(node): opaque AppError::Internal and AppError::Db HTTP bodies (#226) #247 do not cover it).Test plan
cargo test -p gitlawb-node graphql_db_err_is_opaque— helper rejects raw schema textrepos_query_db_error_message_is_opaque— anonymous{ repos { ... } }after a schema break returns only the opaque message (CI /DATABASE_URL)Fixes #250
Summary by CodeRabbit