feat(node): owner-only push enforcement behind GITLAWB_ENFORCE_OWNER_PUSH (#31)#68
Open
FrozenRaspberry wants to merge 1 commit into
Open
feat(node): owner-only push enforcement behind GITLAWB_ENFORCE_OWNER_PUSH (#31)#68FrozenRaspberry wants to merge 1 commit into
FrozenRaspberry wants to merge 1 commit into
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>
📝 WalkthroughWalkthroughAdds an opt-in ChangesOwner-Only Push Enforcement
Sequence Diagram(s)sequenceDiagram
participant Client
participant git_receive_pack
participant owner_push_rejection
participant BranchProtection
participant ReceivePack
Client->>git_receive_pack: POST /{owner}/{repo}/git-receive-pack (signed)
git_receive_pack->>owner_push_rejection: enforce_owner_push, RepoRecord, Option<AuthenticatedDid>
alt enforce=true AND caller not owner
owner_push_rejection-->>git_receive_pack: Err(AppError::Forbidden)
git_receive_pack-->>Client: 403 Forbidden
else enforce=false OR caller is owner
owner_push_rejection-->>git_receive_pack: Ok(())
git_receive_pack->>BranchProtection: is_branch_protected?
git_receive_pack->>ReceivePack: spawn git-receive-pack
git_receive_pack-->>Client: 200 OK (pack data)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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
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
1 participant
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.
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
GITLAWB_ENFORCE_OWNER_PUSHconfiguration option to enforce owner-only pushes, requiring authenticated pushers to be the repository owner.Improvements
Documentation