fix(miner): validate commit SHA is a safe path segment in replay-snapshot planner (#7796)#7992
Conversation
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #7992 +/- ##
==========================================
- Coverage 91.91% 88.75% -3.16%
==========================================
Files 739 95 -644
Lines 75850 21958 -53892
Branches 23019 3784 -19235
==========================================
- Hits 69717 19489 -50228
+ Misses 5041 2291 -2750
+ Partials 1092 178 -914
Flags with carried forward coverage won't be shown. Click here to find out more.
|
c7d5662 to
0624a03
Compare
…shot planner (JSONbored#7796) normalizeCommitSha only trimmed and non-empty-checked its input, so a crafted value such as "../../etc" flowed straight into planReplaySnapshotPath's join(repoPath, REPLAY_SNAPSHOT_SUBDIR, commitSha) and escaped the intended snapshot directory (path traversal). Constrain the SHA to a single safe path segment -- the restricted [A-Za-z0-9._-] charset plus an explicit "."/".." rejection, mirroring repo-clone.ts's isValidRepoSegment guard (JSONbored#5831). A real commit SHA is hex and always satisfies this, so no legitimate caller regresses. Adds regression tests asserting traversal payloads throw invalid_commit_sha while a real hex SHA is confined under REPLAY_SNAPSHOT_SUBDIR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Caution 🛑 LoopOver review result - fixes requiredReview updated: 2026-07-22 02:06:13 UTC
Review summary Nits — 3 non-blocking
CI checks failing
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.
|
|
LoopOver is closing this pull request on the maintainer's behalf (CI is failing (codecov/patch)). This is an automated maintenance action — to pursue this change, please open a new pull request with the issues resolved. Closed PRs may be analyzed later to improve review accuracy, but they are not automatically reopened or re-reviewed. |
Fixes #7796.
Root cause
normalizeCommitShainpackages/loopover-miner/lib/replay-snapshot.tsonly trimmed itsinput and checked it was non-empty — it applied no format validation. The value then flows
into
planReplaySnapshotPath:Because
path.joinresolves..segments, a craftedcommitShasuch as"../../etc"(or anyvalue containing a path separator) escapes the intended
REPLAY_SNAPSHOT_SUBDIR— a realpath-traversal finding. The same unchecked value is also handed to git as a bare revision argument.
Fix approach
Constrain the commit SHA to a single safe path segment, reusing the exact pattern the repo
already established for owner/repo names in
repo-clone.ts'sisValidRepoSegment(#5831): therestricted
[A-Za-z0-9._-]charset plus an explicit"."/".."rejection. A genuine commit SHAis hexadecimal and always satisfies this, so no legitimate caller regresses.
The guard is defined locally rather than imported from
repo-clone.tsto avoid pulling thatmodule's heavier dependency graph (
process-lifecycle,worktree-allocator) into the snapshotpath for a two-line check; a comment cross-references the canonical definition.
Impact
../../etc,..,.,a/b,a\b, …)throws
invalid_commit_sha, and that a real 40-char hex SHA is confined underREPLAY_SNAPSHOT_SUBDIR.Risk / tradeoffs
Minimal and backward-compatible. The charset intentionally matches the existing repo-segment guard
(rather than strict hex) so the change is purely additive safety — it rejects only path-unsafe
values, never any string a real SHA could take. Scope is limited to the two files.