Skip to content

Commit 6f36fc0

Browse files
beardtheliont
andauthored
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 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. * docs(node,git-remote): correct auth-contract comments on receive-pack gating (#119) receive-pack POST is signature-required and separately owner-gated, not visibility_check-gated; push implies read, not read-subset-of-push. * test(node): assert unsigned receive-pack POST is rejected (#119) 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. * test(node): RAII-clean the served-refs test's temp dirs on panic (#119) 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. * test(node): drop git_info_refs from KNOWN_UNGATED now that #119 gates it 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. * style(node,git-remote): tidy #119 review nits (em dashes, dead binding, 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 * test(node): recurse nested api modules in the egress-gate guard 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. * fix(git-remote): keep public fetches anonymous, sign only on a 404 retry (#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. * fix(node): scan nested api sources by relative path in the egress guard (#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. --------- Co-authored-by: t <t@t>
1 parent 563c456 commit 6f36fc0

6 files changed

Lines changed: 1064 additions & 46 deletions

File tree

‎Cargo.lock‎

Lines changed: 1 addition & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎crates/git-remote-gitlawb/Cargo.toml‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,3 +16,6 @@ anyhow = { workspace = true }
1616
reqwest = { workspace = true }
1717
tracing = { workspace = true }
1818
tracing-subscriber = { workspace = true }
19+
20+
[dev-dependencies]
21+
mockito = "1"

0 commit comments

Comments
 (0)