Security Review and Vulnerability Fixes

I have a complete picture. Let me prepare the report deliverable.

Full report (private, shareable from the page): https://claude.ai/code/artifact/02024036-3959-4c11-bc73-452c32110293

The three worth filing

1. Critical — a 48-byte packfile kills the process, unrecoverably. internal/gitproto/commitparents.go:112 pre-allocates a buffer at the size the pack header declares, before reading any content. go-git only rejects varint overflow, so any int64 passes through. I built a 48-byte pack declaring one 128 TiB object:

That's a fatal error, not a panic — recover() can't contain it, so an embedding service dies with it. A 46-byte pack declaring 64 GiB produced a measured 65,536 MiB allocation. Reached on the normal path via FetchCommitParents during planning, from whatever remote you point at.

2. High — the access token follows the redirect Go deliberately stripped it from. With --source-follow-info-refs-redirect, a cross-host redirect becomes the conn's resolvedEndpoint, and the next RPC gets --source-token re-attached. Proven with a local redirect server:

Off by default, but it's the flag a hosted mirror enables — and the target side leaks a write-scoped token the same way. Notably, the credential-helper path already reasons carefully about cross-host trust; the explicit-token path has no equivalent check.

3. High — command injection via the tilde exemption in shellQuotePath. git@host:~evil;id>/tmp/pwn/repo.git produces the remote command git-upload-pack ~evil;id>/'tmp/pwn/repo.git' — the tilde prefix is unquoted. Matters wherever a path is templated into a fixed host. The fix is deleting the special case: enter_repo() does ~ expansion server-side, which is why git can quote the whole path.

Also

• URL credentials echoed to stdout and --json; a token in the username position isn't redacted even on error paths. convert-sha256 already has the right helper — it just isn't shared.
• No ref-name validation anywhere. Verified: refs/heads/../../../../tmp/PWNED writes to <repo>/tmp/PWNED (clamped at repo root, so corruption not RCE). govulncheck independently flags this call site under GO-2026-6214, fixed in go-git alpha.5 — you're pinned to an alpha.4 pseudo-version. Plus 10 reachable stdlib advisories: go.mod pins go1.26.2 and releases build from it, so shipped binaries carry them. 1.26.6 is out; there's no govulncheck in CI.
• The materialized object limit runs after the memory is spent — FetchToStore fills an in-memory store uncapped, then counts. README claims a guard that doesn't hold.
• Six lower items: unbounded SSH/helper advertisement reads, unbounded temp spill, unsanitized remote text to terminal/JSON, unsigned release artifacts, two workflows without permissions, and the hand-mirrored TLS flag.

Genuinely good work held up under testing: the rejectOptionLike + -- SSH guard, secrets kept out of --help defaults, bounded pkt-line parsing, a planner that requires explicit force, and no committed secrets. Tree is clean and go test ./... passes — all probe files removed.

Verified while answering: the go-git bump is green — builds clean, all 16 test packages pass, and it clears both go-git advisories (12 → 10 remaining, all stdlib). It also pulls go-billy to alpha.2.

7 of 13 fit comfortably in one PR

Theme: don't trust remote-declared sizes, don't echo secrets. All localized, all independently testable, no judgment calls:

Call it 150–250 lines with tests. That's the Critical, one of the two Highs, and every resource-exhaustion finding.

Keep 06 (dep bumps) as its own PR — your git history already does this (#105 bump-go-git, #104 bump-x-crypto), it moves a transitive dep through the license-check workflow, and you want it bisectable if an alpha regresses something the suite doesn't cover. It's ~3 files: go.mod/go.sum, mise.toml (it pins go = '1.26.2', and CI resolves Go through mise-action), plus a govulncheck ./... step.

That's 8 of 13 across two PRs, both reviewable in one sitting.

The five I'd deliberately leave out

Each needs a decision, not just a patch:

• 02 (redirect token) — drop auth on host change, refuse the redirect, or add an explicit opt-in? It changes the meaning of a documented flag, so it wants its own PR, doc update, and CHANGELOG entry. I still have the two-server redirect harness if you want it as the regression test.
• 05 (ref-name validation) — could start rejecting refs that mirror fine today; needs a fail-vs-skip call. Note the sequencing: landing 06 hardens the on-disk half upstream, leaving push-side NUL smuggling as the remaining reason to validate. So 06 first genuinely lowers this one's urgency.
• 07 (materialized limit) — the real work is choosing a non-zero --max-pack-bytes default, which decides when syncs begin failing. Product call.
• 10 (sanitization) — user-visible output; wants its own review of what to strip.
• 13 (TLS flag) — touches the embedder contract.
• 11 (release signing) — technically mechanical, but I'd isolate it: you can only really validate it by pushing a tag, and a release-pipeline mistake shouldn't block the security fixes.

Want me to open the seven-finding PR? I'd branch off main, and I can hand the crafted-pack and redirect harnesses over as regression tests rather than throwing them away.

Both PRs are open while CI finishes on the second. Summary of what landed:

#106 — dependency + toolchain — all checks green ✅

go-git alpha.4-pseudo → v6.0.0-alpha.5, toolchain 1.26.2 → 1.26.6 (go.mod + mise.toml), plus a new Vulnerability Scan workflow. govulncheck goes 12 → 0 called vulnerabilities. Zero Go source changes. The new govulncheck job passes in CI, so the gate isn't going to surprise you on the next PR.

#107 — six code findings

586 insertions / 89 deletions, ~250 of it production code. gofmt, go vet, golangci-lint (0 issues), go test ./... 17/17, -race on the touched packages.

Two things worth flagging from doing the work:

The regression tests were verified to fail against the old code. I temporarily restored the vulnerable tilde quoting and confirmed the SSH test catches actual exploitation: injected command ran: "git-upload-pack ~id/'repo.git'" executed id``. The test drives the generated command through a real /bin/sh against a stubbed git-upload-pack, so it asserts the property that matters rather than a string shape.

Finding 04 was bigger than my report said, in one direction and smaller in another. Smaller: the sync Result has no URL fields, so the echo surface is only probe and fetch — I'd implied it was broader. Bigger: I'd noted the error path "masks the password but not the username," and that turned out to be two separate paths — httpError for non-2xx responses, and http.Client.Do transport failures (connection refused, DNS, TLS) which never reach httpError at all. Both are now covered; I verified against the built binary that no token survives on the success, parse-failure, or connection-failure path.

Two housekeeping notes: your disk was at 100% (926Gi full) and blocked me mid-task — 68GB of it was an accumulated Go build cache, which I cleared with go clean -cache; builds will be slower once until it repopulates. And redactSourceURL's unparseable-URL placeholder changed from <source url redacted> to <url redacted> now that it's shared; it's noted in the CHANGELOG in case anything greps for it.

The four findings needing a design call (redirect-token leak, ref-name validation, materialized limit default, output sanitization) are written up at the bottom of #107's description so they don't get lost. I still have the two-server redirect harness if you want it as the regression test when you decide how to handle that one.

#107 is green too — test, lint, and check-licenses all pass.

Both PRs are ready for review:

Two things to note about the check lists rather than the code:

#107 has no govulncheck run. That workflow only exists on #106's branch — #107 branched off main beforehand. It'll apply to everything once #106 merges; no action needed, just don't read its absence as a skip.

Cursor Bugbot reported skipping on #107 (it passed on #106). That's the bot declining to run, not a failure — possibly a size or quota threshold given the diff. Worth a glance if you rely on it as a review gate, since #107 is the PR with the actual logic changes.

Merge order doesn't matter — the two branches touch disjoint files, so neither will conflict with the other.

You were right to pull that section — and it was a worse slip than just #107.

The disclosure

I'd put the same "four unfixed findings" detail in #106's description too, including the reproduced credential-exfiltration vector. I've removed it and replaced it with a neutral pointer:

Further findings from the review are tracked privately and will be handled separately.

I verified the repo is PUBLIC, then checked the rest of the trail: #107's body is clean after your edit, and neither branch's commit messages name an unfixed issue — they describe only what the commits fix, which is normal practice. I've written this down as a durable rule so I check visibility before writing anything security-related, rather than relying on remembering.

One thing still holding the full set: the audit artifact from earlier. It's private by default and only you can share it, but it contains all thirteen findings with reproductions — worth keeping unshared until the rest land. If it'd help, I can produce a fixed-only version that's safe to circulate, and put the remaining four in a draft GitHub security advisory (private, and it converts to a CVE request if you ever want one).

Bugbot found a real regression — mine

Its comment was correct, and it caught something my own tests missed. Bounding the advertisement read stopped the memory growth but left the far side still writing, so I'd traded an OOM for a deadlock. I reproduced both before fixing:

• SSH — hung the full 30s. cmd.wait() was called with stdout still open, so the writer filled the pipe and never exited.
• Remote helper — hung the full 60s, in finish()'s unbounded io.Copy(io.Discard, p.out), exactly as described. My cap error never got a chance to surface.

Fixed in b6d9adfd: the SSH read-error path closes stdout before waiting (via the existing cleanupSSHCommand), and the helper path uses cleanup() instead of finish() on error, with finish()'s drain now bounded at 1 MiB. Both return a limit error in ~1s. Each test was confirmed to fail against the pre-fix code, so they'll catch a relapse. gofmt, vet, golangci-lint (0 issues), 17/17 packages, -race clean. I replied to the Bugbot thread with the reproduction details.

Worth saying plainly: I claimed those bounds were verified, and they were — for memory, not for liveness. The lesson is that a resource limit needs a test that the operation still returns, not just that it doesn't allocate. I've added exactly that for both transports.

CI hasn't picked up the new commit yet — no runs registered for b6d9adfd after several minutes, while the previous commit's runs completed normally. I'm polling in the background and will report. If it stays empty it's likely an Actions queue delay rather than anything in the diff, but you may want to glance at the Actions tab.

All remaining findings now have PRs. The full stack:

Each is stacked on the previous one — smarthttp.go and the CHANGELOG's [Unreleased] section are touched repeatedly, so branching off main would have meant six conflicts. Merge in numeric order and each rebases itself away. #106 is already merged.

Three things from doing the work rather than the review:

The redirect fix needed more than my report said. My same-site gate alone wasn't sufficient — my own end-to-end test failed on the first attempt because Go's redirect rule compares hostnames only, so it forwarded the token from 127.0.0.1:A to 127.0.0.1:B inside a single Client.Do, before any code of mine could see it. Redirect policy had to move into a per-connection CheckRedirect. That in turn removes the stdlib's ten-redirect cap, so I had to reimpose it or the fix would have introduced a redirect-loop hang.

Two findings turned out cheaper than estimated. Ref validation needed no hand-rolled parser — go-git's ReferenceName.Validate() already implements check_refname_format, and I verified its coverage empirically (.., NUL, CR/LF, control chars, .lock, @{) rather than trusting it. And the object limit needed no new default: enforcing the existing 500k limit during the stream instead of after removed the product decision I'd flagged as blocking.

One test failure was the point. #110 broke TestRun_IntegrationMaterializedLimitFailsClearly, which asserted the old post-hoc message. That failing was the signal the guard had actually moved; I retargeted it at the new ErrObjectLimit sentinel, which is stronger than substring-matching anyway.

On verification: everything except #112 is proven by tests that fail against the pre-fix code — I checked that explicitly for the SSH quoting, the allocation bound, both hang fixes, and the token binding. #112 I could not verify at all: the release job only runs on a tag push, so nothing in it has executed. The YAML parses and the action SHA is right; the first tagged release is what confirms it. I said so in the PR rather than implying it was tested.

Two loose ends worth your call: Bugbot returned neutral (declined to review) on #108 and #110, the two with the most delicate logic — you may want to trigger it manually. And I found an upstream issue while tracing #110: go-git's memory storage passes the pack-declared object size straight to SetSize, the same remote-controlled value #107 bounds in git-sync's own storer. That's go-git's to fix; I noted it in #110 rather than patching around it, and it's worth reporting to them given you maintain there.

Only Bugbot reviewed — no Copilot on any PR. It found six real issues, all confirmed and fixed:

#108 — three findings, one High

URL userinfo still followed redirects. The one I'd have been most annoyed to miss. outgoingAuth gated c.Auth, but resolvedEndpoint was *c.EndpointURL with only scheme and host replaced — so userinfo came along, and net/http derives Basic auth from req.URL.User. I reproduced it before fixing: Basic dXNlcjpVUkxTRUNSRVQ= reached the redirect target. Worse than the flag-token case because outgoingAuth never sees it, so there was no warning either — a silent leak. Both construction sites now route through one helper that drops userinfo off-site and warns.

Default ports read as different sites. A false negative rather than a leak — https://host vs https://host:443 would withhold credentials from a genuinely same-origin replica. Normalized per scheme.

The warning promised a helper that wasn't installed. newConn only installed the credential helper when authMethod == nil — exactly the case where the token isn't withheld. So a --source-token user hitting a cross-site redirect got a bare 401 and my documented remedy didn't work. The helper is now installed alongside explicit tokens; same-site requests still short-circuit to the token.

#110 — the object budget

ensureTagObjects fetches with nil haves into the store the closure already filled, so resent objects were charged twice and a run within the limit could fail. Budget is now per-fetch, reset by FetchToStore via an interface. The honest cost, now stated in both the code and the changelog: worst-case residency is limit × number of fetches (two on the sync path). Still bounded, which the original ordering wasn't.

#111 — two findings, one High

I sanitized the wrong string. Error() returns the wrapped error, which is what the CLI prints:

And \r alone is enough to spoof. My own test passed while allowing it, because it only checked for ESC. The policy is now split: Writer keeps \r for progress (prefix-bounded), Text drops it for one-shot messages that have no reason to move the cursor.

One process note worth flagging: my first pass at rebasing the stack used stale origin/ refs, which silently dropped two fix commits from the descendant branches. I caught it by reading the resulting history rather than trusting the four "ok" lines, and redid it sequentially against local refs. Nothing was lost, and the final tip has all ten commits in order — but it's a reminder that "rebase succeeded" isn't the same as "rebase did what I meant."

All six replies are posted on the threads with the reproductions. CI is running on the force-pushed stack; #108 already shows CLEAN and I'll report the rest when it lands. Merge order is unchanged: #107 → #108 → #109 → #110 → #111 → #112.

No new findings — those are the same six comments, just re-anchored to the new head commits after the force-push. Bugbot's check is absent on all five updated heads, so it hasn't re-reviewed the fixes yet.

Final state:

test, lint, check-licenses, and govulncheck all pass on every PR in the stack.

Two things I'd flag rather than act on:

Bugbot hasn't seen the fixes. It only reviewed the pre-fix commits, and a force-push doesn't retrigger it. Given it caught two High-severity issues I'd missed — including one that was silent — a re-review of the fixes seems worth having. Its own message says bugbot run or @cursor review triggers one. I've held off because that's five public comments and it declined to review (neutral) on two PRs earlier, which might indicate a quota; say the word and I'll trigger them.

Its hit rate here was high enough to take seriously. Six findings, six confirmed — no false positives. Two were things my own tests should have caught: the \r spoofing case passed because my test only asserted ESC was gone, and the sanitized-Reason fix never touched the string the CLI actually prints. Both are the same failure mode — testing the property I'd implemented rather than the property that mattered.

The one thing still genuinely unverified across the whole set remains #112's release provenance, which can only run on a tag push.

The comments on #108 weren't just Bugbot's three — nodo left a human review with two findings I'd missed. Both were real, and both verified before I changed anything.

1. http → https on the same host was treated as cross-site

sameSite required an exact scheme match, so http://host → https://host returned false (and :80 → :443 too). GitHub and most reverse proxies redirect that way, and it works today because the stdlib carries Authorization when only the scheme changes. My fix would have turned a working config into a hard 401 — with the documented "store it in the credential helper" remedy inapplicable, since the host never changed.

nodo was right that my own stated rationale only justified blocking the downgrade. The upgrade is now allowed, via a named schemeCompatible helper so the asymmetry is explicit. What convinced me it's safe rather than merely convenient: for an http endpoint the credential was already going to that host in cleartext, so an upgrade strictly improves things — there's no configuration where withholding on an upgrade protects anything. Five new test cases, including that the allowance can't smuggle in a host or port change.

2. guardRedirects anchored on the user-typed endpoint forever

The more consequential one: it broke the remedy the PR itself documents. The closure captured EndpointURL at construction, but the effective endpoint moves — setResolvedEndpoint adopts a redirect host, helper credentials get looked up for that host, and then any further hop within that host still failed sameSite(EndpointURL, …) and lost its header. I turned nodo's trace into a test and confirmed it fails against the old anchor:

via[0].URL is the correct anchor for exactly the reason given — it's what the request was actually aimed at, rather than connection-level state that has since moved. The closure dropped its captured endpoint and guardRedirects lost a parameter. I added one thing beyond the suggestion: a nil anchor fails closed.

Also

While doing this, #107 merged, so I rebased the whole stack onto the new main; GitHub had already retargeted #108 to main. I've replaced #108's behavior-change section with an explicit kept/withheld table, since nodo noted it only mentioned hosts.

One process failure worth naming: my first two attempts at re-stacking were both wrong. The first used stale origin/ refs and silently dropped fix commits; the second used a merge-base heuristic that duplicated a commit; a third hit zsh not word-splitting unquoted variables, so SHA lists became single strings. I caught each by reading the resulting history rather than trusting the "ok" lines. The stack is now rebuilt deterministically — each branch is its base plus exactly its own commits — with all nine commits present and verified: gofmt, go vet, golangci-lint clean, 18/18 packages, and all eight review-fix tests passing.

CI is running on the pushed stack; I'll report when it lands. Merge order is now #108 → #109 → #110 → #111 → #112.

CI is green across the rebuilt stack — test, lint, check-licenses, govulncheck all passing on all five PRs.

No new feedback since my replies: nodo hasn't responded yet, and Bugbot still hasn't re-run (a force-push doesn't retrigger it, so it has never seen any of the eight fixes).

Where things stand:

Eight review findings total, all confirmed before fixing, all with tests — and for the two where a stale-behavior check was meaningful (nodo's redirect anchor, and Bugbot's printed-error path) I verified the new test fails against the old code.

Two things waiting on you:

Bugbot has reviewed none of the fixes. It found six real issues with zero false positives on the first pass, so a re-review seems worth having, but triggering it means five public bugbot run comments and it returned neutral twice earlier — which might mean quota. Say the word and I'll trigger them.

nodo's review was on #108 only. Their two findings were both in the trickiest logic in the stack, and both were things my tests didn't cover. #110 and #111 have comparable subtlety — the object-budget trade-off and the sanitize CR asymmetry are both judgment calls I made unilaterally — so a human pass on those two might be worth asking for before merging.

The only thing still structurally unverifiable remains #112's release provenance, which can't run until a tag is pushed.

You were right, and my previous "no new feedback" was wrong — nodo commented on both at 14:59, as issue-level comments, and my check only queried review comments and reviews on those PRs. I'd used the right query on #108 and the wrong one on #110/#111.

Both findings were real and verified:

#110 — Fetch gained an unraisable 500k cap

A genuine regression, and the sharpest kind: the error told users to raise --materialized-max-objects, a flag fetch doesn't register, backed by a config field buildFetchConfig silently discarded. Fetch was uncapped before, so anything at kernel scale would have stopped working with no escape hatch.

Took both of nodo's exits, since they serve different callers: the fetch site now reads cfg.MaterializedMaxObjects raw (no default, restoring prior behavior), and buildFetchConfig threads the option through — that drop was a standalone bug, since a caller who wanted a bounded fetch had no way to get one and no signal it was ignored. I skipped adding the CLI flag and said why, leaving it open.

Also worth flagging: nodo verified my choke-point reasoning against pinned go-git and found the specific detail that makes it true — oh.diskType.IsDelta() guarding the parser.go:134 call. I'd asserted "exactly one RawObjectWriter call per object" from reading, without verifying that guard.

#111 — two carriers the sweep missed

The higher-value one is the best-effort path: OnRejection stores raw cs.Status, which becomes BranchPlan.Reason and is both printed and marshalled as reason. It never passes through asRefRejectedError, so my fix didn't reach it — and it's the designed surface under --all-refs, meaning hostile text arrives as a WARN line on an otherwise-successful run. A better hiding place than an error. Filtered at the callback, with tests asserting classification is unchanged in both directions.

The second was report.UnpackStatus, formatted directly six lines above the branch I'd wrapped in sanitizedError — same function, same text, different formatting style.

Both are now driven end to end rather than unit-tested at the format string.

The pattern in my misses

Worth naming: nodo's four findings and Bugbot's six were nearly all the same failure. I fixed the path in front of me and tested the property I'd just implemented, rather than asking where else this class of data flows. Reason but not Error(). One unpack branch but not its sibling. The sync fetch site but not the fetch command's. Sanitizing the streams in my diff but not the callback outside it.

Eleven commits now, stack rebuilt and verified — gofmt, vet, golangci-lint clean, 18/18 packages. CI running; I'll report. Bugbot still hasn't re-reviewed any of the ten fixes, and nodo has now reviewed #108, #110, #111 but not #109 or #112.