Skip to content

fix(engine): extend #5831's repo-segment path-safety validation to governor/portfolio/worktree stores #7525

Description

@JSONbored

Context

packages/loopover-miner/lib/repo-clone.ts defines REPO_SEGMENT_PATTERN/isValidRepoSegment/
isPathTraversalSegment specifically to reject an owner/repo segment containing control characters,
whitespace, or a bare ./.. path-traversal segment — its own comment documents the real finding this
closed: a value like ../foo could otherwise make resolveRepoCloneDir's join(cloneBaseDir, owner, repo) escape the intended clone directory. The comment names exactly which parsers were fixed to call
isValidRepoSegment: attempt-cli.ts, claim-ledger-cli.ts, event-ledger-cli.ts, and
claim-ledger.ts (closed issue #5831), plus cross-repo-evaluation.ts (already used it independently).

That fix's own rationale — from #5831's body — was about consistency and CLI input hygiene, not only
traversal risk: "no CLI command silently accepts (and, for the claim/event ledgers, persists) a repo
identifier that would fail validation one layer further down the stack, or that never gets validated at
all." That reasoning applies unchanged to a set of sibling owner/repo parsers #5831 never touched, which
still only check "exactly one slash, both halves non-empty" and persist the result into SQLite:

// packages/loopover-miner/lib/portfolio-queue.ts:100
function normalizeRepoFullName(repoFullName: unknown): string {
  if (typeof repoFullName !== "string") throw new Error("invalid_repo_full_name");
  const trimmed = repoFullName.trim();
  const [owner, repo, extra] = trimmed.split("/");
  if (!owner || !repo || extra !== undefined) throw new Error("invalid_repo_full_name");
  return `${owner}/${repo}`;
}

versus the fixed sibling in claim-ledger.ts:

function normalizeRepoFullName(repoFullName: unknown): string {
  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");
  if (!isValidRepoSegment(owner) || !isValidRepoSegment(repo)) throw new Error("invalid_repo_full_name");
  return `${owner}/${repo}`;
}

A value like owner/.. or a repo segment containing a tab/newline is accepted today by the governor
ledger/governor state, the portfolio queue (and its CLI's --repo filter), and the worktree allocator —
persisted into SQLite and echoed back through list/get CLI output — exactly the class of input #5831
closed off for the claim/event ledgers and attempt-cli.ts. packages/loopover-engine/src/miner/deny-hook-synthesis.ts's
own normalizeRepoFullName (re-exported unchanged by the miner-lib module of the same name) has the
identical unguarded shape.

Requirements

  • Import isValidRepoSegment from packages/loopover-miner/lib/repo-clone.ts into each of the six
    parsers listed below (for the engine-side deny-hook-synthesis.ts, add the equivalent segment check —
    do not introduce a new cross-package import from @loopover/engine back into loopover-miner; either
    duplicate the same regex-based check locally in the engine module with a comment cross-referencing
    repo-clone.ts's definition, or extract a shared constant if the maintainer prefers — either is
    acceptable, but do not skip the engine-side copy).
  • After the existing "exactly one slash, both halves non-empty" check passes, additionally reject when
    either half fails isValidRepoSegment (or the equivalent local check for the engine module), throwing
    the same invalid_repo_full_name error (or, for governor-ledger-cli.ts/portfolio-queue-cli.ts's
    parseRepoArg, returning the same { error: "Repository must be in owner/repo form." } shape they
    already return for the other malformed-input branch — do not invent a different error message for this
    branch).
  • Do not touch cross-repo-evaluation.ts, attempt-cli.ts, claim-ledger-cli.ts, claim-ledger.ts,
    or event-ledger-cli.ts — those are already fixed via fix(miner): several owner/repo CLI parsers skip repo-clone.js's path-safety validation #5831 and are out of scope for this issue.

Deliverables

  • governor-state.ts's normalizeRepoFullName calls isValidRepoSegment on both segments.
  • governor-ledger.ts's normalizeOptionalRepoFullName calls isValidRepoSegment on both segments.
  • governor-ledger-cli.ts's parseRepoArg calls isValidRepoSegment on both segments.
  • portfolio-queue.ts's normalizeRepoFullName calls isValidRepoSegment on both segments.
  • portfolio-queue-cli.ts's parseRepoArg calls isValidRepoSegment on both segments.
  • worktree-allocator.ts's normalizeRepoFullName calls isValidRepoSegment on both segments.
  • packages/loopover-engine/src/miner/deny-hook-synthesis.ts's normalizeRepoFullName rejects the
    same invalid segments (./../control-chars/non-[A-Za-z0-9._-]), documented with a comment
    cross-referencing repo-clone.ts's REPO_SEGMENT_PATTERN.

Test Coverage Requirements

This repo's Codecov patch gate is 99%+, branch-counted, on every changed line/branch in src/**/workers/**
(and this package's own equivalent CI gate). Add a regression test per touched function asserting that a
repo string like "owner/..", "../repo", or a segment containing a literal tab/newline is now rejected
with invalid_repo_full_name (or the CLI's equivalent error shape), alongside the existing
"exactly-one-slash" negative cases each function's test file already has. Every new branch this adds must
be exercised by at least one true and one false case.

Expected Outcome

No owner/repo identifier containing a path-traversal segment or control character can reach any local
SQLite store or CLI-facing parser in this package without validation — the same guarantee #5831 already
gives the claim/event ledgers and attempt-cli.ts now covers governor state/ledger, portfolio queue, and
worktree allocation too.

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