diff --git a/packages/loopover-engine/src/index.ts b/packages/loopover-engine/src/index.ts index 29272ae686..1a88231c50 100644 --- a/packages/loopover-engine/src/index.ts +++ b/packages/loopover-engine/src/index.ts @@ -919,3 +919,8 @@ export { generateAnonSecret, hmacAnonymize } from "./telemetry/anonymize.js"; // Pure PR-target-key parser (#4882) -- parses `"/#"` into its parts; extracted so the // D1-heavy repositories access layer no longer carries this stranded pure logic. export { parsePullRequestTargetKey } from "./parse-pull-request-target-key.js"; + +// Local-branch scenario-input safety guard (#8884) -- shared by the backend scenario-input model and the +// loopover-mcp production local-branch collector so both enforce the same forbidden-source-upload-key / +// oversized-content, metadata-only contract on the real collection path. +export { assertScenarioLocalBranchInputSafe } from "./scenario-input-safety.js"; diff --git a/packages/loopover-engine/src/scenario-input-safety.ts b/packages/loopover-engine/src/scenario-input-safety.ts new file mode 100644 index 0000000000..6e87736465 --- /dev/null +++ b/packages/loopover-engine/src/scenario-input-safety.ts @@ -0,0 +1,40 @@ +// Shared local-branch scenario-input safety guard. Lives in the engine so BOTH the LoopOver backend +// (src/scenarios/input-model.ts re-exports it) and the loopover-mcp production local-branch collector +// (packages/loopover-mcp/lib/local-branch.ts) enforce the exact same metadata-only contract -- the +// forbidden-source-upload-key / oversized-content scan is the documented safety mechanism and must run +// on the real collection path, not just its own unit test. +const FORBIDDEN_SOURCE_UPLOAD_KEYS = + /^(?:sourceContent|sourceContents|fileContent|fileContents|rawSource|rawSourceContent|content|contents|diff|patch|rawDiff)$/i; + +export function assertScenarioLocalBranchInputSafe(payload: Record): void { + if (/^(1|true|yes)$/i.test(String(process.env.LOOPOVER_UPLOAD_SOURCE ?? "false"))) { + throw new Error("LOOPOVER_UPLOAD_SOURCE=true is not supported; scenario inputs remain metadata-only."); + } + for (const key of Object.keys(payload)) { + if (FORBIDDEN_SOURCE_UPLOAD_KEYS.test(key)) { + throw new Error(`Refusing scenario local-branch field ${key}; source contents are never uploaded.`); + } + } + const changedFiles = payload.changedFiles; + // #8328: a present-but-non-array changedFiles (a plain object like { diff: "…source…" }, a string, a number) + // is not the documented array-of-entries shape, and the Array.isArray guard below would silently skip the + // entire forbidden-key/oversize scan for it — letting exactly the source content this validator exists to + // refuse slip through unchecked. Reject it outright; an omitted / undefined changedFiles stays allowed. + if (changedFiles !== undefined && !Array.isArray(changedFiles)) { + throw new Error("Refusing non-array changedFiles; an array of file entries is required."); + } + if (Array.isArray(changedFiles)) { + for (const entry of changedFiles) { + if (!entry || typeof entry !== "object") continue; + for (const nestedKey of Object.keys(entry as Record)) { + if (FORBIDDEN_SOURCE_UPLOAD_KEYS.test(nestedKey)) { + throw new Error(`Refusing changedFiles.${nestedKey}; source contents are never uploaded.`); + } + const value = (entry as Record)[nestedKey]; + if (typeof value === "string" && value.length > 4000) { + throw new Error("Refusing oversized changedFiles payload; metadata-only paths are required."); + } + } + } + } +} diff --git a/packages/loopover-mcp/lib/local-branch.ts b/packages/loopover-mcp/lib/local-branch.ts index b236d92995..cd579ce494 100644 --- a/packages/loopover-mcp/lib/local-branch.ts +++ b/packages/loopover-mcp/lib/local-branch.ts @@ -2,6 +2,7 @@ import { execFileSync } from "node:child_process"; import { realpathSync } from "node:fs"; import { isAbsolute, join, relative, resolve } from "node:path"; import { fileURLToPath } from "node:url"; +import { assertScenarioLocalBranchInputSafe } from "@loopover/engine"; import { isCodeFile, isTestPath as isTestFile } from "@loopover/engine/signals/test-evidence"; import { redactLocalPath } from "./redact-local-path.js"; @@ -122,7 +123,10 @@ export function collectLocalDiff(cwd: string, baseRef: string, workspaceRoots?: } export function collectLocalBranchMetadata(input: CollectLocalBranchMetadataInput): LocalBranchMetadata { - assertSourceUploadDisabled(); + // #8884: run the shared forbidden-source-upload-key / oversized-content scan on the real collection + // entry point (not just the old env-flag-only assertSourceUploadDisabled check) so any source-content + // field smuggled into the metadata-only scenario input is refused before anything is collected. + assertScenarioLocalBranchInputSafe(input); const workspace = resolveWorkspaceCwd(input); const cwd = workspace.cwd; const baseRef = input.baseRef ?? defaultBaseRef(cwd); @@ -704,12 +708,6 @@ function splitCommand(command: unknown): string[] { return String(command).match(/(?:[^\s"]+|"[^"]*")+/g)?.map((part) => part.replace(/^"|"$/g, "")) ?? []; } -function assertSourceUploadDisabled(): void { - if (/^(1|true|yes)$/i.test(process.env.LOOPOVER_UPLOAD_SOURCE ?? "false")) { - throw new Error("LOOPOVER_UPLOAD_SOURCE=true is not supported in v1; local MCP sends metadata only."); - } -} - // Word-boundary the closing keywords (as the server-side extractors in src/db/repositories.ts and // src/signals/engine.ts already do) so a keyword embedded in a longer word does not spuriously link an // issue: without \b, `hotfix 5` / `prefixes 12` matched the `fix`/`fixes` substring and captured the diff --git a/src/scenarios/input-model.ts b/src/scenarios/input-model.ts index 20d2f87542..d7bcfb13c2 100644 --- a/src/scenarios/input-model.ts +++ b/src/scenarios/input-model.ts @@ -1,6 +1,11 @@ import { z } from "zod"; import { sanitizePublicComment } from "../github/commands"; +// #8884: the local-branch scenario-input safety guard now lives in @loopover/engine so the loopover-mcp +// production collector (packages/loopover-mcp/lib/local-branch.ts) can share the exact same scan instead +// of a weaker env-flag-only check. Re-exported here to keep this module's public surface unchanged. +export { assertScenarioLocalBranchInputSafe } from "@loopover/engine"; + export const SCENARIO_INPUT_VERSION = 1 as const; export const SCENARIO_MAX_REPO_FULL_NAME_CHARS = 200; export const SCENARIO_MAX_BRANCH_REF_CHARS = 200; @@ -33,9 +38,6 @@ export type ScenarioSignalSource = (typeof scenarioSignalSources)[number]; export const FORBIDDEN_PUBLIC_LANGUAGE = /wallet|hotkey|coldkey|mnemonic|seed phrase|payout|estimated[-\s]?rewards?|rewards?|reward[-\s]?estimate|rankings?|farming|raw trust|trust[-\s]?score|scoreability|private[-\s]?reviewability|public[-\s]?score[-\s]?(?:estimate|prediction)/i; -const FORBIDDEN_SOURCE_UPLOAD_KEYS = - /^(?:sourceContent|sourceContents|fileContent|fileContents|rawSource|rawSourceContent|content|contents|diff|patch|rawDiff)$/i; - const scenarioSignalEntrySchema = z .object({ id: z.string().min(1).max(120), @@ -187,39 +189,6 @@ export function buildScenarioInput(args: { }); } -export function assertScenarioLocalBranchInputSafe(payload: Record): void { - if (/^(1|true|yes)$/i.test(String(process.env.LOOPOVER_UPLOAD_SOURCE ?? "false"))) { - throw new Error("LOOPOVER_UPLOAD_SOURCE=true is not supported; scenario inputs remain metadata-only."); - } - for (const key of Object.keys(payload)) { - if (FORBIDDEN_SOURCE_UPLOAD_KEYS.test(key)) { - throw new Error(`Refusing scenario local-branch field ${key}; source contents are never uploaded.`); - } - } - const changedFiles = payload.changedFiles; - // #8328: a present-but-non-array changedFiles (a plain object like { diff: "…source…" }, a string, a number) - // is not the documented array-of-entries shape, and the Array.isArray guard below would silently skip the - // entire forbidden-key/oversize scan for it — letting exactly the source content this validator exists to - // refuse slip through unchecked. Reject it outright; an omitted / undefined changedFiles stays allowed. - if (changedFiles !== undefined && !Array.isArray(changedFiles)) { - throw new Error("Refusing non-array changedFiles; an array of file entries is required."); - } - if (Array.isArray(changedFiles)) { - for (const entry of changedFiles) { - if (!entry || typeof entry !== "object") continue; - for (const nestedKey of Object.keys(entry as Record)) { - if (FORBIDDEN_SOURCE_UPLOAD_KEYS.test(nestedKey)) { - throw new Error(`Refusing changedFiles.${nestedKey}; source contents are never uploaded.`); - } - const value = (entry as Record)[nestedKey]; - if (typeof value === "string" && value.length > 4000) { - throw new Error("Refusing oversized changedFiles payload; metadata-only paths are required."); - } - } - } - } -} - export function scenarioInputFromLocalBranchMetadata(args: { scenarioType: ScenarioType; login: string; diff --git a/test/unit/local-branch.test.ts b/test/unit/local-branch.test.ts index d95b927c67..27107f2d5c 100644 --- a/test/unit/local-branch.test.ts +++ b/test/unit/local-branch.test.ts @@ -1980,6 +1980,18 @@ describe("local MCP git metadata collection", () => { expect(() => collectLocalBranchMetadata({ cwd: tempDir, baseRef: "HEAD", login: "oktofeesh1" })).toThrow(/not supported/); }); + it("refuses forbidden source-upload fields and oversized changedFiles on the production collector (#8884)", async () => { + const { collectLocalBranchMetadata } = await import("../../packages/loopover-mcp/lib/local-branch.js"); + delete process.env.LOOPOVER_UPLOAD_SOURCE; + // The scan runs before any git access, so a bare forbidden-key input is rejected outright -- no repo needed. + expect(() => collectLocalBranchMetadata({ login: "oktofeesh1", sourceContent: "secret source" } as never)).toThrow(/never uploaded/i); + expect(() => collectLocalBranchMetadata({ login: "oktofeesh1", changedFiles: [{ path: "a.ts", diff: "code" }] } as never)).toThrow(/never uploaded/i); + expect(() => collectLocalBranchMetadata({ login: "oktofeesh1", changedFiles: [{ path: "a.ts", body: "x".repeat(5000) }] } as never)).toThrow( + /oversized changedFiles/i, + ); + expect(() => collectLocalBranchMetadata({ login: "oktofeesh1", changedFiles: { diff: "x".repeat(5000) } } as never)).toThrow(/non-array changedFiles/i); + }); + it("selects and validates cwd from MCP roots without leaking local paths", async () => { const { collectLocalBranchMetadata, normalizeMcpWorkspaceRoots, resolveWorkspaceCwd } = await import("../../packages/loopover-mcp/lib/local-branch.js"); tempDir = mkdtempSync(join(tmpdir(), "loopover-local-")); diff --git a/test/unit/scenario-input-model.test.ts b/test/unit/scenario-input-model.test.ts index 48d984d715..d194765986 100644 --- a/test/unit/scenario-input-model.test.ts +++ b/test/unit/scenario-input-model.test.ts @@ -4,7 +4,6 @@ import { SCENARIO_MAX_LINKED_ISSUE_NUMBERS, SCENARIO_MAX_REPO_FULL_NAME_CHARS, SCENARIO_MAX_SIGNAL_DETAIL_CHARS, - assertScenarioLocalBranchInputSafe, buildScenarioInput, createScenarioSignalEntry, normalizeScenarioInput, @@ -13,6 +12,10 @@ import { serializeScenarioInputPrivate, serializeScenarioInputPublic, } from "../../src/scenarios/input-model"; +// Import the guard from its engine-src home (#8884) so v8 attributes coverage to +// packages/loopover-engine/src/scenario-input-safety.ts rather than the built dist copy reached via the +// @loopover/engine barrel. src/scenarios/input-model.ts re-exports the same symbol for the public surface. +import { assertScenarioLocalBranchInputSafe } from "../../packages/loopover-engine/src/scenario-input-safety"; const FORBIDDEN_PUBLIC_LANGUAGE = /wallet|hotkey|coldkey|mnemonic|seed phrase|payout|estimated[-\s]?rewards?|rewards?|reward[-\s]?estimate|rankings?|farming|raw trust|trust[-\s]?score|scoreability|private[-\s]?reviewability|public[-\s]?score[-\s]?(?:estimate|prediction)/i;