From 01281f6159a26c2d5b1e315e31136ff4e2219463 Mon Sep 17 00:00:00 2001 From: Daniel Ehrhardt Date: Sun, 4 Oct 2026 20:27:55 +0200 Subject: [PATCH] Secrets never block a task: take them out of the branch and push it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Coding tasks landed in Blocked far too often with "The changes on contain a secret saved in the vault, so Godmode didn't push them". The check compared the whole `git diff base...HEAD` (context lines, removed lines and file paths included) with every value the vault masks, and that is every value of every custom MCP server's env and headers — `production`, URLs, regions. Any branch touching a config file could trip it. What counts now - Only what the commits a push would publish add (neither on the base nor on the remote yet): added lines, file names, commit messages, files that look like secrets (.env, keys) and every version of a binary file (read whole, up to 20 MB). Removed and surrounding lines don't count, a moved file adds only what changed. - Only values stored as a secret (`pushSecrets`, a subset of the redaction list): passwords, 2FA secrets, API keys, tokens; of MCP env/header and messaging maps the values whose name says secret (API_KEY, signingKey, SENTRY_DSN, SLACK_WEBHOOK_URL, Authorization — not publishable/anon keys, not SORT_KEY=created_at), bearer tokens and URL passwords; never a single plain word or number. Redaction in transcripts and logs is unchanged. What happens instead of Blocked (`removeSecrets`, tasks/git.ts) - The secret is replaced with GODMODE_REMOVED_SECRET in text files; files that look like secrets, binaries holding one and files that can't be rewritten safely (not UTF-8, a link, not writable) are left out — they stay in the worktree, and one the branch already had keeps the remote's version. - The unpushed commits become one commit on top of what the remote has (the branch's own last commit first), so no pushed commit or message carries the secret and nothing on the remote is rewritten. The branch as the agent left it stays in the worktree as refs/worktree/godmode/with-secrets/. - Exactly the commit that was checked is pushed. A turn that stages or commits during the fix (or before a merge with someone else's push) makes Godmode skip this push; that turn's end pushes the branch. - A warning names what was changed; the agent's brief asks it to read secrets from the environment; PR title and body never carry a saved secret, even with redaction off. SecretInBranch and its three catch sites (publish, keepWork, pushTaskBranch) are gone. Tasks already blocked with the old message push normally from the board or when moved to Todo. --- docs/ARCHITECTURE.md | 28 +- packages/core/src/tasks/git.ts | 196 ++++++++++++-- packages/core/src/tasks/service.ts | 60 +++-- packages/core/src/vault/vault.ts | 60 ++++- packages/core/test/fixtures/fake-claude.ts | 34 +++ packages/core/test/tasks.test.ts | 285 ++++++++++++++++++++- packages/core/test/vault.test.ts | 44 ++++ 7 files changed, 630 insertions(+), 77 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index e09fec1e..6d3a9c7d 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -696,14 +696,36 @@ a global one. Every change is pushed as `task.updated` / `task.deleted` and patc without one when it can't be created (with a notification). Deleting a task removes its worktree and prunes it from the repository; its branch stays (while the worktree exists, the branch can't be checked out elsewhere — merge it). * **Coding tasks** publish their branch: when a run succeeds, Godmode commits what the agent left uncommitted (new - `.env`/key files are left out), refuses to push when the branch adds such files or its diff contains a secret from - the vault, merges commits someone else pushed to the branch since Godmode's last push (a conflict blocks the task), + `.env`/key files are left out), takes secrets out of the commits the remote doesn't have yet (see the next point), + merges commits someone else pushed to the branch since Godmode's last push (a conflict blocks the task), and pushes with an explicit lease on what it saw — so nothing pushed meanwhile is overwritten. It then opens a pull - request with `gh pr create` (body: the agent's summary, redacted); without `gh`, or for GitLab, the task links to the + request with `gh pr create` (body: the agent's summary, redacted — saved secrets are taken out of the title and body + even with redaction off); without `gh`, or for GitLab, the task links to the page that opens one. A branch without commits on top of its base goes to review without a pull request; a local repository without a remote keeps the commits on the task's branch. Restarting fast-forwards the worktree to the remote branch first; only the task's branch is fetched and pushed (nothing is written to the repository's config). When a `general` or `research` task's run ends, what it changed is committed on its branch, which isn't pushed. +* **Secrets never stop a push and never go along**: before a task's branch is pushed (when a run ends, or from the + board), Godmode checks the commits that are neither on the base nor on the remote yet, and only what they add: files + that look like secrets (`.env`, keys), and secrets saved in the vault in an added line, a file name, any version of + a binary file those commits add (read whole, up to 20 MB) or a commit message. Lines a change removes or merely surrounds don't + count, a moved file adds only what changed, and what the remote already has isn't checked again. A value counts + when it is stored as a secret (passwords, 2FA secrets, API keys, tokens; of a custom MCP server's env variables and + headers the ones whose name says so, like `API_KEY`, `signingKey`, `SENTRY_DSN`, `SLACK_WEBHOOK_URL` or + `Authorization` — not public keys like `STRIPE_PUBLISHABLE_KEY`, nor a snake_case identifier under a key's name like + `SORT_KEY=created_at` —, plus bearer tokens and passwords inside URLs) and isn't a single plain word or number — a + server's other settings (`NODE_ENV=production`, URLs) are only masked in + transcripts. When something is found, Godmode fixes the branch instead of blocking the task: such files are left out + (they stay in the worktree; one the branch already had keeps the version the remote has), the secret is replaced + with `GODMODE_REMOVED_SECRET` in text files (a file that can't be rewritten safely — binary, not UTF-8, a link, not + writable — is left out too), and the unpushed commits become one commit on top of what the remote has (the branch's + own last commit as its first parent), so no pushed commit or commit message carries the secret and nothing on the + remote is rewritten. Exactly the commit that was checked is pushed: when a turn that started meanwhile stages or + commits something during the fix (or before a merge with someone else's push), nothing is pushed now and that + turn's end pushes the branch. The branch as the agent left it stays in the worktree as + `refs/worktree/godmode/with-secrets/` (never pushed; its reflog keeps the commits for git's 90 days even + when the repository is cleaned up from another checkout), a notification names the files, and the agent's brief + tells it to read secrets from the environment. With a locked vault only the file names are checked. * **When a run ends** (any run in the task's conversation, so the human's follow-ups count too): succeeded → `in_review` (after publishing, for coding tasks), failed or stopped → `blocked` with the reason, and a `task_report_blocked` call during the run → `blocked` with what the agent needs. A follow-up puts a delivered or diff --git a/packages/core/src/tasks/git.ts b/packages/core/src/tasks/git.ts index c44825bb..d4dd7511 100644 --- a/packages/core/src/tasks/git.ts +++ b/packages/core/src/tasks/git.ts @@ -8,7 +8,7 @@ * Godmode's clone of a remote repository: one bare clone per URL (/repos/.tasks), shared by its tasks' worktrees. */ import { createHash } from "node:crypto"; -import { existsSync, mkdirSync, realpathSync, renameSync, rmSync, statSync, writeFileSync } from "node:fs"; +import { existsSync, lstatSync, mkdirSync, readFileSync, realpathSync, renameSync, rmSync, statSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { basename, dirname, join, resolve } from "node:path"; import type { PullRequestState, TaskPullRequest } from "@godmode/shared"; @@ -35,7 +35,7 @@ export function __setGhForTests(path: string | null | undefined) { export class GitError extends Error {} -/** New files that look like secrets (env files, keys, keystores); they are never committed. */ +/** New files that look like secrets (env files, keys, keystores); they are never committed, and never pushed. */ const SECRET_FILE = /(^|\/)(\.env(\.(?!example$|sample$|template$|dist$)[\w.-]+)?|[^/]*\.(pem|key|p12|pfx|keystore|jks)|id_(rsa|dsa|ecdsa|ed25519)|credentials\.json|\.netrc)$/i; function env() { @@ -67,6 +67,11 @@ async function gitOk(args: string[], cwd: string): Promise { } } +/** Who Godmode's own commits are by, in a repository without a configured author. */ +async function identity(dir: string): Promise { + return (await gitOk(["config", "user.email"], dir)) ? [] : ["-c", "user.name=Godmode", "-c", "user.email=godmode@localhost"]; +} + function hasRef(dir: string, ref: string): Promise { return gitOk(["rev-parse", "--verify", "--quiet", ref], dir); } @@ -99,9 +104,9 @@ async function baseRef(dir: string, base: string): Promise { return (await hasRef(dir, `refs/remotes/origin/${base}`)) ? `origin/${base}` : base; } -/** Commits the checked out branch has on top of its base. */ -export async function commitsAhead(dir: string, base: string): Promise { - return Number(await git(["rev-list", "--count", `${await baseRef(dir, base)}..HEAD`], dir)) || 0; +/** Commits the checked out branch (or `head`) has on top of its base. */ +export async function commitsAhead(dir: string, base: string, head = "HEAD"): Promise { + return Number(await git(["rev-list", "--count", `${await baseRef(dir, base)}..${head}`], dir)) || 0; } /* ------------------------------------------------------------------ */ @@ -308,21 +313,159 @@ export async function commitWork(opts: { dir: string; message: string }): Promis await git(["add", "-A"], dir); if (skipped.length) await git(["reset", "-q", "--", ...skipped], dir); if (await git(["diff", "--cached", "--name-only"], dir)) { - const identity = (await gitOk(["config", "user.email"], dir)) ? [] : ["-c", "user.name=Godmode", "-c", "user.email=godmode@localhost"]; - await git([...identity, "commit", "-m", opts.message], dir); + await git([...(await identity(dir)), "commit", "-m", opts.message], dir); } return { skipped }; } -/** Files the branch adds on top of its base that look like secrets (env files, keys) — committed by the agent itself. */ -export async function secretFilesAdded(dir: string, base: string): Promise { - const added = await git(["diff", "--name-only", "--diff-filter=A", "-z", `${await baseRef(dir, base)}...HEAD`], dir); - return added.split("\0").filter((f) => f && SECRET_FILE.test(f)); +/** Text with its secrets replaced; text without one comes back unchanged. */ +type Clean = (text: string) => string; + +/** Patches as the lines they add, whatever the human's git config says. A moved file adds only the lines that changed (-M). */ +const ADDED = ["-U0", "-M", "--no-color", "--no-ext-diff", "--no-textconv"]; +/** Where the branch stays as the agent left it when Godmode rewrote it: refs of the task's worktree only, never pushed. */ +const WITH_SECRETS = "refs/worktree/godmode/with-secrets"; +/** Binary files show no lines, so they are read whole — up to this size (larger ones are media, not configuration). */ +const MAX_BINARY_READ = 20 * 1024 * 1024; + +/** The lines `-U0` patches add (in a merge: the ones none of its parents has), without their "+". */ +function addedLines(patch: string): string { + const lines: string[] = []; + let plus = ""; + for (const line of patch.split("\n")) { + const hunk = /^@@+/.exec(line); + if (hunk) plus = "+".repeat(hunk[0].length - 1); + else if (line.startsWith("diff --")) plus = ""; + else if (plus && line.startsWith(plus)) lines.push(line.slice(plus.length)); + } + return lines.join("\n"); } -/** The branch's changes on top of its base (for checks before pushing). */ -export async function branchDiff(dir: string, base: string): Promise { - return git(["diff", "--no-color", "--no-ext-diff", `${await baseRef(dir, base)}...HEAD`], dir); +/** + * Take secrets out of what a push would publish — the commits that are neither on the base nor on the remote yet — so + * the branch can always be pushed. Only what those commits add counts: files that look like secrets, and what `clean` + * changes (a saved secret) in a file name, an added line, a binary file the branch ends with, or a commit message. + * Such files go back to what the remote has (new ones stay in the worktree, untracked), a secret in a text file is + * replaced there, and a file that still adds one (binary, not UTF-8, a link, unwritable) is left out too. Every commit + * is pushed, not only the last, so those commits become one on top of what the remote has; the branch as it was stays + * in the worktree as the ref `kept`. `head` is the commit to push — null when the branch moved meanwhile (a turn that + * started during the push committed): that turn's end pushes it. `removed` is null when there was nothing to take out. + */ +export async function removeSecrets(opts: { + dir: string; + base: string; + lastPushed: string | null; + message: string; + clean: Clean; +}): Promise<{ head: string | null; removed: { left: string[]; replaced: string[]; kept: string } | null }> { + const { dir, clean } = opts; + const hit = (text: string) => clean(text) !== text; + const old = await git(["rev-parse", "HEAD"], dir); + const unpushed = ["--ignore-missing", old, "--not", await baseRef(dir, opts.base), "--remotes=origin", ...(opts.lastPushed ? [opts.lastPushed] : [])]; + const log = (...args: string[]) => git(["log", "--no-show-signature", ...args, ...unpushed], dir); + const added = (await log("--cc", "--no-renames", "--diff-filter=A", "--name-only", "-z", "--format=")).split("\0"); + const left = new Set(added.filter((f) => f && (SECRET_FILE.test(f) || hit(f)))); + // Binary files (numstat "-") show no lines: every version the commits add is read whole; the last one is left out. + const binary = new Set((await log("--numstat", "--no-renames", "-z", "--format=")).split("\0").filter((l) => l.startsWith("-\t-\t")).map((l) => l.slice(4))); + let inBinary = false; + if (binary.size) { + const raw = (await log("--raw", "--no-abbrev", "--no-renames", "-z", "--format=")).split("\0"); // ": ", path + const hits = new Map>(); + for (let i = 0; i + 1 < raw.length; i += 2) { + const [sha, path] = [raw[i]!.split(" ")[3], raw[i + 1]!]; + if (!sha || /^0+$/.test(sha) || !binary.has(path) || hits.get(path)?.has(sha)) continue; + if (Number(await git(["cat-file", "-s", sha], dir)) > MAX_BINARY_READ || !hit(await git(["cat-file", "blob", sha], dir))) continue; + hits.set(path, (hits.get(path) ?? new Set()).add(sha)); + } + inBinary = hits.size > 0; + if (inBinary) { + for (const entry of (await git(["--literal-pathspecs", "ls-tree", "-z", old, "--", ...hits.keys()], dir)).split("\0")) { + const tab = entry.indexOf("\t"); + if (tab > 0 && hits.get(entry.slice(tab + 1))?.has(entry.slice(0, tab).split(" ")[2]!)) left.add(entry.slice(tab + 1)); + } + } + } + const inText = hit(await log("--format=%B")) || hit(addedLines(await log("-p", "--cc", ...ADDED, "--format="))); + if (!left.size && !inText && !inBinary) return { head: old, removed: null }; + + const kept = `${WITH_SECRETS}/${old}`; + // With a reflog: git cleaning up from another checkout of the repository doesn't see this worktree's refs, only their logs. + await git(["update-ref", "--create-reflog", "-m", "godmode: before removing secrets", kept, old], dir); + // The commits the unpushed ones start from: all on the remote already, so pushing the new commit never overwrites anything. + const edges = (await git(["rev-list", "--boundary", ...unpushed], dir)).split("\n").filter((l) => l.startsWith("-")); + if (!edges.length) throw new GitError("The branch shares no history with its base."); + // The branch's own one first (git lists a merged base first): left-out files go back to the branch's version. + const line = (await git(["rev-list", "--first-parent", ...unpushed], dir)).split("\n").at(-1); + const own = await git(["rev-parse", "--verify", "--quiet", `${line}^`], dir).catch(() => ""); + const parents = (await git(["merge-base", "--independent", ...edges.map((l) => l.slice(1))], dir)).split("\n").sort((a, b) => Number(b === own) - Number(a === own)); + /** The changes from `parent` to what git has staged — or to `tree` once it is written. */ + const diff = (parent: string, args: string[], paths: string[] = [], tree?: string) => + git(["--literal-pathspecs", "diff", ...(tree ? [] : ["--cached"]), ...args, parent, ...(tree ? [tree] : []), "--", ...paths], dir); + /** Per parent, the staged files that were moved (new name → old one): checked together, a moved file adds only what changed. */ + const moves = new Map>(); + const movedFrom = async (parent: string, path: string) => { + if (!moves.has(parent)) { + const out = (await diff(parent, ["-M", "--diff-filter=R", "--name-status", "-z"])).split("\0"); + const map = new Map(); + for (let i = 0; i + 2 < out.length; i += 3) map.set(out[i + 2]!, out[i + 1]!); + moves.set(parent, map); + } + return moves.get(parent)!.get(path); + }; + /** Whether the new commit (staged, or `tree`) adds a secret (to `path`): only what none of the parents has counts. */ + const adds = async (path?: string, tree?: string) => { + for (const parent of parents) { + const from = path && (await movedFrom(parent, path)); + if (!hit(addedLines(await diff(parent, ADDED, [...(path ? [path] : []), ...(from ? [from] : [])], tree)))) return false; + } + return true; + }; + + const replaced: string[] = []; + const changed = inText && (await adds()) ? (await diff(parents[0]!, ["--no-renames", "--diff-filter=d", "--name-only", "-z"])).split("\0") : []; + for (const path of changed) { + if (!path || left.has(path) || !(await adds(path))) continue; + const file = join(dir, path); + try { + const bytes = lstatSync(file, { throwIfNoEntry: false })?.isFile() ? readFileSync(file) : null; + const text = bytes?.toString() ?? ""; + // Only plain text is rewritten: never through a link, never binary data or a file that wouldn't come back byte for byte. + if (bytes && !bytes.includes(0) && Buffer.from(text).equals(bytes) && hit(text)) { + writeFileSync(file, clean(text)); + if ((await gitOk(["--literal-pathspecs", "add", "--", path], dir)) && !(await adds(path))) { + replaced.push(path); + continue; + } + } + } catch { + /* can't be read or written: left out */ + } + left.add(path); + } + if (left.size) await git(["--literal-pathspecs", "reset", "-q", parents[0]!, "--", ...left], dir); + + const tree = await git(["write-tree"], dir); + /** The tree is the checked commit plus the files taken care of, as they were left — nothing a turn staged meanwhile. */ + const checked = async () => { + const touched = new Set([...replaced, ...left]); + if ((await git(["diff", "--name-only", "--no-renames", "-z", old, tree], dir)).split("\0").some((f) => f && !touched.has(f))) return false; + if (left.size && (await diff(parents[0]!, ["--name-only"], [...left], tree))) return false; + for (const path of replaced) if (await adds(path, tree)) return false; + return true; + }; + if (await checked()) { + // Nothing but secrets on top of what the remote has: the branch goes back to that. + const nothing = parents.length === 1 && tree === (await git(["rev-parse", `${parents[0]}^{tree}`], dir)); + const message = clean(`${opts.message}\n\n${await log("--reverse", "--format=- %s")}`); + const commit = nothing ? parents[0]! : await git([...(await identity(dir)), "commit-tree", tree, ...parents.flatMap((p) => ["-p", p]), "-m", message], dir); + // HEAD must still be the checked commit. + if (await gitOk(["update-ref", "-m", "godmode: secrets removed before pushing", "HEAD", commit, old], dir)) { + return { head: commit, removed: { left: [...left].filter((f) => lstatSync(join(dir, f), { throwIfNoEntry: false })), replaced, kept } }; + } + } + // A turn that started meanwhile staged or committed something unchecked: nothing is pushed now, its end pushes the branch. + await gitOk(["update-ref", "-d", kept], dir); + return { head: null, removed: null }; } /** @@ -331,23 +474,32 @@ export async function branchDiff(dir: string, base: string): Promise { * nothing that arrived meanwhile is overwritten; the agent's own rewrites of what Godmode pushed may replace it. Only * the task's branch is fetched, and nothing is written to the repository's config (it may be the human's own). */ -export async function pushBranch(opts: { dir: string; base: string; branch: string; lastPushed: string | null }): Promise<{ pushed: boolean; sha: string }> { +export async function pushBranch(opts: { + dir: string; + base: string; + branch: string; + lastPushed: string | null; + /** The commit to push (the one checked for secrets) — not what a turn that started meanwhile commits. Default: HEAD. */ + head?: string; +}): Promise<{ pushed: boolean; sha: string }> { const { dir, base, branch } = opts; return serialized(await repoKey(dir), async () => { - const sha = () => git(["rev-parse", "HEAD"], dir); - if (!(await commitsAhead(dir, base))) return { pushed: false, sha: await sha() }; + let head = opts.head ?? (await git(["rev-parse", "HEAD"], dir)); + if (!(await commitsAhead(dir, base, head))) return { pushed: false, sha: head }; const listed = await git(["ls-remote", "origin", `refs/heads/${branch}`], dir, PUSH_TIMEOUT_MS); const remote = listed.split(/\s/)[0] || null; - if (remote && remote !== opts.lastPushed && !(await gitOk(["merge-base", "--is-ancestor", remote, "HEAD"], dir))) { + if (remote && remote !== opts.lastPushed && !(await gitOk(["merge-base", "--is-ancestor", remote, head], dir))) { + // Merged in the worktree, so only while it still has the checked commit: a turn that started meanwhile pushes when it ends. + if ((await git(["rev-parse", "HEAD"], dir)) !== head) return { pushed: false, sha: head }; await git(["fetch", "--quiet", "origin", `+refs/heads/${branch}:refs/remotes/origin/${branch}`], dir, CLONE_TIMEOUT_MS); - const identity = (await gitOk(["config", "user.email"], dir)) ? [] : ["-c", "user.name=Godmode", "-c", "user.email=godmode@localhost"]; - if (!(await gitOk([...identity, "merge", "--no-edit", `origin/${branch}`], dir))) { + if (!(await gitOk([...(await identity(dir)), "merge", "--no-edit", `origin/${branch}`], dir))) { await gitOk(["merge", "--abort"], dir); throw new GitError(`Someone pushed to ${branch} and it conflicts with the agent's work — resolve it on the branch, then move the task to Todo.`); } + head = await git(["rev-parse", "HEAD"], dir); } - await git(["push", `--force-with-lease=refs/heads/${branch}:${remote ?? ""}`, "origin", `HEAD:refs/heads/${branch}`], dir, PUSH_TIMEOUT_MS); - return { pushed: true, sha: await sha() }; + await git(["push", `--force-with-lease=refs/heads/${branch}:${remote ?? ""}`, "origin", `${head}:refs/heads/${branch}`], dir, PUSH_TIMEOUT_MS); + return { pushed: true, sha: head }; }); } diff --git a/packages/core/src/tasks/service.ts b/packages/core/src/tasks/service.ts index 213126f7..42234379 100644 --- a/packages/core/src/tasks/service.ts +++ b/packages/core/src/tasks/service.ts @@ -26,7 +26,7 @@ import { all, get, getMeta, insert, run as sql, setMeta, tx, update } from "../d import { bus } from "../events/bus"; import { logger } from "../log"; import { HttpError, badRequest, conflict, newId, notFound, now, slugify } from "../util"; -import { containsSecret, redact } from "../vault/vault"; +import { SECRET_PLACEHOLDER, redact, withoutSecrets } from "../vault/vault"; import { getAgent } from "../agents/service"; import { activeRunForConversation, cancelRun, getRun, listActiveRuns, waitForRun } from "../runner/runner"; import { pauseOf, toPause } from "../services/pauses"; @@ -48,7 +48,6 @@ import { notify } from "../services/notifications"; import { workingDirectoryProblem } from "../services/folders"; import { isRepoFolder, listSources, reposDir } from "../services/workspaceSources"; import { - branchDiff, commitWork, commitsAhead, needsClone, @@ -57,7 +56,7 @@ import { pullRequestState, pushBranch, removeCheckout, - secretFilesAdded, + removeSecrets, type TaskRepo, } from "./git"; @@ -543,6 +542,7 @@ function worktreeBrief(task: TaskRow, w: Worktree): string { `You work in your own git worktree of ${w.repo} (your current directory), on the branch \`${w.branch}\` created from \`${w.base}\`. Other tasks and the human's own copy of the repository have their own files, so nothing you do here gets in their way.`, "Make every change here — not in other copies of the repository you may see. It's a fresh checkout: install dependencies first if you need to build or run something.", "The repository's stash, branches and settings are shared with other tasks (and the human's copy): don't use `git stash` (commit work in progress instead), don't switch or delete other branches, and don't change the git config.", + `Keep secrets out of the branch: never write passwords, API keys or tokens into files or commit messages — read them from the environment — and don't commit \`.env\` or key files. Before the branch is pushed, Godmode leaves such files out and replaces saved secrets with ${SECRET_PLACEHOLDER}.`, ...(task.type === "coding" ? [] : ["When you finish, Godmode commits what you changed here on this branch (it isn't pushed)."]), ].join("\n"); } @@ -784,17 +784,19 @@ async function keepWork(task: TaskRow, runId: string): Promise { else await commitLeftovers(task); } catch (err) { log.warn(`task ${task.id}: could not commit or push its changes`, err); - if (task.pushed_sha) { - const reason = err instanceof SecretInBranch ? `${err.message}, then push it again from the task.` : err instanceof Error ? err.message : String(err); - notify("warning", `Task #${task.number}: ${task.branch} wasn't pushed`, redact(reason), link); - } + if (task.pushed_sha) notify("warning", `Task #${task.number}: ${task.branch} wasn't pushed`, redact(err instanceof Error ? err.message : String(err)), link); } if (deliver(task.id, runId)) notify("success", `Task #${task.number} is ready for review`, task.title, link); } +/** The subject of the commits Godmode makes for a task — never with a secret, also when redaction is off. */ +function commitSubject(task: TaskRow): string { + return `${withoutSecrets(redact(task.title))} (#${task.number})`; +} + /** Commit what was left uncommitted in the task's worktree, except new files that look like secrets. */ async function commitLeftovers(task: TaskRow): Promise { - const { skipped } = await commitWork({ dir: checkoutDir(task.id), message: `${redact(task.title)} (#${task.number})` }); + const { skipped } = await commitWork({ dir: checkoutDir(task.id), message: commitSubject(task) }); if (skipped.length) notify("warning", `Task #${task.number}: files left out`, `Not committed because they look like secrets: ${skipped.join(", ")}`, `/tasks?task=${task.id}`); } @@ -817,24 +819,27 @@ function prBody(task: TaskRow, summary: string | null): string { ].join("\n"); } -/** The branch carries a secret, so it isn't pushed. The message ends with where to remove it. */ -class SecretInBranch extends Error {} - /** - * Commit and push the task's branch — never when its changes contain a secret from the vault (new env/key files are - * left out of the commit). `false` when it has no commits on top of its base. + * Commit and push the task's branch. Secrets never go along and never stop the push: new env/key files are left out + * of the commit, and what the agent committed itself is taken out first (see removeSecrets) — the human is told what + * Godmode changed. `false` when the branch has no commits on top of its base, or a turn that started meanwhile + * committed (its end pushes the branch). */ async function pushWork(task: TaskRow): Promise { const dir = checkoutDir(task.id); await commitLeftovers(task); - const secretFiles = await secretFilesAdded(dir, task.base_branch); - if (secretFiles.length) { - throw new SecretInBranch(`The branch ${task.branch} adds files that look like secrets (${secretFiles.join(", ")}), so Godmode didn't push it. Remove them from the branch (the task's worktree is in ${dir})`); + const { head, removed } = await removeSecrets({ dir, base: task.base_branch, lastPushed: task.pushed_sha, message: commitSubject(task), clean: withoutSecrets }); + if (!head) return false; + if (removed) { + const { left, replaced, kept } = removed; + const body = [ + left.length ? `Left out of the push because they look like secrets or hold one (they stay in the worktree): ${left.join(", ")}.` : "", + replaced.length ? `A saved secret was replaced with ${SECRET_PLACEHOLDER} in ${replaced.join(", ")} — make the code read it from the environment.` : "", + `The commits that weren't pushed yet were rewritten without the secrets; the branch as the agent left it stays in ${dir} as ${kept}.`, + ]; + notify("warning", `Task #${task.number}: secrets kept out of ${task.branch}`, withoutSecrets(body.filter(Boolean).join(" ")), `/tasks?task=${task.id}`); } - if (containsSecret(await branchDiff(dir, task.base_branch))) { - throw new SecretInBranch(`The changes on ${task.branch} contain a secret saved in the vault, so Godmode didn't push them. Remove it from the branch (the task's worktree is in ${dir})`); - } - const { pushed, sha } = await pushBranch({ dir, base: task.base_branch, branch: task.branch!, lastPushed: task.pushed_sha }); + const { pushed, sha } = await pushBranch({ dir, base: task.base_branch, branch: task.branch!, lastPushed: task.pushed_sha, head }); if (pushed) sql("UPDATE tasks SET pushed_sha = ? WHERE id = ?", sha, task.id); return pushed; } @@ -847,8 +852,9 @@ async function openTaskPullRequest(task: TaskRow, summary: string | null) { url: task.repo_url, base: task.base_branch, branch: task.branch!, - title: redact(task.title), - body: redact(prBody(task, summary)), + // Never a secret, also when redaction is off: the summary may quote what the agent wrote into a file. + title: withoutSecrets(redact(task.title)), + body: withoutSecrets(redact(prBody(task, summary))), }); const pr = result.pullRequest; if (pr) sql("UPDATE tasks SET pr_url = ?, pr_number = ?, pr_state = ? WHERE id = ?", pr.url, pr.number, pr.state, task.id); @@ -872,14 +878,7 @@ async function publish(task: TaskRow, summary: string | null, runId: string): Pr return; } setActivity(id, "Pushing the branch…"); - let pushed: boolean; - try { - pushed = await pushWork(task); - } catch (err) { - if (err instanceof SecretInBranch) return block(id, `${err.message}, then move the task to Todo.`); - throw err; - } - if (!pushed) { + if (!(await pushWork(task))) { if (deliver(id, runId)) notify("info", `Task #${task.number}: no code changes`, "The agent finished without changing the code.", link); return; } @@ -925,7 +924,6 @@ export async function pushTaskBranch(id: string, opts: { pullRequest: boolean }) } } catch (err) { if (err instanceof HttpError) throw err; - if (err instanceof SecretInBranch) throw conflict(`${err.message}, then try again.`); throw new HttpError(502, redact(`Couldn't push ${task.branch}: ${err instanceof Error ? err.message : String(err)}`), "push_failed"); } finally { activity.delete(id); diff --git a/packages/core/src/vault/vault.ts b/packages/core/src/vault/vault.ts index 81c6a70c..2225527b 100644 --- a/packages/core/src/vault/vault.ts +++ b/packages/core/src/vault/vault.ts @@ -22,6 +22,29 @@ let autoLockMinutes = 0; const knownSecrets = new Set(); /** Bumped when the known secrets are forgotten (the vault locked): see `redactionEpoch`. */ let secretsForgotten = 0; +/** + * The ones kept out of what Godmode pushes: stored as a secret (not a note, not a server's plain settings) and not a + * plain word or number that code has anyway. Always in `knownSecrets` too. + */ +const pushSecrets = new Set(); +/** What a secret taken out of a file is replaced with: plain letters, so it fits wherever the secret stood. */ +export const SECRET_PLACEHOLDER = "GODMODE_REMOVED_SECRET"; +/** Sealed values that aren't one secret: free-text notes, and the JSON of a map whose values are remembered one by one. */ +const SEALED_NOT_SECRET = /^(credentials\.notes|mcp_servers\.(env|headers)|messaging_connections\.secrets):/; +/** Names of env variables and headers that hold a secret (API_KEY, botToken, Authorization, SENTRY_DSN) — the rest is settings. */ +const SECRET_NAME = /(pass(word|wd|phrase)|secret|token|(api|private|access)key|authorization|credentials?|cookie|signature|dsn|webhook([_-]?url)?|connection[_-]?string|(^|[_-])(pass|pwd|key|auth|pat))$/i; +/** …except keys meant to be public (STRIPE_PUBLISHABLE_KEY, NEXT_PUBLIC_…, an anon key). */ +const PUBLIC_NAME = /(^|[_-])(public|publishable|anon)([_-]|$)/i; + +/** Whether a value of an env/header map is a secret, going by its name. */ +function secretValue(name: string, value: string): boolean { + const words = name.replace(/([a-z0-9])([A-Z])/g, "$1_$2"); // signingKey → signing_Key + return SECRET_NAME.test(words) && !PUBLIC_NAME.test(words) && !(/key$/i.test(words) && KEY_SETTING.test(value)); +} +/** A single word or number ("postgres", "12345678"): in code by chance, so never treated as a secret there. */ +const PLAIN = /^([a-z]{1,15}|\d{1,15})$/i; +/** A snake_case identifier under a key-sounding name (SORT_KEY=created_at, cacheKey=user_id): a setting, not a key. */ +const KEY_SETTING = /^[a-z]+(_[a-z]+)*$/; function keychainName(): string { return `vault-dek-${sha256(config().dataDir).slice(0, 16)}`; @@ -111,6 +134,7 @@ export function lock() { if (dek) dek.fill(0); dek = null; knownSecrets.clear(); + pushSecrets.clear(); secretsForgotten++; emitStatus(); } @@ -214,7 +238,7 @@ function requireKey(): Buffer { /** Encrypt a secret field. `context` binds ciphertext to its location (AAD), e.g. "credentials.password:". */ export function seal(plaintext: string, context: string): string { const key = requireKey(); - rememberSecret(plaintext); + rememberSecret(plaintext, !SEALED_NOT_SECRET.test(context)); return encrypt(key, plaintext, context); } @@ -290,16 +314,27 @@ export function hasAppSecret(key: string): boolean { /** Words every login page and every tool message uses: masking them hides nothing and garbles what agents read. */ const NOT_A_SECRET = new Set(["password", "passwort", "username", "benutzername", "secret", "passphrase"]); -export function rememberSecret(value: string | null | undefined) { - if (value && value.length >= 6 && !NOT_A_SECRET.has(value.toLowerCase())) knownSecrets.add(value); +/** + * Remember a value for redaction. `secret: false` = masked in transcripts only (it may be plain configuration); + * otherwise it is also kept out of pushes — unless it is too short or plain ("postgres") to tell from ordinary code. + */ +export function rememberSecret(value: string | null | undefined, secret = true) { + if (!value || value.length < 6 || NOT_A_SECRET.has(value.toLowerCase())) return; + knownSecrets.add(value); + if (secret && value.length >= 8 && !PLAIN.test(value)) pushSecrets.add(value); } -/** Remember every value of an env/header map, plus the token of "Bearer "-style auth values. */ +/** + * Remember every value of an env/header map, plus the token of "Bearer "-style auth values and the password + * inside a URL. Only values under a name that says secret count as one: NODE_ENV=production is a setting. + */ export function rememberSecretValues(values: Record) { - for (const value of Object.values(values)) { + for (const [name, value] of Object.entries(values)) { if (typeof value !== "string") continue; - rememberSecret(value); + rememberSecret(value, secretValue(name, value)); rememberSecret(/^(?:bearer|basic|token|bot|key|apikey)\s+(\S+)$/i.exec(value.trim())?.[1]); + const password = /\/\/[^/\s:@]*:([^/\s@]+)@/.exec(value)?.[1]; + if (password && !PLAIN.test(password)) rememberSecret(password); } } @@ -382,13 +417,16 @@ function loadKnownSecrets() { } } -/** Replace every known secret value in `text` with a mask. */ -/** The text contains a secret Godmode knows (a password, 2FA secret, API key) — regardless of the redaction setting. */ -export function containsSecret(text: string): boolean { - for (const secret of knownSecrets) if (secret.length >= 8 && text.includes(secret)) return true; - return false; +/** + * `text` with every secret that must not be pushed replaced by the placeholder (the longest first: one may contain + * another) — regardless of the redaction setting. + */ +export function withoutSecrets(text: string): string { + const found = [...pushSecrets].filter((secret) => text.includes(secret)).sort((a, b) => b.length - a.length); + return found.reduce((out, secret) => out.split(secret).join(SECRET_PLACEHOLDER), text); } +/** Replace every known secret value in `text` with a mask. */ export function redact(text: string): string { if (!text || knownSecrets.size === 0 || !getSettings().security.redactSecrets) return text; return maskKnownSecrets(text); diff --git a/packages/core/test/fixtures/fake-claude.ts b/packages/core/test/fixtures/fake-claude.ts index 4c3d292c..b74042ca 100755 --- a/packages/core/test/fixtures/fake-claude.ts +++ b/packages/core/test/fixtures/fake-claude.ts @@ -22,6 +22,10 @@ * TASK_COMMIT_ENV write and commit .env.production in the cwd * TASK_ENV write .env and feature.txt into the cwd * TASK_LEAK: write config.txt containing into the cwd + * TASK_HISTORY: commit .env.production and a config.txt containing (also named in the commit message), + * then commit config.txt without it and feature.txt + * TASK_LEAK_BYTES: write legacy.txt containing in Latin-1 (not UTF-8), and feature.txt, into the cwd + * TASK_TIDY in settings.txt, remove the `old_token=` line and add plain settings (a region, a base URL, a database) * TASK_SHOTS: answer with a summary naming the files in in every way an agent does (code, links, paths) * TASK_BLOCKED call the gateway's task_report_blocked and answer "BLOCKED {json}" * CRASH print to stderr and exit 3 without a result @@ -349,6 +353,36 @@ if (slash?.[1] === "clear") { const text = "Wrote the config."; textTurn(text); result(text); +} else if (prompt.includes("TASK_HISTORY:")) { + out(init); + const value = /TASK_HISTORY:(\S+)/.exec(prompt)![1]!; + const git = (...a: string[]) => Bun.spawnSync(["git", "-c", "user.name=Agent", "-c", "user.email=agent@example.com", ...a], { cwd: process.cwd() }); + writeFileSync(join(process.cwd(), ".env.production"), "API_TOKEN=abc123\n"); + writeFileSync(join(process.cwd(), "config.txt"), `token=${value}\n`); + git("add", "-f", ".env.production", "config.txt"); + git("commit", "-qm", `Configure with ${value}`); + writeFileSync(join(process.cwd(), "config.txt"), "token=$API_TOKEN\n"); + writeFileSync(join(process.cwd(), "feature.txt"), "a feature\n"); + git("add", "config.txt", "feature.txt"); + git("commit", "-qm", "Read the token from the environment"); + const text = "Configured it."; + textTurn(text); + result(text); +} else if (prompt.includes("TASK_LEAK_BYTES:")) { + out(init); + const line = `token=${/TASK_LEAK_BYTES:(\S+)/.exec(prompt)![1]}\n`; + writeFileSync(join(process.cwd(), "legacy.txt"), Buffer.concat([Buffer.from([0x63, 0x61, 0x66, 0xe9, 0x0a]), Buffer.from(line)])); + writeFileSync(join(process.cwd(), "feature.txt"), "a feature\n"); + const text = "Wrote the legacy config."; + textTurn(text); + result(text); +} else if (prompt.includes("TASK_TIDY")) { + out(init); + const file = join(process.cwd(), "settings.txt"); + writeFileSync(file, `${readFileSync(file, "utf8").replace(/^old_token=.*\n/m, "")}region=eu-central-1\nbase=https://api.example.com/v1\ndb=postgres\n`); + const text = "Tidied the settings."; + textTurn(text); + result(text); } else if (prompt.includes("TASK_SHOTS:")) { out(init); const dir = /TASK_SHOTS:(\S+)/.exec(prompt)![1]!; diff --git a/packages/core/test/tasks.test.ts b/packages/core/test/tasks.test.ts index 4eaf730b..a47488d5 100644 --- a/packages/core/test/tasks.test.ts +++ b/packages/core/test/tasks.test.ts @@ -27,9 +27,10 @@ import { import { getFollowup, scheduleFollowup } from "../src/services/followups"; import { pauseConversation } from "../src/services/pauses"; import { sendMessage } from "../src/services/conversations"; -import { __setGhForTests, compareUrl, openPullRequest, repoCacheDir } from "../src/tasks/git"; +import { __setGhForTests, compareUrl, openPullRequest, pushBranch, removeSecrets, repoCacheDir } from "../src/tasks/git"; import { HttpError } from "../src/util"; -import { rememberSecret } from "../src/vault/vault"; +import { redact, rememberSecret, rememberSecretValues, withoutSecrets } from "../src/vault/vault"; +import { listNotifications } from "../src/services/notifications"; import { updateSettings } from "../src/services/settings"; import { mkdtempSync } from "node:fs"; @@ -701,22 +702,286 @@ describe("races and safety", () => { expect(files).not.toContain(".env"); }); - test("a secret-looking file the agent committed itself blocks the push", async () => { + const secret = "Zq9-vault-Secret-4242"; + /** Everything a remote has: the messages and patches of every commit on every branch. */ + const published = (bare: string) => git(["log", "--all", "-p", "--format=%B"], bare); + const secretsNotice = (t: Task) => listNotifications().find((n) => n.title === `Task #${t.number}: secrets kept out of ${t.branch}`); + /** The branch as the agent left it, kept in the task's worktree. */ + const keptRef = (id: string) => git(["for-each-ref", "--format=%(refname)", "refs/worktree/godmode"], checkoutDir(id)); + + test("a branch that only adds a secret-looking file has nothing to push", async () => { const task = createTask({ workspaceId, title: "TASK_COMMIT_ENV configure production", type: "coding", agentId: wsAgent.id }); - await settled(task.id, ["blocked"]); + await settled(task.id, ["in_review"]); const t = getTask(task.id); - expect(t.blockedReason).toContain(".env.production"); expect(await git(["branch", "--list", t.branch!], remote)).toBe(""); + // Still there for the agent and the human, just not on the branch anymore. + expect(existsSync(join(checkoutDir(t.id), ".env.production"))).toBe(true); + expect(await git(["status", "--porcelain"], checkoutDir(t.id))).toBe("?? .env.production"); + expect(secretsNotice(t)?.body).toContain(".env.production"); }); - test("changes containing a vault secret are not pushed", async () => { - const secret = "Zq9-vault-Secret-4242"; + test("secrets the agent committed are taken out of every commit before the push", async () => { + rememberSecret(secret); + const task = createTask({ workspaceId, title: "Configure it", description: `TASK_HISTORY:${secret}`, type: "coding", agentId: wsAgent.id }); + await settled(task.id, ["in_review"]); + const t = getTask(task.id); + expect(t.branchPushed).toBe(true); + expect((await git(["ls-tree", "-r", "--name-only", t.branch!], remote)).split("\n").sort()).toEqual(["README.md", "config.txt", "feature.txt"]); + // The agent's second commit removed the secret again, but its first would have been pushed too: they became one. + expect(await git(["log", "--format=%s%n%b", `main..${t.branch}`], remote)).toBe( + `${t.title} (#${t.number})\n- Configure with GODMODE_REMOVED_SECRET\n- Read the token from the environment`, + ); + const all = await published(remote); + expect(all).not.toContain(secret); + expect(all).not.toContain("API_TOKEN=abc123"); + // Nothing is lost: the file stays in the worktree and the agent's commits stay reachable there. + const dir = checkoutDir(t.id); + expect(readFileSync(join(dir, ".env.production"), "utf8")).toBe("API_TOKEN=abc123\n"); + const kept = await keptRef(t.id); + expect(await git(["log", "--format=%s", kept], dir)).toContain(`Configure with ${secret}`); + const notice = secretsNotice(t)!; + expect(notice.kind).toBe("warning"); + expect(notice.body).toContain(".env.production"); + expect(notice.body).toContain(kept); + expect(notice.body).not.toContain(secret); + }); + + test("a vault secret in the agent's changes is replaced, never pushed; a follow-up's too", async () => { rememberSecret(secret); const task = createTask({ workspaceId, title: "Configure it", description: `TASK_LEAK:${secret}`, type: "coding", agentId: wsAgent.id }); - await settled(task.id, ["blocked"]); + await settled(task.id, ["in_review"]); const t = getTask(task.id); - expect(t.blockedReason).toContain("secret saved in the vault"); - expect(await git(["branch", "--list", t.branch!], remote)).toBe(""); + const dir = checkoutDir(t.id); + expect(await git(["show", `${t.branch}:config.txt`], remote)).toBe("token=GODMODE_REMOVED_SECRET"); + expect(readFileSync(join(dir, "config.txt"), "utf8")).toBe("token=GODMODE_REMOVED_SECRET\n"); + expect(await git(["show", `${await keptRef(t.id)}:config.txt`], dir)).toBe(`token=${secret}`); + expect(secretsNotice(t)?.body).toContain("config.txt"); + expect(await git(["status", "--porcelain"], dir)).toBe(""); + + // Pushed before: only the new commit is rewritten, and it lands on top of what the remote has. + const first = await git(["rev-parse", t.branch!], remote); + await sendTaskMessage(t.id, `TASK_LEAK:${secret}-again`); + await until(() => getTask(t.id).runId !== t.runId, 5_000, "follow-up run"); + await settled(t.id, ["in_review"]); + expect(await git(["rev-parse", `${t.branch}~1`], remote)).toBe(first); + expect(await git(["show", `${t.branch}:config.txt`], remote)).toBe("token=GODMODE_REMOVED_SECRET-again"); + expect(await published(remote)).not.toContain(secret); + expect((await keptRef(t.id)).split("\n")).toHaveLength(2); + }); + + test("a file whose secret can't be replaced is left out of the push", async () => { + rememberSecret(secret); + const task = createTask({ workspaceId, title: "Keep the legacy config", description: `TASK_LEAK_BYTES:${secret}`, type: "coding", agentId: wsAgent.id }); + await settled(task.id, ["in_review"]); + const t = getTask(task.id); + // Not UTF-8, so rewriting it could damage it: the rest of the work is pushed, the file stays where it is. + expect((await git(["ls-tree", "-r", "--name-only", t.branch!], remote)).split("\n").sort()).toEqual(["README.md", "feature.txt"]); + expect(await published(remote)).not.toContain(secret); + expect(readFileSync(join(checkoutDir(t.id), "legacy.txt")).includes(`token=${secret}`)).toBe(true); + expect(secretsNotice(t)?.body).toContain("legacy.txt"); + }); + + test("plain settings, removed lines and the lines around a change never count as a secret", async () => { + rememberSecret(secret); + // A saved password that is an ordinary word, and what a custom MCP server is given: settings, and one token. + rememberSecret("postgres"); + rememberSecretValues({ AWS_REGION: "eu-central-1", API_BASE: "https://api.example.com/v1", API_TOKEN: "tok-Config-Value-9911" }); + expect(redact("region=eu-central-1 db=postgres")).toBe("region=•••••••• db=••••••••"); + + const { url, bare } = makeRemote("plain-settings"); + const seed = mkdtempSync(join(env.dataDir, "seed-")); + await git(["clone", "-q", url, seed], env.dataDir); + writeFileSync(join(seed, "settings.txt"), `name=app\ntoken=${secret}\nport=3000\nold_token=${secret}\n`); + await git(["add", "-A"], seed); + await git(["-c", "user.name=Human", "-c", "user.email=h@example.com", "commit", "-qm", "Settings"], seed); + await git(["push", "-q", "origin", "main"], seed); + + // The agent removes one line with the secret and adds settings right below another one the repository already had. + const task = createTask({ title: "TASK_TIDY the settings", type: "coding", repoUrl: url, agentId: agent.id }); + await settled(task.id, ["in_review"]); + const t = getTask(task.id); + expect(await git(["show", `${t.branch}:settings.txt`], bare)).toBe(`name=app\ntoken=${secret}\nport=3000\nregion=eu-central-1\nbase=https://api.example.com/v1\ndb=postgres`); + // Pushed as it was committed: nothing rewritten, nothing kept aside, nothing to tell. + expect(await git(["log", "--format=%s", `main..${t.branch}`], bare)).toBe(`${t.title} (#${t.number})`); + expect(await keptRef(t.id)).toBe(""); + expect(secretsNotice(t)).toBeUndefined(); + }); + + test("pushing from the board takes secrets out instead of refusing", async () => { + rememberSecret(secret); + const { url, bare } = makeRemote("board-secret"); + const task = createTask({ title: "Note it down", description: `TASK_LEAK:${secret}`, repoUrl: url, agentId: agent.id }); + await settled(task.id, ["in_review"]); + const t = getTask(task.id); + // Not pushed, so nothing was touched: the agent's file is committed as it is. + expect(t.branchPushed).toBe(false); + expect(await git(["show", `${t.branch}:config.txt`], checkoutDir(t.id))).toBe(`token=${secret}`); + + const pushed = await pushTaskBranch(t.id, { pullRequest: false }); + expect(pushed.branchPushed).toBe(true); + expect(await git(["show", `${t.branch}:config.txt`], bare)).toBe("token=GODMODE_REMOVED_SECRET"); + expect(await published(bare)).not.toContain(secret); + expect(secretsNotice(t)?.body).toContain("config.txt"); + + // Follow-ups keep the pushed branch up to date — cleaned the same way, without a "wasn't pushed" warning. + await sendTaskMessage(t.id, `TASK_LEAK:${secret}-again`); + await until(() => getTask(t.id).runId !== t.runId, 5_000, "follow-up run"); + await settled(t.id, ["in_review"]); + expect(await git(["show", `${t.branch}:config.txt`], bare)).toBe("token=GODMODE_REMOVED_SECRET-again"); + expect(await published(bare)).not.toContain(secret); + expect(listNotifications().some((n) => n.title.includes(`${t.branch} wasn't pushed`))).toBe(false); + }); + + test("a secret added after the base was merged into a pushed branch: the cleaned commit keeps both, so the push fast-forwards", async () => { + rememberSecret(secret); + const { url, bare } = makeRemote("merged-base"); + const dir = mkdtempSync(join(env.dataDir, "merged-")); + const agentGit = (...args: string[]) => git(["-c", "user.name=Agent", "-c", "user.email=agent@example.com", ...args], dir); + const commit = async (message: string, file: string, content: string | Buffer) => { + writeFileSync(join(dir, file), content); + await agentGit("add", "-A"); + await agentGit("commit", "-qm", message); + }; + // Saved in the vault too, but the base has had it for long: already public, so a file moved from there isn't touched. + const known = "Base-known-Secret-7731"; + rememberSecret(known); + const legacy = Buffer.from([0x63, 0x61, 0x66, 0xe9, 0x0a]); // Latin-1: can't be rewritten safely + const branch = "godmode/1-task"; + await git(["clone", "-q", url, dir], env.dataDir); + await agentGit("checkout", "-q", "-b", branch); + await commit("First", "legacy.txt", legacy); + await commit("Second", "a.txt", "one\n"); + const first = (await pushBranch({ dir, base: "main", branch, lastPushed: null })).sha; + // The base moves on; the agent merges it, moves a file of the base, then leaks — in text, in a binary file, in a file it can't rewrite. + await agentGit("checkout", "-q", "main"); + await commit("Base moves on", "old.js", `const a = 1;\nconst b = 2;\nconst c = 3;\nconst key = "${known}";\nconst d = 4;\n`); + await agentGit("push", "-q", "origin", "main"); + await agentGit("checkout", "-q", branch); + await agentGit("merge", "-q", "--no-edit", "origin/main"); + await agentGit("mv", "old.js", "new.js"); + await commit("Move it", "new.js", `const a = 1;\nconst b = 2;\nconst c = 3;\nconst key = "${known}";\nconst d = 5;\n`); + await commit("Configure", "config.txt", `token=${secret}\n`); + await commit("Store it", "data.db", Buffer.concat([Buffer.from([0, 0, 1]), Buffer.from(`token=${secret}`)])); + await commit("Note it", "legacy.txt", Buffer.concat([legacy, Buffer.from(`token=${secret}\n`)])); + + const { head, removed } = await removeSecrets({ dir, base: "main", lastPushed: first, message: "Task (#1)", clean: withoutSecrets }); + expect(removed?.replaced).toEqual(["config.txt"]); + expect(removed?.left.sort()).toEqual(["data.db", "legacy.txt"]); + // The branch's own commit first: its history reads as the branch's, and left-out files keep the branch's version. + expect((await agentGit("log", "-1", "--format=%P")).split(" ")).toEqual([first, await agentGit("rev-parse", "origin/main")]); + expect((await pushBranch({ dir, base: "main", branch, lastPushed: first, head: head! })).pushed).toBe(true); + // What a pull request shows: the agent's work only — and nothing the remote had was replaced. + expect((await git(["diff", "--name-only", `main...${branch}`], bare)).split("\n").sort()).toEqual(["a.txt", "config.txt", "legacy.txt", "new.js"]); + expect(await git(["show", `${branch}:legacy.txt`], bare)).toBe("caf�"); + expect(await git(["show", `${branch}:new.js`], bare)).toContain(known); + expect(await git(["merge-base", "--is-ancestor", first, branch], bare)).toBe(""); + expect(await published(bare)).not.toContain(secret); + expect(await agentGit("show", `${removed!.kept}:config.txt`)).toBe(`token=${secret}`); + }); + + test("a file Godmode can't rewrite is left out instead of stopping the push", async () => { + rememberSecret(secret); + const { url, bare } = makeRemote("read-only"); + const dir = mkdtempSync(join(env.dataDir, "read-only-")); + const agentGit = (...args: string[]) => git(["-c", "user.name=Agent", "-c", "user.email=agent@example.com", ...args], dir); + await git(["clone", "-q", url, dir], env.dataDir); + await agentGit("checkout", "-q", "-b", "godmode/2-task"); + writeFileSync(join(dir, "config.txt"), `token=${secret}\n`); + writeFileSync(join(dir, "feature.txt"), "a feature\n"); + chmodSync(join(dir, "config.txt"), 0o444); + await agentGit("add", "-A"); + await agentGit("commit", "-qm", "Configure"); + + const { head, removed } = await removeSecrets({ dir, base: "main", lastPushed: null, message: "Task (#2)", clean: withoutSecrets }); + expect(removed).toEqual({ left: ["config.txt"], replaced: [], kept: expect.stringContaining("refs/worktree/godmode/with-secrets/") }); + expect((await pushBranch({ dir, base: "main", branch: "godmode/2-task", lastPushed: null, head: head! })).pushed).toBe(true); + expect((await git(["ls-tree", "-r", "--name-only", "godmode/2-task"], bare)).split("\n").sort()).toEqual(["README.md", "feature.txt"]); + expect(readFileSync(join(dir, "config.txt"), "utf8")).toBe(`token=${secret}\n`); + }); + + test("a binary file that held a secret only in an earlier commit is taken out of the history", async () => { + rememberSecret(secret); + const { url, bare } = makeRemote("binary-history"); + const dir = mkdtempSync(join(env.dataDir, "binary-")); + const agentGit = (...args: string[]) => git(["-c", "user.name=Agent", "-c", "user.email=agent@example.com", ...args], dir); + await git(["clone", "-q", url, dir], env.dataDir); + await agentGit("checkout", "-q", "-b", "godmode/4-task"); + writeFileSync(join(dir, "dump.bin"), Buffer.concat([Buffer.from([0, 1]), Buffer.from(`token=${secret}`)])); + writeFileSync(join(dir, "feature.txt"), "a feature\n"); + await agentGit("add", "-A"); + await agentGit("commit", "-qm", "Dump"); + await agentGit("rm", "-q", "dump.bin"); + await agentGit("commit", "-qm", "Drop the dump"); + + const { head, removed } = await removeSecrets({ dir, base: "main", lastPushed: null, message: "Task (#4)", clean: withoutSecrets }); + expect(removed?.left).toEqual([]); + expect((await pushBranch({ dir, base: "main", branch: "godmode/4-task", lastPushed: null, head: head! })).pushed).toBe(true); + expect((await git(["ls-tree", "-r", "--name-only", "godmode/4-task"], bare)).split("\n").sort()).toEqual(["README.md", "feature.txt"]); + expect(await git(["cat-file", "--batch-all-objects", "--batch"], bare)).not.toContain(secret); + }); + + test("what a turn stages or commits while Godmode pushes is never pushed unchecked", async () => { + rememberSecret(secret); + const { url, bare } = makeRemote("busy-turn"); + const dir = mkdtempSync(join(env.dataDir, "busy-")); + const agentGit = (...args: string[]) => git(["-c", "user.name=Agent", "-c", "user.email=agent@example.com", ...args], dir); + const branch = "godmode/3-task"; + await git(["clone", "-q", url, dir], env.dataDir); + await agentGit("checkout", "-q", "-b", branch); + writeFileSync(join(dir, "config.txt"), `token=${secret}\n`); + await agentGit("add", "-A"); + await agentGit("commit", "-qm", "Configure"); + const old = await agentGit("rev-parse", "HEAD"); + + // The agent stages a file while Godmode rewrites config.txt: the fix is dropped, that turn's end pushes. + let staged = false; + const clean = (text: string) => { + if (!staged && text === `token=${secret}\n`) { + staged = true; + writeFileSync(join(dir, "agent-new.txt"), `token=${secret}\n`); + Bun.spawnSync(["git", "add", "agent-new.txt"], { cwd: dir }); + } + return withoutSecrets(text); + }; + expect(await removeSecrets({ dir, base: "main", lastPushed: null, message: "Task (#3)", clean })).toEqual({ head: null, removed: null }); + expect(staged).toBe(true); + expect(await agentGit("rev-parse", "HEAD")).toBe(old); + expect(await git(["for-each-ref", "refs/worktree/godmode"], dir)).toBe(""); + + // The agent stages config.txt again, with the secret, right after Godmode cleaned it. + await agentGit("reset", "-q", "--hard", old); + let restaged = false; + const again = (text: string) => { + if (!restaged && text.startsWith("token=GODMODE_REMOVED_SECRET")) { + restaged = true; + writeFileSync(join(dir, "config.txt"), `token=${secret}\nmore=1\n`); + Bun.spawnSync(["git", "add", "config.txt"], { cwd: dir }); + } + return withoutSecrets(text); + }; + expect(await removeSecrets({ dir, base: "main", lastPushed: null, message: "Task (#3)", clean: again })).toEqual({ head: null, removed: null }); + expect(restaged).toBe(true); + expect(await agentGit("rev-parse", "HEAD")).toBe(old); + + // Someone else pushed to the branch, and the agent committed after the check: no merge of unchecked commits, no push. + await agentGit("reset", "-q", "--hard", old); + const checked = (await removeSecrets({ dir, base: "main", lastPushed: null, message: "Task (#3)", clean: withoutSecrets })).head!; + const first = (await pushBranch({ dir, base: "main", branch, lastPushed: null, head: checked })).sha; + const reviewer = mkdtempSync(join(env.dataDir, "reviewer-")); + await git(["clone", "-q", "--branch", branch, url, reviewer], env.dataDir); + writeFileSync(join(reviewer, "REVIEW.md"), "fix\n"); + await git(["add", "-A"], reviewer); + await git(["-c", "user.name=Reviewer", "-c", "user.email=r@example.com", "commit", "-qm", "Review"], reviewer); + await git(["push", "-q", "origin", branch], reviewer); + writeFileSync(join(dir, "feature.txt"), "a feature\n"); + await agentGit("add", "-A"); + await agentGit("commit", "-qm", "Feature"); + const head = await agentGit("rev-parse", "HEAD"); + writeFileSync(join(dir, "leak.txt"), `token=${secret}\n`); + await agentGit("add", "-A"); + await agentGit("commit", "-qm", "Leak after the check"); + expect(await pushBranch({ dir, base: "main", branch, lastPushed: first, head })).toEqual({ pushed: false, sha: head }); + expect(await published(bare)).not.toContain(secret); }); test("task numbers are never reused", async () => { diff --git a/packages/core/test/vault.test.ts b/packages/core/test/vault.test.ts index ddca2ab0..986aaa95 100644 --- a/packages/core/test/vault.test.ts +++ b/packages/core/test/vault.test.ts @@ -183,4 +183,48 @@ describe("vault lifecycle", () => { expect(vault.hasAppSecret("openai_api_key")).toBe(false); expect(vault.getAppSecret("openai_api_key")).toBeNull(); }); + + test("only what is stored as a secret is kept out of pushes; everything stays redacted", () => { + // What a custom MCP server is given: settings, a token, and URLs with a password in them. + vault.rememberSecretValues({ + AWS_REGION: "eu-central-1", + API_BASE: "https://api.example.com/v1", + API_TOKEN: "tok-Config-Value-9911", + DATABASE_URL: "postgres://app:S3cr3t-pw-123@db.example.com/app", + DEV_DATABASE_URL: "postgres://dev:localhost@127.0.0.1/dev", + }); + vault.seal("Production account, ask Dana", "credentials.notes:cred_n"); + // A saved password that is an ordinary word: code is full of it anyway. + vault.seal("postgres", "credentials.password:cred_p"); + const text = "eu-central-1 https://api.example.com/v1 tok-Config-Value-9911 S3cr3t-pw-123 postgres Production account, ask Dana"; + expect(vault.withoutSecrets(text)).toBe("eu-central-1 https://api.example.com/v1 GODMODE_REMOVED_SECRET GODMODE_REMOVED_SECRET postgres Production account, ask Dana"); + expect(vault.redact(text)).toBe("•••••••• •••••••• •••••••• •••••••• •••••••• ••••••••"); + // The plain password of a URL isn't a secret by itself, so the word stays readable in transcripts. + expect(vault.redact("runs on localhost")).toBe("runs on localhost"); + // Names in any style; keys meant to be public stay readable in code. + vault.rememberSecretValues({ + signingKey: "sign-Value-0001", + SENTRY_DSN: "https://abc123@o1.ingest.sentry.io/42", + SLACK_WEBHOOK_URL: "https://hooks.slack.com/services/T0/B0/xyzXYZ", + AZURE_STORAGE_CONNECTION_STRING: "AccountName=acme;AccountKey=a1b2c3d4==", + REDIS_URL: "redis://:R3dis-pw-55@cache:6379", + STRIPE_PUBLISHABLE_KEY: "pk_live_Public-0001", + NEXT_PUBLIC_SUPABASE_ANON_KEY: "anon-Public-0002", + // A setting under a secret-sounding name: an identifier code is full of. + SORT_KEY: "created_at", + }); + expect(vault.withoutSecrets("ORDER BY created_at DESC")).toBe("ORDER BY created_at DESC"); + // Passphrases are secrets, however plain their words: saved as a password, or under a name that says so. + vault.seal("pasta-jockey-clinic", "credentials.password:cred_w"); + vault.rememberSecretValues({ DB_PASSWORD: "spoken_glider_amaze", DATABASE_URL: "postgres://app:dev-db-pass@db/app" }); + expect(vault.withoutSecrets("pasta-jockey-clinic spoken_glider_amaze dev-db-pass")).toBe(Array(3).fill("GODMODE_REMOVED_SECRET").join(" ")); + const named = "sign-Value-0001 https://abc123@o1.ingest.sentry.io/42 https://hooks.slack.com/services/T0/B0/xyzXYZ AccountName=acme;AccountKey=a1b2c3d4== R3dis-pw-55 pk_live_Public-0001 anon-Public-0002"; + expect(vault.withoutSecrets(named)).toBe(`${Array(5).fill("GODMODE_REMOVED_SECRET").join(" ")} pk_live_Public-0001 anon-Public-0002`); + // A secret inside another one goes with it. + vault.seal("Summer-2024", "credentials.password:cred_a"); + vault.seal("Summer-2024-Berlin", "credentials.password:cred_b"); + expect(vault.withoutSecrets("a=Summer-2024-Berlin b=Summer-2024")).toBe("a=GODMODE_REMOVED_SECRET b=GODMODE_REMOVED_SECRET"); + vault.lock(); + expect(vault.withoutSecrets(text)).toBe(text); + }); });