From 308103a14c8c4eeb009c6e0fce1eec1308bb6a61 Mon Sep 17 00:00:00 2001 From: nghetienhiep <13849419+nghetienhiep@users.noreply.github.com> Date: Thu, 16 Jul 2026 02:42:21 +0000 Subject: [PATCH] refactor(mcp): consolidate local-path redaction into one shared module MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The MCP CLI carried three separately-maintained local-path redactors — `redactLocalPath` (lib/local-branch.js), `redactLocalValidationPaths` and `sanitizeDiagnosticText` (bin/loopover-mcp.js) — so a single redaction fix had to be repeated and kept in sync three times. Consolidate them into packages/loopover-mcp/lib/redact-local-path.js, keeping both mechanisms the call sites need as named functions: `redactLocalPath` (regex heuristic that detects an unknown path in free text) and `redactKnownLocalPaths` (exact substring substitution of known tokens/paths). The heuristic is the strict superset of the two former regex variants, so no call site redacts less than before. All three former call sites now import from the shared module; behavior is preserved. A future redaction fix now happens in one place. Closes #6264 --- packages/loopover-mcp/bin/loopover-mcp.js | 46 ++++-------- packages/loopover-mcp/lib/local-branch.js | 10 +-- .../loopover-mcp/lib/redact-local-path.js | 55 ++++++++++++++ packages/loopover-mcp/package.json | 2 +- scripts/check-mcp-package.mjs | 2 +- test/unit/redact-local-path.test.ts | 71 +++++++++++++++++++ 6 files changed, 143 insertions(+), 43 deletions(-) create mode 100644 packages/loopover-mcp/lib/redact-local-path.js create mode 100644 test/unit/redact-local-path.test.ts diff --git a/packages/loopover-mcp/bin/loopover-mcp.js b/packages/loopover-mcp/bin/loopover-mcp.js index bffdbd6fcb..4f68ef2c6c 100755 --- a/packages/loopover-mcp/bin/loopover-mcp.js +++ b/packages/loopover-mcp/bin/loopover-mcp.js @@ -11,6 +11,7 @@ import { z } from "zod"; import { buildBranchAnalysisPayload, collectLocalDiff, collectLocalBranchMetadata, probeLocalScorer, referenceScorePreviewExample, resolveScorePreviewCommand, resolveWorkspaceCwd, sanitizeLocalScorerStatus, setupGuidanceForLocalScorer, isTestFile } from "../lib/local-branch.js"; import { formatTable } from "../lib/format-table.js"; import { argsWantJson, describeCliError, reportCliFailure } from "../lib/cli-error.js"; +import { redactKnownLocalPaths, redactLocalPath } from "../lib/redact-local-path.js"; // Read name/version from this package's own package.json (always present in any install -- // global, npx, or local -- npm ships it regardless of the "files" allowlist) instead of hand-synced @@ -3707,17 +3708,10 @@ function parseDurationMs(value) { function sanitizeValidationText(value, maxLength = 240) { const text = String(value ?? "").replace(/[\r\n\t]+/g, " ").trim(); if (!text) return undefined; - const redacted = redactPrivateValidationMetrics(redactLocalValidationPaths(text)); + const redacted = redactPrivateValidationMetrics(redactLocalPath(text)); return redacted.length <= maxLength ? redacted : `${redacted.slice(0, maxLength - 3)}...`; } -function redactLocalValidationPaths(text) { - const pathSegment = "[^\\\\/\\s\"'`,;)]+(?:\\s+[^\\\\/\\s\"'`,;)]+)*(?=[\\\\/])"; - const pathTail = "[^\\\\/\\s\"'`,;)]+"; - const localPathPattern = new RegExp(`(^|[\\s"'\\\`=])((?:~[\\\\/]|[A-Za-z]:[\\\\/]|/)(?:${pathSegment}[\\\\/])*${pathTail})`, "g"); - return text.replace(localPathPattern, (_, prefix) => `${prefix}`); -} - function redactPrivateValidationMetrics(text) { return text.replace( /\b(?:wallet|hotkey|coldkey|mnemonic|raw[-_\s]?trust|private[-_\s]?reviewability|trust[-_\s]?score)\b(?:\s*[:=]\s*(?:"[^"]*"|'[^']*'|[^\s"'`,;)]+))?/gi, @@ -3943,7 +3937,7 @@ function isForbiddenCacheKey(key) { } function sanitizeCacheString(value) { - return redactPrivateValidationMetrics(redactLocalValidationPaths(sanitizeDiagnosticText(value))); + return redactPrivateValidationMetrics(redactLocalPath(sanitizeDiagnosticText(value))); } function decisionPackCacheFiles() { @@ -4032,30 +4026,16 @@ function findExecutable(name) { } function sanitizeDiagnosticText(value, extraPaths = []) { - if (value === undefined || value === null) return value; - let text = String(value); - const sensitiveValues = [ - process.env.LOOPOVER_API_TOKEN, - process.env.LOOPOVER_MCP_TOKEN, - process.env.LOOPOVER_TOKEN, - config.session?.token, - ...profileSessions(config).map((entry) => entry.session.token), - ].filter((candidate) => typeof candidate === "string" && candidate.length > 0); - for (const token of sensitiveValues) { - text = text.split(token).join("[redacted]"); - } - const localPaths = [ - configPath, - process.env.LOOPOVER_CONFIG_PATH, - process.env.LOOPOVER_CONFIG_DIR, - process.cwd(), - homedir(), - ...extraPaths, - ].filter((candidate) => typeof candidate === "string" && candidate.length > 1); - for (const localPath of localPaths.sort((left, right) => right.length - left.length)) { - text = text.split(localPath).join("[local-path]"); - } - return text; + return redactKnownLocalPaths(value, { + tokens: [ + process.env.LOOPOVER_API_TOKEN, + process.env.LOOPOVER_MCP_TOKEN, + process.env.LOOPOVER_TOKEN, + config.session?.token, + ...profileSessions(config).map((entry) => entry.session.token), + ], + paths: [configPath, process.env.LOOPOVER_CONFIG_PATH, process.env.LOOPOVER_CONFIG_DIR, process.cwd(), homedir(), ...extraPaths], + }); } function loadConfig() { diff --git a/packages/loopover-mcp/lib/local-branch.js b/packages/loopover-mcp/lib/local-branch.js index 11b225b972..1faa522417 100644 --- a/packages/loopover-mcp/lib/local-branch.js +++ b/packages/loopover-mcp/lib/local-branch.js @@ -3,8 +3,10 @@ import { realpathSync } from "node:fs"; import { dirname, isAbsolute, join, relative, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import { isCodeFile, isTestPath as isTestFile } from "@loopover/engine/signals/test-evidence"; +import { redactLocalPath } from "./redact-local-path.js"; export { isCodeFile, isTestFile }; +export { redactLocalPath }; const packageRoot = join(dirname(fileURLToPath(import.meta.url)), ".."); @@ -211,14 +213,6 @@ export function referenceScorePreviewExample(kind = "metadata") { return `${interpreter} ./node_modules/@loopover/mcp/scripts/${script}`; } -export function redactLocalPath(value) { - const text = String(value ?? ""); - if (!text) return text; - return text - .replace(/(?:~\/|[A-Za-z]:\\)[^\s"'`,;)]+/g, "") - .replace(/(^|[\s"'`=])\/(?:[^\s"'`,;)]+(?:\/[^\s"'`,;)]+)*)/g, (_, prefix) => `${prefix}`); -} - export function redactScorerCommand(command) { const text = String(command ?? "").trim(); if (!text) return text; diff --git a/packages/loopover-mcp/lib/redact-local-path.js b/packages/loopover-mcp/lib/redact-local-path.js new file mode 100644 index 0000000000..ddd5745418 --- /dev/null +++ b/packages/loopover-mcp/lib/redact-local-path.js @@ -0,0 +1,55 @@ +// #6264: the one shared local-filesystem-path redactor for the MCP CLI. Three call sites used to +// carry their own copy of this logic (`redactLocalPath` here, `redactLocalValidationPaths` and +// `sanitizeDiagnosticText` in bin/loopover-mcp.js), so a single redaction fix had to be made — and +// kept in sync — three times. They are consolidated here so a future fix happens once. +// +// Two genuinely different mechanisms are needed, so both stay available as named functions rather +// than being forced into one: +// - `redactLocalPath` DETECTS an unknown absolute/home path in free text via a regex heuristic +// (stack traces, scorer stderr, pasted validation output) → ``. +// - `redactKnownLocalPaths` redacts KNOWN sensitive strings (session tokens, config dirs, cwd/home) +// supplied by the caller, by exact substring substitution → `[redacted]` / +// `[local-path]`. It cannot detect an arbitrary path; the heuristic cannot +// redact a token it was never told about. Each solves a distinct problem. + +/** + * Redact any absolute or home-anchored local path found in free text, replacing it with the + * `` placeholder. Heuristic (matches an unknown path by shape), so it never needs the + * concrete path in advance — the counterpart to the exact-match `redactKnownLocalPaths` below. + */ +export function redactLocalPath(value) { + const text = String(value ?? ""); + if (!text) return text; + // Both `/g` patterns are rebuilt per call so no `lastIndex` state carries between invocations. + // Delimiter-anchored roots (`~/`, `~\`, `C:\`, `C:/`, `/`) whose interior segments may contain + // spaces, e.g. `/Users/Alice Smith/project` — the anchoring prefix is preserved, only the path swaps. + const pathSegment = "[^\\\\/\\s\"'`,;)]+(?:\\s+[^\\\\/\\s\"'`,;)]+)*(?=[\\\\/])"; + const pathTail = "[^\\\\/\\s\"'`,;)]+"; + const rootedPath = new RegExp(`(^|[\\s"'\\\`=])((?:~[\\\\/]|[A-Za-z]:[\\\\/]|/)(?:${pathSegment}[\\\\/])*${pathTail})`, "g"); + return text + .replace(rootedPath, (_, prefix) => `${prefix}`) + // Home/Windows roots that appear mid-token with no leading delimiter (so the anchored pass skips + // them); run second so it only mops up what the anchored, space-aware pass could not claim. + .replace(/(?:~\/|[A-Za-z]:\\)[^\s"'`,;)]+/g, ""); +} + +/** + * Redact KNOWN sensitive strings from free text by exact substring substitution: every entry of + * `tokens` becomes `[redacted]` and every entry of `paths` becomes `[local-path]`. Non-string / + * empty entries are ignored; a token must be non-empty and a path longer than one character (a bare + * `/` is not a "known path"). Paths are applied longest-first so a nested path (e.g. cwd under home) + * is redacted before a shorter prefix would swallow its tail. `undefined`/`null` pass through + * untouched so callers can hand diagnostics straight in. + */ +export function redactKnownLocalPaths(value, { tokens = [], paths = [] } = {}) { + if (value === undefined || value === null) return value; + let text = String(value); + for (const token of tokens) { + if (typeof token === "string" && token.length > 0) text = text.split(token).join("[redacted]"); + } + const knownPaths = paths.filter((candidate) => typeof candidate === "string" && candidate.length > 1); + for (const localPath of knownPaths.sort((left, right) => right.length - left.length)) { + text = text.split(localPath).join("[local-path]"); + } + return text; +} diff --git a/packages/loopover-mcp/package.json b/packages/loopover-mcp/package.json index b7e2d9cd80..515f9ba59f 100644 --- a/packages/loopover-mcp/package.json +++ b/packages/loopover-mcp/package.json @@ -35,7 +35,7 @@ "CHANGELOG.md" ], "scripts": { - "build": "node --check bin/loopover-mcp.js && node --check lib/cli-error.js && node --check lib/local-branch.js && node --check lib/format-table.js && node --check scripts/gittensor-score-preview.mjs" + "build": "node --check bin/loopover-mcp.js && node --check lib/cli-error.js && node --check lib/local-branch.js && node --check lib/format-table.js && node --check lib/redact-local-path.js && node --check scripts/gittensor-score-preview.mjs" }, "dependencies": { "@loopover/engine": "^3.0.0", diff --git a/scripts/check-mcp-package.mjs b/scripts/check-mcp-package.mjs index a10884204c..60b46138b8 100644 --- a/scripts/check-mcp-package.mjs +++ b/scripts/check-mcp-package.mjs @@ -14,7 +14,7 @@ if (result.status !== 0) { const [pack] = JSON.parse(result.stdout); const files = pack.files.map((file) => file.path).sort(); -const allowed = [/^bin\/loopover-mcp\.js$/, /^lib\/cli-error\.js$/, /^lib\/local-branch\.js$/, /^lib\/format-table\.js$/, /^scripts\/gittensor-score-preview\.(mjs|py)$/, /^package\.json$/, /^README\.md$/, /^CHANGELOG\.md$/, /^LICENSE$/]; +const allowed = [/^bin\/loopover-mcp\.js$/, /^lib\/cli-error\.js$/, /^lib\/local-branch\.js$/, /^lib\/format-table\.js$/, /^lib\/redact-local-path\.js$/, /^scripts\/gittensor-score-preview\.(mjs|py)$/, /^package\.json$/, /^README\.md$/, /^CHANGELOG\.md$/, /^LICENSE$/]; const forbiddenPath = /(^|\/)(\.dev\.vars|\.env|\.npmrc|.*\.pem|.*private.*key.*|.*secret.*)$/i; const forbiddenContent = /(BEGIN (RSA |EC |OPENSSH )?PRIVATE KEY|github_pat_[A-Za-z0-9_]+|gh[pousr]_[A-Za-z0-9_]+|gts_[0-9a-f]{64}|[A-Z0-9_]*(TOKEN|SECRET|PRIVATE_KEY)=)/; const stalePackageText = /(private beta|zeronode\.workers\.dev|preview URL)/i; diff --git a/test/unit/redact-local-path.test.ts b/test/unit/redact-local-path.test.ts new file mode 100644 index 0000000000..1e988ea202 --- /dev/null +++ b/test/unit/redact-local-path.test.ts @@ -0,0 +1,71 @@ +import { describe, expect, it } from "vitest"; + +// #6264: the three former MCP redaction call sites now share packages/loopover-mcp/lib/redact-local-path.js. +// This is the single home for the redaction contract, so it is tested once here; the call-site tests +// (local-scorer-adapter.test.ts, mcp-cli-packets.test.ts) still assert the wired-up behavior end to end. +// @ts-expect-error package helper is plain JS because the local wrapper ships as a Node bin package. +const { redactLocalPath, redactKnownLocalPaths } = await import("../../packages/loopover-mcp/lib/redact-local-path.js"); + +describe("redactLocalPath (heuristic: detect an unknown path in free text)", () => { + it("redacts an absolute unix path, preserving any leading delimiter", () => { + expect(redactLocalPath("/home/user/proj/file.js")).toBe(""); + expect(redactLocalPath("failed under /home/user/proj/file.js")).toBe("failed under "); + expect(redactLocalPath('config="/etc/app/conf.d/main"')).toBe('config=""'); + }); + + it("redacts a path whose segments contain spaces (the space-aware validation shape)", () => { + expect(redactLocalPath("node /Users/Alice Smith/project/run.js")).toBe("node "); + }); + + it("redacts both Windows slash forms and a home-anchored path", () => { + expect(redactLocalPath("log=C:\\Users\\Alice Smith\\raw.log")).toBe("log="); + expect(redactLocalPath("C:/Users/bob/tmp/x")).toBe(""); + expect(redactLocalPath("~/secrets/key.pem")).toBe(""); + expect(redactLocalPath("~\\AppData\\Local\\thing")).toBe(""); + }); + + it("still redacts a home/Windows root that appears mid-token without a leading delimiter", () => { + const redacted = redactLocalPath("see~/private/notes here"); + expect(redacted).toContain(""); + expect(redacted).not.toContain("~/private"); + }); + + it("leaves text with no local path untouched, including a bare slash", () => { + expect(redactLocalPath("just some text, version 1.2.3")).toBe("just some text, version 1.2.3"); + expect(redactLocalPath("pass --flag / or | here")).toBe("pass --flag / or | here"); + }); + + it("coerces nullish and empty input to an empty string", () => { + expect(redactLocalPath(undefined)).toBe(""); + expect(redactLocalPath(null)).toBe(""); + expect(redactLocalPath("")).toBe(""); + }); +}); + +describe("redactKnownLocalPaths (exact substitution: redact a KNOWN token/path)", () => { + it("replaces known tokens with [redacted] and known paths with [local-path]", () => { + expect(redactKnownLocalPaths("token abc123 at /home/me/app", { tokens: ["abc123"], paths: ["/home/me/app"] })).toBe( + "token [redacted] at [local-path]", + ); + }); + + it("applies the longest known path first so a shorter prefix cannot swallow the tail", () => { + expect(redactKnownLocalPaths("/home/me/app/src/x", { paths: ["/home/me", "/home/me/app/src"] })).toBe("[local-path]/x"); + }); + + it("ignores empty tokens, one-character paths, and non-string entries", () => { + expect(redactKnownLocalPaths("keep / and x", { tokens: ["", 123 as unknown as string], paths: ["/", null as unknown as string] })).toBe( + "keep / and x", + ); + }); + + it("coerces a non-string value before substituting", () => { + expect(redactKnownLocalPaths(123, { tokens: ["2"] })).toBe("1[redacted]3"); + }); + + it("passes undefined/null through untouched and defaults its options", () => { + expect(redactKnownLocalPaths(undefined)).toBeUndefined(); + expect(redactKnownLocalPaths(null)).toBeNull(); + expect(redactKnownLocalPaths("plain text")).toBe("plain text"); + }); +});