feat(node): owner-only push enforcement behind GITLAWB_ENFORCE_OWNER_PUSH (#31)#68
Conversation
…PUSH (Gitlawb#31) A valid did:key signature on git-receive-pack is authentication, not authorization: any party can generate a key, derive its DID, sign, and push to an unprotected branch. Owner checks only ran on protected branches, so a non-owner could write to unprotected branches of an arbitrary repo. Add an opt-in flag GITLAWB_ENFORCE_OWNER_PUSH (default false, mirroring GITLAWB_REQUIRE_SIGNED_PEER_WRITES) that requires the authenticated pusher to be the repo owner on every branch. Default-off keeps live nodes unaffected. Design follows the maintainer's review on Gitlawb#31: - Authorize on the canonical AuthenticatedDid injected by require_signature, via Extension<AuthenticatedDid> on the handler — not a re-parse of the headers. The legacy branch-protection check is left as-is. - Fail closed: an absent identity or a non-owner caller is rejected. The policy lives in a pure fn (owner_push_rejection) so it is unit-tested without a DB. - The gate runs before branch protection, so one rejection yields one error body. - Factor the duplicated owner-match into auth::is_repo_owner, reused by receive-pack, protect.rs, and visibility.rs (each keeps its own message). events.rs / peers.rs are left out — they aren't owner checks. - Name the push gate by intent (caller_authorized_to_push) so the planned Phase 2 UCAN git/push capability is a pure addition, not a rewrite. Tests: owner (full + short DID) allowed, non-owner rejected, missing-DID rejected, flag-off legacy. Docs: .env.example + docs/RUN-A-NODE.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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 (2)
📝 WalkthroughWalkthroughAdds an opt-in ChangesOwner-Only Push Enforcement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/gitlawb-node/src/api/visibility.rs (1)
55-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
path_globwildcard validation is bypassable for non-trailing*patterns.Line 55 only rejects
*when the string does not end with/**, so values like/a*b/**(and/a/**/**) currently pass even though they violate the “only trailing/**wildcard” rule.Suggested fix
fn validate_path_glob(path_glob: &str) -> Result<()> { @@ - if path_glob.contains('*') && !path_glob.ends_with("/**") { + if let Some(prefix) = path_glob.strip_suffix("/**") { + if prefix.contains('*') { + return Err(AppError::BadRequest( + "the only supported wildcard is a single trailing '/**'".into(), + )); + } + } else if path_glob.contains('*') { return Err(AppError::BadRequest( - "the only supported wildcard is a trailing '/**'".into(), + "the only supported wildcard is a single trailing '/**'".into(), )); } Ok(()) }fn rejects_malformed_globs() { @@ - for g in ["", "secret/**", "/**", "/secret/", "/a*b", "/*/x"] { + for g in [ + "", + "secret/**", + "/**", + "/secret/", + "/a*b", + "/*/x", + "/a*b/**", + "/a/**/**", + ] { assert!(validate_path_glob(g).is_err(), "{g} should be rejected"); } }Also applies to: 240-241
🤖 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 55 - 58, The wildcard validation in the path_glob check at line 55 is incomplete and allows invalid patterns to pass through. The current logic only rejects paths that contain '*' and don't end with "/**", but patterns like "/a*b/**" bypass this check because they still end with "/**" despite having invalid wildcards elsewhere. Fix this by ensuring that '*' characters only appear as part of the trailing "/**" pattern and nowhere else. You can verify this by checking if the path_glob contains '*' anywhere outside of the "/**" suffix (for example, by checking if removing the trailing "/**" still leaves a '*' in the remaining string). Apply the same fix at both locations mentioned (line 55-58 and line 240-241).
🧹 Nitpick comments (1)
crates/gitlawb-node/src/api/repos.rs (1)
514-523: ⚡ Quick winUse one canonical DID source for all push authorization checks.
Line 518 establishes
auth.0as the canonical identity, but the protected-branch path later re-parses headers. Keeping both paths onAuthenticatedDidavoids drift and parser mismatch between authorization checks.Suggested refactor
- let pusher_did_for_check = extract_did_from_auth(&headers); + let pusher_did_for_check = Some(auth.0.as_str()); @@ - let owner_short = record - .owner_did - .split(':') - .next_back() - .unwrap_or(&record.owner_did); - let is_owner = pusher_did_for_check - .as_deref() - .map(|did| did == record.owner_did || did == owner_short) - .unwrap_or(false); + let is_owner = pusher_did_for_check + .map(|did| crate::auth::is_repo_owner(&record, did)) + .unwrap_or(false);🤖 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 514 - 523, The authorization checks are using inconsistent DID sources: the owner_push_rejection function correctly uses auth.0 (AuthenticatedDid) as the canonical identity, but the protected-branch authorization path re-parses headers instead. Refactor the protected-branch authorization check to use the same auth.0 AuthenticatedDid value consistently, removing any header re-parsing in that path to ensure both authorization checks use the same canonical identity source and avoid parser mismatch.
🤖 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.
Outside diff comments:
In `@crates/gitlawb-node/src/api/visibility.rs`:
- Around line 55-58: The wildcard validation in the path_glob check at line 55
is incomplete and allows invalid patterns to pass through. The current logic
only rejects paths that contain '*' and don't end with "/**", but patterns like
"/a*b/**" bypass this check because they still end with "/**" despite having
invalid wildcards elsewhere. Fix this by ensuring that '*' characters only
appear as part of the trailing "/**" pattern and nowhere else. You can verify
this by checking if the path_glob contains '*' anywhere outside of the "/**"
suffix (for example, by checking if removing the trailing "/**" still leaves a
'*' in the remaining string). Apply the same fix at both locations mentioned
(line 55-58 and line 240-241).
---
Nitpick comments:
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 514-523: The authorization checks are using inconsistent DID
sources: the owner_push_rejection function correctly uses auth.0
(AuthenticatedDid) as the canonical identity, but the protected-branch
authorization path re-parses headers instead. Refactor the protected-branch
authorization check to use the same auth.0 AuthenticatedDid value consistently,
removing any header re-parsing in that path to ensure both authorization checks
use the same canonical identity source and avoid parser mismatch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e39834c-ba63-4bad-a7b4-137ba8def076
📒 Files selected for processing (7)
.env.examplecrates/gitlawb-node/src/api/protect.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/api/visibility.rscrates/gitlawb-node/src/auth/mod.rscrates/gitlawb-node/src/config.rsdocs/RUN-A-NODE.md
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Took a full pass over the owner-push gate. The core is solid: it fails closed, runs ahead of branch protection, the did:key suffix match isn't spoofable since the authenticated caller is always the full did:key: form, and the new Extension(auth) extractor is safe because the route sits behind require_signature. owner_push_rejection being a pure, unit-tested function is the right call. A few things to tighten before it's ready to merge:
Findings
-
[P2] Return 403 for the authorization denial, not 400
crates/gitlawb-node/src/api/repos.rs:484
owner_push_rejectionrejects withAppError::BadRequest, butAppError::Forbiddenalready exists and maps to 403, the correct status for "authenticated but not the owner." 400 reads as a malformed request and some git/CI clients retry it. The same applies to the branch-protection denial and the owner checks inprotect.rs/visibility.rs, so this is cleanest as one small sweep. -
[P2] Fold the remaining inline owner-matches into
is_repo_owner
crates/gitlawb-node/src/api/repos.rs:549
This PR addsis_repo_ownerand routes protect/visibility through it, but two hand-rolledsplit(':').next_back()checks remain: the branch-protection check here (which also reads the pusher DID fromextract_did_from_auth(&headers)rather than the verifiedauth.0), and the list filter atrepos.rs:165. Routing both through the helper finishes the consolidation and drops the second identity source on the push path. -
[P2] Assert the error variant in the rejection tests
crates/gitlawb-node/src/api/repos.rs:1315
The rejection tests assert.is_some(), so the 400 to 403 change above would pass silently. Match the variant instead. Worth adding a handler-level test that runsgit_receive_packwith the flag on so a wiring regression onExtension(auth)is caught, plus direct tests foris_repo_owner/caller_authorized_to_push. -
[P3] Warn that enabling the flag blocks delegated and CI agents
docs/RUN-A-NODE.md:478
The route verifies a UCANgit/pushchain inrequire_ucan_chainthen discards it;caller_authorized_to_pushonly accepts the literal owner. So turning this on today hard-blocks every delegated or CI agent that isn't the owner, and thegl ucan_delegatepath forgit/pushis a dead end until Phase 2. One line of caution in the hardening section would save someone locking out their own automation.
The path_glob validation issue is pre-existing (identical on main, untouched here) and filed separately as #74, so it's out of scope here.
…tch consolidation Per @beardthelion's review: - Return 403 Forbidden (not 400) for authorization denials: the owner-push gate, the branch-protection denial, and the owner checks in protect.rs / visibility.rs. 400 reads as a malformed request and some git/CI clients retry it. - Finish the is_repo_owner consolidation: route the branch-protection check and the repos list filter through the helper. The branch-protection check now uses the verified AuthenticatedDid (auth.0) instead of re-parsing the headers. - Drop the second identity source on the push path entirely: the post-push event recording also uses auth.0, so extract_did_from_auth (and the handler's HeaderMap param + import) are removed. - Tests assert the Forbidden variant (so a future status regression fails loudly) and add direct coverage for is_repo_owner / caller_authorized_to_push. - Docs: warn that enabling the flag blocks delegated and CI agents until the Phase 2 UCAN git/push capability is honored. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the thorough pass. All addressed in [P2] 403 not 400 — Done as one sweep: the owner-push gate, the branch-protection denial, and the owner checks in [P2] Finish the [P2] Assert the variant — The rejection tests now match On the handler-level [P3] Doc caution — Added to the hardening section: enabling the flag blocks every non-owner pusher including delegated/CI agents (UCAN Re #74 (the |
There was a problem hiding this comment.
Approving. I verified the security core against the code rather than the PR summary, and confirmed the prior round's four findings are fully resolved.
What I checked:
- Identity on the push path is signature-bound: the verifying key is derived from the keyid, the signature is checked against it, and
AuthenticatedDidis set only after every check passes (crates/gitlawb-node/src/auth/mod.rs:257). The route sits behindrequire_signatureviaadd_auth_layers(crates/gitlawb-node/src/server.rs:159), soauth.0cannot be spoofed or absent at the handler. - The gate fails closed and runs before branch protection,
acquire_write, and the receive-pack spawn. is_repo_ownershort/full matching has no forgeable collision: thez6Mk…form is a 1:1 encoding of the Ed25519 key and the push caller always arrives as the full verified DID.- All four items from the prior round are fully resolved: 403 at every owner-mismatch site, the consolidation routes through
is_repo_owneron the verifiedauth.0(crates/gitlawb-node/src/api/repos.rs:165and:547),assert_forbiddenpins the variant so a regression to 400 fails loudly, and the docs caution is in place.extract_did_from_authis gone with no dangling references. Build is green, clippy is clean, and the 7 new tests pass.
None of the following block the merge. Noting them so they are tracked.
Findings (non-blocking)
- [P3] Extend the owner-match consolidation one hop further.
crates/gitlawb-node/src/visibility.rs:17still carries its ownis_ownerwith the samesplit(':').next_back()idiom. It predates this PR and is behavior-identical, but a sharedis_did_owner(owner_did, caller)that both it andis_repo_ownerdelegate to would close the last copy. - [P3] Document that
GITLAWB_ENFORCE_OWNER_PUSHtakes effect on restart only. It is read once intoConfigat startup (crates/gitlawb-node/src/config.rs:58), so flipping it on a live node is a no-op until restart. A one-line note inRUN-A-NODE.mdand.env.examplewould set the right expectation. - [P3] The rejection
warn!fires on every blocked push (crates/gitlawb-node/src/api/repos.rs:523). When the flag is intentionally on, rejections are expected traffic, so a probing or misconfigured client can inflate log volume.info!or a per-DID rate-limited line would be calmer. - [P3, pre-existing] The git remote helper bails on a non-2xx without reading the body (
crates/git-remote-gitlawb/src/main.rs:297), so the descriptive rejection message never reaches the pushing user. Filed as #77.
Phase 2 (tracked on #31, not this PR): once UCAN git/push is honored, add owner_did and a structured error code to the 403 body so agent clients can act on the rejection rather than just log it, and plan the capability-injection path from require_ucan_chain into the push authorization check.
@kevincodex1 this is ready when you are. The security core is sound, the prior round is fully resolved, and everything above is a non-blocking follow-up.
beardthelion
added
kind:feature
|
hi @FrozenRaspberry thank you for this contribution. kindly rebase to main and please fix conflicts. |
# Conflicts: # crates/gitlawb-node/src/api/protect.rs # crates/gitlawb-node/src/api/visibility.rs
|
Resolved the merge conflict with
Reconciled onto
This also closes the loop on the prior P3 "fold the last owner-match into a shared helper" — there's now a single owner-comparison (
|
beardthelion
dismissed
their stale review
Dismissing my own stale approval: it predates the merge head 8ab7e50, which reconciled onto did_matches and removed is_repo_owner. Re-reviewing the new head.
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Re-approving on the merge head 8ab7e50a. The reconciliation onto did_matches (removing is_repo_owner) is substantive, so I re-verified the security core against the merged code rather than the diff summary.
What I checked:
did_matches(crates/gitlawb-node/src/api/mod.rs:63) is sound on both sides: exact-match short-circuit, stripsdid:key:from each operand, and the!ka.contains(':') && !kb.contains(':')guard blocks cross-method collisions (did:gitlawb:X/did:web:Xkeep their colon after the no-op strip, so they cannot match a bareX). It is symmetric, so the differing argument order between the push gate and the branch-protection check is harmless.- The owner-push gate fails closed and runs before branch protection,
acquire_write, and the receive-pack spawn (repos.rs:531<557<577).auth.0is the verifiedsig.key_idfromrequire_signature, andgit_receive_packis wired throughadd_auth_layers, so the non-OptionExtension(auth)extractor cannot be absent or header-spoofed. extract_did_from_authis fully removed with no remaining callers — this closes the prior header-reparse divergence. The cert / p2p / webhook-payload paths now use the verified DID directly; nothing depended on the old"unknown"/empty sentinels.- The dropped
is_repo_ownerunit test is covered bydid_matches's own tests; the retained owner-push and Forbidden-variant tests pass. Build green, clippy clean.
Two non-blocking follow-ups, both cheap:
Findings
- [P2] Enroll
git_receive_packin the owner-gate guard test.crates/gitlawb-node/src/api/mod.rs:171— the new opt-in push gate is the one in-scope mutation handler with no row inauthz_guard::every_in_scope_mutation_has_its_gate. That table is the mechanical guard against a gate being silently removed, so the new gate should be enrolled: add(repos, "git_receive_pack", "owner_push_rejection("). The marker is real code in the handler body (repos.rs:531), so it satisfies the comment-strippedfn_bodycheck. - [P3] Unit-test the branch-protection rejection arm.
crates/gitlawb-node/src/api/repos.rs:557— thedid_matchessubstitution and the 400→403 change on the protected-branch path have no test. Correct by inspection, but worth pinning: protected + non-owner → Forbidden, protected + owner → passes, unprotected + non-owner → passes.
Neither blocks. @kevincodex1 this is ready when you are.
3 participants
Closes #31.
What
Adds an opt-in flag
GITLAWB_ENFORCE_OWNER_PUSH(defaultfalse) that requires the authenticated pusher to be the repo owner on every branch ofgit-receive-pack, not just protected ones.Why
A valid
did:keyRFC 9421 signature is authentication, not authorization:did:keyis self-certifying, so anyone can generate a key, derive its DID, sign, and push. Today the owner check only runs when the target branch isis_branch_protected, so a non-owner can write to unprotected branches of an arbitrary repo. This is the first item under Security hardening indocs/MAINTAINER-ROADMAP.mdand the Phase 1 slice agreed on in #31.Default
false(mirroringGITLAWB_REQUIRE_SIGNED_PEER_WRITES) means live nodes are unaffected until an operator opts in.How — addressing the review on #31
Extension<AuthenticatedDid>(the canonical DID injected byrequire_signature) instead of re-scrapingkeyidfrom the headers. The legacy branch-protection check is left as-is per your note.unwrap_or(false)fall-through. The decision is a pure fn (owner_push_rejection) so it's unit-testable without a DB or git backend.require_signaturealready rejects unsigned requests upstream; the gate only compares the authenticated DID against the owner.auth::is_repo_owner(record, caller), reused by receive-pack,protect.rs, andvisibility.rs. Each call site keeps its own error message.events.rsandpeers.rsare intentionally left out — they aren't owner checks.caller_authorized_to_push, so the Phase 2 UCANgit/pushcapability becomes a pure addition (is_repo_owner(..) || ucan_grants_push(..)) rather than a rewrite.Out of scope (per your note)
pulls/{n}/mergeandhookswarrant the same gating — kept out of this PR so it stays scoped to owner-push; happy to take the broader write-path review on its own track.Testing
cargo fmt --all -- --checkclean;cargo clippy -p gitlawb-node --all-targets -- -D warningsclean.did:key:…and bare suffix form), non-owner rejected, missing-DID rejected (fail-closed), and flag-off legacy behavior.gitlawb-nodebin suite: 113 passing. The only failures are 3 pre-existingsync::testscases that fail withinvalid filter-spec 'blob:limit=10g'from the local git binary — reproduced identically on a clean tree without this change, so unrelated.Docs
.env.example: documentedGITLAWB_ENFORCE_OWNER_PUSH=false.docs/RUN-A-NODE.md: new "Hardening: owner-only push" section (default off, when to enable, effect).Summary by CodeRabbit
New Features
Documentation