fix(api,sync): Delimit positional arguments in git clone mirror invocations - #463
cairn-intern wants to merge 6 commits into
Conversation
…ations Pass "--" before positional path/URL arguments in Command invocations for "git clone --mirror" across repos.rs, sync.rs, and gl mirror.rs. Fixes Twigpine#374
Fix cargo fmt line-length break for git clone --mirror arguments. Refs Twigpine#376
The delimiter change had no test, so nothing stopped it being reverted as cosmetic. What it guards is real: git parses a remote beginning with a dash as an option, and --upload-pack=<cmd> is one git hands to a shell. No path reaches that today. Peer URLs are gated by is_public_http_url, so the origin clone_repo composes can only begin with http:// or https://; fork paths are joins under repos_dir; and clap rejects a leading-dash positional in gl mirror. The delimiter is what removes git's dependence on those three unrelated gates staying correct. Assert git reports the whole argument as a repository it cannot find, which is only true once the delimiter forces it to be a path. Undelimited, git consumes it as an option and fails against the destination instead, never naming the injected string, so the assertion is red before the fix and green after. Refs Twigpine#374
Carry the positional argument delimiter into fetch_repo when setting the remote origin URL and into setup_partial_clone across all clone branches. Correct the error string quoted in the clone regression test docstring. Refs Twigpine#374
The delimiter contract is duplicated across five direct `git` argv
constructions, but only `sync::clone_repo` had a test that went red when
`--` was deleted. Removing or misplacing the delimiter in any other sink
left the suite green, so the hardening rested on review rather than on
the tests.
Each changed sink now has a subprocess-facing regression driven through
real git: the three `setup_partial_clone` shapes (plain, `--branch`, and
the sparse/promisor arm), `fetch_repo`'s `remote set-url`, `gl mirror`,
and the fork clone.
Two of these could not carry the obvious assertion:
- `git_global` and `git_run` both format failures as
`git {args:?} failed: {stderr}`, so the injected value is already in
the message via the debug-printed argv. Asserting that the error
merely contains it passes with `--` deleted. The clone tests assert on
git's own single-quoted `repository '<value>'` instead, which only
appears when git read the value as a repository.
- `remote set-url` does not fail on a delimited option-shaped value, it
stores it. The `fetch_repo` test asserts the stored URL rather than an
error: undelimited, git exits 129 with `unknown option` and leaves the
previous URL in place.
`gl mirror` and `fork_repo` needed their argv extracted to be testable.
`mirror::run` loads a keypair and contacts a node before reaching the
clone, and bails with a message that interpolates the source whether or
not the delimiter is present. The extraction keeps the production
`.status()` call, so a large mirror clone still streams git's progress
to the terminal instead of being captured.
Every one of the five was confirmed red with its delimiter removed and
green with it restored.
Refs Twigpine#374
`emit_warning` sanitizes a line before writing it, which strips ANSI escape and bidi characters from untrusted text. Three warning sites in the blob-recovery paths still used a bare `eprintln!` while interpolating `oid`, which comes from node JSON or an Arweave gateway manifest, so those escapes reached the user's terminal raw. Refs Twigpine#374
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Warning Review limit reachedNext included review available in 59 minutes. View limit detailsLimit details: You’ve used the included review currently available. Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (4)
Comment |
beardthelion
added
crate:gl
beardthelion
left a comment
beardthelion
left a comment
There was a problem hiding this comment.
Verified on 23e7a07c against real git, both directions. Removing each new -- reddens its paired test on the assertion it names, across the clone_repo, set-url, and all three setup_partial_clone argv shapes, plus the extracted fork and mirror helpers. git remote set-url origin -- --upload-pack=false stores the value verbatim where the undelimited form exits 129 on unknown option, and the promisor arm's option ordering is pinned by the existing positive-path test (a misplaced -- turns the filter into a positional and fails the clone outright).
On reachability: peer URLs are built from is_public_http_url-validated origins and the fork paths are server-derived, so nothing stored can be option-shaped today. This lands as sink-side hardening and is framed that way. The three eprintln! reroutes through emit_warning do strip control and bidi characters via sanitize_node_msg.
Not asks, recorded only. gl mirror's create-repo error path (crates/gl/src/mirror.rs:123) still prints the node's message verbatim on failure, the one sanitize_node_msg outlier among the touched files; same class as the warnings rerouted here, worth a follow-up. The remote-derived default-branch name reaches git checkout -q undelimited at crates/gl/src/clone.rs:222; if that is ever worth hardening it wants a leading-dash reject, not --, which would reinterpret the name as a pathspec. And the mirror clone still inherits the terminal via .status() (crates/gl/src/mirror.rs:95), so a source server the user points it at can write sideband and progress bytes to stderr unfiltered, the same as running git clone by hand; deliberate for progress streaming, but a quiet or sanitized mode would pay off for hostile-source runs. All three predate this PR. The fork and mirror tests pin the extracted argv helpers rather than the fork_repo/run call sites, so a future inline refactor would drop that coverage silently; the doc comments already say as much.
One process note, not a finding: the PR body's Changes list is a commit behind the diff (it omits fetch_repo, clone.rs, and the warning reroutes), worth a refresh so the description matches what lands. PR Checks reads action_required on this fork head; that is mine to approve, not yours.
euxaristia
left a comment
euxaristia
left a comment
There was a problem hiding this comment.
Re-submission of the closed #376 (argument delimiting in the git clone mirror invocations). The closed round's approach was correct; this needs the same rebase onto current sync.rs plus tests proving the delimiter actually lands in the argv (the closed round lacked them, which is why it stalled).
euxaristia
left a comment
euxaristia
left a comment
There was a problem hiding this comment.
LGTM. Re-review of head 23e7a07 confirms positional argument delimiting with -- across clone, fork, and mirror invocations is fully verified.
3 participants
Summary
Passes
--before positional path and URL arguments ingit clone --mirrorsubprocess invocations to ensure positional inputs cannot be interpreted as command-line flags.Changes
crates/gitlawb-node/src/api/repos.rs: Add--before positional arguments infork_repo.crates/gitlawb-node/src/sync.rs: Add--before positional arguments inclone_repo.crates/gl/src/mirror.rs: Add--before positional arguments ingl mirror.Test plan
cargo check --workspacegit diff HEAD --checkFixes #374
Summary by CodeRabbit
Bug Fixes
Tests
Recreated from closed PR #376 by @euxaristia (approved but unmerged). Original branch: euxaristia/node:fix/git-clone-positional-delimiter