Skip to content

engine: three repo-full-name normalizers skip the #7525/#8350 path-safety segment guard #9610

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

packages/loopover-engine/src/governor-ledger.ts:39-59 already carries the engine-side copy of the
repo-segment path-safety guard that closed #5831 / #7525 / #8350:

const REPO_SEGMENT_PATTERN = /^[A-Za-z0-9._-]+$/;
function isValidRepoSegment(segment: string): boolean {
  return REPO_SEGMENT_PATTERN.test(segment) && segment !== "." && segment !== "..";
}

with a comment (lines 54-56) spelling out the exact value class: "Without this, "../evilrepo"
normalized unchanged -- owner ".." and repo "evilrepo" both pass the non-empty/one-slash check --
and reached persistence, the exact value class #7525 exists to stop."

Three sibling normalizers in the same package perform the same owner/repo normalization and skip
that guard entirely:

  1. idea-intake.ts:104/^[A-Za-z0-9._-]+\/[A-Za-z0-9._-]+$/. Same character class, no
    "."/".." rejection. validateIdeaSubmission({ targetRepo: "../evilrepo" }) returns ok: true
    with targetRepo = { kind: "existing", repo: "../evilrepo" }, which buildClaimPlan (line 285)
    copies verbatim into every ClaimStep.targetRepo. The comment immediately above line 104 claims the
    opposite — "each segment a GitHub-legal slug -- an uninstallable/malformed repo is rejected at
    intake"
    . ".." is not a GitHub-legal slug. This is the renter-supplied freeform intake path,
    reachable over HTTP via POST /v1/loop/intake-idea and POST /v1/loop/plan-idea-claims
    (src/api/routes.ts), whose zod schema is deliberately loose (targetRepo: z.string().optional(),
    src/openapi/schemas.ts) precisely because "the engine's validateIdeaSubmission owns the real
    bounds/format checks"
    .
  2. discovery-index-contract.ts:128-132normalizeRepoFullName checks only "exactly one slash,
    both halves non-empty". It feeds normalizeDiscoveryIndexCandidate, which then derives owner and
    repo by slicing (lines 199-201). The module header states this data comes from "the OPTIONAL,
    only-partially-trusted hosted index"
    , and the module already hardened a different untrusted-input
    vector in discovery-index-contract.ts caps every request-side list but leaves the response candidate list unbounded #6774 (line 239).
  3. discovery-soft-claim.ts:58-63 — an identical unchecked copy, whose own JSDoc claims it
    "mirrors the discovery-index contract / claim-ledger repo validation". The claim-ledger validation
    it names (packages/loopover-miner/lib/claim-ledger.ts:108) does call isValidRepoSegment.

This is the fourth member of the same fix family: #5831 (miner CLI parsers), #7525 (governor/portfolio/
worktree stores), #8350 (governor-ledger.ts's engine-side copy) — all closed as gittensor:bug.

Requirements

  • Add a new module packages/loopover-engine/src/repo-segment.ts exporting exactly two symbols:
    • REPO_SEGMENT_PATTERN (/^[A-Za-z0-9._-]+$/)
    • isValidRepoSegment(segment: string): booleanREPO_SEGMENT_PATTERN.test(segment) && segment !== "." && segment !== "..",
      byte-for-byte the semantics of governor-ledger.ts:43-47.
  • Both symbols MUST be re-exported from packages/loopover-engine/src/index.ts in the existing
    explicit named-export barrel style.
  • governor-ledger.ts MUST delete its local REPO_SEGMENT_PATTERN / isValidRepoSegment and import
    them from the new module. Its externally observable behaviour MUST NOT change.
  • idea-intake.ts's targetRepo string branch MUST split the value on / and reject it with the
    existing target_repo_malformed error code unless there are exactly two segments and
    isValidRepoSegment returns true for both. No new error code.
  • discovery-index-contract.ts's normalizeRepoFullName MUST return null when either segment fails
    isValidRepoSegment. A candidate whose repoFullName is rejected continues to be dropped with the
    existing "DiscoveryIndexResponse dropped an invalid or boundary-violating candidate." warning — no
    new warning string.
  • discovery-soft-claim.ts's normalizeRepoFullName MUST return null when either segment fails
    isValidRepoSegment, which makes buildSoftClaimRequest return null for such a claim, exactly as
    it already does for a slashless value.
  • Do NOT touch packages/loopover-miner/lib/repo-clone.ts — the miner package's own copy stays where
    it is (the engine must not import from the miner package; see governor-ledger.ts:39-42).

⚠️ Required pattern: the new isValidRepoSegment MUST be a straight lift of
packages/loopover-engine/src/governor-ledger.ts:43-47, and the three call sites MUST follow
packages/loopover-miner/lib/repo-clone.ts:87-93's shape (split on /, reject on extra segments,
then isValidRepoSegment both halves). What does NOT satisfy this issue: leaving governor-ledger.ts
with its own duplicate copy; a fourth independently-written regex; adding validation to only one or
two of the three normalizers; introducing a new error code or warning string; or widening the
character class to add hostname/URL checks.

Deliverables

  • packages/loopover-engine/src/repo-segment.ts exists and exports REPO_SEGMENT_PATTERN and
    isValidRepoSegment; both are re-exported from packages/loopover-engine/src/index.ts.
  • packages/loopover-engine/src/governor-ledger.ts no longer declares its own copy and imports
    from ./repo-segment.js; the existing governor-ledger suite still passes unmodified.
  • validateIdeaSubmission({ id, title, body, targetRepo: "../evilrepo" }) returns
    { ok: false, errors: ["target_repo_malformed"] }, asserted by a new named regression test.
    "./x", "a/..", and "../.." are covered by the same test block; "acme/widgets" still
    returns ok: true.
  • normalizeDiscoveryIndexCandidate({ repoFullName: "../evil", issueNumber: 1, title: "x" })
    returns null, asserted by a new named regression test in the discovery-index-contract suite;
    normalizeDiscoveryIndexResponse drops that candidate and emits the existing
    invalid-candidate warning.
  • buildSoftClaimRequest({ repoFullName: "../evil", issueNumber: 1, claimedAt: "...", status: "active" })
    returns null, asserted by a new named regression test.
  • A valid "owner/repo" still round-trips unchanged through all three normalizers, asserted in
    each of the three suites.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for
example fixing idea-intake.ts (the highest-exposure path) and leaving the two discovery normalizers
unguarded, or adding the shared module without migrating governor-ledger.ts off its duplicate — does
not resolve this issue.

Test Coverage Requirements

packages/loopover-engine/src/**/*.ts is inside coverage.include in vitest.config.ts and
carries its own engine Codecov flag; the 99%+ branch-counted codecov/patch gate applies here in
full. Every new conditional needs both arms tested: isValidRepoSegment must have a passing segment,
a pattern-failing segment, a "." segment, and a ".." segment; each of the three call sites needs
both an accepted and a rejected value. The four traversal-rejection cases above are the required named
regression tests for this fix.

Expected Outcome

A ".." or "." owner/repo segment can no longer pass intake validation, survive discovery-index
candidate normalization, or be built into a soft-claim request — closing the last three engine-package
normalizers left over from the #5831#7525#8350 fix family, with one shared guard instead of a
fourth hand-written copy.

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