Skip to content

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

Description

@JSONbored

Context

packages/loopover-engine/src/governor-ledger.ts's normalizeOptionalRepoFullName (lines 39-45) is the
engine-package normalizer used inside normalizeGovernorLedgerEvent — the function that validates a
governor-ledger row before it is persisted. It only checks that repoFullName.split("/") yields exactly two
non-empty segments:

function normalizeOptionalRepoFullName(repoFullName: unknown): string | null {
  if (repoFullName === undefined || repoFullName === null) return null;
  if (typeof repoFullName !== "string") throw new Error("invalid_repo_full_name");
  const [owner, repo, extra] = repoFullName.trim().split("/");
  if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name");
  return `${owner}/${repo}`;
}

This is the exact unguarded shape issues #5831 and #7525 already fixed across ten sibling parsers in
packages/loopover-miner/lib/** (claim-ledger.ts, governor-state.ts, governor-ledger.ts,
governor-ledger-cli.ts, portfolio-queue.ts, portfolio-queue-cli.ts, worktree-allocator.ts, etc.), and
#7795 extended the same fix to 4 more. The fixed pattern additionally rejects a ./../control-char segment
via isValidRepoSegment/REPO_SEGMENT_PATTERN (packages/loopover-miner/lib/repo-clone.ts:72-80:
/^[A-Za-z0-9._-]+$/ combined with rejecting a bare ./.. segment) before a value can be persisted to
SQLite or echoed back through a CLI.

packages/loopover-miner/lib/governor-ledger.ts DOES have its own locally-defined, correctly-guarded
normalizeOptionalRepoFullName (lines 81-89, with the #7525 guard) — but that guarded copy is only used for
readGovernorEvents/readGovernorDecisions/purgeByRepo (lines 206, 214, 225). The function that actually
performs the SQLite INSERT, appendGovernorEvent (line 191-192), calls normalizeGovernorLedgerEvent
imported from @loopover/engine instead — which resolves to the unguarded engine copy shown above, not
the miner-lib's own guarded one. So the read/purge paths are protected, but the write path is not:

appendGovernorEvent({
  eventType: "denied",
  repoFullName: "../evilrepo",
  actionClass: "open_pr",
  decision: "deny",
  reason: "x",
});
// normalizes to "../evilrepo" unchanged (owner="..", repo="evilrepo" both pass the
// "non-empty, exactly one slash" check) and is written into governor_events.repo_full_name —
// exactly the value class #7525's own rationale says must never reach persistence.

This is a genuinely new instance of the #5831/#7525/#7795 vulnerability class, on the one write path none of
those three issues touched (they fixed packages/loopover-miner/lib/** files directly; this bug lives in the
packages/loopover-engine package those files delegate to for writes). The engine package cannot import
isValidRepoSegment from packages/loopover-miner/lib/repo-clone.ts (wrong dependency direction — miner
depends on engine, not the reverse), so the fix must add an equivalent, self-contained guard directly inside
the engine's own governor-ledger.ts.

Requirements

⚠️ Required pattern. Add a local, self-contained guard inside
packages/loopover-engine/src/governor-ledger.ts — do NOT import anything from
packages/loopover-miner/lib/repo-clone.ts (the engine package must not depend on the miner package). The
guard must reject exactly the same value class isValidRepoSegment does: a segment that is not entirely
[A-Za-z0-9._-] characters, OR a segment that is exactly . or ... Match REPO_SEGMENT_PATTERN's regex
(/^[A-Za-z0-9._-]+$/) and the path-traversal check (segment === "." || segment === "..") exactly —
matching packages/loopover-miner/lib/repo-clone.ts:72-80's semantics is the acceptance bar, not just "some
validation."

  • Add the guard to normalizeOptionalRepoFullName in packages/loopover-engine/src/governor-ledger.ts so
    both owner and repo are validated before the function returns, in addition to the existing
    "exactly two non-empty segments" check — reject with the same "invalid_repo_full_name" error the function
    already throws for other invalid shapes.
  • Do not change normalizeOptionalRepoFullName's exported signature, return type, or the null-passthrough
    behavior for undefined/null input.
  • Do not modify packages/loopover-miner/lib/governor-ledger.ts's own already-guarded
    normalizeOptionalRepoFullName (lines 81-89) — it is correct today and out of scope for this issue.

Deliverables

  • packages/loopover-engine/src/governor-ledger.ts's normalizeOptionalRepoFullName rejects a ./..
    owner or repo segment and a segment containing any character outside [A-Za-z0-9._-] (including
    control characters), matching isValidRepoSegment's semantics.
  • A regression test in packages/loopover-engine/test/governor-ledger.test.ts (create it if it does not
    already exist, following the node --test style of its siblings in that directory) asserting that
    normalizeGovernorLedgerEvent throws invalid_repo_full_name for repoFullName values like
    "../evilrepo", "owner/..", "./owner/repo"-shaped single-extra-segment cases already caught by the
    existing split check, and a segment containing a tab/newline character — while still accepting a
    normal "owner/repo" value.
  • A regression test under root test/** exercising the same fixed behavior through
    packages/loopover-miner/lib/governor-ledger.ts's appendGovernorEvent (the real call site this bug
    was found through), confirming a ./../control-char repoFullName can no longer be persisted via
    the write path — matching the existing assertion style at test/unit/miner-claim-ledger.test.ts:139.

Test Coverage Requirements

packages/loopover-engine/src/governor-ledger.ts is under coverage.include in vitest.config.ts and
subject to the 99% Codecov codecov/patch branch-counted gate — the new guard branch (and its rejection of
each invalid-character/path-traversal case) must be covered by the root-level test, not just the engine
package's own node --test suite (which, per this repo's own documented engine/Codecov coverage gap, is
invisible to Codecov). If packages/loopover-engine/test/governor-ledger.test.ts does not already exist,
adding one does not by itself satisfy Codecov — the root test/** regression test is the one that counts
toward the patch gate.

Expected Outcome

normalizeGovernorLedgerEvent's repo-full-name validation is consistent between the engine package and every
packages/loopover-miner/lib/** sibling parser #5831/#7525/#7795 already hardened — a malformed or
path-traversal-shaped repoFullName can no longer reach the governor_events.repo_full_name SQLite column
through appendGovernorEvent, closing the one write path that fell through the cracks of the earlier fixes.

Links & Resources

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions