fix(engine): reject path-traversal repo segments on the governor-ledger write path#8469
Conversation
|
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 Report✅ All modified and coverable lines are covered by tests. 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-24 14:59:38 UTC
Review summary Nits — 5 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.
|
Summary
packages/loopover-engine/src/governor-ledger.ts'snormalizeOptionalRepoFullNamevalidated only "exactly two non-empty segments", so a traversal value normalized through unchanged: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 sharerepo-clone.ts'sisValidRepoSegment), and miner-lib'sgovernor-ledger.tseven has its own correctly-guarded copy — but that copy only servesreadGovernorEvents/readGovernorDecisions/purgeByRepo.appendGovernorEvent, the function that performs the SQLiteINSERT, delegates tonormalizeGovernorLedgerEventfrom@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 sameinvalid_repo_full_namethe function already throws for other invalid shapes. The exported signature, return type, andundefined/nullpassthrough 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 --testregression inpackages/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'snode --testsuite runs againstdist/, so it is invisible to Codecov, which measurespackages/loopover-engine/src/**. I therefore also extended the existing root vitest suitetest/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 — plusacme/widgets, `acme-co/my_widget.js`, `a/b` still normalizing (dots, dashes and underscores are legal in real slugs and must not regress).Scope
type(scope): short summaryConventional Commit format, for examplefix(api): restore profile access checks.CONTRIBUTING.mdand does not reintroduce GitHub Pages, VitePress,site/, orCNAME.Closes #123) — a linked open issue is required for every contributor PR.Validation
git diff --checknpm run actionlintnpm run typechecknpm run test:coveragelocally;codecov/patchrequires ≥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:workersnpm run build:mcpnpm run test:mcp-packnpm run ui:openapi:checknpm run ui:lintnpm run ui:typechecknpm run ui:buildnpm audit --audit-level=moderateIf any required check was skipped, explain why:
actionlint(no workflow change),build:mcp/test:mcp-pack(no MCP change),ui:*(no UI/OpenAPI change),test:workers(no worker change), andnpm audit(no dependency change) are not exercised by it.node --testfile passes (3 tests) against a freshnpm run build --workspace @loopover/engine, roottsc --noEmitis clean, andgit diff --checkis clean.Safety
UI Evidencesection 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.UI Evidence
Not applicable — an input-validation guard in
packages/loopover-engine; no visible UI, frontend, docs, or extension change.Notes