Skip to content

fix(miner): reject path-traversal repo segments in the 4 remaining normalizeRepoFullName parsers - #8060

Closed
davion-knight wants to merge 1 commit into
JSONbored:mainfrom
davion-knight:fix-normalize-repo-path-guard-v2
Closed

fix(miner): reject path-traversal repo segments in the 4 remaining normalizeRepoFullName parsers#8060
davion-knight wants to merge 1 commit into
JSONbored:mainfrom
davion-knight:fix-normalize-repo-path-guard-v2

Conversation

@davion-knight

Copy link
Copy Markdown
Contributor

Problem

repo-clone.ts's isValidRepoSegment rejects a ./../control-char owner or repo segment before it's persisted to SQLite or echoed through a CLI. #5831 and #7525 rolled it out to ten sibling parsers, but four were missed and still only check "exactly one slash, both halves non-empty":

  • contribution-profile-cache.ts, prediction-ledger.ts, replay-snapshot.ts, run-state.ts

Inputs like "owner/..", "../repo", and segments with tab/newline pass these four but are correctly rejected by the already-fixed siblings — and each backs a SQLite key (*.sqlite3) and is echoed through its sibling CLI, exactly the rationale #5831/#7525 used.

Fix

Import and call isValidRepoSegment(owner)/isValidRepoSegment(repo) in all four normalizeRepoFullName functions, matching the exact call shape of the ten already-fixed siblings (claim-ledger.ts, governor-state.ts, …) — no new validation approach.

Tests

A per-file regression test drives each module's own store method (get/put, appendPrediction/readPredictions, getSnapshot, getRunState/setRunState) with "../etc" (owner-invalid) and "o/.." (repo-invalid) — covering both operands of the new || branch — plus invalid-character cases, asserting .toThrow("invalid_repo_full_name"), mirroring test/unit/miner-claim-ledger.test.ts's existing assertion. 84 tests pass across the 4 files.

Verification

Each file's new guard + import lines are covered (guard-line hits 30/25/83/47); the only uncovered lines in the report are pre-existing, unrelated to this diff. Root typecheck clean. The compiled .js are gitignored, so the diff is the 4 .ts sources + 4 test files.

Closes #7795

…rmalizeRepoFullName parsers

repo-clone.ts's isValidRepoSegment rejects a ./../control-char owner or repo segment before it is persisted to SQLite or echoed through a CLI. JSONbored#5831 and JSONbored#7525 rolled it out to ten sibling parsers, but four were missed and still only checked 'exactly one slash, both halves non-empty': contribution-profile-cache.ts, prediction-ledger.ts, replay-snapshot.ts, and run-state.ts. Inputs like 'owner/..', '../repo', or segments with tab/newline characters passed these four while being correctly rejected everywhere else.

Import and call isValidRepoSegment on both owner and repo in all four, matching the exact call shape of the ten already-fixed siblings. Adds a per-file regression test asserting a path-traversal/invalid-character segment throws invalid_repo_full_name, covering both operands, mirroring test/unit/miner-claim-ledger.test.ts.

Closes JSONbored#7795
@davion-knight
davion-knight requested a review from JSONbored as a code owner July 22, 2026 17:34
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@davion-knight

Copy link
Copy Markdown
Contributor Author

Superseded — rebasing onto current main to resolve a base overlap; a clean PR will follow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 sibling normalizeRepoFullName parsers still skip the path-safety guard #5831/#7525 already added to every other sibling

2 participants