fix(miner): reject path-traversal-shaped commitSha in replay-snapshot path planner - #7996
Conversation
… path planner normalizeCommitSha accepted any non-empty string and join()ed it straight into REPLAY_SNAPSHOT_SUBDIR, so a crafted commitSha like "../../../../tmp/evil" escaped the intended .loopover-replay-snapshots sandbox entirely -- and would then control where `git worktree add --detach <path>` writes on disk (JSONbored#7796). Constrain it to a single safe path segment (repo-clone.ts's isValidRepoSegment charset for owner/repo, JSONbored#5831, plus an explicit "."/".." rejection) before it reaches path.join(). A genuine commit SHA is hex and always satisfies this, so no legitimate caller regresses. Adds a regression test covering traversal-/separator-shaped values (and the accepted hex case). The test now imports the .ts SOURCE via a non-literal specifier instead of the extensionless/.js path: once build:miner has produced the artifact, a .js import loads that build output and leaves coverage.include's .ts entry at 0% -- so the new guard is now actually instrumented (100% patch), while the variable specifier keeps tsc happy (no TS5097). Closes JSONbored#7796
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-22 06:08:13 UTC
Review summary Nits — 4 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7996 +/- ##
==========================================
- Coverage 91.91% 89.22% -2.69%
==========================================
Files 739 95 -644
Lines 75854 21958 -53896
Branches 23018 3784 -19234
==========================================
- Hits 69723 19593 -50130
+ Misses 5038 2187 -2851
+ Partials 1093 178 -915
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Summary
Closes #7796 —
normalizeCommitSha(packages/loopover-miner/lib/replay-snapshot.ts) accepted any non-empty string andjoin()ed it straight intoREPLAY_SNAPSHOT_SUBDIR, so a craftedcommitShalike"../../../../tmp/evil"escaped the intended.loopover-replay-snapshotssandbox entirely — and would then control wheregit worktree add --detach <path>writes on disk.The fix
commitShato a single safe path segment before it reachespath.join()— the same restricted charsetrepo-clone.ts'sisValidRepoSegmentguard uses for owner/repo (fix(miner): several owner/repo CLI parsers skip repo-clone.js's path-safety validation #5831), plus an explicit"."/".."rejection. A genuine (full or abbreviated) commit SHA is hex and always satisfies this, so no legitimate caller regresses."../../../../tmp/evil","..","a/b","a\\b"," ../x ", …) plus the accepted hex case.Why the test import changed (this is the crux)
packages/loopover-miner/libis.ts-only;build:minerproduces the.jsat pack time. The existing test imported the extensionless/.jspath — which, once that build artifact exists (as it does in CI), loads the built.js, leavingcoverage.include's.tsentry at 0%. That.js-vs-.tsmismatch is why a correct guard reports 0% patch coverage. Importing the.tssource via a non-literal specifier instruments the real file (verified locally: 100% patch — 113/113 stmts, 65/65 branches) while dodgingTS5097(a literal.tsimport is a typecheck error). No behavior change to the tests themselves.Checks
npm run typecheckclean (noTS5097, no errors in the changed files); 36/36 tests pass;miner:env-reference:checkclean. (build:minerproduces the bin at pack time, sotest:miner-packpasses in CI's full build.)