fix(node,git-remote): gate receive-pack advertisement, sign client fetch/push#119
fix(node,git-remote): gate receive-pack advertisement, sign client fetch/push#119beardthelion wants to merge 9 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe node now gates ChangesVisibility gate and symmetric signing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (2)
crates/gitlawb-node/src/api/repos.rs (1)
443-445: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFix the authorization invariant wording.
Line 444 says read access is a subset of push access, but the intended invariant is that a legitimate pusher also has read access. The current wording is backwards and could mislead future auth changes.
Suggested wording
- // git-receive-pack POST; read access is a strict subset of push access, so a - // legitimate pusher (the owner) always clears this gate. + // git-receive-pack POST; push access implies read access here, so a + // legitimate pusher (the owner) always clears this gate.🤖 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 443 - 445, The authorization comment in repos.rs is using the wrong invariant wording: it currently says read access is a subset of push access, but it should state that a legitimate pusher also has read access. Update the nearby explanatory comment around the push/read gating logic in the repos.rs authorization flow so it clearly reflects that push implies read for the owner, and keep the wording aligned with the existing owner-gated git-receive-pack behavior.crates/git-remote-gitlawb/src/main.rs (1)
323-327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the receive-pack POST gate description.
These lines say
git-receive-packPOST also runs the read visibility gate, but the node-side contract describes receive-pack POST as separately owner-gated. Keep the comment aligned with the actual cross-layer auth contract.Suggested wording
-/// services. The node read-gates the git-upload-pack POST as well as the -/// git-receive-pack POST (each runs visibility_check at "/"), and the Phase-1 -/// advertisement signature does not carry to this separate request — so a private +/// services. The node read-gates the git-upload-pack POST and separately +/// owner-gates the git-receive-pack POST; the Phase-1 advertisement signature +/// does not carry to this separate request — so a private🤖 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/git-remote-gitlawb/src/main.rs` around lines 323 - 327, The comment in the Phase-2 pack POST flow is describing the receive-pack auth gate incorrectly; update the wording near the code that builds and signs the POST in main.rs to reflect that git-receive-pack is owner-gated separately, not read-visibility-gated like git-upload-pack. Keep the explanation aligned with the actual cross-layer auth contract by distinguishing the two request paths and preserving the note that the Phase-1 advertisement signature does not apply to this separate request.
🤖 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.
Nitpick comments:
In `@crates/git-remote-gitlawb/src/main.rs`:
- Around line 323-327: The comment in the Phase-2 pack POST flow is describing
the receive-pack auth gate incorrectly; update the wording near the code that
builds and signs the POST in main.rs to reflect that git-receive-pack is
owner-gated separately, not read-visibility-gated like git-upload-pack. Keep the
explanation aligned with the actual cross-layer auth contract by distinguishing
the two request paths and preserving the note that the Phase-1 advertisement
signature does not apply to this separate request.
In `@crates/gitlawb-node/src/api/repos.rs`:
- Around line 443-445: The authorization comment in repos.rs is using the wrong
invariant wording: it currently says read access is a subset of push access, but
it should state that a legitimate pusher also has read access. Update the nearby
explanatory comment around the push/read gating logic in the repos.rs
authorization flow so it clearly reflects that push implies read for the owner,
and keep the wording aligned with the existing owner-gated git-receive-pack
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8d12c35e-80f0-45f4-8862-7eaf8fc1c8e1
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (4)
crates/git-remote-gitlawb/Cargo.tomlcrates/git-remote-gitlawb/src/main.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/test_support.rs
beardthelion
added
kind:security
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found one issue that should be cleaned up before this is ready.
Findings
- [P3] Complete CodeRabbit's request to correct the auth-contract comments
crates/git-remote-gitlawb/src/main.rs:323
CodeRabbit's two comment requests are still valid in the current patch. The helper comment says the node read-gates bothgit-upload-packandgit-receive-packPOSTs withvisibility_check("/"), but the actual receive-pack route is signature-required and separately owner-gated; similarly, the node comment says read access is a strict subset of push access, which reverses the intended invariant that push implies read. These comments sit directly on the security-sensitive gate/signing code this PR is changing, so please complete the CodeRabbit request and make both comments match the implemented auth contract.
…uard (#122) The guard counted a handler as gated on bare substring presence of a gate marker, so git_info_refs false-passed on visibility_check( even though that check only runs under `if service == "git-upload-pack"` and leaves the git-receive-pack advertisement ungated. Add gate_runs_unconditionally, which ignores markers that sit only inside an `if service ==` block, and track git_info_refs in KNOWN_UNGATED until #119 makes the gate unconditional. When that lands, the staleness assert forces the entry's removal.
… gating (#119) receive-pack POST is signature-required and separately owner-gated, not visibility_check-gated; push implies read, not read-subset-of-push.
No test covered the signature-required half of the receive-pack auth contract. Wire the POST route behind require_signature as production does and assert an unsigned POST returns 401 before reaching the handler; 401 (not the handler's 404/500) proves require_signature rejected it pre-handler.
|
Addressed both auth-contract comments in 8ea7281.
While in there I added a test (47fd5d0) pinning the signature-gating half: an unsigned git-receive-pack POST returns 401 from |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
This PR fixes the private-repo info/refs advertisement leak by running visibility_check for both git-upload-pack and git-receive-pack, and updates git-remote-gitlawb to RFC-9421 sign both the Phase-1 advertisement GET and the Phase-2 pack POST for both services when an identity keypair is present.
I found one small test-isolation cleanup item; the core auth fix and the cross-crate seam tests look solid.
The review covered the gate change, the new request builders, the prior jatmn/CodeRabbit auth-contract comments, test isolation, and duplicate/superseded PR checks.
Findings
- [P3] Clean up the served-content test's temp directories on failure
crates/gitlawb-node/src/test_support.rs:1083
advertisement_serves_real_refs_only_to_authorized_callerscreates real git repos under/tmp/{slug}/{name}.gitand removes them only after the last assertion, so a failed assertion leaves those directories behind. The names are randomized, so this does not cause cross-run collisions, but it does leak state on the test runner. Please use a RAII guard (a wrapper that callsremove_dir_allinDrop, ortempfile::TempDirwhere the path layout allows) so cleanup runs even when the test panics.
advertisement_serves_real_refs_only_to_authorized_callers created /tmp/<slug>/<name>.git and /tmp/gl-seam-src-<short> and only removed them after the final assertion, leaking those dirs on the runner when an earlier assertion failed. Wrap each known path in a DirGuard whose Drop calls remove_dir_all, bound right after creation, so cleanup runs on success or panic. tempfile::TempDir does not fit here because repo_store::for_testing fixes the path layout.
|
Addressed the temp-dir cleanup in 8b8a4ea.
Verified the panic path by execution: injected a panic before the end and confirmed all three dirs were removed during unwind; on the success path the dirs are removed too, leaving only the empty |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one repository-state blocker that needs to be handled before this is ready.
Findings
- [P1] Rebase on main and resolve the test-support conflict first
crates/gitlawb-node/src/test_support.rs
GitHub currently reports this branch asCONFLICTING/DIRTY, and a merge-tree check against the currentmainreports a content conflict incrates/gitlawb-node/src/test_support.rs. Please rebase on the latestmainand resolve that conflict first, so the auth/test cleanup changes are applied to the code that will actually be merged.
…122) * test(node): guard that every repo-scoped handler is visibility-gated Mirrors the existing every_in_scope_mutation_has_its_gate source-string CI test on the egress side: scrapes every repo-scoped handler (any pub/pub(crate) async fn taking Path<(String,String..)>) and asserts it carries an authz marker, either a read gate (authorize_repo_read / visibility_check) or a write/owner/self gate (require_repo_owner / require_owner / did_matches / &auth.0), or is listed in KNOWN_UNGATED with its tracking issue. Verb-agnostic on purpose: a repo-scoped read named outside list_/get_ (git_info_refs, replicate_encrypted_blobs, withheld_paths) is still checked, so a new ungated egress of any name fails CI instead of shipping as the next leak. A completeness scan over src/api/ asserts every module declaring a repo-scoped handler is wired into the guard, so a whole new file cannot add an ungated handler the scrape never looks at. KNOWN_UNGATED is the live gap list (certs/issues/labels/ bounties/stars per #120, plus events/webhooks/replicas/protected pending PR #113); the staleness assert forces each entry out the moment its gate lands. A count floor trips if the scrape silently breaks, and unit tests pin handler_names and is_repo_scoped. Verified by execution: removing an allowlist entry trips the missing-gate assert; listing a gated handler trips the staleness assert; dropping a source file trips the completeness scan; breaking the parser trips the count floor; and a non-list/get handler (git_info_refs) is confirmed scraped and gate-checked. * test(node): don't let a service-conditional gate satisfy the egress guard (#122) The guard counted a handler as gated on bare substring presence of a gate marker, so git_info_refs false-passed on visibility_check( even though that check only runs under `if service == "git-upload-pack"` and leaves the git-receive-pack advertisement ungated. Add gate_runs_unconditionally, which ignores markers that sit only inside an `if service ==` block, and track git_info_refs in KNOWN_UNGATED until #119 makes the gate unconditional. When that lands, the staleness assert forces the entry's removal. * fix(review): harden gate_runs_unconditionally against unclosed blocks (#122) Code review of the egress-guard hardening: - Clamp the span-advance index so an unclosed `if service ==` block can't slice past the body end and panic (latent: real source is balanced, but the scraper should not crash on a pathological future body). Keep span-to-EOF as the fail-safe direction. - Document that only `if service ==` is detected, not `match service`, so a future match-arm gate isn't silently treated as full. - Add unit cases: gate inside both service blocks (conditional), gate inside and outside (full), and the unclosed-block no-panic regression.
… gating (#119) receive-pack POST is signature-required and separately owner-gated, not visibility_check-gated; push implies read, not read-subset-of-push.
No test covered the signature-required half of the receive-pack auth contract. Wire the POST route behind require_signature as production does and assert an unsigned POST returns 401 before reaching the handler; 401 (not the handler's 404/500) proves require_signature rejected it pre-handler.
advertisement_serves_real_refs_only_to_authorized_callers created /tmp/<slug>/<name>.git and /tmp/gl-seam-src-<short> and only removed them after the final assertion, leaking those dirs on the runner when an earlier assertion failed. Wrap each known path in a DirGuard whose Drop calls remove_dir_all, bound right after creation, so cleanup runs on success or panic. tempfile::TempDir does not fit here because repo_store::for_testing fixes the path layout.
beardthelion
force-pushed
the
fix/gate-receive-pack-advertisement
branch
from
8b8a4ea to
ae15f37
Compare
The repo-scoped egress guard's allowlist (added in #122, merged to main after this branch was opened) still listed git_info_refs as ungated. This branch makes the info/refs visibility gate unconditional for both git-upload-pack and git-receive-pack, so the handler is now gated and the staleness assert requires removing it from the allowlist.
…g, test header) Post-rebase ce-code-review pass on #119. No behavior change: - replace em dashes in the added comments with commas/colons/parentheses - drop the dead `let _ = COVERED_COMPONENTS;` binding (and its import); the `missing_components().is_empty()` assert already covers the intent - add a section header above the new git-info-refs test block to match the module's sectioning convention
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/api/mod.rs (1)
367-393: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRecurse into nested API modules in the completeness scan.
This loop only inspects direct
src/api/*.rsentries. A futuresrc/api/<module>/mod.rsor nested.rsfile can add a repo-scoped handler without ever tripping this “whole new module” guard, so the guarantee in the comment is currently broader than the implementation.🤖 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/mod.rs` around lines 367 - 393, The completeness scan in the API guard only checks direct files from `src/api`, so nested modules like `mod.rs` under subdirectories can bypass the repo-scoped handler check. Update the test logic around the `sources`, `listed`, and `handler_names` scan to recurse through subdirectories under `src/api` and inspect all `.rs` files except the top-level module entrypoint. Make sure the assertion that rejects unlisted repo-scoped handlers applies to every discovered API source, not just immediate children.
🤖 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.
Nitpick comments:
In `@crates/gitlawb-node/src/api/mod.rs`:
- Around line 367-393: The completeness scan in the API guard only checks direct
files from `src/api`, so nested modules like `mod.rs` under subdirectories can
bypass the repo-scoped handler check. Update the test logic around the
`sources`, `listed`, and `handler_names` scan to recurse through subdirectories
under `src/api` and inspect all `.rs` files except the top-level module
entrypoint. Make sure the assertion that rejects unlisted repo-scoped handlers
applies to every discovered API source, not just immediate children.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aecc6384-6a53-4461-808e-c66cad46cebd
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (5)
crates/git-remote-gitlawb/Cargo.tomlcrates/git-remote-gitlawb/src/main.rscrates/gitlawb-node/src/api/mod.rscrates/gitlawb-node/src/api/repos.rscrates/gitlawb-node/src/test_support.rs
✅ Files skipped from review due to trivial changes (1)
- crates/git-remote-gitlawb/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/gitlawb-node/src/api/repos.rs
- crates/git-remote-gitlawb/src/main.rs
- crates/gitlawb-node/src/test_support.rs
…e to 0.4.0 The repo-scoped egress guard's allowlist (added in #122, merged after this branch was opened) still listed the read surfaces this branch gates. Now that list_labels, list_repo_events, list_webhooks, list_replicas, and list_protected_branches are read-visibility gated, the staleness assert requires removing them from the allowlist. The #120 surfaces (certs, issues, bounties, stars) stay listed; git_info_refs stays until #119 lands. Lockfile versions follow the 0.4.0 release the rebase landed on.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one remaining item to address.
Findings
- [P3] Complete CodeRabbit's request to recurse through nested API modules
crates/gitlawb-node/src/api/mod.rs:379
CodeRabbit's latest request is still valid in the current patch. The completeness scan only usesstd::fs::read_dir(api_dir), so it checks directsrc/api/*.rsfiles and skips subdirectories entirely. That means a futuresrc/api/<module>/mod.rsor nested.rsfile could add a repo-scoped handler without being included insourcesand without tripping this guard, even though the guard comment says a whole new module is covered. Please complete the CodeRabbit request by walking nested API source files too, so the assertion applies to every discovered API source rather than only immediate children.
|
@jatmn fixed in ef61b57. The completeness scan now walks |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/gitlawb-node/src/api/mod.rs (1)
429-450: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBasename-only
listedmatch risks masking a same-named nested file.
listed.contains(fname.as_str())(Line 436) skips by bare filename. If a future nested module reuses a basename already present in the top-levelsourceslist (e.g.api/<module>/repos.rsalongside the existing top-levelapi/repos.rs), the nested file would be silently skipped even though it's a distinct, unscanned file — defeating the completeness guarantee the recursive walk was added for. Comparing the file's path relative toapi_rootagainst the listed entries (which are also bare names today, so this would need thesourcesdeclarations to carry relative paths too) would close this gap.♻️ Possible hardening direction
- if path == api_root.join("mod.rs") || listed.contains(fname.as_str()) { + let rel_str = rel.to_string_lossy(); + if path == api_root.join("mod.rs") || listed.contains(rel_str.as_ref()) { continue; }(Requires
sourcesentries to store paths relative toapi_rootrather than bare filenames, or an equivalent path-aware lookup.)🤖 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/mod.rs` around lines 429 - 450, The recursive scan in the api guard is using a basename-only check in the `collect_rs_files` loop, so a nested module can be skipped if it reuses a filename already present in `listed`. Update the lookup in `crates/gitlawb-node/src/api/mod.rs` to compare paths relative to `api_root` instead of `fname`, and make the `sources`/`listed` entries path-aware as needed so `api/<module>/...` files are not silently excluded. Keep the repo-scoped handler assertion in the same loop over `collect_rs_files`, but ensure it only skips exact matching entries, not same-named files in different directories.
🤖 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.
Nitpick comments:
In `@crates/gitlawb-node/src/api/mod.rs`:
- Around line 429-450: The recursive scan in the api guard is using a
basename-only check in the `collect_rs_files` loop, so a nested module can be
skipped if it reuses a filename already present in `listed`. Update the lookup
in `crates/gitlawb-node/src/api/mod.rs` to compare paths relative to `api_root`
instead of `fname`, and make the `sources`/`listed` entries path-aware as needed
so `api/<module>/...` files are not silently excluded. Keep the repo-scoped
handler assertion in the same loop over `collect_rs_files`, but ensure it only
skips exact matching entries, not same-named files in different directories.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6d792293-3c15-475c-bb69-c876d28a7c9f
📒 Files selected for processing (1)
crates/gitlawb-node/src/api/mod.rs
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found a repository-state blocker plus a couple of remaining review items.
Findings
-
[P1] Rebase on main and resolve the merge conflict first
crates/git-remote-gitlawb/src/main.rs
GitHub currently reports this branch asCONFLICTING/DIRTY, and a merge-tree check against the currentmainreports a content conflict incrates/git-remote-gitlawb/src/main.rs. Please rebase on the latestmainand resolve the conflict before addressing the review items below, so the signing/helper changes are applied to the code that will actually be merged. -
[P2] Keep public fetches anonymous when an identity exists
crates/git-remote-gitlawb/src/main.rs:209
git-remote-gitlawbnow callsbuild_advertisement_request(..., keypair)for everygit-upload-packconnection, and that builder attachesSignature-Input/Signaturewhenever~/.gitlawb/identity.pemexists. That means a normal user who has created an identity now discloses their DID to any node they fetch a public repo from, even though public repos do not need authentication and the PR summary says public repos still work anonymously. Please keep public clone/fetch anonymous unless authentication is actually needed, or make the identity disclosure explicit/opt-in so this does not become a silent privacy regression for public reads. -
[P3] Complete CodeRabbit's request with a path-aware source skip
crates/gitlawb-node/src/api/mod.rs:430
CodeRabbit's latest request is still valid in the current patch. The recursive completeness scan walks nested files, but it still skips files by basename withlisted.contains(fname.as_str()); if a future nested module addssrc/api/<module>/repos.rs, it will be skipped because the top-levelrepos.rsis already listed, even though the nested file is a distinct unscanned source file. Please complete the request by comparing paths relative toapi_root(and makingsources/listedpath-aware as needed), so only the exact already-covered source files are skipped.
…tch/push
The info/refs ref advertisement was gated only for service=git-upload-pack
(the visibility check sat inside `if service == "git-upload-pack"`), so an
anonymous GET /{owner}/{repo}.git/info/refs?service=git-receive-pack returned a
private repo's branch names and commit tips, bypassing the read gate. Run the
visibility "/" check for both services.
Now that the advertisement is gated for both services, git-remote-gitlawb has to
authenticate its own requests, or a private repo's owner can neither fetch nor
push it. Sign the Phase-1 advertisement GET and the Phase-2 pack POST for both
services over path_and_query when an identity keypair is present; public repos
stay anonymous. This also repairs a pre-existing break where an owner could not
fetch their own private repo, because the upload-pack POST was sent unsigned.
Tests: node-side gate (anon denied, owner-signed cleared, both services); a
real-signature seam proving the client's own signature verifies under the node's
verification primitives; real served-content withholding against an on-disk repo
(the owner gets the ref bytes, a denied caller's 404 omits them); and mockito
coverage that the client attaches and omits signatures correctly.
… gating (#119) receive-pack POST is signature-required and separately owner-gated, not visibility_check-gated; push implies read, not read-subset-of-push.
No test covered the signature-required half of the receive-pack auth contract. Wire the POST route behind require_signature as production does and assert an unsigned POST returns 401 before reaching the handler; 401 (not the handler's 404/500) proves require_signature rejected it pre-handler.
advertisement_serves_real_refs_only_to_authorized_callers created /tmp/<slug>/<name>.git and /tmp/gl-seam-src-<short> and only removed them after the final assertion, leaking those dirs on the runner when an earlier assertion failed. Wrap each known path in a DirGuard whose Drop calls remove_dir_all, bound right after creation, so cleanup runs on success or panic. tempfile::TempDir does not fit here because repo_store::for_testing fixes the path layout.
The repo-scoped egress guard's allowlist (added in #122, merged to main after this branch was opened) still listed git_info_refs as ungated. This branch makes the info/refs visibility gate unconditional for both git-upload-pack and git-receive-pack, so the handler is now gated and the staleness assert requires removing it from the allowlist.
…g, test header) Post-rebase ce-code-review pass on #119. No behavior change: - replace em dashes in the added comments with commas/colons/parentheses - drop the dead `let _ = COVERED_COMPONENTS;` binding (and its import); the `missing_components().is_empty()` assert already covers the intent - add a section header above the new git-info-refs test block to match the module's sectioning convention
The completeness scan in every_repo_scoped_handler_is_gated used a flat read_dir over src/api, so a future api/<module>/mod.rs could add a repo-scoped handler the scrape never inspects. Walk the api tree recursively via a new collect_rs_files helper (covered by its own unit test), skipping only the top-level mod.rs so nested module files are checked too.
…try (#119) git-remote-gitlawb signed every git-upload-pack request whenever an identity existed, disclosing the caller's DID to any node it fetched a public repo from, even though public repos need no auth. Fetch now sends the advertisement GET anonymously and escalates to a single signed retry only when the node denies it with 404 (how it withholds a private repo from an unauthenticated reader); if the retry was needed, the upload-pack POST is signed too. Push (git-receive-pack) is unchanged and signs from the first request, since a push can never be anonymous. Verified against the node by execution: an unsigned private fetch returns 404, not 401, and a signed retry clears the read gate for the owner and for a granted non-owner reader, while a signed non-reader stays 404. Tests drive handle_connect end to end (recording mock): public fetch stays anonymous for GET and POST, private fetch retries signed once and signs the POST, a keyless fetch does not retry, a still-denied signed retry surfaces 404 once, and push signs from the first request.
…rd (#119) The egress-gate completeness scan skipped already-covered files by basename (listed.contains(fname)), so a nested api/<module>/repos.rs would be skipped because the top-level repos.rs is listed, letting a future nested module add an ungated repo-scoped handler the scrape never inspects. Extract the skip logic into unlisted_source_files and match on the path relative to api_root, so only the exact top-level files already covered are skipped and a nested file with a colliding basename is still scanned. Completes CodeRabbit's outstanding request.
beardthelion
force-pushed
the
fix/gate-receive-pack-advertisement
branch
from
ef61b57 to
7b64cc4
Compare
|
Rebased on latest main and pushed. The only conflict was in the P1 (rebase). Done, on current main; conflict resolved. P2 (public fetches stay anonymous). One fact the design keys on: an unsigned private fetch returns 404, not 401, so the retry triggers on 404. Verified end to end against a running node: a public fetch with an identity present stays anonymous, the owner's private fetch retries signed and clones, a valid signature from a non-reader still gets 404, and a keyless fetch surfaces the 404 without retrying. P3 (nested source skip). The egress-gate completeness scan now skips already-covered files by their path relative to Ready for another look. |
2 participants
Closes #116.
The
info/refsref advertisement was gated only forservice=git-upload-pack(the visibility check sat insideif service == "git-upload-pack"), so an anonymousGET /{owner}/{repo}.git/info/refs?service=git-receive-packreturned a private repo's branch names and commit tips. This runs the visibility"/"check for both services, so a denied caller gets a 404 with nothing leaked.Gating both services means
git-remote-gitlawbhas to authenticate its own requests, or a private repo's owner can neither fetch nor push it. The helper now signs the Phase-1 advertisement GET and the Phase-2 pack POST for both services overpath_and_querywhen an identity keypair is present; public repos still work anonymously. That also repairs a pre-existing break where an owner could not fetch their own private repo, because the upload-pack POST was sent unsigned.Verification:
git-remote-gitlawbactually emits verifies under the node's own verification primitives, over the path it transmits (a tampered path fails).git clone gitlawb://against a running node: the owner succeeds with content; anonymous and a non-owner are denied (404, no content).Two pre-existing issues surfaced while verifying this end to end, both independent of this change: #117 (incremental-fetch deadlock in the helper) and #118 (owner-push enforcement default posture).
Summary by CodeRabbit
New Features
git-upload-packandgit-receive-packwhen an identity keypair is available.Bug Fixes
info/refsread-visibility enforcement so private repositories are hidden for both supported services.git-receive-packPOST) are rejected.Tests