Skip to content

fix(engine): reject path-traversal repo segments on the governor-ledger write path#8469

Merged
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-engine-repo-segment-8350
Jul 24, 2026
Merged

fix(engine): reject path-traversal repo segments on the governor-ledger write path#8469
loopover-orb[bot] merged 1 commit into
JSONbored:mainfrom
RealDiligent:fix/critical-issue-engine-repo-segment-8350

Conversation

@RealDiligent

Copy link
Copy Markdown
Contributor

Summary

packages/loopover-engine/src/governor-ledger.ts's normalizeOptionalRepoFullName validated only "exactly two non-empty segments", so a traversal value normalized through unchanged:

appendGovernorEvent({ eventType: "denied", repoFullName: "../evilrepo", ... });
// owner ".." and repo "evilrepo" both pass the non-empty/one-slash check
// -> written into governor_events.repo_full_name

This is a fresh instance of the #5831 / #7525 / #7795 vulnerability class on the one write path none of them touched. Those issues hardened the parsers in packages/loopover-miner/lib/** (which share repo-clone.ts's isValidRepoSegment), and miner-lib's governor-ledger.ts even has its own correctly-guarded copy — but that copy only serves readGovernorEvents/readGovernorDecisions/purgeByRepo. appendGovernorEvent, the function that performs the SQLite INSERT, delegates to normalizeGovernorLedgerEvent from @loopover/engine — the unguarded copy. Read and purge were protected; the write was not.

Fix. The guard is restated locally inside the engine package, per the issue's required pattern — the engine must not import from the miner package (miner depends on engine, not the reverse). It matches isValidRepoSegment's semantics exactly: /^[A-Za-z0-9._-]+$/ and rejecting a bare . or .. segment, throwing the same invalid_repo_full_name the function already throws for other invalid shapes. The exported signature, return type, and undefined/null passthrough are unchanged, and miner-lib's own guarded copy is untouched.

Closes #8350

Tests — and why there are two files

The issue asks for a node --test regression in packages/loopover-engine/test/, which I added (3 tests: traversal segments, out-of-charset segments, and the still-accepted legitimate slugs + null passthrough).

That file alone would not satisfy codecov/patch, though: the engine's node --test suite runs against dist/, so it is invisible to Codecov, which measures packages/loopover-engine/src/**. I therefore also extended the existing root vitest suite test/unit/governor-ledger.test.ts, which imports the engine source directly (../../packages/loopover-engine/src/governor-ledger) — that is what actually exercises the changed lines under coverage.

Verified the tests genuinely pin the fix rather than merely passing alongside it: deleting the new guard line makes the root vitest regression fail; restoring it makes all 9 pass. Cases covered: ../evilrepo, acme/.., ./acme, acme/., a space, ;, a `` control char — plus acme/widgets, `acme-co/my_widget.js`, `a/b` still normalizing (dots, dashes and underscores are legal in real slugs and must not regress).

Scope

  • The PR title follows type(scope): short summary Conventional Commit format, for example fix(api): restore profile access checks.
  • This PR is focused and does not mix unrelated backend, UI, MCP, docs, dependency, and deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • I linked a currently open issue this PR resolves (e.g. Closes #123) — a linked open issue is required for every contributor PR.

Validation

  • git diff --check
  • npm run actionlint
  • npm run typecheck
  • npm run test:coverage locally; codecov/patch requires ≥99% coverage of the lines AND branches you changed (aim for 100% on your diff so CI variance does not fail near the threshold). Global coverage is a non-blocking trend with a loose 90% backstop, not the gate.
  • npm run test:workers
  • npm run build:mcp
  • npm run test:mcp-pack
  • npm run ui:openapi:check
  • npm run ui:lint
  • npm run ui:typecheck
  • npm run ui:build
  • npm audit --audit-level=moderate
  • New or changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries

If any required check was skipped, explain why:

  • This diff touches one engine source file and two test files, so actionlint (no workflow change), build:mcp/test:mcp-pack (no MCP change), ui:* (no UI/OpenAPI change), test:workers (no worker change), and npm audit (no dependency change) are not exercised by it.
  • Verified locally: the root vitest suite passes (9 tests), the new engine node --test file passes (3 tests) against a fresh npm run build --workspace @loopover/engine, root tsc --noEmit is clean, and git diff --check is clean.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • Auth, cookie, CORS, GitHub App, Cloudflare, or session changes include negative-path tests.
  • API/OpenAPI/MCP behavior is updated and tested where needed.
  • UI changes use live API data or real empty/error/loading states, not production mock/demo fallbacks.
  • Visible UI changes include a UI Evidence section below with JPG/JPEG or PNG screenshots arranged as organized, captioned, clickable thumbnails. SVG screenshots are not used as review evidence. Review-only screenshots or recordings are not committed to the repository.
  • Public docs/changelogs are updated where needed; changelogs are only edited for release-prep PRs.

UI Evidence

Not applicable — an input-validation guard in packages/loopover-engine; no visible UI, frontend, docs, or extension change.

Notes

@RealDiligent
RealDiligent requested a review from JSONbored as a code owner July 24, 2026 14:18
@superagent-security

Copy link
Copy Markdown
Contributor

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

…er write path

normalizeOptionalRepoFullName in the engine only checked for exactly two
non-empty segments, so "../evilrepo" normalized unchanged (owner "..", repo
"evilrepo") and was persisted by appendGovernorEvent's SQLite INSERT. The
miner-lib siblings already reject this class via repo-clone.ts's
isValidRepoSegment (JSONbored#5831/JSONbored#7525/JSONbored#7795), and miner-lib's own guarded copy covers
its read/purge paths -- but the write path delegates to this engine function,
which never got the guard.

Restates the guard locally (the engine must not import from the miner package,
which depends on it) with isValidRepoSegment's exact semantics: /^[A-Za-z0-9._-]+$/
plus rejecting a bare "."/".." segment. Signature, return type, and the
null-passthrough are unchanged.

Closes JSONbored#8350
@codecov

codecov Bot commented Jul 24, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.49%. Comparing base (7f5550b) to head (4c0859b).
⚠️ Report is 17 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #8469       +/-   ##
===========================================
+ Coverage   80.86%   92.49%   +11.63%     
===========================================
  Files         791      791               
  Lines       79320    79333       +13     
  Branches    23954    23960        +6     
===========================================
+ Hits        64142    73382     +9240     
+ Misses      12156     4807     -7349     
+ Partials     3022     1144     -1878     
Flag Coverage Δ
shard-1 55.47% <ø> (-2.90%) ⬇️
shard-2 49.89% <ø> (-1.21%) ⬇️
shard-3 56.86% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/loopover-engine/src/governor-ledger.ts 100.00% <ø> (ø)

... and 241 files with indirect coverage changes

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 24, 2026
@loopover-orb

loopover-orb Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Tip

✅ LoopOver review result - approve/merge recommended

Review updated: 2026-07-24 14:59:38 UTC

3 files · 1 AI reviewer · no blockers · readiness 93/100 · CI green · clean

✅ Suggested Action - Approve/Merge

  • safe to merge

Review summary
This PR closes a real gap: the engine-side `normalizeOptionalRepoFullName` (the function backing `appendGovernorEvent`'s SQLite INSERT via `normalizeGovernorLedgerEvent`) previously accepted any two non-empty slash-separated segments, letting `../evilrepo` persist unchanged. The fix duplicates miner-lib's `isValidRepoSegment` semantics locally (charset + bare-dot/dot-dot rejection) since the engine cannot import from miner, and is verified against the actual call chain shown in `governor-ledger.ts` (miner-lib). Tests are added in both a real `node --test` file (engine's own dist-based suite) and a vitest regression in `test/unit/governor-ledger.test.ts` exercising the exact traversal, invalid-charset, and legitimate-slug cases, with all CI checks passing.

Nits — 5 non-blocking
  • The `isValidRepoSegment` logic is now duplicated verbatim in two packages (packages/loopover-engine/src/governor-ledger.ts and packages/loopover-miner/lib/repo-clone.ts) with only a comment tying them together — consider extracting to a tiny shared zero-dependency package if a third copy ever appears.
  • The control-character case (`acme/wid\u0000gets`) is tested in `test/unit/governor-ledger.test.ts` but not in the `packages/loopover-engine/test/governor-ledger.test.ts` node:test file — minor asymmetry between the two suites.
  • The inline comments in governor-ledger.ts:39-45 are fairly long for a small guard; could be trimmed once the historical context is captured in the PR description/issue.
  • Consider factoring `REPO_SEGMENT_PATTERN`/`isValidRepoSegment` into a tiny shared internal utility package if a third instance of this duplication emerges (engine, miner-lib, and any future package).
  • Add the control-character case to packages/loopover-engine/test/governor-ledger.test.ts for parity with the vitest suite.

Decision drivers

  • ✅ Code review — No blockers (1 reviewer)
  • ✅ Gate result — Passing (No configured blocker found.)
Context & advisory signals — never blocks the verdict
Signal Result Evidence
Linked issue ✅ Linked #8350
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (1 linked issue).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 361 registered-repo PR(s), 142 merged, 36 issue(s).
Contributor context ✅ Confirmed Gittensor contributor RealDiligent; Gittensor profile; 361 PR(s), 36 issue(s).
Improvement ✅ Minor risk: clean · value: minor · LLM: moderate
Linked issue satisfaction

Addressed
The diff adds a local isValidRepoSegment/REPO_SEGMENT_PATTERN guard directly in packages/loopover-engine/src/governor-ledger.ts's normalizeOptionalRepoFullName matching the required regex and '.'/'..' rejection semantics without importing from the miner package, and includes regression tests covering traversal, invalid-character, and control-character segments while confirming legitimate slugs sti

Review context
  • Author: RealDiligent
  • Role context: outside_contributor
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 361 PR(s), 36 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Start here: Triage stale or unlinked PRs.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.
🧪 Chat with LoopOver

Ask 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.

  • @loopover ask &lt;question&gt; answers contribution-quality Q&A with source citations and freshness.
  • @loopover chat &lt;question&gt; answers in natural prose from cached decision-pack facts via local inference (maintainer/collaborator; read-only).
  • A plain-language @loopover mention with a real question is routed to the closest matching read-only command automatically — no exact syntax required.

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.

  • Re-run LoopOver review

@loopover-orb loopover-orb Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LoopOver approves — the gate is satisfied and CI is green.

@loopover-orb
loopover-orb Bot merged commit 434aada into JSONbored:main Jul 24, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

governor-ledger.ts's engine-side normalizeOptionalRepoFullName skips the #7525 path-safety guard its own miner-lib sibling has

1 participant