feat(node): enforce per-route authorization across the REST and GraphQL surface#87
Merged
Conversation
Adds a #[cfg(test)] test-support module: a migrated AppState over a real #[sqlx::test] pool (test_state), a DB-free variant (test_state_lazy), the assembled router (app), and signed_request_as for injecting an authenticated DID without RFC-9421 signing. A test-callable run_migrations reuses the production migrate() path, and sqlx gains the macros/migrate features so #[sqlx::test] is available. CI's test job gets a postgres service behind a pg_isready health gate so the DB-backed tests can run. The proving test exercises the owner gate on PUT /visibility end to end: non-owner is rejected with 400, owner succeeds. signed_request_as sets a JSON content-type so the Json extractor does not 415 before the handler runs.
Closes the missing-authorization cluster on the signed REST surface, where 'authenticated' meant 'any participant' because identities are permissionless. Adds two shared helpers in api/mod.rs: require_repo_owner (403) and did_matches, a method-safe DID comparison that collapses the full did:key vs bare short form without matching across methods (did:web/did:gitlawb share the base58 space). Per-route gates: - merge_pr is owner-only (subsumes branch protection); close_pr and close_issue allow the repo owner or the PR/issue author (the issue author is read from the git-JSON blob, with a None author falling back to owner-only). - create_review, create_comment, create_issue_comment, and create_bounty are read-gated (participants need read access, not ownership; this also closes the private-repo bounty leak). - the task handlers bind the acting DID to the authenticated signer. - dispute_bounty, previously ungated, now requires the creator or claimant; the other bounty and replica identity checks switch from raw equality to did_matches. A source-level guard test asserts every in-scope mutation handler still carries its expected gate marker, so a removed gate or an unclassified new route fails CI. DB-backed tests (via the integration harness) cover merge owner-only and the task signer-binding; did_matches has unit coverage for the cross-method case.
GraphQL mutations were a fully unauthenticated parallel write path to the task system (N2): the /graphql route had no auth layer and the resolvers took the acting DID as plain arguments. Apply optional_signature to the /graphql POST so a verified DID is attached when a signature is present (queries stay open), and thread it into request-scoped data. Each mutation now reads the verified signer via require_signer and binds the acting DID to it (rejecting an unsigned request or one whose claimed delegator/assignee/by_did differs from the signer), reusing did_matches for the comparison. Read-only subscriptions (/graphql/ws) are left open. A resolver-level test covers the unsigned, mismatched, and matching cases.
get_tree gated on the repo root ("/") regardless of the requested subtree, so
a caller denied a withheld subtree could still enumerate its filenames and blob
SHAs (N3). Gate on the requested path instead, mirroring get_blob, and reject
traversal segments. A cross-caller test shows the rejection is path-scoped: a
non-reader is denied the withheld subtree but passes the gate on a non-withheld
path.
Two authorization gaps surfaced by an adversarial review of the auth stack, both necessary-but-insufficient siblings of fixes already in it. complete_task/fail_task bound the acting DID to the signer but never checked the caller against the task's assignee, and finish_task updated by id alone, so any authenticated did:key could finish or fail any task in any state. Load the task and require the caller to be its assignee (REST and GraphQL), and add an 'AND status=claimed' predicate so only a claimed task transitions. The now-unused by_did request fields are removed. create_pr/create_issue resolved the repo with get_repo but skipped authorize_repo_read, so a non-reader could open a PR (firing the owner's webhooks) or file an issue against a private repo they cannot read. Gate both on read access like their create_review/create_comment/create_bounty siblings. Adds DB-backed tests covering non-assignee rejection (including the empty-body bypass), the claimed-state predicate, and private-repo PR/issue denial.
The guard asserted a marker STRING appeared in each handler body, which a bare identifier in a comment or log line could satisfy even after the real gate was deleted, and its body slice over-ran on any handler not declared exactly 'pub async fn'. - Markers are now gate-shaped: a call (require_repo_owner(, did_matches(, authorize_repo_read() or a binding/comparison expression (caller != &record.owner_did, let owner_did = auth.0), never a field name. - Full-line comments are stripped before matching, so a gate that survives only as a comment no longer counts as enforced. - The body slice bounds at the next top-level fn item across pub async, pub(crate) async, async, pub, and bare fn forms. - Adds rows for the now-read-gated create_pr/create_issue and for fork_repo, create_repo, set_profile, claim_bounty; register is excluded with a note (it trusts the body did, tracked as P3 D3-1). - Adds a self-test proving a comment-only marker does not satisfy a row.
Five low-severity items from the stack review. The sixth (a did_matches false-negative on non-canonical multibase encodings of the same key) is left as-is: it is a self-inflicted owner lockout unreachable with the conforming client, never an impersonation vector, so decoding keys in the auth path is not warranted. - register bound the registered DID to the request body, not the signer, so a signed caller could create or refresh a trust row under a victim DID. Bind it to auth.0 (403 on mismatch) and add a drift-guard row now that it is gated. - Bounty submit/approve/cancel returned 400 for an identity-mismatch denial where dispute returned 403. Return Forbidden, matching the rest of the surface. - The visibility/protect owner gates (require_owner and the protect.rs inline checks) returned 400 for a non-owner; return 403 like require_repo_owner, and assert the exact code in the harness. - get_tree rejected ./.. segments but not empty interior segments, so a path like secret//x could reach git while the gate saw a different string. Reject empty interior segments too (the empty path remains the root listing). - Tighten the get_tree path-scope test to assert an exact 200 on the non-withheld branch so a future upstream 4xx/5xx cannot masquerade as gate-pass. Adds a register-binding regression test.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughA DID-aware authorization framework is added with ChangesAuthorization Enforcement Across All API Handlers
Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler
participant authorize_repo_read
participant did_matches
participant DB
Client->>Handler: HTTP request + AuthenticatedDid
Handler->>authorize_repo_read: caller DID + repo path
authorize_repo_read->>DB: load repo record + visibility rules
DB-->>authorize_repo_read: (record, rules)
authorize_repo_read->>did_matches: normalize caller vs owner DID
did_matches-->>authorize_repo_read: match true/false
alt match passes
authorize_repo_read-->>Handler: Ok(record, rules)
Handler-->>Client: 200 / 201
else match fails
authorize_repo_read-->>Handler: Forbidden / NotFound
Handler-->>Client: 403 / 404
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
crates/gitlawb-node/src/api/visibility.rs (1)
28-35: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse
did_matchesinsiderequire_owner.Visibility owner checks still authorize via the last colon-delimited segment. That misses bare/full
did:keynormalization and preserves the cross-method collision shape the shared helper was added to avoid.🔒 Proposed fix
fn require_owner(record: &crate::db::RepoRecord, caller: &str) -> Result<()> { - let owner_short = record - .owner_did - .split(':') - .next_back() - .unwrap_or(&record.owner_did); - if caller != record.owner_did && caller != owner_short { + if !crate::api::did_matches(caller, &record.owner_did) { return Err(AppError::Forbidden( "only the repo owner can manage visibility".into(), ));🤖 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/api/visibility.rs` around lines 28 - 35, The require_owner function is manually splitting the owner_did string to extract the last colon-delimited segment for comparison, which duplicates logic and misses proper DID normalization. Replace the manual string splitting logic (the split(':').next_back().unwrap_or() call) with a call to the did_matches helper function, passing record.owner_did and caller as arguments. This will ensure consistent DID normalization across all visibility checks and eliminate the cross-method collision vulnerability.crates/gitlawb-node/src/api/protect.rs (1)
27-35: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse
did_matchesfor branch owner checks.These owner gates still compare against
split(':').next_back(), bypassing the new DID-safe normalization. That keeps the full/baredid:keymismatch and trailing-segment collision risk in branch protection.🔒 Proposed fix
// Only the repo owner can protect branches let caller = &auth.0; - let owner_short = record - .owner_did - .split(':') - .next_back() - .unwrap_or(&record.owner_did); - if caller != &record.owner_did && caller != owner_short { + if !crate::api::did_matches(caller.as_str(), &record.owner_did) { return Err(AppError::Forbidden( "only the repo owner can protect branches".into(), )); } @@ let caller = &auth.0; - let owner_short = record - .owner_did - .split(':') - .next_back() - .unwrap_or(&record.owner_did); - if caller != &record.owner_did && caller != owner_short { + if !crate::api::did_matches(caller.as_str(), &record.owner_did) { return Err(AppError::Forbidden( "only the repo owner can unprotect branches".into(), )); }Also applies to: 65-72
🤖 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/api/protect.rs` around lines 27 - 35, Replace the manual DID comparison logic in the branch protection owner gates with the new did_matches function. Instead of using split(':').next_back() to extract and compare the DID segment, locate the did_matches function and use it to safely compare the caller DID against record.owner_did. This change needs to be applied in two locations: the owner gate check around line 27-35 where the split and comparison occurs, and the second location mentioned at lines 65-72. Remove the owner_short variable and the split logic, replacing it with a did_matches call that properly handles DID normalization.crates/gitlawb-node/src/api/repos.rs (1)
312-334: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse the authorized normalized path for the git lookup.
gate_pathis built fromnormalized, butls_treestill receives the originaltree_path, so the path checked by visibility rules can differ from the path resolved by git. Pass the normalized path through to the lookup and response.🛡️ Proposed fix
let gate_path = if normalized.is_empty() { "/".to_string() } else { format!("/{normalized}") }; let (record, _rules) = crate::api::authorize_repo_read(&state, &owner, &name, caller, &gate_path).await?; @@ let head_ref = store::resolve_head(&disk_path, &record.default_branch); - let entries = store::ls_tree(&disk_path, &head_ref, &tree_path).unwrap_or_default(); + let entries = store::ls_tree(&disk_path, &head_ref, normalized).unwrap_or_default(); Ok(Json( - serde_json::json!({ "entries": entries, "path": tree_path }), + serde_json::json!({ "entries": entries, "path": normalized }), )) }🤖 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/api/repos.rs` around lines 312 - 334, The function validates and normalizes the tree_path into normalized and constructs gate_path for authorization checking, but then passes the original tree_path to the store::ls_tree function. This creates a mismatch where the authorization check uses the normalized path while the git lookup uses the unnormalized path, potentially allowing path traversal bypass. Replace the tree_path argument passed to store::ls_tree with the normalized variable to ensure the git lookup uses the same validated path that was authorized.
🧹 Nitpick comments (3)
crates/gitlawb-node/src/server.rs (1)
23-35: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd one HTTP-level GraphQL auth regression test.
The resolver tests inject
AuthenticatedDiddirectly, so they do not proveoptional_signatureplusgraphql_handleractually threads the verified signer into GraphQL request data. A small/graphqlrouter test with a signed mutation would protect this integration seam.Also applies to: 63-66
🤖 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/server.rs` around lines 23 - 35, The graphql_handler function threads the verified AuthenticatedDid into GraphQL request data via the optional_signature middleware, but this integration is not tested at the HTTP level (only resolver-level tests inject AuthenticatedDid directly). Add a regression test for the /graphql router that sends a signed mutation request and verifies that the authenticated signer is properly threaded through the optional_signature middleware into the GraphQL request data, ensuring the integration seam between authentication and request-scoped GraphQL data is protected.crates/gitlawb-node/src/db/mod.rs (1)
1965-1984: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winMake task finishing authorization part of the atomic update.
complete_task/fail_taskauthorize via a priorget_task, butfinish_taskonly updates byidandstatus. Passing the expected stored assignee into this query and adding it to theWHEREclause keeps the auth check and state transition inseparable.🔒 Proposed hardening
pub async fn finish_task( &self, id: &str, new_status: &str, result: Option<&str>, + expected_assignee_did: &str, ) -> Result<AgentTask> { let now = Utc::now().to_rfc3339(); let row = sqlx::query( "UPDATE agent_tasks SET status=$2, result=$3, updated_at=$4 - WHERE id=$1 AND status='claimed' + WHERE id=$1 AND status='claimed' AND assignee_did=$5 RETURNING id, repo_id, kind, status, delegator_did, assignee_did, capability, ucan_token, payload, result, created_at, updated_at, deadline", ) .bind(id) .bind(new_status) .bind(result) .bind(&now) + .bind(expected_assignee_did) .fetch_optional(&self.pool) .await?; row.map(row_to_task) - .ok_or_else(|| anyhow::anyhow!("task not found or not in claimed state")) + .ok_or_else(|| anyhow::anyhow!("task not found, not in claimed state, or not assigned to caller")) }🤖 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/db/mod.rs` around lines 1965 - 1984, The finish_task method currently authorizes via a separate prior get_task call, but the atomic update only checks id and claimed status. Add an expected_assignee parameter to the finish_task function signature and include a check for assignee_did in the WHERE clause of the UPDATE query (comparing against the passed expected_assignee value). This makes the authorization verification and state transition inseparable at the database level, preventing race conditions where authorization could change between the separate authorization check and the actual update.crates/gitlawb-node/src/test_support.rs (1)
45-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider using a more obviously invalid database URL.
The placeholder URL
postgres://localhost/gitlawb_test_placeholdercould accidentally connect if localhost has a database with that name. Consider using an obviously fake URL likepostgres://test-placeholder-never-connect/invalidto make accidental connections fail fast with a clear error.🤖 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/test_support.rs` around lines 45 - 51, The database URL passed to connect_lazy in the test_state_lazy function uses a plausible database name that could potentially connect to an actual database if someone creates it locally. Replace the current URL postgres://localhost/gitlawb_test_placeholder with an obviously invalid and unreachable URL such as postgres://test-placeholder-never-connect/invalid to ensure the lazy connection fails fast with a clear error if it's ever actually attempted.
🤖 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/api/mod.rs`:
- Around line 207-212: The guard expressions for the protect and visibility
operations need to be updated to enforce DID-safe owner matching. For the
protect rows (protect_branch and unprotect_branch), replace the legacy raw owner
comparison "caller != &record.owner_did" with a guard that uses proper DID-safe
comparison instead of direct string equality. For the visibility rows
(set_visibility, remove_visibility, and list_visibility), replace the current
guard pattern "require_owner(" which only checks for method presence with a
guard that verifies the actual use of did_matches for owner validation, ensuring
the implementation properly validates DIDs rather than using trailing-segment
matching.
---
Outside diff comments:
In `@crates/gitlawb-node/src/api/protect.rs`:
- Around line 27-35: Replace the manual DID comparison logic in the branch
protection owner gates with the new did_matches function. Instead of using
split(':').next_back() to extract and compare the DID segment, locate the
did_matches function and use it to safely compare the caller DID against
record.owner_did. This change needs to be applied in two locations: the owner
gate check around line 27-35 where the split and comparison occurs, and the
second location mentioned at lines 65-72. Remove the owner_short variable and
the split logic, replacing it with a did_matches call that properly handles DID
normalization.
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 312-334: The function validates and normalizes the tree_path into
normalized and constructs gate_path for authorization checking, but then passes
the original tree_path to the store::ls_tree function. This creates a mismatch
where the authorization check uses the normalized path while the git lookup uses
the unnormalized path, potentially allowing path traversal bypass. Replace the
tree_path argument passed to store::ls_tree with the normalized variable to
ensure the git lookup uses the same validated path that was authorized.
In `@crates/gitlawb-node/src/api/visibility.rs`:
- Around line 28-35: The require_owner function is manually splitting the
owner_did string to extract the last colon-delimited segment for comparison,
which duplicates logic and misses proper DID normalization. Replace the manual
string splitting logic (the split(':').next_back().unwrap_or() call) with a call
to the did_matches helper function, passing record.owner_did and caller as
arguments. This will ensure consistent DID normalization across all visibility
checks and eliminate the cross-method collision vulnerability.
---
Nitpick comments:
In `@crates/gitlawb-node/src/db/mod.rs`:
- Around line 1965-1984: The finish_task method currently authorizes via a
separate prior get_task call, but the atomic update only checks id and claimed
status. Add an expected_assignee parameter to the finish_task function signature
and include a check for assignee_did in the WHERE clause of the UPDATE query
(comparing against the passed expected_assignee value). This makes the
authorization verification and state transition inseparable at the database
level, preventing race conditions where authorization could change between the
separate authorization check and the actual update.
In `@crates/gitlawb-node/src/server.rs`:
- Around line 23-35: The graphql_handler function threads the verified
AuthenticatedDid into GraphQL request data via the optional_signature
middleware, but this integration is not tested at the HTTP level (only
resolver-level tests inject AuthenticatedDid directly). Add a regression test
for the /graphql router that sends a signed mutation request and verifies that
the authenticated signer is properly threaded through the optional_signature
middleware into the GraphQL request data, ensuring the integration seam between
authentication and request-scoped GraphQL data is protected.
In `@crates/gitlawb-node/src/test_support.rs`:
- Around line 45-51: The database URL passed to connect_lazy in the
test_state_lazy function uses a plausible database name that could potentially
connect to an actual database if someone creates it locally. Replace the current
URL postgres://localhost/gitlawb_test_placeholder with an obviously invalid and
unreachable URL such as postgres://test-placeholder-never-connect/invalid to
ensure the lazy connection fails fast with a clear error if it's ever actually
attempted.
🪄 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: cc8f8fdc-22dd-4bed-be14-8d3bec94a10a
📒 Files selected for processing (19)
.github/workflows/pr-checks.ymlcrates/gitlawb-node/Cargo.tomlcrates/gitlawb-node/src/api/bounties.rscrates/gitlawb-node/src/api/issues.rscrates/gitlawb-node/src/api/labels.rscrates/gitlawb-node/src/api/mod.rscrates/gitlawb-node/src/api/protect.rscrates/gitlawb-node/src/api/pulls.rscrates/gitlawb-node/src/api/register.rscrates/gitlawb-node/src/api/replicas.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/api/tasks.rscrates/gitlawb-node/src/api/visibility.rscrates/gitlawb-node/src/api/webhooks.rscrates/gitlawb-node/src/db/mod.rscrates/gitlawb-node/src/graphql/mutation.rscrates/gitlawb-node/src/main.rscrates/gitlawb-node/src/server.rscrates/gitlawb-node/src/test_support.rs
CodeRabbit flagged that protect_branch/unprotect_branch and visibility's
require_owner used a trailing-segment owner compare (split(':').next_back())
distinct from the hardened did_matches, so the two owner-match idioms could
drift. It is not exploitable (the authenticated caller is always the full did:
form while the short form is a bare suffix, so no wrong caller passes), but it
is a real inconsistency, and the trailing-segment form is a false-negative for a
bare-stored owner. Replace both with did_matches (collapses did:key full vs bare
on both sides, never across methods), and extend the drift guard to assert
require_owner itself uses did_matches, not just that it is called.
Collaborator
Author
|
@kevincodex1 ready for you when you have a chance. CI is green and CodeRabbit's one comment is addressed and resolved. The description has the full rundown; short version, it locks down per-route authorization across the REST and GraphQL write surface and adds the DB-backed test harness that was missing (143 node tests now run against real Postgres in CI). |
This was referenced Jun 28, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Authenticated never meant authorized on the node's signed API. Identities are self-issued
did:key, so a valid signature passed every gate regardless of who the caller was. This locks down the write surface: each mutation checks the caller against the resource it touches, and a DB-backed test harness exercises those checks against real Postgres in CI.What's enforced
get_treegates on the requested subtree rather than the repo root, so a caller denied a withheld path can no longer enumerate its filenames and blob SHAs.A source-level drift guard asserts every in-scope handler still carries its gate, so a future change that drops one fails CI.
Test harness
The node had no DB-backed tests, and CI ran without Postgres, so the entire handler-to-database path was uncovered. This adds a Postgres service to the PR pipeline and a
#[sqlx::test]harness, with behavioral tests for the authorization paths above.Known limitation
did_matchescomparesdid:keyidentities by their multibase string. A key presented in a non-canonical multibase encoding (for example base16 rather than base58btc) will not match the canonical form, so a key-holder on a non-conforming client could be denied their own owner gate. This is a false-negative only, never an impersonation vector (producing any encoding still requires the private key), and the conforming client always emits canonical base58btc. Left as-is rather than decoding keys in the auth path; noted here so it is on record.Verification
143 node tests pass against real Postgres; clippy and rustfmt are clean. The commits are ordered harness, REST gates, GraphQL,
get_tree, then three follow-ups (task-completion authorization, drift-guard hardening, and a small consistency batch) so they can be reviewed or bisected independently.Summary by CodeRabbit
Bug Fixes
Tests