fix(security): harden git argv against second-order command-line injection#8
Merged
Conversation
6 tasks
…ction Source-descriptor fields flow into the `git` argv via `execFileSync`: `repo`/`url` reach `git clone`/`git fetch` and `ref`/`sha` reach `git checkout --detach <wanted>`. `execFileSync` avoids a shell, but `git` itself treats a leading-`-` argument as an option (e.g. a `ref` of `--upload-pack=<cmd>`), so an option-like value is a second-order argument injection (CodeQL js/second-order-command-line-injection, 3 high alerts in src/resolve.ts). Extend the `reqArg` guard (reject a leading `-`) to every source field that reaches the git argv, applied in `normalizeSource` before any git invocation: - `reqArg` on github.repo, git.url, git-subdir.url, url.url - new `optArg` on the optional ref/sha of every git variant (rejected rather than escaped, since `git checkout` does not accept `--` cleanly before a ref) `resolveSource` already calls `normalizeSource` first, so the guard runs ahead of clone/fetch/checkout for all three alert sites. Spec: add FR-004-CON-3 + FR-004-AC-8 (argv injection guard) and TC-022 rows; src/** stays at 100% coverage; quire validate clean. Closes #6 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two bypasses of the second-order command-line-injection guard found in
review:
F1: reqArg validated the RAW field, but toGitUrl does raw.trim() before
the value reaches `git clone`/`git fetch`. A leading-whitespace-then-dash
payload (e.g. " --upload-pack=… ext://x") passed the raw startsWith("-")
check yet trimmed to a git option at the argv (and toGitUrl passed it
through unwrapped on its `://`). Affected github.repo, git.url,
git-subdir.url. Fix: guard the TRIMMED value so the value validated is
the value that reaches the argv.
F2: git-subdir.path used req (non-empty only), so an option-like path
("--stdin", "-X") reached `git sparse-checkout set <path>` as a flag.
Fix: validate it with reqArg too.
Adds TC-023 (trim-bypass on repo/url) and TC-024 (option-like subdir
path); updates FR-004-CON-3 + AC-9/AC-10 and the test matrix. 100% src
coverage retained.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kreneskyp
force-pushed
the
fix/git-argv-hardening
branch
from
533e156 to
779d0f4
Compare
5 tasks
Reconcile the git argv-injection hardening (PR #8) with the npm source resolution that landed on main (PR #3). - src/sources.ts: unify to one trimmed `reqArg` guard used on both the npm `package` field (#3) and the git argv fields (#8: repo/url/path + optArg on ref/sha). - spec/functional/FR-004: keep #3's npm ACs/CONs; renumber #8's additions after the highest existing id — CON-3→CON-5, AC-8/9/10→AC-14/15/16. - spec/tests.md: renumber #8's git TCs after the npm block — TC-022/023/024→TC-029/030/031; update FR-coverage, Test Index, Constraint Boundary, Error-Path tables and coverage counts. - tests/index.test.ts: update the three guard tests' TC tags to TC-029/030/031 so spec and code agree 1:1. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
kreneskyp
added a commit
that referenced
this pull request
Jun 28, 2026
… + fix CodeQL Integrate main's npm-source-resolution (PR #3) and git-security (PR #8) work with the plugin-discovery feature (search.ts). Resolved conflicts in spec/spec.md, spec/non-functional/NFR-003, and spec/tests.md so both main's npm/git content and the discovery content survive. Renumber the discovery test cases off main's TC-001..TC-031 block: the contiguous discovery block TC-022..TC-060 shifts +10 to TC-032..TC-070 (uniform map, relative order preserved). Updated every occurrence across tests.md (FR/NFR coverage, Test Index, Constraint Boundary, Error-Path, Edge Cases, coverage-summary prose), the discovery FR files (FR-008..FR-012), and the TC tags in tests/search.test.ts. FR-004 (main) left intact. Fix 4 HIGH CodeQL js/incomplete-url-substring-sanitization alerts in tests/search.test.ts by replacing url.includes("<host>.com") routing checks with strict `new URL(url).host === "<host>"` comparisons. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This was referenced Jun 28, 2026
Merged
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 #6. Resolves the 3 high-severity CodeQL
js/second-order-command-line-injectionalerts insrc/resolve.ts(pre-existing onmain, in the git resolver — not from the discovery/npm features).Problem
Source-descriptor fields (
repo,url,ref,sha) flow into thegitargv viaexecFileSync. That avoids a shell, butgitinterprets a leading--value as an option (e.g. arefof--upload-pack=<cmd>or--output=…) — second-order argument injection.Fix
src/sources.ts: added thereqArgguard (rejects leading-; mirrors the PR feat: resolve npm sources via npm pack #3 precedent, not yet on main) + a newoptArgfor optional fields. InnormalizeSource:reqArgongithub.repo,git.url,git-subdir.url,url.url;optArgonref/shafor every git variant. Rejection (not an argv--separator), sincegit checkoutdoesn't accept--cleanly before a ref.normalizeSourceruns before any git invocation (clone/fetch/checkout) — confirmed and preserved.FR-004argument-injection Behavior note,FR-004-CON-3,FR-004-AC-8; matrix rows + TC-022 inspec/tests.md.Gates
src/**100% coverage. TC-022 written Red-first (confirmed failing pre-fix).quire validateclean (pre-existing duplicate-archetype warnings only).🤖 Generated with Claude Code