diff --git a/apps/loopover-miner-ui/src/auth.test.ts b/apps/loopover-miner-ui/src/auth.test.ts index f704ea80f3..00e4199f26 100644 --- a/apps/loopover-miner-ui/src/auth.test.ts +++ b/apps/loopover-miner-ui/src/auth.test.ts @@ -1,6 +1,12 @@ import { describe, expect, it } from "vitest"; -import { authPlugin, handleAuthRequest, isAuthenticatedRequest } from "../vite-auth"; +import { + AUTH_BOOTSTRAP_PATH, + authPlugin, + handleAuthRequest, + handleBootstrapRequest, + isAuthenticatedRequest, +} from "../vite-auth"; const TOKEN = "deterministic-test-token"; @@ -25,6 +31,13 @@ describe("isAuthenticatedRequest (#4858)", () => { expect(isAuthenticatedRequest("loopover_miner_ui_token=wrong-value", TOKEN)).toBe(false); }); + it("rejects the auth cookie when its value is a different length than the server's token", () => { + // Exercises the length-mismatch short-circuit ahead of timingSafeEqual, which throws on unequal-length + // buffers -- a shorter or longer guess must be rejected, not crash the request handler. + expect(isAuthenticatedRequest("loopover_miner_ui_token=short", TOKEN)).toBe(false); + expect(isAuthenticatedRequest(`loopover_miner_ui_token=${TOKEN}-and-then-some`, TOKEN)).toBe(false); + }); + it("accepts the auth cookie when its value matches the server's token exactly", () => { expect(isAuthenticatedRequest(`loopover_miner_ui_token=${TOKEN}`, TOKEN)).toBe(true); }); @@ -52,15 +65,46 @@ describe("handleAuthRequest (#4858)", () => { }); }); +describe("handleBootstrapRequest (GHSA-v6v4-mh5m-5mqq)", () => { + it("falls through (null) for a request with no url", () => { + expect(handleBootstrapRequest(undefined, TOKEN)).toBeNull(); + }); + + it("falls through (null) for any path other than the bootstrap path", () => { + expect(handleBootstrapRequest("/", TOKEN)).toBeNull(); + expect(handleBootstrapRequest("/api/portfolio-queue", TOKEN)).toBeNull(); + }); + + it("redirects to / when the bootstrap path is visited with the correct token", () => { + expect(handleBootstrapRequest(`${AUTH_BOOTSTRAP_PATH}?token=${TOKEN}`, TOKEN)).toEqual({ redirectTo: "/" }); + }); + + it("returns a 404 (not a 401) for the bootstrap path with a missing token", () => { + expect(handleBootstrapRequest(AUTH_BOOTSTRAP_PATH, TOKEN)).toEqual({ status: 404, body: "Not Found" }); + }); + + it("returns a 404 for the bootstrap path with the wrong token", () => { + expect(handleBootstrapRequest(`${AUTH_BOOTSTRAP_PATH}?token=wrong`, TOKEN)).toEqual({ + status: 404, + body: "Not Found", + }); + }); +}); + type CapturedRequestHandler = ( req: { url?: string; headers: { cookie?: string } }, res: { statusCode: number; setHeader: (k: string, v: string) => void; end: (body: string) => void }, next: () => void, ) => void; -function captureMiddleware(deps = { generateToken: () => TOKEN }): CapturedRequestHandler { +function captureMiddleware( + deps: { generateToken?: () => string; logToken?: (token: string) => void } = { + generateToken: () => TOKEN, + logToken: () => {}, + }, +): CapturedRequestHandler { let captured: CapturedRequestHandler | undefined; - const plugin = authPlugin(deps); + const plugin = authPlugin({ logToken: () => {}, ...deps }); const server = { middlewares: { use: (fn: CapturedRequestHandler) => (captured = fn) } }; // @ts-expect-error -- the test double only implements the subset of Vite's ViteDevServer this plugin reads. plugin.configureServer(server); @@ -93,23 +137,21 @@ function fakeResponse() { }; } -describe("authPlugin (#4858)", () => { - it("stamps the Set-Cookie header on a non-/api/ request that falls through (the initial page load)", () => { +describe("authPlugin (#4858, hardened for GHSA-v6v4-mh5m-5mqq)", () => { + it("regression: NEVER stamps Set-Cookie on an unauthenticated, non-bootstrap request -- not even the initial page load", () => { + // This is the exact vulnerability: an earlier version stamped Set-Cookie on every response that + // wasn't itself rejected, including a plain unauthenticated `GET /`. That let ANY local process -- + // not just the intended browser -- read a valid, replayable session token off one unauthenticated + // request (`curl http://127.0.0.1:4174/`), then use it against the mutating /api/* routes. const middleware = captureMiddleware(); - const { res, headers } = fakeResponse(); - let calledNext = false; - middleware({ url: "/", headers: {} }, res, () => { - calledNext = true; - }); - expect(headers["Set-Cookie"]).toBe(`loopover_miner_ui_token=${TOKEN}; HttpOnly; SameSite=Strict; Path=/`); - expect(calledNext).toBe(true); + for (const url of ["/", "/index.html", "/assets/index.js", "/api/portfolio-queue"]) { + const { res, headers } = fakeResponse(); + middleware({ url, headers: {} }, res, () => {}); + expect(headers["Set-Cookie"], `Set-Cookie must be absent for unauthenticated ${url}`).toBeUndefined(); + } }); - it("rejects an unauthenticated /api/* request with 401, never calls next(), and NEVER leaks the token via Set-Cookie", () => { - // Regression test: an earlier version set Set-Cookie on every response BEFORE checking auth, so an - // unauthenticated caller could read the valid token straight off this 401's own headers and replay it -- - // completely defeating the mechanism. The token must only ever reach a caller that is already - // authenticated, or a non-/api/* (page/asset) request. + it("rejects an unauthenticated /api/* request with 401, never calls next(), and never leaks the token via Set-Cookie", () => { const middleware = captureMiddleware(); const { res, headers, getEnded, getStatus } = fakeResponse(); let calledNext = false; @@ -135,6 +177,44 @@ describe("authPlugin (#4858)", () => { expect(headers["Set-Cookie"]).toBe(`loopover_miner_ui_token=${TOKEN}; HttpOnly; SameSite=Strict; Path=/`); }); + it("re-stamps the cookie on an already-authenticated non-/api/ request too (keeps the session rolling)", () => { + const middleware = captureMiddleware(); + const { res, headers } = fakeResponse(); + let calledNext = false; + middleware({ url: "/", headers: { cookie: `loopover_miner_ui_token=${TOKEN}` } }, res, () => { + calledNext = true; + }); + expect(calledNext).toBe(true); + expect(headers["Set-Cookie"]).toBe(`loopover_miner_ui_token=${TOKEN}; HttpOnly; SameSite=Strict; Path=/`); + }); + + it("mints the cookie and redirects to / when the bootstrap path is visited with the correct token", () => { + const middleware = captureMiddleware(); + const { res, headers, getStatus } = fakeResponse(); + let calledNext = false; + middleware({ url: `/__auth/bootstrap?token=${TOKEN}`, headers: {} }, res, () => { + calledNext = true; + }); + expect(getStatus()).toBe(302); + expect(headers.Location).toBe("/"); + expect(headers["Set-Cookie"]).toBe(`loopover_miner_ui_token=${TOKEN}; HttpOnly; SameSite=Strict; Path=/`); + expect(calledNext).toBe(false); + }); + + it("404s the bootstrap path (never sets a cookie) when the token is missing or wrong", () => { + const middleware = captureMiddleware(); + + const missing = fakeResponse(); + middleware({ url: "/__auth/bootstrap", headers: {} }, missing.res, () => {}); + expect(missing.getStatus()).toBe(404); + expect(missing.headers["Set-Cookie"]).toBeUndefined(); + + const wrong = fakeResponse(); + middleware({ url: "/__auth/bootstrap?token=wrong", headers: {} }, wrong.res, () => {}); + expect(wrong.getStatus()).toBe(404); + expect(wrong.headers["Set-Cookie"]).toBeUndefined(); + }); + it("uses deps.generateToken so a fixed test token is deterministic across requests", () => { const middleware = captureMiddleware({ generateToken: () => "fixed-token-123" }); const { res } = fakeResponse(); @@ -145,9 +225,28 @@ describe("authPlugin (#4858)", () => { expect(calledNext).toBe(true); }); + it("calls deps.logToken once with the generated token so an operator can complete the bootstrap flow", () => { + const logged: string[] = []; + authPlugin({ generateToken: () => "logged-token", logToken: (token) => logged.push(token) }); + expect(logged).toEqual(["logged-token"]); + }); + + it("uses console.log as the default logToken when no override is given", () => { + const originalLog = console.log; + const calls: unknown[][] = []; + console.log = (...args: unknown[]) => calls.push(args); + try { + authPlugin({ generateToken: () => "console-logged-token" }); + } finally { + console.log = originalLog; + } + expect(calls).toHaveLength(1); + expect(String(calls[0]?.[0])).toContain("console-logged-token"); + }); + it("also attaches via configurePreviewServer for `vite preview`", () => { let captured: CapturedRequestHandler | undefined; - const plugin = authPlugin(); + const plugin = authPlugin({ logToken: () => {} }); const server = { middlewares: { use: (fn: CapturedRequestHandler) => (captured = fn) } }; // @ts-expect-error -- same partial test double as configureServer above. plugin.configurePreviewServer(server); diff --git a/apps/loopover-miner-ui/vite-auth.ts b/apps/loopover-miner-ui/vite-auth.ts index e1b3dc20a6..b89bdd43bd 100644 --- a/apps/loopover-miner-ui/vite-auth.ts +++ b/apps/loopover-miner-ui/vite-auth.ts @@ -1,23 +1,31 @@ -import { randomBytes } from "node:crypto"; +import { randomBytes, timingSafeEqual } from "node:crypto"; import type { Plugin } from "vite"; -// Local miner-ui API auth (#4858): the miner-ui's /api/* endpoints previously relied on undocumented, implicit -// loopback-only trust -- and even that was inconsistent (vite-run-state-api.ts's own isLoopbackAddress gated -// only ONE of the three API plugins; vite-portfolio-queue-api.ts and vite-ledgers-api.ts had no gate at all). -// Neither is real authentication: loopback-IP checks don't stop another local process, or a malicious page the -// user has open in the SAME browser, from hitting the loopback API. +// Local miner-ui API auth (#4858, hardened for GHSA-v6v4-mh5m-5mqq): the miner-ui's /api/* endpoints +// previously relied on undocumented, implicit loopback-only trust -- and even that was inconsistent +// (vite-run-state-api.ts's own isLoopbackAddress gated only ONE of the three API plugins; +// vite-portfolio-queue-api.ts and vite-ledgers-api.ts had no gate at all). Neither is real +// authentication: loopback-IP checks don't stop another local process, or a malicious page the user +// has open in the SAME browser, from hitting the loopback API. // // This adds a minimal-but-real mechanism instead of touching each API file individually: a random token -// generated ONCE per dev-server process, delivered to the browser as a same-origin HttpOnly SameSite=Strict -// cookie on every response the caller is authorized to receive (so the very first page load already carries -// it going forward -- NOT on an unauthenticated /api/* request's own 401, which must never leak the token it -// is rejecting the caller for lacking), and required on every /api/* request thereafter. HttpOnly keeps it -// unreachable from any XSS in the SPA itself; SameSite=Strict -// means a cross-origin page -- including one from a DNS-rebinding attack, which resolves an ATTACKER-CONTROLLED -// hostname to 127.0.0.1 rather than reusing this dev server's own origin -- never has it attached automatically -// by the browser. Because it rides on the browser's own cookie jar, no client-side fetch call needs to change: -// the browser attaches it automatically to every same-origin request, including the existing fetchPortfolioQueue/ -// fetchLedgers/fetchRunState calls. +// generated ONCE per dev-server process, required on every /api/* request via a same-origin HttpOnly +// SameSite=Strict cookie. HttpOnly keeps it unreachable from any XSS in the SPA itself; SameSite=Strict +// means a cross-origin page -- including one from a DNS-rebinding attack, which resolves an +// ATTACKER-CONTROLLED hostname to 127.0.0.1 rather than reusing this dev server's own origin -- never +// has it attached automatically by the browser. Because it rides on the browser's own cookie jar, no +// client-side fetch call needs to change: the browser attaches it automatically to every same-origin +// request, including the existing fetchPortfolioQueue/fetchLedgers/fetchRunState calls. +// +// GHSA-v6v4-mh5m-5mqq: an earlier version of this plugin stamped Set-Cookie on EVERY response that +// wasn't itself rejected -- including the very first, wholly unauthenticated `GET /` and every static +// asset. That gave the token to any local process (not just a browser) that could `curl` the root path, +// with no proof it had ever been authorized to see it. The fix here is to never mint the cookie for an +// anonymous request: it is only ever (re)stamped on a request that ALREADY presents a valid cookie, or +// minted for the first time via the one-shot bootstrap path below, which requires the operator to copy +// the token off the server's own stdout/journal -- something only console/log access can provide, not an +// ordinary loopback request. `isAuthenticatedRequest` also now compares in constant time so a valid +// token can't be inferred from response-timing differences on a byte-by-byte guess. // // Registered as the FIRST plugin in vite.config.ts so its middleware runs before runStateApiPlugin/ // portfolioQueueApiPlugin/ledgersApiPlugin's own middlewares in the Connect chain: an unauthenticated /api/* @@ -26,13 +34,25 @@ import type { Plugin } from "vite"; const COOKIE_NAME = "loopover_miner_ui_token"; +/** One-shot bootstrap path: a browser visits this ONCE per server start, with `?token=` copied from the + * server's own stdout/journal, to obtain the session cookie. Presenting the token here is exactly as + * strong a proof as the cookie itself -- an attacker without console/log access to the server process + * can only guess it, not read it off an ordinary request the way the prior unconditional Set-Cookie did. */ +export const AUTH_BOOTSTRAP_PATH = "/__auth/bootstrap"; + export type AuthDeps = { /** Injectable so tests get a deterministic token instead of a real random one. */ generateToken: () => string; + /** Injectable so tests capture the printed bootstrap instructions instead of writing real stdout. */ + logToken: (token: string) => void; }; const defaultDeps: AuthDeps = { generateToken: () => randomBytes(24).toString("hex"), + logToken: (token) => + console.log( + `[loopover-miner-ui:auth] visit ${AUTH_BOOTSTRAP_PATH}?token=${token} once per server start to authenticate this browser (treat this token like a password): ${token}`, + ), }; function parseCookieHeader(cookieHeader: string | undefined): Record { @@ -48,16 +68,28 @@ function parseCookieHeader(cookieHeader: string | undefined): Record void; end: (body: string) => void }; + +/** Vite dev/preview middleware: generates one token per process, logs it (see defaultDeps.logToken) so an + * operator with console/log access can complete the one-shot bootstrap, and then on every request either + * rejects it, bootstraps it, or lets it through -- (re)stamping the cookie ONLY when the request already + * proved it holds the token, or when it just proved that via a valid bootstrap token (GHSA-v6v4-mh5m-5mqq). + * An anonymous page/asset load gets neither the cookie nor any indication a token exists. */ +export function authPlugin(deps: Partial = {}): Plugin { + const { generateToken, logToken } = { ...defaultDeps, ...deps }; + const token = generateToken(); + logToken(token); + const setSessionCookie = (res: ConnectResponse) => + res.setHeader("Set-Cookie", `${COOKIE_NAME}=${token}; HttpOnly; SameSite=Strict; Path=/`); const attach = (middlewares: { - use: ( - fn: ( - req: { url?: string; headers: { cookie?: string } }, - res: { statusCode: number; setHeader: (k: string, v: string) => void; end: (body: string) => void }, - next: () => void, - ) => void, - ) => void; + use: (fn: (req: ConnectRequest, res: ConnectResponse, next: () => void) => void) => void; }) => { middlewares.use((req, res, next) => { + const bootstrap = handleBootstrapRequest(req.url, token); + if (bootstrap) { + if ("redirectTo" in bootstrap) { + setSessionCookie(res); + res.statusCode = 302; + res.setHeader("Location", bootstrap.redirectTo); + res.end(""); + return; + } + res.statusCode = bootstrap.status; + res.end(bootstrap.body); + return; + } + const rejection = handleAuthRequest(req.url, req.headers.cookie, token); if (rejection) { res.statusCode = rejection.status; @@ -98,7 +156,11 @@ export function authPlugin(deps: AuthDeps = defaultDeps): Plugin { res.end(rejection.body); return; } - res.setHeader("Set-Cookie", `${COOKIE_NAME}=${token}; HttpOnly; SameSite=Strict; Path=/`); + + // Only re-stamp the cookie for a request that already presented it -- an anonymous page/asset + // load (no cookie yet) falls through with NO Set-Cookie at all, closing the GHSA-v6v4-mh5m-5mqq + // leak where `curl http://127.0.0.1:4174/` alone was enough to read a valid, replayable token. + if (isAuthenticatedRequest(req.headers.cookie, token)) setSessionCookie(res); next(); }); }; diff --git a/packages/loopover-mcp/lib/local-branch.ts b/packages/loopover-mcp/lib/local-branch.ts index cd579ce494..c6f67f5a0a 100644 --- a/packages/loopover-mcp/lib/local-branch.ts +++ b/packages/loopover-mcp/lib/local-branch.ts @@ -105,6 +105,25 @@ export function parseGitRemote(remoteUrl: unknown): string | undefined { return undefined; } +// GHSA-v3j4-j27j-fxw6: a `baseRef` (or a ref value derived from it, e.g. the ORIGIN_BRANCH extracted from +// "origin/ORIGIN_BRANCH") starting with '-' is parsed by git as an OPTION, not a revision -- e.g. +// `git diff --name-status -M -z "--output=/some/path" --` silently writes/truncates an arbitrary file, +// because gitOutput (below) swallows all errors. Checked at this single choke point AND, separately, in +// the zod input schemas in bin/loopover-mcp.ts (defense in depth -- the schema is not the only entry +// point into this function; see resolveWorkspaceCwd's own "outside the MCP roots" throw for the same +// pattern applied to `cwd`). +const LEADING_DASH_REF = /^-/; + +export function isSafeGitRef(ref: string): boolean { + return !LEADING_DASH_REF.test(ref); +} + +export function assertSafeGitRef(ref: string, label = "baseRef"): void { + if (!isSafeGitRef(ref)) { + throw new Error(`${label} must not start with '-' (git would parse ${JSON.stringify(ref)} as an option, not a revision).`); + } +} + export function collectLocalDiff(cwd: string, baseRef: string, workspaceRoots?: McpRootInput[]): LocalDiff { const metadata = collectLocalBranchMetadata({ cwd, baseRef, login: "local", workspaceRoots }); return { @@ -130,19 +149,33 @@ export function collectLocalBranchMetadata(input: CollectLocalBranchMetadataInpu const workspace = resolveWorkspaceCwd(input); const cwd = workspace.cwd; const baseRef = input.baseRef ?? defaultBaseRef(cwd); + // Defense in depth (GHSA-v3j4-j27j-fxw6): reject a leading-dash baseRef here too, not only in the zod + // input schemas -- this function has other direct callers (buildBranchAnalysisPayload, and any future + // one) that don't necessarily go through zod validation first. + assertSafeGitRef(baseRef); const remoteUrl = gitLines(cwd, ["config", "--get", "remote.origin.url"])[0] ?? ""; const repoFullName = input.repoFullName ?? parseGitRemote(remoteUrl); if (!repoFullName) throw new Error("Could not infer repoFullName from git remote; pass --repo owner/repo."); const branchName = input.branchName ?? gitLines(cwd, ["branch", "--show-current"])[0] ?? "local-branch"; const headRef = input.headRef ?? gitLines(cwd, ["rev-parse", "--abbrev-ref", "HEAD"])[0] ?? branchName; - const baseSha = gitLines(cwd, ["rev-parse", "--verify", baseRef])[0]; - const headSha = gitLines(cwd, ["rev-parse", "--verify", "HEAD"])[0]; - const mergeBaseSha = gitLines(cwd, ["merge-base", baseRef, "HEAD"])[0]; + // GHSA-v3j4-j27j-fxw6 fix item 3: pre-resolve baseRef to a real SHA ONCE here, and pass that SHA (never + // the raw, caller-supplied string) to every downstream git invocation below that takes a revision -- + // `--end-of-options` on every one of those invocations is the belt, this is the suspenders. Falls back + // to the (already leading-dash-checked) baseRef itself when it doesn't resolve, matching this file's + // existing graceful-degradation behavior (an invalid ref simply yields empty diff/log output, not a + // thrown error) for e.g. a not-yet-fetched remote branch. + const baseSha = gitLines(cwd, ["rev-parse", "--verify", "--end-of-options", baseRef])[0]; + const resolvedBaseRef = baseSha ?? baseRef; + const headSha = gitLines(cwd, ["rev-parse", "--verify", "--end-of-options", "HEAD"])[0]; + const mergeBaseSha = gitLines(cwd, ["merge-base", "--end-of-options", resolvedBaseRef, "HEAD"])[0]; + // collectRemoteTrackingSha needs the ORIGINAL baseRef (e.g. "origin/main") to pattern-match the remote + // branch name out of it -- a resolved SHA has no such structure. It applies its own leading-dash guard + // to the extracted branch name before ever calling git (see below). const remoteTrackingSha = collectRemoteTrackingSha(cwd, baseRef); - const changedFiles = collectChangedFiles(cwd, baseRef); - const pendingCommitCount = input.pendingCommitCount ?? collectPendingCommitCount(cwd, baseRef); - const ciStatusHints = input.ciStatusHints ?? collectCiStatusHints(cwd, baseRef, changedFiles); - const commitMessages = input.commitMessages ?? collectCommitMessages(cwd, baseRef); + const changedFiles = collectChangedFiles(cwd, resolvedBaseRef); + const pendingCommitCount = input.pendingCommitCount ?? collectPendingCommitCount(cwd, resolvedBaseRef); + const ciStatusHints = input.ciStatusHints ?? collectCiStatusHints(cwd, resolvedBaseRef, changedFiles); + const commitMessages = input.commitMessages ?? collectCommitMessages(cwd, resolvedBaseRef); const title = input.title ?? titleFromBranch(branchName) ?? firstCommitTitle(commitMessages); const linkedIssues = [...new Set([...(input.linkedIssues ?? []), ...extractLinkedIssues([branchName, title, input.body, ...commitMessages].filter(Boolean).join("\n"))])].sort( (left, right) => left - right, diff --git a/systemd/loopover-miner-ui.service.example b/systemd/loopover-miner-ui.service.example index 67e8f81b5c..0371acdc66 100644 --- a/systemd/loopover-miner-ui.service.example +++ b/systemd/loopover-miner-ui.service.example @@ -43,7 +43,11 @@ User=loopover WorkingDirectory=/var/lib/loopover-miner/src # REQUIRED: absolute path to the workspace-scoped npm binary (see `which npm`; typically /usr/bin or # /usr/local/bin, or an nvm/asdf shim path). `vite preview` binds 127.0.0.1 by default -- do not add -# `--host` unless you have put real auth in front of this port; the dashboard has none of its own. +# `--host` regardless: the dashboard's auth (apps/loopover-miner-ui/vite-auth.ts) is a same-host, +# console-access mechanism, not a network-facing one. On first start, `journalctl -u loopover-miner-ui` +# prints a one-shot bootstrap URL (visit it once from a browser on this host to authenticate; treat the +# token in it like a password) -- GHSA-v6v4-mh5m-5mqq fixed the earlier behavior where any local process +# could read a valid session token straight off an ordinary `curl http://127.0.0.1:4174/`. ExecStart=/usr/bin/npm --workspace @loopover/ui-miner run preview # A crashed dashboard should come back; it holds no in-memory state of its own to lose. Restart=on-failure diff --git a/test/unit/local-branch.test.ts b/test/unit/local-branch.test.ts index d9c6dfcd61..bb8ab044c4 100644 --- a/test/unit/local-branch.test.ts +++ b/test/unit/local-branch.test.ts @@ -1,5 +1,5 @@ import { execFileSync } from "node:child_process"; -import { mkdtempSync, mkdirSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { mkdtempSync, mkdirSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { pathToFileURL } from "node:url"; @@ -2551,6 +2551,72 @@ describe("local MCP git metadata collection (#7329 coverage)", () => { }); }); +describe("git argument-injection hardening (GHSA-v3j4-j27j-fxw6)", () => { + let tempDir: string | null = null; + + afterEach(() => { + if (tempDir) rmSync(tempDir, { recursive: true, force: true }); + tempDir = null; + }); + + it("isSafeGitRef rejects any ref starting with '-', accepts everything else", async () => { + const { isSafeGitRef } = await import("../../packages/loopover-mcp/lib/local-branch"); + expect(isSafeGitRef("--output=/tmp/evil")).toBe(false); + expect(isSafeGitRef("-x")).toBe(false); + expect(isSafeGitRef("origin/main")).toBe(true); + expect(isSafeGitRef("HEAD~1")).toBe(true); + expect(isSafeGitRef("a1b2c3d4")).toBe(true); + expect(isSafeGitRef("feature/my-branch")).toBe(true); + }); + + it("assertSafeGitRef throws for a leading-dash ref and is silent for a safe one", async () => { + const { assertSafeGitRef } = await import("../../packages/loopover-mcp/lib/local-branch"); + expect(() => assertSafeGitRef("--output=/tmp/evil")).toThrow(/must not start with '-'/); + expect(() => assertSafeGitRef("origin/main")).not.toThrow(); + }); + + it("collectLocalBranchMetadata rejects a malicious baseRef instead of letting git interpret it as an option", async () => { + const { collectLocalBranchMetadata } = await import("../../packages/loopover-mcp/lib/local-branch"); + tempDir = mkdtempSync(join(tmpdir(), "loopover-local-")); + git(tempDir, "init"); + git(tempDir, "config", "user.email", "test@example.com"); + git(tempDir, "config", "user.name", "LoopOver Test"); + git(tempDir, "config", "commit.gpgsign", "false"); + writeFileSync(join(tempDir, "victim.txt"), "VICTIM CONTENT\n"); + git(tempDir, "add", "victim.txt"); + git(tempDir, "commit", "-m", "initial commit"); + + const maliciousBaseRef = `--output=${join(tempDir, "victim.txt")}`; + expect(() => collectLocalBranchMetadata({ cwd: tempDir!, baseRef: maliciousBaseRef, login: "oktofeesh1" })).toThrow(/must not start with '-'/); + // The file must survive completely untouched -- this is the actual exploit this issue reports. + expect(readFileSync(join(tempDir, "victim.txt"), "utf8")).toBe("VICTIM CONTENT\n"); + }); + + it("still resolves ordinary refs (branch name, HEAD~N, short SHA) exactly as before the fix", async () => { + const { collectLocalBranchMetadata } = await import("../../packages/loopover-mcp/lib/local-branch"); + tempDir = mkdtempSync(join(tmpdir(), "loopover-local-")); + git(tempDir, "init"); + git(tempDir, "config", "user.email", "test@example.com"); + git(tempDir, "config", "user.name", "LoopOver Test"); + git(tempDir, "config", "commit.gpgsign", "false"); + git(tempDir, "remote", "add", "origin", "git@github.com:entrius/allways-ui.git"); + writeFileSync(join(tempDir, "a.ts"), "a\n"); + git(tempDir, "add", "a.ts"); + git(tempDir, "commit", "-m", "base commit"); + git(tempDir, "checkout", "-b", "feature"); + writeFileSync(join(tempDir, "b.ts"), "b\n"); + git(tempDir, "add", "b.ts"); + git(tempDir, "commit", "-m", "feature commit"); + + const byBranchName = collectLocalBranchMetadata({ cwd: tempDir, baseRef: "HEAD~1", login: "oktofeesh1" }); + expect(byBranchName.changedFiles).toEqual(expect.arrayContaining([expect.objectContaining({ path: "b.ts", status: "added" })])); + + const baseSha = execFileSync("git", ["rev-parse", "HEAD~1"], { cwd: tempDir }).toString().trim(); + const byShaRef = collectLocalBranchMetadata({ cwd: tempDir, baseRef: baseSha, login: "oktofeesh1" }); + expect(byShaRef.changedFiles).toEqual(expect.arrayContaining([expect.objectContaining({ path: "b.ts", status: "added" })])); + }); +}); + const repo: RepositoryRecord = { fullName: "entrius/allways-ui", owner: "entrius",