diff --git a/apps/connector/src/acp.ts b/apps/connector/src/acp.ts index f8cae8734..ca858dfbc 100644 --- a/apps/connector/src/acp.ts +++ b/apps/connector/src/acp.ts @@ -7,6 +7,7 @@ import type { } from "@agentclientprotocol/sdk"; import { PROCESSES, stopBackend } from "./planner"; import { type Sandbox, spawnIn } from "./sandbox"; +import { withoutGitHub } from "./github-token"; import { VERSION } from "./version"; /** How long a stopped agent's process group has to exit before it is killed. */ @@ -240,7 +241,8 @@ export async function runAgent(options: AgentOptions): Promise { app, options.command, options.cwd, - options.env ? { ...process.env, ...options.env } : undefined, + // Nothing of the machine's GitHub setup reaches a run's agent; its own comes in `env`. + { ...withoutGitHub(process.env), ...options.env }, options.stderr, { ...(options.sandbox ? { sandbox: options.sandbox } : {}), diff --git a/apps/connector/src/binary.test.ts b/apps/connector/src/binary.test.ts index 6d004e27a..f73c4e409 100644 --- a/apps/connector/src/binary.test.ts +++ b/apps/connector/src/binary.test.ts @@ -7,6 +7,7 @@ import { Server } from "@modelcontextprotocol/sdk/server/index.js"; import { WebStandardStreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/webStandardStreamableHttp.js"; import { CallToolRequestSchema, ListToolsRequestSchema } from "@modelcontextprotocol/sdk/types.js"; import { compiledChopin } from "./testing/binary"; +import { answerTokens } from "./testing/github-grant"; import { git } from "./workspace"; let scratch = await mkdtemp(join(tmpdir(), "chopin-binary-")); @@ -181,6 +182,9 @@ test.skipIf(!binary)( if (path === "/api/connector/connections") { return Response.json({ token: "connection-token" }); } + // The run's GitHub token, which the binary asks for before anything starts. + let issued = await answerTokens(request); + if (issued) return issued; let token = request.headers.get("authorization")?.replace(/^Bearer /, "") ?? ""; if (path !== "/connector/mcp" || !["connection-token", "run-token"].includes(token)) { return new Response(null, { status: 401 }); diff --git a/apps/connector/src/checkouts.test.ts b/apps/connector/src/checkouts.test.ts index 0bccea57e..adf1ecf18 100644 --- a/apps/connector/src/checkouts.test.ts +++ b/apps/connector/src/checkouts.test.ts @@ -5,7 +5,14 @@ import { join } from "node:path"; import { spawnSync } from "node:child_process"; import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { checkouts, clonePath, clones, noCheckout, registerCheckouts, sshCause } from "./checkouts"; -import { gitCredentials, githubEnvironment, type GitHubToken } from "./github-token"; +import { + gitCredentials, + githubEnvironment, + githubRefusal, + GitHubRefused, + type GitHubToken, +} from "./github-token"; +import { BOT } from "./testing/github-grant"; import { git } from "./workspace"; const TOKEN = "ghu_MachineToken0123456789"; @@ -78,6 +85,9 @@ async function localGitHub(global = "") { token: TOKEN, expiresAt: Date.now() + 3_600_000, file, + config: join(scratch, "gh"), + identity: BOT, + expired: new AbortController().signal, secrets: () => [TOKEN], stop: async () => {}, }; @@ -195,13 +205,14 @@ test.skipIf(process.platform === "win32")( }, ); -test("work for a repository with no checkout and no GitHub token fails naming --checkout", async () => { +test("work for a repository with no checkout fails as Chopin refused its GitHub token, or naming --checkout without a way to ask", async () => { let base = await mkdtemp(join(tmpdir(), "chopin-checkouts-")); try { let machine = checkouts({ directory: base, registered: {} }); - await expect(machine.resolve("org/repo", async () => undefined)).rejects.toThrow( - noCheckout("org/repo"), - ); + await expect(machine.resolve("org/repo", async () => { + throw new GitHubRefused("repository-forbidden", "org/repo"); + })).rejects.toThrow(githubRefusal("repository-forbidden", "org/repo")); + await expect(machine.resolve("org/repo")).rejects.toThrow(noCheckout("org/repo")); expect(noCheckout("org/repo")).toContain("chopin connect --checkout "); await expect(machine.resolve("../repo")).rejects.toThrow("invalid repository"); } finally { @@ -295,10 +306,21 @@ test.skipIf(process.platform === "win32")( git(root, "init", "--quiet"); git(root, "remote", "add", "origin", "https://github.com/Org/Repo.git"); let env = githubEnvironment( - { token: TOKEN, expiresAt: 0, file: "/nowhere", secrets: () => [], stop: async () => {} }, + { + token: TOKEN, + expiresAt: 0, + file: "/nowhere", + config: "/nowhere-gh", + identity: BOT, + expired: new AbortController().signal, + secrets: () => [], + stop: async () => {}, + }, "Org/Repo", ); - expect(env.GH_TOKEN).toBe(TOKEN); + // gh reads the launch's own configuration; no token variable would win over it. + expect(env.GH_CONFIG_DIR).toBe("/nowhere-gh"); + expect(env.GH_TOKEN).toBeUndefined(); let url = (environment: Record, ...args: string[]) => effective(SSH_REWRITE, environment, "-C", root, "remote", "get-url", ...args); expect(url({}, "--push", "origin")).toBe("git@github.com:Org/Repo.git"); diff --git a/apps/connector/src/checkouts.ts b/apps/connector/src/checkouts.ts index 5e147378b..c4f57c4cd 100644 --- a/apps/connector/src/checkouts.ts +++ b/apps/connector/src/checkouts.ts @@ -114,12 +114,12 @@ export function checkouts(options: { directory: string; registered: Registered } return { registered: options.registered, /** - * The checkout for work in `repository`; `token` gives the machine's GitHub token for it, - * asked for only when there is no registered checkout. + * The checkout for work in `repository`; `token` gives Chopin's GitHub token for it, asked + * for here only when there is no registered checkout, and rejecting when Chopin gave none. */ async resolve( repository: string, - token?: () => Promise, + token?: () => Promise, ): Promise { let key = Object.keys(options.registered).find(name => name.toLowerCase() === repository.toLowerCase() diff --git a/apps/connector/src/config.ts b/apps/connector/src/config.ts index babe5e4a4..0606f7486 100644 --- a/apps/connector/src/config.ts +++ b/apps/connector/src/config.ts @@ -34,7 +34,7 @@ export type ConnectSettings = { env?: Record; /** `--keep-runs`: how many finished runs' worktrees to keep, 0 for all; absent keeps 15. */ keepRuns?: number; - /** `--require-sandbox`, `--no-sandbox`, `--sandbox-host` and `--sandbox-write`. */ + /** `--require-sandbox`, `--no-sandbox`, `--sandbox-host`, `--sandbox-write` and `--sandbox-hide`. */ sandbox?: SandboxSettings; }; diff --git a/apps/connector/src/git-host.test.ts b/apps/connector/src/git-host.test.ts index b189384c3..4facddb47 100644 --- a/apps/connector/src/git-host.test.ts +++ b/apps/connector/src/git-host.test.ts @@ -375,7 +375,7 @@ test("a commondir made after the connector first saw the checkout sends its git expect(made.fired()).toEqual([]); }); -test("the ssh command a person chose is kept over the one a repository names, and a person's GIT_SSH_COMMAND over both", async () => { +test("the connector's git never runs an ssh command: not the person's, not one in its environment, not ssh on PATH, and not the one a repository names", async () => { let made = repository("ssh"); let home = join(made.root, "home"); mkdirSync(home); @@ -384,22 +384,30 @@ test("the ssh command a person chose is kept over the one a repository names, an join(home, ".gitconfig"), `[core]\n\tsshCommand = ${join(made.root, "person-ssh")}\n`, ); + let bin = join(made.root, "bin"); + mkdirSync(bin); + program(join(bin, "ssh"), `touch '${made.ran}/path'; exit 1`); + plain(made.repo, "config", "core.sshCommand", join(made.evil, "ssh")); + plain(made.repo, "config", "remote.origin.url", "git@github.com:org/repo.git"); let env: NodeJS.ProcessEnv = { ...process.env, HOME: home, XDG_CONFIG_HOME: join(home, ".config"), + PATH: `${bin}:${process.env.PATH}`, + SSH_AUTH_SOCK: join(made.root, "agent.sock"), }; delete env.GIT_SSH_COMMAND; // The fixture home is the person's whole configuration, whatever the runner has set. delete env.GIT_CONFIG_GLOBAL; delete env.GIT_CONFIG_SYSTEM; await gitAsync(["-C", made.repo, "fetch", "origin"], { env }).catch(() => undefined); - expect(made.fired()).toEqual(["person"]); program(join(made.root, "environment-ssh"), `touch '${made.ran}/environment'; exit 1`); await gitAsync(["-C", made.repo, "fetch", "origin"], { - env: { ...env, GIT_SSH_COMMAND: join(made.root, "environment-ssh") }, + env: { ...env, GIT_SSH_COMMAND: join(made.root, "environment-ssh"), GIT_SSH: "ssh" }, }).catch(() => undefined); - expect(made.fired()).toEqual(["environment", "person"]); + expect(made.fired()).toEqual([]); + expect(hostGitEnvironment(env)).toMatchObject({ GIT_SSH_COMMAND: "false" }); + expect(hostGitEnvironment(env).SSH_AUTH_SOCK).toBeUndefined(); }); /** The `GIT_CONFIG_*` entries an environment holds, in order. */ @@ -437,7 +445,8 @@ test("every setting the connector's git is given: helpers reset and none of the expect(hardened).toMatchObject({ GIT_CONFIG_NOSYSTEM: "1", GIT_ATTR_NOSYSTEM: "1", - GIT_SSH_COMMAND: "ssh -i key", + // Never the person's SSH, whatever they set: GitHub is reached over https with Chopin's token. + GIT_SSH_COMMAND: "false", GIT_DIR: join(made.repo, ".git"), GIT_COMMON_DIR: join(made.repo, ".git"), GIT_WORK_TREE: made.repo, diff --git a/apps/connector/src/git-host.ts b/apps/connector/src/git-host.ts index 68c140515..f746a4951 100644 --- a/apps/connector/src/git-host.ts +++ b/apps/connector/src/git-host.ts @@ -21,12 +21,13 @@ import { promisify } from "node:util"; * own: `GIT_CONFIG_NOSYSTEM` and `GIT_CONFIG_GLOBAL` set aside the system's and the person's. * - **What a repository's configuration may set** for the programs git runs is replaced in the * environment (`GIT_CONFIG_COUNT`, after any entries the caller gave, where they win over every - * configuration file as `-c` does): the hooks, the file-system monitor, the ssh command (also - * pinned in `GIT_SSH_COMMAND` to the person's own, which wins over any configuration), the + * configuration file as `-c` does): the hooks, the file-system monitor, the ssh command, the * global attributes file, remotes that are commands (`protocol.ext.allow`) and the local * transport (`protocol.file.allow`), which is never allowed. Every credential helper any * configuration named is reset and none is named again: what a command needs to authenticate, * its caller gives in the environment (`GIT_CONFIG_*`, as `gitCredentials` does). + * - **No SSH.** `GIT_SSH_COMMAND` is `false`, which wins over any configuration: the connector + * reaches GitHub only over https with Chopin's token, never with the person's SSH keys or agent. * * What this does not do: a repository's configuration is still read, so a filter, a proxy or * another setting a repository carries applies to the connector's commands as it would to anyone's. @@ -112,13 +113,11 @@ export function repositoryAt(path: string): Repository | undefined { type Person = { /** A file in the connector's own temporary directory: what a fetch and a checkout need. */ file: string; - /** The ssh command git would use without a repository's say. */ - ssh: string; }; /** * What a checkout and a fetch need of a person's configuration: where to fetch from and how to - * reach it (`url.*`, `http.*`, `ssh.*`, `transfer.*`, `fetch.*`, `safe.*`), how files are written (line endings, symlinks and names, + * reach it (`url.*`, `http.*`, `transfer.*`, `fetch.*`, `safe.*`), how files are written (line endings, symlinks and names, * `core.*` of those kinds) and their own filters (`filter.*`, `lfs.*`), which is what a Git LFS * checkout needs. The rest, a person's aliases, editor, pager, hooks and monitor among it, is not. */ @@ -126,7 +125,6 @@ const SECTIONS = new Set([ "url", "http", "https", - "ssh", "safe", "transfer", "fetch", @@ -196,7 +194,6 @@ function personConfig(env: NodeJS.ProcessEnv, repository?: Repository): Person { env.XDG_CONFIG_HOME, env.GIT_CONFIG_GLOBAL, env.GIT_CONFIG_SYSTEM, - env.GIT_SSH, repository?.gitDir, ].join("\0"); let known = people.get(key); @@ -208,12 +205,10 @@ function personConfig(env: NodeJS.ProcessEnv, repository?: Repository): Person { let entries = [...entriesOf("--system", reading), ...entriesOf("--global", reading)]; let file = join(privateDirectory(), `gitconfig-${people.size}`); let kept: string[] = []; - let ssh = ""; for (let [name, value] of entries) { let first = name.indexOf("."); let last = name.lastIndexOf("."); let section = name.slice(0, Math.max(first, 0)); - if (name === "core.sshcommand") ssh = value; if (first > 0 && (SECTIONS.has(section) || CORE.has(name))) { // A section with its subsection, as git writes them, and the value quoted. let subsection = first === last ? "" : ` "${quote(name.slice(first + 1, last))}"`; @@ -221,7 +216,7 @@ function personConfig(env: NodeJS.ProcessEnv, repository?: Repository): Person { } } writeFileSync(file, kept.join(""), { mode: 0o600 }); - let person = { file, ssh: ssh || env.GIT_SSH || "ssh" }; + let person = { file }; people.set(key, person); return person; } @@ -257,7 +252,12 @@ export function hostGitEnvironment( hardened.GIT_CONFIG_NOSYSTEM = "1"; hardened.GIT_CONFIG_GLOBAL = person.file; hardened.GIT_ATTR_NOSYSTEM = "1"; - if (!env.GIT_SSH_COMMAND) hardened.GIT_SSH_COMMAND = person.ssh; + // Never the person's SSH keys or agent: GitHub is reached over https with Chopin's token. + hardened.GIT_SSH_COMMAND = "false"; + delete hardened.GIT_SSH; + delete hardened.SSH_AUTH_SOCK; + delete hardened.GIT_ASKPASS; + delete hardened.SSH_ASKPASS; if (repository) { hardened.GIT_DIR = repository.gitDir; hardened.GIT_COMMON_DIR = repository.commonDir; diff --git a/apps/connector/src/github-token.test.ts b/apps/connector/src/github-token.test.ts new file mode 100644 index 000000000..4ae1b0bde --- /dev/null +++ b/apps/connector/src/github-token.test.ts @@ -0,0 +1,291 @@ +import { expect, test } from "bun:test"; +import { spawnSync } from "node:child_process"; +import { existsSync } from "node:fs"; +import { mkdir, mkdtemp, readFile, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { + githubEnvironment, + githubExpired, + githubRefusal, + GitHubRefused, + githubToken, + inheritsGitHub, + withoutGitHub, +} from "./github-token"; +import { BOT, STARTER } from "./testing/github-grant"; + +const MACHINE = `chopin_m_${"M".repeat(43)}`; + +/** + * A Chopin whose github-token route answers each request with the next of `answers` (the last + * repeats) and records what it was asked. + */ +function chopin(answers: Array<() => Response>) { + let asked: unknown[] = []; + let server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + async fetch(request) { + if (new URL(request.url).pathname !== "/api/connector/github-token") { + return new Response(null, { status: 404 }); + } + expect(request.headers.get("authorization")).toBe(`Bearer ${MACHINE}`); + asked.push(await request.json()); + let answer = answers[Math.min(asked.length - 1, answers.length - 1)]!; + return answer(); + }, + }); + return { url: `http://127.0.0.1:${server.port}`, asked, stop: () => server.stop(true) }; +} + +let issued = (token: string, expiresAt = Date.now() + 3_600_000) => () => + Response.json({ + token, + expiresAt, + kind: "installation", + permissions: { contents: "write", pull_requests: "write", metadata: "read" }, + identity: BOT, + starter: STARTER, + }); + +/** git with only `global` as the person's configuration, under `env`. */ +function git(cwd: string, global: string, env: Record, ...args: string[]) { + return spawnSync("git", args, { + cwd, + encoding: "utf8", + input: args[0] === "credential" ? "protocol=https\nhost=github.com\n\n" : undefined, + env: { + PATH: process.env.PATH, + HOME: global.replace(/\/[^/]+$/, ""), + GIT_CONFIG_NOSYSTEM: "1", + GIT_CONFIG_GLOBAL: global, + ...env, + }, + }); +} + +test("a launch's token is asked for with what it is for, and gh and git read it, renewed, from the launch's own files", async () => { + let first = `ghs_${"A".repeat(36)}`; + let second = `ghs_${"B".repeat(36)}`; + let server = chopin([issued(first, Date.now() + 1000), issued(second)]); + let folder = await mkdtemp(join(tmpdir(), "chopin-github-token-")); + try { + let github = await githubToken({ + url: server.url, + machine: MACHINE, + repository: "org/repo", + purpose: "run", + run: "run-1", + document: "doc-1", + folder, + // Due at once: the first token is replaced as soon as it is written. + renewBeforeMs: 60_000, + }); + expect(server.asked[0]).toEqual({ + repository: "org/repo", + purpose: "run", + run: "run-1", + document: "doc-1", + }); + expect(github.identity).toEqual(BOT); + expect(github.starter).toEqual(STARTER); + for (let i = 0; i < 100 && server.asked.length < 2; i++) await Bun.sleep(10); + for (let i = 0; i < 100 && !(await readFile(github.file, "utf8")).includes(second); i++) { + await Bun.sleep(10); + } + expect(await readFile(github.file, "utf8")).toBe(`${second}\n`); + expect((await Bun.file(github.file).stat()).mode & 0o777).toBe(0o600); + let env = githubEnvironment(github, "org/repo"); + // gh itself, as an agent runs it, finds the renewed token in the launch's configuration. + if (Bun.which("gh")) { + let gh = spawnSync("gh", ["auth", "token", "--hostname", "github.com"], { + encoding: "utf8", + env: { PATH: process.env.PATH, HOME: folder, ...env }, + }); + expect(gh.stdout.trim()).toBe(second); + } + // git is shown it for github.com, whatever helper the person's configuration names. + let global = join(folder, "gitconfig"); + await writeFile(global, `[credential]\n\thelper = "!echo password=theirs; :"\n`); + let filled = git(folder, global, env, "credential", "fill"); + expect(filled.stdout).toContain(`password=${second}\n`); + expect(filled.stdout).not.toContain("theirs"); + expect(github.secrets()).toEqual([first, second]); + await github.stop(); + expect(existsSync(github.file)).toBe(false); + expect(existsSync(github.config)).toBe(false); + } finally { + server.stop(); + await rm(folder, { recursive: true, force: true }); + } +}); + +test("every refusal stops the launch before anything starts, naming the repository and never the machine's own login", async () => { + for ( + let code of [ + "repository-forbidden", + "app-cannot-push", + "starter-cannot-push", + "run-unknown", + "no-grant", + ] + ) { + let server = chopin([ + () => Response.json({ error: code, message: `refused for org/repo` }, { status: 403 }), + ]); + let folder = await mkdtemp(join(tmpdir(), "chopin-github-refused-")); + try { + let asking = githubToken({ + url: server.url, + machine: MACHINE, + repository: "org/repo", + purpose: "planner", + document: "doc-1", + folder, + }); + await expect(asking).rejects.toBeInstanceOf(GitHubRefused); + await expect(asking).rejects.toThrow(githubRefusal(code, "org/repo")); + expect(githubRefusal(code, "org/repo")).toContain("org/repo"); + expect(githubRefusal(code, "org/repo")).not.toContain("uses its own GitHub login"); + // Nothing was written for a launch to find. + expect(existsSync(join(folder, "github"))).toBe(false); + expect(existsSync(join(folder, "gh"))).toBe(false); + } finally { + server.stop(); + await rm(folder, { recursive: true, force: true }); + } + } +}); + +test("a token that expires without a renewal is removed, and what used it hears why", async () => { + let server = chopin([ + issued(`ghs_${"C".repeat(36)}`, Date.now() + 150), + () => new Response("unavailable", { status: 503 }), + ]); + let folder = await mkdtemp(join(tmpdir(), "chopin-github-lapse-")); + try { + let github = await githubToken({ + url: server.url, + machine: MACHINE, + repository: "org/repo", + purpose: "run", + run: "run-1", + folder, + renewBeforeMs: 100, + }); + for (let i = 0; i < 300 && !github.expired.aborted; i++) await Bun.sleep(10); + expect(github.expired.aborted).toBe(true); + expect((github.expired.reason as Error).message).toBe(githubExpired("org/repo")); + expect(existsSync(github.file)).toBe(false); + expect(existsSync(join(github.config, "hosts.yml"))).toBe(false); + await github.stop(); + } finally { + server.stop(); + await rm(folder, { recursive: true, force: true }); + } +}); + +test("a launch inherits none of the machine's GitHub setup, and keeps everything else", () => { + let inherited = { + PATH: "/usr/bin", + HOME: "/home/person", + GH_TOKEN: "decoy", + GH_HOST: "github.example", + GH_ENTERPRISE_TOKEN: "decoy", + GH_CONFIG_DIR: "/home/person/.config/gh", + GITHUB_TOKEN: "decoy", + GITHUB_ENTERPRISE_TOKEN: "decoy", + GIT_ASKPASS: "/usr/bin/askpass", + SSH_ASKPASS: "/usr/bin/askpass", + SSH_AUTH_SOCK: "/tmp/agent.sock", + GIT_SSH: "ssh", + GIT_SSH_COMMAND: "ssh -i key", + GIT_CONFIG_COUNT: "1", + GIT_CONFIG_KEY_0: "credential.helper", + GIT_CONFIG_VALUE_0: "store", + GIT_CONFIG_GLOBAL: "/home/person/.gitconfig", + GIT_AUTHOR_EMAIL: "person@example.test", + COPILOT_GITHUB_TOKEN: "model-login", + CHOPIN_URL: "https://chopin.example", + }; + expect(withoutGitHub(inherited)).toEqual({ + PATH: "/usr/bin", + HOME: "/home/person", + // A model provider's own login is the agent's, not a way to the repository. + COPILOT_GITHUB_TOKEN: "model-login", + CHOPIN_URL: "https://chopin.example", + }); + expect(inheritsGitHub("gh_token")).toBe(true); +}); + +test.skipIf(process.platform === "win32")( + "under a launch's environment, a commit is made as Chopin's App, unsigned, and the repository's SSH address goes over https, whatever the person's configuration says", + async () => { + let folder = await mkdtemp(join(tmpdir(), "chopin-github-identity-")); + try { + let helper = join(folder, "helper-ran"); + let global = join(folder, "gitconfig"); + await writeFile( + global, + [ + "[user]", + "\tname = Decoy Person", + "\temail = decoy@example.test", + "[commit]", + "\tgpgsign = true", + "[tag]", + "\tgpgsign = true", + "[gpg]", + "\tprogram = /bin/false", + "[credential]", + `\thelper = "!touch '${helper}'; echo password=theirs; :"`, + '[url "git@github.com:"]', + "\tinsteadOf = https://github.com/", + "\tpushInsteadOf = https://github.com/", + "", + ].join("\n"), + ); + let tokens = join(folder, "tokens"); + await mkdir(join(tokens, "gh"), { recursive: true }); + await writeFile(join(tokens, "github"), `ghs_${"D".repeat(36)}\n`); + let env = githubEnvironment({ + token: `ghs_${"D".repeat(36)}`, + expiresAt: Date.now() + 3_600_000, + file: join(tokens, "github"), + config: join(tokens, "gh"), + identity: BOT, + starter: STARTER, + expired: new AbortController().signal, + secrets: () => [], + stop: async () => {}, + }, "org/repo"); + expect(env).toMatchObject({ + GH_CONFIG_DIR: join(tokens, "gh"), + GIT_SSH_COMMAND: "false", + CHOPIN_GITHUB_TOKEN_FILE: join(tokens, "github"), + CHOPIN_STARTER_LOGIN: STARTER.login, + CHOPIN_STARTER_NAME: STARTER.name, + CHOPIN_STARTER_EMAIL: STARTER.email, + }); + expect(Object.keys(env).filter(name => /^(GH|GITHUB)_TOKEN$/.test(name))).toEqual([]); + let repo = join(folder, "repo"); + await mkdir(repo); + git(repo, global, {}, "init", "-q"); + git(repo, global, {}, "remote", "add", "origin", "git@github.com:org/repo.git"); + let committed = git(repo, global, env, "commit", "-q", "--allow-empty", "-m", "change"); + expect(committed.stderr).toBe(""); + let made = git(repo, global, env, "log", "-1", "--format=%an <%ae>|%cn <%ce>|%G?"); + let bot = `${BOT.name} <${BOT.email}>`; + expect(made.stdout.trim()).toBe(`${bot}|${bot}|N`); + expect(git(repo, global, env, "remote", "get-url", "--push", "origin").stdout.trim()) + .toBe("https://github.com/org/repo.git"); + expect(git(repo, global, env, "credential", "fill").stdout).toContain( + `password=ghs_${"D".repeat(36)}`, + ); + expect(existsSync(helper)).toBe(false); + } finally { + await rm(folder, { recursive: true, force: true }); + } + }, +); diff --git a/apps/connector/src/github-token.ts b/apps/connector/src/github-token.ts index 5722d7d56..69978291a 100644 --- a/apps/connector/src/github-token.ts +++ b/apps/connector/src/github-token.ts @@ -1,15 +1,56 @@ -import { rename, rm, writeFile } from "node:fs/promises"; -import { errorCode, machineGitHubToken } from "./machine"; +import { mkdir, rename, rm, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import { + errorCode, + GITHUB_REFUSALS, + type GitHubAccount, + type GitHubGrantRequest, + machineGitHubToken, +} from "./machine"; /** Chopin hands out a token with at least 30 minutes left; this one is replaced 25 before. */ export const RENEW_BEFORE_MS = 25 * 60_000; /** How soon a renewal that failed is tried again, until the token expires. */ const RETRY_MS = 60_000; -/** What the connector says, once for each, when the GitHub App can't push to a repository. */ -export function appCannotPush(repository: string) { - return `The GitHub App can't push to ${repository} (it needs Contents and Pull requests write), ` - + "so this machine uses its own GitHub login for it."; +/** + * Why a launch got no GitHub token from Chopin, said so the person knows which repository and what + * fixes it. Nothing starts after one: the machine never falls back to its own GitHub login. + */ +export class GitHubRefused extends Error { + constructor(readonly code: string, readonly repository: string) { + super(githubRefusal(code, repository)); + this.name = "GitHubRefused"; + } +} + +/** What the connector says when Chopin refuses a launch a GitHub token for `repository`. */ +export function githubRefusal(code: string, repository: string) { + switch (code) { + case "repository-forbidden": + return `Chopin gave this machine no GitHub token for ${repository}: its owner can't push ` + + `there, so nothing ran. Ask for write access to ${repository}, or choose another machine.`; + case "app-cannot-push": + return `Chopin's GitHub App can't write to ${repository}; it needs Contents and Pull requests ` + + "write, which an admin of the repository must grant. This machine won't use its own GitHub " + + "login, so nothing ran."; + case "starter-cannot-push": + return `Whoever started this can't push to ${repository}, so Chopin gave it no GitHub token ` + + `and nothing ran. Ask for write access to ${repository}, then start it again.`; + case "run-unknown": + return `Chopin no longer knows this work, so it gave no GitHub token for ${repository} and ` + + "nothing ran."; + case "no-grant": + return `This Chopin has no GitHub App to give this machine a token for ${repository}, so ` + + "nothing ran. This machine won't use its own GitHub login."; + default: + return `Chopin refused this machine a GitHub token for ${repository} (${code}), so nothing ran.`; + } +} + +/** Said when a token lapsed because Chopin couldn't renew it, and what used it was stopped. */ +export function githubExpired(repository: string) { + return `The GitHub token for ${repository} expired and Chopin couldn't renew it, so this stopped.`; } export type GitHubToken = { @@ -18,51 +59,97 @@ export type GitHubToken = { expiresAt: number; /** The 0600 file that always holds the current token and a newline. */ file: string; + /** gh's configuration folder for the launch, whose `hosts.yml` holds the current token. */ + config: string; + /** The account commits are made as: Chopin's GitHub App. */ + identity: GitHubAccount; + /** The person the work is for, when Chopin names one. */ + starter?: GitHubAccount; + /** Aborted, with `githubExpired`, once the token lapsed without a renewal. */ + expired: AbortSignal; /** Every token handed out so far, to mask wherever they might be printed. */ secrets(): string[]; - /** Stops renewing and deletes the file. */ + /** Stops renewing and deletes the file and gh's configuration. */ stop(): Promise; }; +/** A YAML double-quoted scalar, as JSON writes a string. */ +let yaml = (text: string) => JSON.stringify(text); + /** - * The machine's GitHub token for `repository`, for one Planner command or run: written to `file` - * (mode 0600), and replaced there `renewBeforeMs` before it expires for as long as it lasts. Any - * refusal or failure gives undefined, so nothing gets a token Chopin didn't check; `refused` hears - * the line to say when the reason is that the GitHub App can't push there. + * gh's configuration for one account: `hosts.yml` in the layout gh 2.40 and later reads (its + * `users` map, and the active user's token beside it), and `config.yml` saying that layout is + * already in use, so gh never migrates it or looks in a keyring. */ -export async function githubToken(options: { - url: string; - machine: string; - repository: string; - file: string; - refused?: (message: string, repository: string) => void; - renewBeforeMs?: number; -}): Promise { - let { file } = options; - let ask = () => machineGitHubToken(options.url, options.machine, options.repository); - let first; - try { - first = await ask(); - } catch (error) { - if (errorCode(error) === "app-cannot-push") { - options.refused?.(appCannotPush(options.repository), options.repository); +function ghHosts(token: string, login: string) { + return [ + "github.com:", + " users:", + ` ${yaml(login)}:`, + ` oauth_token: ${yaml(token)}`, + " git_protocol: https", + ` oauth_token: ${yaml(token)}`, + ` user: ${yaml(login)}`, + "", + ].join("\n"); +} +const GH_CONFIG = ['version: "1"', "git_protocol: https", "prompt: disabled", ""].join("\n"); + +/** + * Chopin's GitHub token for one Planner command or run, asked for with what it is for: written to + * `github` in `folder` (mode 0600) and to gh's configuration in `gh/`, and replaced in both + * `renewBeforeMs` before it expires for as long as it lasts. A refusal rejects with + * `GitHubRefused`; any other failure rejects as it came. Once a token expires without a renewal, + * both are removed and `expired` aborts, so what used it stops instead of failing on its own. + */ +export async function githubToken( + options: GitHubGrantRequest & { + url: string; + machine: string; + folder: string; + renewBeforeMs?: number; + }, +): Promise { + let { folder, repository } = options; + let request: GitHubGrantRequest = { + repository, + purpose: options.purpose, + ...(options.run ? { run: options.run } : {}), + ...(options.document ? { document: options.document } : {}), + }; + let ask = async () => { + try { + return await machineGitHubToken(options.url, options.machine, request); + } catch (error) { + let code = errorCode(error); + if (code && GITHUB_REFUSALS.has(code)) throw new GitHubRefused(code, repository); + throw error; } - return undefined; - } + }; + let first = await ask(); + let file = join(folder, "github"); + let config = join(folder, "gh"); let seen = [first.token]; + let put = async (path: string, text: string) => { + let temporary = `${path}.${process.pid}.tmp`; + await writeFile(temporary, text, { mode: 0o600 }); + await rename(temporary, path); + }; let save = async (token: string) => { - let temporary = `${file}.${process.pid}.tmp`; - await writeFile(temporary, `${token}\n`, { mode: 0o600 }); - await rename(temporary, file); + await put(file, `${token}\n`); + await put(join(config, "hosts.yml"), ghHosts(token, first.identity.login)); }; - try { - await save(first.token); - } catch { - return undefined; - } + await mkdir(config, { recursive: true, mode: 0o700 }); + await put(join(config, "config.yml"), GH_CONFIG); + await save(first.token); + let lapsed = new AbortController(); let timer: ReturnType | undefined; let renewing: Promise | undefined; let stopped = false; + let remove = async () => { + await rm(file, { force: true }); + await rm(config, { recursive: true, force: true }); + }; let schedule = (expiresAt: number, delay?: number) => { if (stopped) return; let wait = delay ?? expiresAt - Date.now() - (options.renewBeforeMs ?? RENEW_BEFORE_MS); @@ -79,11 +166,14 @@ export async function githubToken(options: { seen.push(next.token); await save(next.token); schedule(next.expiresAt); - } catch { + } catch (error) { if (stopped) return; - // Once it has expired, a stale token in the file would only fail; better none. - if (Date.now() >= expiresAt) await rm(file, { force: true }); - else schedule(expiresAt, Math.min(RETRY_MS, expiresAt - Date.now())); + // A refusal is final, and once the token has expired a stale one would only fail: what + // used it is stopped rather than left to fail on its own. + if (error instanceof GitHubRefused || Date.now() >= expiresAt) { + await remove(); + lapsed.abort(new Error(githubExpired(repository))); + } else schedule(expiresAt, Math.min(RETRY_MS, expiresAt - Date.now())); } }; schedule(first.expiresAt); @@ -91,33 +181,96 @@ export async function githubToken(options: { token: first.token, expiresAt: first.expiresAt, file, + config, + identity: first.identity, + ...(first.starter ? { starter: first.starter } : {}), + expired: lapsed.signal, secrets: () => [...seen], async stop() { stopped = true; clearTimeout(timer); // A renewal still writing the file finishes before it goes. await renewing; - await rm(file, { force: true }); + await remove(); }, }; } /** - * What a run's agent gets to push and open pull requests with the machine's token: `GH_TOKEN` and - * `GITHUB_TOKEN` for gh and other tools, and Git configuration, through the environment, whose - * credential helper for github.com reads the token file, so a renewed token is used as soon as it - * is written. Earlier helpers for github.com are cleared, so the agent's own login doesn't win; - * nothing goes into the repository's own configuration. With `repository`, the person's - * `url..insteadOf` rules don't send it to SSH either (see `gitCredentials`). + * What a launch inherits that would reach GitHub or an SSH key as someone other than Chopin's App: + * gh's own variables and tokens, git's askpass, SSH agent and command, and any Git configuration + * through the environment. Removed from the connector's environment and the `--env` values before + * a launch is given its own (see `githubEnvironment`). + */ +export function inheritsGitHub(name: string) { + let key = name.toUpperCase(); + return key.startsWith("GH_") || key.startsWith("GIT_SSH") || key.startsWith("GIT_CONFIG_") + || [ + "GITHUB_TOKEN", + "GITHUB_ENTERPRISE_TOKEN", + "GIT_ASKPASS", + "SSH_ASKPASS", + "SSH_AUTH_SOCK", + "GIT_CONFIG", + "GIT_AUTHOR_NAME", + "GIT_AUTHOR_EMAIL", + "GIT_COMMITTER_NAME", + "GIT_COMMITTER_EMAIL", + ].includes(key); +} + +/** `env` without what `inheritsGitHub` names. */ +export function withoutGitHub( + env: Record, +): Record { + return Object.fromEntries(Object.entries(env).filter(([name]) => !inheritsGitHub(name))); +} + +/** + * Everything a launch gets to reach GitHub with Chopin's token, and nothing of the machine's: + * + * - **gh** reads `GH_CONFIG_DIR`, the launch's own configuration, rewritten with each renewed + * token, never the person's `~/.config/gh`. No `GH_TOKEN` or `GITHUB_TOKEN` is set: either would + * win over the configuration and go stale at the first renewal. + * - **git** has one credential helper for github.com, which reads the token file, and the + * repository's SSH URLs map to https (`gitCredentials`); `GIT_SSH_COMMAND` is `false`, so no SSH + * key or agent is ever used. + * - **Commits** are made as Chopin's App (`GIT_AUTHOR_*`, `GIT_COMMITTER_*`), never as whoever the + * machine's Git configuration names (`user.useConfigOnly`), and are not signed with the machine's + * keys (`commit.gpgsign`, `tag.gpgsign`). + * - **The person it is for**, when Chopin names one, in `CHOPIN_STARTER_LOGIN`, `_NAME` and + * `_EMAIL`, for a pull request's "Requested by" and a commit's `Co-authored-by`. + * - `CHOPIN_GITHUB_TOKEN_FILE` names the token file, for a backend's own use of the token. + * + * It goes on top of an environment `withoutGitHub` cleared. */ export function githubEnvironment( github: GitHubToken, repository?: string, ): Record { + let { identity, starter } = github; return { - GH_TOKEN: github.token, - GITHUB_TOKEN: github.token, - ...gitCredentials(github.file, repository), + GH_CONFIG_DIR: github.config, + GH_PROMPT_DISABLED: "1", + GIT_TERMINAL_PROMPT: "0", + GIT_SSH_COMMAND: "false", + CHOPIN_GITHUB_TOKEN_FILE: github.file, + GIT_AUTHOR_NAME: identity.name, + GIT_AUTHOR_EMAIL: identity.email, + GIT_COMMITTER_NAME: identity.name, + GIT_COMMITTER_EMAIL: identity.email, + ...(starter + ? { + CHOPIN_STARTER_LOGIN: starter.login, + CHOPIN_STARTER_NAME: starter.name, + CHOPIN_STARTER_EMAIL: starter.email, + } + : {}), + ...gitCredentials(github.file, repository, [ + ["user.useConfigOnly", "true"], + ["commit.gpgsign", "false"], + ["tag.gpgsign", "false"], + ]), }; } @@ -154,7 +307,11 @@ export function githubUrlRules(repository: string): Array<[string, string]> { * stay on https whatever `url..insteadOf` the person has (see `githubUrlRules`). The rest of * the person's Git configuration applies as it is. */ -export function gitCredentials(file: string, repository?: string): Record { +export function gitCredentials( + file: string, + repository?: string, + more: Array<[string, string]> = [], +): Record { let quoted = `'${file.replaceAll("'", "'\\''")}'`; let entries: Array<[string, string]> = [ ["credential.https://github.com.helper", ""], @@ -163,6 +320,7 @@ export function gitCredentials(file: string, repository?: string): Record = { GIT_CONFIG_COUNT: String(entries.length) }; entries.forEach(([key, value], index) => { diff --git a/apps/connector/src/investigation-run.test.ts b/apps/connector/src/investigation-run.test.ts index 23a8ecc9d..557170aed 100644 --- a/apps/connector/src/investigation-run.test.ts +++ b/apps/connector/src/investigation-run.test.ts @@ -6,6 +6,7 @@ import { ConnectorError } from "./mcp"; import { createOutput } from "./output"; import { checkouts } from "./checkouts"; import { runWork } from "./run"; +import { testGitHubToken } from "./testing/github-grant"; import { git, prepareWorkspace } from "./workspace"; test("an authorized retry starts despite an older receipt and reports why its agent submitted no result", async () => { @@ -68,6 +69,7 @@ test("an authorized retry starts despite an older receipt and reports why its ag url: "http://127.0.0.1:1", signal: new AbortController().signal, output, + github: request => testGitHubToken(request.folder), }, ); let reported = calls.find(([name]) => name === "fail_experiment")?.[1]; diff --git a/apps/connector/src/machine.ts b/apps/connector/src/machine.ts index 1d872e532..9e37e721a 100644 --- a/apps/connector/src/machine.ts +++ b/apps/connector/src/machine.ts @@ -413,19 +413,102 @@ export async function reportPlannerFailure( * draft pull requests as its owner, and when it expires (ms). Chopin refuses when it holds no such * token, the owner can't push there, or its GitHub App can't write there. */ +/** A GitHub account as Chopin names it: the App's bot that writes, or the person who started. */ +export type GitHubAccount = { login: string; id: number; name: string; email: string }; + +/** What a launch asks Chopin's GitHub token for: its repository, and the work that needs it. */ +export type GitHubGrantRequest = { + repository: string; + purpose: "planner" | "run"; + /** The run's id, for a build, investigation or spike. */ + run?: string; + /** The document's id, when the launch knows it. */ + document?: string; +}; + +function account(value: unknown): GitHubAccount | undefined { + if (!value || typeof value !== "object") return undefined; + let { login, id, name, email } = value as Record; + if (typeof login !== "string" || !login || typeof id !== "number") return undefined; + if (typeof email !== "string" || !email) return undefined; + return { login, id, name: typeof name === "string" && name ? name : login, email }; +} + +/** + * The GitHub token Chopin gives this machine for one launch: the token, when it expires (ms), the + * account its commits are made as and the person they are made for. A refusal rejects with the + * server's code (see `errorCode`); so does an answer without a token or an identity. + */ export async function machineGitHubToken( + url: string, + token: string, + request: GitHubGrantRequest, + signal?: AbortSignal, +) { + let value = await post(url + "/api/connector/github-token", request, token, signal); + let identity = account(value.identity); + if (typeof value.token !== "string" || typeof value.expiresAt !== "number" || !identity) { + throw new Error("Chopin's answer had no GitHub token or identity."); + } + let starter = account(value.starter); + return { + token: value.token, + expiresAt: value.expiresAt, + identity, + ...(starter ? { starter } : {}), + }; +} + +/** + * Whether Chopin would give this machine a token to push to `repository`, asked without one being + * made: `ok`, the refusal's code, or `unknown` when Chopin couldn't be asked or doesn't know how. + */ +export async function machineGitHubCheck( url: string, token: string, repository: string, signal?: AbortSignal, +): Promise { + try { + let value = await post( + url + "/api/connector/github-token", + { repository, purpose: "check" }, + token, + signal, + ); + return value.ok === true ? "ok" : "unknown"; + } catch (error) { + let code = errorCode(error); + return code && GITHUB_REFUSALS.has(code) ? code : "unknown"; + } +} + +/** + * A token for Chopin's own tools at `/mcp`, for one launch: minted from the launch's own + * credential, a Planner's or a run's, and lasting no longer than it. + */ +export async function machineMcpToken( + url: string, + token: string, + credential: { planner: string } | { run: string }, + signal?: AbortSignal, ) { - let value = await post(url + "/api/connector/github-token", { repository }, token, signal); + let value = await post(url + "/api/connector/mcp-token", credential, token, signal); if (typeof value.token !== "string" || typeof value.expiresAt !== "number") { - throw new Error("Missing GitHub token."); + throw new Error("Missing token for Chopin's tools."); } return { token: value.token, expiresAt: value.expiresAt }; } +/** Why Chopin gives no GitHub token for a launch; none of these is retried. */ +export const GITHUB_REFUSALS = new Set([ + "repository-forbidden", + "app-cannot-push", + "starter-cannot-push", + "run-unknown", + "no-grant", +]); + /** Revoke this machine on the server and forget its token, even if the server already had. */ export async function unpair(url: string, directory: string): Promise<"unpaired" | "not-paired"> { let token = (await stored(directory)).servers[url]?.token; diff --git a/apps/connector/src/main.test.ts b/apps/connector/src/main.test.ts index ceb8ed036..33a6c243e 100644 --- a/apps/connector/src/main.test.ts +++ b/apps/connector/src/main.test.ts @@ -43,7 +43,7 @@ async function checkout() { return root; } -const MESSAGE = "chopin 0.0.1 speaks connector protocol 6, but this server needs protocol 2 to 5. " +const MESSAGE = "chopin 0.0.1 speaks connector protocol 7, but this server needs protocol 2 to 6. " + "Run `chopin update 0.1.3`, then connect again."; /** @@ -154,12 +154,12 @@ async function connect( } } -test("connect sends protocol 6 and stops when a protocol 5 server refuses it", async () => { +test("connect sends its protocol and stops when an older server refuses it", async () => { let result = await connect("pairing"); expect(result.code).toBe(1); expect(result.stderr).toBe(`${MESSAGE}\n`); expect(result.requests).toEqual([ - ["/api/connector/pairings", VERSION, "6"], + ["/api/connector/pairings", VERSION, String(CONNECTOR_PROTOCOL)], ]); // A connect that never connected saves nothing for service install. expect(await Bun.file(join(result.directory, "config.json")).exists()).toBe(false); @@ -398,6 +398,8 @@ test("connect saves the sandbox options for service install, says what the sandb "--sandbox-host=*.corp.test", "--sandbox-write", write, + "--sandbox-hide", + join(write, "sign-in"), "--", "agent", ]); @@ -411,6 +413,7 @@ test("connect saves the sandbox options for service install, says what the sandb require: true, hosts: ["store.example.com:8443", "*.corp.test"], writable: [write], + hidden: [join(write, "sign-in")], }); let off = await connect("nothing", [root, "--no-sandbox", "--", "agent"]); expect(off.code).toBe(0); diff --git a/apps/connector/src/main.ts b/apps/connector/src/main.ts index 9fb317286..fa63bf92a 100644 --- a/apps/connector/src/main.ts +++ b/apps/connector/src/main.ts @@ -13,6 +13,7 @@ import { StdioServerTransport } from "@modelcontextprotocol/sdk/server/stdio.js" import { acpCommand, AGENTS, nodeMajor, nodeOlder } from "@chopin/protocol/agents"; import type { CodingAgent } from "@chopin/protocol/agents"; import { checkAgent } from "./acp"; +import { withoutGitHub } from "./github-token"; import type { AgentInfo } from "./acp"; import { ConnectorStopped, redacted, serveWork } from "./connection"; import { remote } from "./mcp"; @@ -59,7 +60,6 @@ import { sandboxLabel } from "@chopin/protocol/connector"; import { chosenPlanners, connectors, - githubFallback, lockWorkspace, pruneRuns, startFailures, @@ -379,13 +379,15 @@ function runsToKeep(value: string | undefined) { /** * What the sandbox flags choose: `--require-sandbox` and `--no-sandbox` are opposites, each * `--sandbox-host` is a host (`host`, `*.host` or `host:port`) agents may reach beyond the - * built-in list, and each `--sandbox-write` an absolute directory they may write. + * built-in list, each `--sandbox-write` an absolute directory they may write, and each + * `--sandbox-hide` a file or folder they may neither read nor write, wherever it is. */ function sandboxChoice( require: boolean, off: boolean, hosts: string[], writable: string[], + hidden: string[] = [], ): SandboxSettings { if (require && off) throw new Error("--require-sandbox and --no-sandbox can't be used together."); for (let host of hosts) { @@ -396,11 +398,13 @@ function sandboxChoice( } } let directories = writable.map(path => resolve(path)); + let concealed = hidden.map(path => resolve(path)); return { ...(require ? { require } : {}), ...(off ? { off } : {}), ...(hosts.length ? { hosts } : {}), ...(directories.length ? { writable: directories } : {}), + ...(concealed.length ? { hidden: concealed } : {}), }; } @@ -416,7 +420,7 @@ export async function connect( let parsed = parseArgs(args, { flags: ["service", "install-service", "skip-check", "require-sandbox", "no-sandbox"], options: ["planner-command", "key", "name", "keep-runs", "agent"], - lists: ["planner", "env", "checkout", "sandbox-host", "sandbox-write"], + lists: ["planner", "env", "checkout", "sandbox-host", "sandbox-write", "sandbox-hide"], }); let service = parsed.flags.has("service"); let directory = stateDirectory(); @@ -480,6 +484,7 @@ export async function connect( parsed.flags.has("no-sandbox"), parsed.lists.get("sandbox-host") ?? [], parsed.lists.get("sandbox-write") ?? [], + parsed.lists.get("sandbox-hide") ?? [], ); if (!service) { requireProgram(command, process.cwd()); @@ -574,7 +579,8 @@ export async function connect( started[at] = which(started[at]!) ?? started[at]!; void checkAgent(started, { cwd: checkDirectory(), - env: { ...process.env, ...env }, + // Through the seam, as a run's agent starts: with nothing of the machine's GitHub setup. + env: withoutGitHub({ ...process.env, ...env }), timeoutMs: 10_000, signal, ...(sandbox ? { sandbox } : {}), @@ -633,7 +639,6 @@ export async function connect( let prune = () => pruneRuns(directory, keep).then(() => {}, () => {}); await prune(); let chosen = chosenPlanners(directory, directory); - let fallback = githubFallback(directory, directory); let checkouts = machineCheckouts({ directory, registered }); let abort = new AbortController(); // The first Ctrl-C reports running work as stopped, then disconnects; a second forces exit. @@ -679,10 +684,6 @@ export async function connect( }, }); let report = sandbox.report; - // Said once for each repository, and kept for chopin status, when the GitHub App can't push there. - let githubRefused = (message: string, repository: string) => { - if (fallback.record(repository)) output.say(output.style.dim(message)); - }; try { for (;;) { // A machine paired once reconnects with its stored token; an unpaired one pairs first. @@ -734,7 +735,6 @@ export async function connect( signal: AbortSignal.any([abort.signal, planners.signal]), failures, env, - githubRefused, sandbox, }; let planning = servePlanners(backends); @@ -807,7 +807,6 @@ export async function connect( failures, env, machine, - githubRefused, sandbox, }).finally(prune), }); diff --git a/apps/connector/src/mcp-token.ts b/apps/connector/src/mcp-token.ts new file mode 100644 index 000000000..ff8a1d8c9 --- /dev/null +++ b/apps/connector/src/mcp-token.ts @@ -0,0 +1,114 @@ +import { rename, rm, writeFile } from "node:fs/promises"; +import { classifyConnectorError, machineMcpToken } from "./machine"; +import { transient } from "./reconnect"; + +/** A run's token for Chopin's tools lasts an hour; it is replaced 10 minutes before that ends. */ +export const RENEW_MCP_BEFORE_MS = 10 * 60_000; +/** How soon a renewal that failed for want of a connection is tried again. */ +const RETRY_MS = 30_000; +/** How long one request for a token may go unanswered. */ +const REQUEST_MS = 15_000; +/** How many of the tokens handed out are remembered, to mask wherever one is printed. */ +const REMEMBERED = 8; + +/** What a launch shows Chopin to get its token for Chopin's tools: its own credential. */ +export type McpCredential = { planner: string } | { run: string }; + +export type McpTokenFile = { + /** The 0600 file that holds the current token and a newline, while there is one. */ + file: string; + /** Asks for a new token with the launch's credential as it is now, as a Planner's is renewed. */ + renew(credential: McpCredential): Promise; + /** Every token handed out so far, to mask wherever the launch might print one. */ + secrets(): string[]; + /** Stops renewing and deletes the file. */ + stop(): Promise; +}; + +/** + * The token a launch's backend shows Chopin's own tools at `/mcp`: a Chopin credential that reaches + * only those tools, only for the launch's documents and only while it lasts, never a GitHub token + * or the launch's own credential. Minted from the launch's credential (`credential`), written to + * `file` (mode 0600) and replaced before it expires: a run's on its own schedule, a Planner's + * whenever its credential is renewed (`renew`), as it lasts no longer than that. Chopin refusing + * one deletes the file, so the tools refuse too, but the launch goes on; one out of reach is tried + * again, and its token is deleted once it has expired. + */ +export async function mcpTokenFile(options: { + url: string; + machine: string; + file: string; + credential: McpCredential; + /** Whether this launch renews its token on its own schedule: a run does, a Planner doesn't. */ + scheduled: boolean; + renewBeforeMs?: number; + retryMs?: number; +}): Promise { + let { file } = options; + let seen: string[] = []; + let credential = options.credential; + let timer: ReturnType | undefined; + let current: Promise | undefined; + let stopped = false; + let asking = new AbortController(); + /** When the token in the file expires, once there is one. */ + let expires: number | undefined; + let save = async (token: string) => { + let temporary = `${file}.${process.pid}.tmp`; + await rm(temporary, { force: true }); + await writeFile(temporary, `${token}\n`, { mode: 0o600, flag: "wx" }); + await rename(temporary, file); + }; + let schedule = (delay: number) => { + if (stopped) return; + clearTimeout(timer); + timer = setTimeout(() => void ask(), Math.min(Math.max(delay, 0), 2 ** 31 - 1)); + timer.unref?.(); + }; + let ask = () => + current = (async () => { + try { + let next = await machineMcpToken( + options.url, + options.machine, + credential, + AbortSignal.any([asking.signal, AbortSignal.timeout(REQUEST_MS)]), + ); + if (stopped) return; + seen.push(next.token); + if (seen.length > REMEMBERED) seen.shift(); + await save(next.token); + expires = next.expiresAt; + if (options.scheduled) { + schedule(next.expiresAt - Date.now() - (options.renewBeforeMs ?? RENEW_MCP_BEFORE_MS)); + } + } catch (error) { + if (stopped) return; + if (transient(classifyConnectorError(error))) { + // Kept while it lasts; a stale one would only be refused. + if (expires !== undefined && Date.now() >= expires) await rm(file, { force: true }); + schedule(options.retryMs ?? RETRY_MS); + } else { + clearTimeout(timer); + await rm(file, { force: true }); + } + } + })(); + await ask(); + return { + file, + async renew(next) { + credential = next; + await current; + if (!stopped) await ask(); + }, + secrets: () => [...seen], + async stop() { + stopped = true; + clearTimeout(timer); + asking.abort(); + await current; + await rm(file, { force: true }); + }, + }; +} diff --git a/apps/connector/src/planner-token.ts b/apps/connector/src/planner-token.ts index a40cfd0fc..80d31f06e 100644 --- a/apps/connector/src/planner-token.ts +++ b/apps/connector/src/planner-token.ts @@ -39,6 +39,8 @@ export async function plannerTokenFile(options: { document: string; file: string; first: { token: string; expiresAt: number }; + /** Hears each renewed credential once it is in the file, as what is minted from it is renewed. */ + renewed?: (token: string) => Promise; /** How long before it expires a credential is replaced, and how soon a failed renewal is retried; for tests. */ renewBeforeMs?: number; retryMs?: number; @@ -87,6 +89,7 @@ export async function plannerTokenFile(options: { if (seen.length > REMEMBERED) seen.shift(); await save(next.token); schedule(next.expiresAt); + await options.renewed?.(next.token).catch(() => {}); } catch (error) { if (stopped) return; if (transient(classifyConnectorError(error))) { diff --git a/apps/connector/src/planner.test.ts b/apps/connector/src/planner.test.ts index b7ee9295e..667085fb7 100644 --- a/apps/connector/src/planner.test.ts +++ b/apps/connector/src/planner.test.ts @@ -2,6 +2,9 @@ import { afterAll, expect, mock, test } from "bun:test"; import { mkdtemp, readdir, readFile, realpath, rm, stat, writeFile } from "node:fs/promises"; import { homedir, tmpdir } from "node:os"; import { dirname, join } from "node:path"; +import { answerTokens, BOT, STARTER } from "./testing/github-grant"; +import { githubRefusal } from "./github-token"; +import { existsSync } from "node:fs"; import { machineFile } from "./machine"; import { createOutput } from "./output"; import { @@ -18,6 +21,26 @@ import { git, lockWorkspace, startFailures } from "./workspace"; import { checkouts } from "./checkouts"; import { CONNECTOR_PROTOCOL } from "./version"; +/** + * A fake Chopin, as `Bun.serve` makes one, that also answers a launch's asking for its GitHub token + * and its token for Chopin's tools, as a Chopin with its GitHub App does. + */ +function serve(options: { + hostname: string; + port: number; + fetch: (request: Request) => Response | Promise; + /** The GitHub token refusal each named repository gets. */ + refusing?: Record; +}) { + let { refusing, ...rest } = options; + return Bun.serve({ + ...rest, + async fetch(request) { + return await answerTokens(request, refusing) ?? await options.fetch(request); + }, + }); +} + let scratch = await mkdtemp(join(tmpdir(), "chopin-planner-")); afterAll(() => rm(scratch, { recursive: true, force: true })); @@ -73,7 +96,7 @@ test( async () => { let issued: string[] = []; let asked: Array<{ authorization: string | null; body: unknown }> = []; - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, async fetch(request): Promise { @@ -228,7 +251,7 @@ test( let log = join(directory, "launches.jsonl"); await writeFile(script, STAYING_BACKEND); let issued: Array<{ machine: string; token: string }> = []; - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, async fetch(request): Promise { @@ -351,7 +374,7 @@ function alive(pid: number) { test.skipIf(process.platform === "win32")( "a backend that starts in the sandbox finds its own worktree of the checkout made, as a linked worktree, and one that starts without a sandbox finds none made", async () => { - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, fetch: () => @@ -424,7 +447,7 @@ test.skipIf(process.platform === "win32")( test.skipIf(process.platform === "win32")( "a backend's own children are killed with it, whether it exits by itself or is stopped", async () => { - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, fetch: () => @@ -492,7 +515,7 @@ setInterval(() => {}, 1000); test.skipIf(process.platform === "win32")( "chopin status shows a Planner backend that exits early until one stays up", async () => { - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, fetch: () => @@ -565,7 +588,7 @@ test.skipIf(process.platform === "win32")( async () => { let token = `chopin_p_${"P".repeat(43)}`; let secret = "review-opaque-api-secret"; - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, fetch: () => @@ -658,7 +681,7 @@ test.skipIf(process.platform === "win32")( async () => { let token = `chopin_p_${"Kq4Zt9Wm".repeat(6).slice(0, 43)}`; let marker = "[chopin: dropped a stderr line longer than 65536 characters]"; - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, fetch: () => @@ -781,7 +804,7 @@ test.skipIf(process.platform === "win32")( "a Planner backend that keeps exiting early reports to Chopin why, without credentials or the home directory, and clears it once one stays up", async () => { let reports: Array<{ authorization: string | null; body: any }> = []; - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, async fetch(request): Promise { @@ -861,7 +884,7 @@ test.skipIf(process.platform === "win32")( async () => { let reports = 0; let credentials = 0; - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, fetch(request): Response { @@ -912,7 +935,7 @@ test.skipIf(process.platform === "win32")( "chopin status shows a Planner stopped for good, not retrying, once Chopin refuses its credential", async () => { let asked = 0; - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, fetch: () => @@ -1009,7 +1032,7 @@ test.skipIf(process.platform === "win32")( let polls: string[][] = []; let release = Promise.withResolvers(); let document = ""; - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, async fetch(request): Promise { @@ -1211,7 +1234,7 @@ test.skipIf(process.platform === "win32")( let script = join(directory, "backend.ts"); let log = join(directory, "launches.jsonl"); await writeFile(script, LEAVING_BACKEND); - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, async fetch(request): Promise { @@ -1327,7 +1350,7 @@ test.skipIf(process.platform === "win32")( let script = join(directory, "backend.ts"); let log = join(directory, "launches.jsonl"); await writeFile(script, LEAVING_BACKEND); - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, async fetch(request): Promise { @@ -1425,7 +1448,7 @@ test.skipIf(process.platform === "win32")( test.skipIf(process.platform === "win32")( "a dispatched Planner that exits by itself starts again only once Chopin's Planner poll still asks for it", async () => { - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, fetch: request => @@ -1496,7 +1519,7 @@ test.skipIf(process.platform === "win32")( test.skipIf(process.platform === "win32")( "a dispatched Planner that exited starts again at once when Chopin says a person asked, and a running one is left as it is", async () => { - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, fetch: request => @@ -1607,7 +1630,7 @@ test.skipIf(process.platform === "win32")( test.skipIf(process.platform === "win32")( "a dispatched Planner whose document was renamed goes by its new address, in status, output and its next start", async () => { - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, fetch: async request => @@ -1693,9 +1716,11 @@ test.skipIf(process.platform === "win32")( D_notes: "octo-org/notes", D_site: "octo-org/site", }; - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, + // Chopin gives no token to push to the repository with no checkout here. + refusing: { "octo-org/site": "repository-forbidden" }, async fetch(request) { let path = new URL(request.url).pathname; if (path !== "/api/connector/planner") return new Response(null, { status: 404 }); @@ -1762,15 +1787,13 @@ test.skipIf(process.platform === "win32")( CHOPIN_WORKTREE: join(directory, "planners", "octo-org", "notes", "D_notes"), }, }); - // A repository with no checkout here, and no GitHub token from Chopin to clone it with, - // never starts; it says how to give it one. + // A repository Chopin gives no GitHub token for never starts; it says why. expect(launched.has("D_site")).toBe(false); await until(async () => said.includes("D_site"), 5_000); expect(said).toContain( - `Can't start the Planner for ${origin}/documents/D_site: This machine has no checkout of ` - + "octo-org/site", + `Stopped the Planner for ${origin}/documents/D_site: ` + + githubRefusal("repository-forbidden", "octo-org/site"), ); - expect(said).toContain("chopin connect --checkout "); } finally { abort.abort(); await planners.stop(); @@ -1811,7 +1834,7 @@ test.skipIf(process.platform === "win32")( let own = ""; let assigned = ""; let polls = 0; - let chopin = Bun.serve({ + let chopin = serve({ hostname: "127.0.0.1", port: 0, async fetch(request): Promise { @@ -1959,11 +1982,17 @@ let log = process.argv[2]; let url = spawnSync("git", ["ls-remote", "--get-url", "https://github.com/octo-org/score.git"], { encoding: "utf8" }).stdout.trim(); let file = process.env.CHOPIN_GITHUB_TOKEN_FILE; let seen = () => file ? { content: readFileSync(file, "utf8"), mode: (statSync(file).mode & 0o777).toString(8) } : null; +let hosts = process.env.GH_CONFIG_DIR ? readFileSync(process.env.GH_CONFIG_DIR + "/hosts.yml", "utf8") : ""; appendFileSync(log, JSON.stringify({ token: process.env.CHOPIN_GITHUB_TOKEN ?? null, expiresAt: process.env.CHOPIN_GITHUB_TOKEN_EXPIRES_AT ?? null, file: file ?? null, url, + ghToken: process.env.GH_TOKEN ?? process.env.GITHUB_TOKEN ?? null, + gh: hosts.match(/^ oauth_token: "(.*)"$/m)?.[1] ?? null, + author: [process.env.GIT_AUTHOR_NAME, process.env.GIT_AUTHOR_EMAIL], + starter: process.env.CHOPIN_STARTER_LOGIN ?? null, + mcp: process.env.CHOPIN_MCP_TOKEN_FILE ? readFileSync(process.env.CHOPIN_MCP_TOKEN_FILE, "utf8").trim() : null, argv: process.argv, seen: seen(), }) + "\\n"); @@ -1980,10 +2009,18 @@ function githubChopin(github: (asked: number) => Response) { async fetch(request) { let path = new URL(request.url).pathname; if (path === "/api/connector/github-token") { - expect(await request.json()).toEqual({ repository: "octo-org/score" }); + expect(await request.json()).toEqual({ + repository: "octo-org/score", + purpose: "planner", + document: "D_1", + }); expect(request.headers.get("authorization")).toBe(`Bearer ${MACHINE}`); return github(asked++); } + if (path === "/api/connector/mcp-token") { + expect(await request.json()).toEqual({ planner: `chopin_p_${"P".repeat(43)}` }); + return Response.json({ token: "chopin_mcp_planner", expiresAt: Date.now() + 900_000 }); + } return Response.json({ token: `chopin_p_${"P".repeat(43)}`, expiresAt: Date.now() + 15 * 60_000, @@ -1999,14 +2036,19 @@ function githubChopin(github: (asked: number) => Response) { } test.skipIf(process.platform === "win32")( - "a Planner command gets the machine's GitHub token and a private file kept fresh, removed when it stops", + "a Planner command gets Chopin's GitHub token for its document, in a private file and gh's own configuration kept fresh, commits as Chopin's App, and the files go when it stops", async () => { - let tokens = [`ghu_${"A".repeat(36)}`, `ghu_${"B".repeat(36)}`]; + let tokens = [`ghs_${"A".repeat(36)}`, `ghs_${"B".repeat(36)}`]; let expiries: number[] = []; let chopin = githubChopin(asked => { // Short-lived, so the connector replaces it before it expires. expiries.push(Date.now() + 1_500); - return Response.json({ token: tokens[Math.min(asked, 1)], expiresAt: expiries.at(-1) }); + return Response.json({ + token: tokens[Math.min(asked, 1)], + expiresAt: expiries.at(-1), + identity: BOT, + starter: STARTER, + }); }); let directory = await mkdtemp(join(scratch, "github-")); let script = join(directory, "backend.ts"); @@ -2047,9 +2089,16 @@ test.skipIf(process.platform === "win32")( seen: { content: `${tokens[0]}\n`, mode: "600" }, // Its own git and its agent's keep to https for this repository, rewrite or not. url: "https://github.com/octo-org/score.git", + // gh finds the same token in the launch's own configuration, and nothing overrides it. + ghToken: null, + gh: tokens[0], + author: [BOT.name, BOT.email], + starter: STARTER.login, + // Chopin's own tools take a Chopin token, minted from the Planner's credential. + mcp: "chopin_mcp_planner", }); // Never in its arguments. - expect(JSON.stringify(first.argv)).not.toContain("ghu_"); + expect(JSON.stringify(first.argv)).not.toContain("ghs_"); // Replaced before it expires, without restarting the command. await until( async () => (await entries(log)).some(entry => entry.later?.content === `${tokens[1]}\n`), @@ -2072,19 +2121,30 @@ test.skipIf(process.platform === "win32")( ); test.skipIf(process.platform === "win32")( - "without a token from Chopin a Planner command gets none, and an App that can't push is said once", + "a Planner Chopin refuses a GitHub token never starts, says why naming the repository, and one Chopin can't answer is tried again", async () => { - for (let answer of ["app-cannot-push", "not-found", "unavailable"]) { + for ( + let answer of [ + "app-cannot-push", + "repository-forbidden", + "starter-cannot-push", + "unavailable", + ] + ) { let chopin = githubChopin(() => - Response.json({ error: answer }, { - status: answer === "not-found" ? 404 : answer === "unavailable" ? 503 : 409, - }) + Response.json({ error: answer }, { status: answer === "unavailable" ? 503 : 403 }) ); let directory = await mkdtemp(join(scratch, "github-refused-")); let script = join(directory, "backend.ts"); let log = join(directory, "launches.jsonl"); await writeFile(script, GITHUB_BACKEND); - let said: string[] = []; + let said = ""; + let capture = { + write: (chunk: string | Uint8Array) => { + said += String(chunk); + return true; + }, + }; let abort = new AbortController(); let serving = servePlanners({ url: chopin.url, @@ -2093,21 +2153,22 @@ test.skipIf(process.platform === "win32")( command: [process.execPath, script, log], checkouts: checkouts({ directory, registered: { "octo-org/score": directory } }), directory, - output: createOutput({ write: () => true }, { write: () => true }, {}), + output: createOutput(capture, capture, {}), signal: abort.signal, - githubRefused: message => said.push(message), }); try { - await until(async () => (await entries(log)).length > 0, 10_000); - expect((await entries(log))[0]).toMatchObject({ token: null, expiresAt: null, file: null }); - expect(said).toEqual( - answer === "app-cannot-push" - ? [ - "The GitHub App can't push to octo-org/score (it needs Contents and Pull requests " - + "write), so this machine uses its own GitHub login for it.", - ] - : [], - ); + if (answer === "unavailable") { + await until(async () => chopin.asked() >= 1, 10_000); + expect(said).toContain("Couldn't get a GitHub token for octo-org/score from Chopin"); + } else { + await serving; + expect(said).toContain( + `Stopped the Planner for D_1: ${githubRefusal(answer, "octo-org/score")}`, + ); + expect(chopin.asked()).toBe(1); + } + // Never started without one, and never with the machine's own login. + expect(existsSync(log)).toBe(false); } finally { abort.abort(); await serving; diff --git a/apps/connector/src/planner.ts b/apps/connector/src/planner.ts index 60f1dfbb3..36f4228b0 100644 --- a/apps/connector/src/planner.ts +++ b/apps/connector/src/planner.ts @@ -15,8 +15,15 @@ import { import { ConnectorError } from "./mcp"; import { backoff, delay, HEALTHY_MS, transient } from "./reconnect"; import type { Output } from "./output"; -import { gitCredentials, type GitHubToken, githubToken } from "./github-token"; +import { + githubEnvironment, + GitHubRefused, + type GitHubToken, + githubToken, + withoutGitHub, +} from "./github-token"; import { type PlannerTokenFile, plannerTokenFile } from "./planner-token"; +import { type McpTokenFile, mcpTokenFile } from "./mcp-token"; import { type ChosenPlanners, plannerWorktree, @@ -96,8 +103,6 @@ export async function servePlanners(options: { /** The `--env` variables connect saved, added to each backend's environment. */ env?: Record; healthyMs?: number; - /** Hears, once each time, why Chopin gave no GitHub token when its App can't push. */ - githubRefused?: (message: string, repository: string) => void; /** How long before it expires the GitHub token is replaced; for tests. */ renewGitHubBeforeMs?: number; /** Where each backend starts: the machine's process sandbox, else plainly. */ @@ -134,8 +139,6 @@ export function dispatchedPlanners(options: { chosen?: ChosenPlanners; env?: Record; healthyMs?: number; - /** Hears, once each time, why Chopin gave no GitHub token when its App can't push. */ - githubRefused?: (message: string, repository: string) => void; /** How long before it expires the GitHub token is replaced; for tests. */ renewGitHubBeforeMs?: number; sandbox?: Sandbox; @@ -281,8 +284,6 @@ async function servePlanner(options: { failures?: StartFailures; env?: Record; healthyMs?: number; - /** Hears, once each time, why Chopin gave no GitHub token when its App can't push. */ - githubRefused?: (message: string, repository: string) => void; /** How long before it expires the GitHub token is replaced; for tests. */ renewGitHubBeforeMs?: number; sandbox?: Sandbox; @@ -361,28 +362,57 @@ async function servePlanner(options: { // Its tokens are kept in a folder of their own, which is all of them a sandbox is shown. let tokenDirectory = tokenFolder(worktree); await mkdir(tokenDirectory, { recursive: true, mode: 0o700 }).catch(() => {}); - // The machine's own GitHub token, where Chopin gives one; without one the backend still starts - // in a registered checkout. - let github = await githubToken({ - url: options.url, - machine: options.machine, - repository, - file: join(tokenDirectory, "github"), - ...(options.githubRefused ? { refused: options.githubRefused } : {}), - ...(options.renewGitHubBeforeMs === undefined - ? {} - : { renewBeforeMs: options.renewGitHubBeforeMs }), - }); + // Chopin's GitHub token for the document's repository, which the backend needs before it + // starts: a refusal stops this Planner with the reason, and the machine never falls back to its + // own GitHub login. Chopin out of reach is retried, as a failed start is. let launched = Date.now(); let tokens: PlannerTokenFile | undefined; - let outcome: { how: string; stderr?: string; reason?: string }; - let checkout = await options.checkouts.resolve(repository, async () => github).then( - value => value.root, - (error: unknown) => { - outcome = { how: error instanceof Error ? error.message : String(error) }; - return undefined; - }, - ); + let mcp: McpTokenFile | undefined; + let outcome: { how: string; stderr?: string; reason?: string } | undefined; + let github: GitHubToken | undefined; + try { + github = await githubToken({ + url: options.url, + machine: options.machine, + repository, + purpose: "planner", + ...(credential.document ?? options.id + ? { document: credential.document ?? options.id } + : {}), + folder: tokenDirectory, + ...(options.renewGitHubBeforeMs === undefined + ? {} + : { renewBeforeMs: options.renewGitHubBeforeMs }), + }); + } catch (error) { + if (signal.aborted) return; + if (error instanceof GitHubRefused) { + output.error(`Stopped the Planner for ${document}: ${error.message}`); + await options.failures?.failed({ + kind: "planner", + document, + how: error.message, + stopped: true, + at: Date.now(), + }); + report(reportedAs, { attempts: 1, how: plain(error.message), at: Date.now() }); + return; + } + outcome = { + how: `Couldn't get a GitHub token for ${repository} from Chopin (${ + redacted(error instanceof Error ? error.message : String(error)).split("\n")[0] + })`, + }; + } + let checkout = github + ? await options.checkouts.resolve(repository, async () => github!).then( + value => value.root, + (error: unknown) => { + outcome = { how: error instanceof Error ? error.message : String(error) }; + return undefined; + }, + ) + : undefined; if (checkout) { output.say(`› Planner for ${document} started.`); let healthy = setTimeout(() => { @@ -394,12 +424,21 @@ async function servePlanner(options: { } }, healthyMs); // The backend may outlive its credential when its link drops, so it can read a new one. + // Chopin's own tools take a token minted from it, renewed with it, never a GitHub token. + mcp = await mcpTokenFile({ + url: options.url, + machine: options.machine, + file: join(tokenDirectory, "mcp"), + credential: { planner: credential.token }, + scheduled: false, + }); tokens = await plannerTokenFile({ url: options.url, machine: options.machine, document, file: join(tokenDirectory, "planner"), first: credential, + renewed: token => mcp!.renew({ planner: token }), }); // A backend in the sandbox can't make its own worktree of the checkout (see // `preparePlannerWorktree`), so it is made here. Never fatal: a backend that has no use @@ -415,15 +454,22 @@ async function servePlanner(options: { ) ); } + // A token that lapsed without a renewal stops the backend, which starts again with a new one. + let lapsed = github!.expired; outcome = await launch( - { ...options, document, cwd: checkout }, + { ...options, document, cwd: checkout, signal: AbortSignal.any([signal, lapsed]) }, credential.token, worktree, - github, + github!, repository, tokens, + mcp, ); + if (lapsed.aborted && !signal.aborted) { + outcome = { how: (lapsed.reason as Error).message }; + } await tokens?.stop(); + await mcp?.stop(); clearTimeout(healthy); } else { output.error(`Can't start the Planner for ${document}: ${outcome!.how}`); @@ -437,6 +483,7 @@ async function servePlanner(options: { let secrets = [ credential.token, ...tokens?.secrets() ?? [], + ...mcp?.secrets() ?? [], ...github?.secrets() ?? [], ...values, ]; @@ -580,9 +627,10 @@ async function launch( }, token: string, worktree: string, - github?: GitHubToken, - repository?: string, + github: GitHubToken, + repository: string, tokens?: PlannerTokenFile, + mcp?: McpTokenFile, ): Promise<{ how: string; stderr?: string; reason?: string }> { let child; try { @@ -590,24 +638,19 @@ async function launch( command: options.command, cwd: options.cwd, env: { - ...process.env, - ...options.env, - // Its own git, and its agent's, reach GitHub with this token over https, whatever - // the person's Git configuration says about GitHub URLs or SSH. - ...(github && repository ? gitCredentials(github.file, repository) : {}), + // Nothing of the machine's GitHub setup: its own git, gh and agent reach GitHub with + // Chopin's token over https, and commit as Chopin's App (see `githubEnvironment`). + ...withoutGitHub({ ...process.env, ...options.env }), + ...githubEnvironment(github, repository), CHOPIN_URL: options.url, CHOPIN_DOCUMENT: options.document, CHOPIN_PLANNER_TOKEN: token, ...(tokens ? { CHOPIN_PLANNER_TOKEN_FILE: tokens.file } : {}), + ...(mcp ? { CHOPIN_MCP_TOKEN_FILE: mcp.file } : {}), CHOPIN_CHECKOUT: options.cwd, CHOPIN_WORKTREE: worktree, - ...(github - ? { - CHOPIN_GITHUB_TOKEN: github.token, - CHOPIN_GITHUB_TOKEN_EXPIRES_AT: new Date(github.expiresAt).toISOString(), - CHOPIN_GITHUB_TOKEN_FILE: github.file, - } - : {}), + CHOPIN_GITHUB_TOKEN: github.token, + CHOPIN_GITHUB_TOKEN_EXPIRES_AT: new Date(github.expiresAt).toISOString(), }, stdin: "ignore", // It writes its own worktree, which the connector made for it, and nothing of the checkout @@ -623,7 +666,8 @@ async function launch( let secrets = () => [ token, ...tokens?.secrets() ?? [], - ...github?.secrets() ?? [], + ...mcp?.secrets() ?? [], + ...github.secrets(), ...Object.values(options.env ?? {}), ]; let first: string | undefined; diff --git a/apps/connector/src/run.test.ts b/apps/connector/src/run.test.ts index f805ca426..b159b2347 100644 --- a/apps/connector/src/run.test.ts +++ b/apps/connector/src/run.test.ts @@ -24,6 +24,8 @@ import { timedOut } from "./acp"; import { git } from "./workspace"; import { ConnectorError, remote } from "./mcp"; import { register } from "./machine"; +import { BOT, STARTER, testGitHubToken } from "./testing/github-grant"; +import { githubRefusal } from "./github-token"; import { Server } from "@modelcontextprotocol/sdk/server/index.js"; import { WebStandardStreamableHTTPServerTransport } from "@modelcontextprotocol/sdk/server/webStandardStreamableHttp.js"; import { CallToolRequestSchema, ListToolsRequestSchema } from "@modelcontextprotocol/sdk/types.js"; @@ -85,6 +87,7 @@ async function run( url: "http://127.0.0.1:1", signal, heartbeatMs: 20, + github: request => testGitHubToken(request.folder), ...options, }); await inspect?.({ root, state, id }); @@ -272,6 +275,7 @@ test.each([true, false])( url: `http://127.0.0.1:${chopin.port}`, signal: new AbortController().signal, output: terminal.output, + github: request => testGitHubToken(request.folder), }); expect(terminal.out()).toContain( spike @@ -810,6 +814,7 @@ async function connected(options: { output: terminal.output, heartbeatMs: 20, random: () => 0.01, + github: request => testGitHubToken(request.folder), ...options.runOptions, }), }); @@ -1125,6 +1130,7 @@ test("a run whose connection a restarted server forgot stops its agent and the c url: server.url, signal, output, + github: request => testGitHubToken(request.folder), heartbeatMs: 20, random: () => 0.01, forgetMs: 300, @@ -1139,41 +1145,124 @@ test("a run whose connection a restarted server forgot stops its agent and the c } }); +/** + * A machine whose own GitHub setup is all decoys, as design calls for: a HOME with gh logged in as + * two other accounts, a credential helper, SSH rewrites, an identity and signing of its own, an SSH + * key; GH_TOKEN, GITHUB_TOKEN and an SSH agent in the environment; and a `gh` and `ssh` first on + * PATH that note it when anything reaches them other than through the launch's own gh + * configuration. Restores everything on `restore`. + */ +async function decoyMachine(scratch: string) { + let home = join(scratch, "home"); + let tripwire = join(scratch, "tripwire"); + await mkdir(join(home, ".config", "gh"), { recursive: true }); + await writeFile( + join(home, ".config", "gh", "hosts.yml"), + "github.com:\n users:\n decoy-active:\n oauth_token: decoy-active-token\n" + + " decoy-other:\n oauth_token: decoy-other-token\n" + + " oauth_token: decoy-active-token\n user: decoy-active\n", + ); + await mkdir(join(home, ".ssh")); + await writeFile(join(home, ".ssh", "id_ed25519"), "decoy key\n", { mode: 0o600 }); + await writeFile( + join(home, ".gitconfig"), + [ + "[user]", + "\tname = Decoy Person", + "\temail = decoy@example.test", + "[commit]", + "\tgpgsign = true", + "[gpg]", + "\tprogram = /bin/false", + "[credential]", + `\thelper = "!f() { echo helper >> '${tripwire}'; echo password=decoy-helper; }; f"`, + '[url "git@github.com:"]', + "\tinsteadOf = https://github.com/", + "\tpushInsteadOf = https://github.com/", + "", + ].join("\n"), + ); + let bin = join(scratch, "bin"); + await mkdir(bin); + // gh as an agent runs it: a call through the launch's configuration records the token it holds + // and its arguments; anything else is a call that reached the machine's own gh. + await writeFile( + join(bin, "gh"), + `#!/bin/sh +if [ -n "$GH_TOKEN$GITHUB_TOKEN" ] || [ -z "$GH_CONFIG_DIR" ] || [ "$GH_CONFIG_DIR" = "${home}/.config/gh" ]; then + echo "gh $*" >> '${tripwire}'; exit 1 +fi +token=$(sed -n 's/^ oauth_token: "\\(.*\\)"$/\\1/p' "$GH_CONFIG_DIR/hosts.yml") +printf '%s\\n' "$token" "$@" > '${join(scratch, "gh")}' +`, + { mode: 0o755 }, + ); + await writeFile(join(bin, "ssh"), `#!/bin/sh\necho "ssh $*" >> '${tripwire}'\nexit 255\n`, { + mode: 0o755, + }); + let names = [ + "HOME", + "PATH", + "GH_TOKEN", + "GITHUB_TOKEN", + "SSH_AUTH_SOCK", + "GIT_CONFIG_GLOBAL", + "GIT_ASKPASS", + ] as const; + let saved = Object.fromEntries(names.map(name => [name, process.env[name]])); + process.env.HOME = home; + process.env.PATH = `${bin}:${process.env.PATH}`; + process.env.GH_TOKEN = "decoy-env"; + process.env.GITHUB_TOKEN = "decoy-env"; + process.env.SSH_AUTH_SOCK = join(scratch, "agent.sock"); + process.env.GIT_CONFIG_GLOBAL = join(home, ".gitconfig"); + process.env.GIT_ASKPASS = join(bin, "ssh"); + return { + home, + /** What reached the machine's own gh, credential helper or SSH, one line each. */ + tripped: async () => existsSync(tripwire) ? (await readFile(tripwire, "utf8")).trim() : "", + restore() { + for (let [name, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[name]; + else process.env[name] = value; + } + }, + }; +} + +/** A Chopin that issues a GitHub App installation token for runs, and a token for its tools. */ +function issuingChopin(token: string, answer?: () => Response) { + let asked: unknown[] = []; + let server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + async fetch(request) { + let { pathname } = new URL(request.url); + if (pathname === "/api/connector/mcp-token") { + return Response.json({ token: "chopin_mcp_run", expiresAt: Date.now() + 3_600_000 }); + } + if (pathname !== "/api/connector/github-token") return new Response(null, { status: 404 }); + asked.push(await request.json()); + return answer?.() ?? Response.json({ + token, + expiresAt: Date.now() + 3_600_000, + kind: "installation", + identity: BOT, + starter: STARTER, + }); + }, + }); + return { url: `http://127.0.0.1:${server.port}`, asked, stop: () => server.stop(true) }; +} + test.skipIf(process.platform === "win32")( - "a build's agent pushes and opens draft pull requests with the machine's GitHub token, gone when the run ends", + "a build's agent reaches GitHub only with the token Chopin issued for it, commits as Chopin's App for the person who started it, and nothing of the machine's own GitHub setup is used", async () => { - let token = `ghu_${"R".repeat(36)}`; - let chopin = Bun.serve({ - hostname: "127.0.0.1", - port: 0, - async fetch(request) { - if (new URL(request.url).pathname !== "/api/connector/github-token") { - return new Response(null, { status: 404 }); - } - expect(await request.json()).toEqual({ repository: "org/repo" }); - return Response.json({ token, expiresAt: Date.now() + 8 * 3_600_000 }); - }, - }); + let token = `ghs_${"R".repeat(36)}`; + let chopin = issuingChopin(token); let scratch = await mkdtemp(join(tmpdir(), "chopin-github-run-")); - let bin = join(scratch, "bin"); - await mkdir(bin); - // gh as the agent runs it: it records the token it was given and its arguments. - await writeFile( - join(bin, "gh"), - `#!/bin/sh\nprintf '%s\\n' "$GH_TOKEN" "$@" > "${join(scratch, "gh")}"\n`, - ); - await chmod(join(bin, "gh"), 0o755); + let machine = await decoyMachine(scratch); let seen = join(scratch, "github.json"); - // The person's own Git configuration sends GitHub over SSH, which a service can't use. - let global = join(scratch, "gitconfig"); - await writeFile( - global, - '[url "git@github.com:"]\n\tinsteadOf = https://github.com/\n\tpushInsteadOf = https://github.com/\n', - ); - let { GIT_CONFIG_GLOBAL } = process.env; - process.env.GIT_CONFIG_GLOBAL = global; - let path = process.env.PATH; - process.env.PATH = `${bin}:${path}`; process.env.CHOPIN_TEST_GITHUB = seen; let written = ""; let capture = { @@ -1183,23 +1272,40 @@ test.skipIf(process.platform === "win32")( }, }; try { - await run({}, ["--github"], undefined, undefined, { - url: `http://127.0.0.1:${chopin.port}`, + let { id, documentId } = await run({}, ["--github"], undefined, undefined, { + url: chopin.url, machine: `chopin_m_${"M".repeat(43)}`, + github: undefined, output: createOutput(capture, capture, {}), - }, async ({ state, id }) => { - // The token file was the run's alone, and goes with it. + }, async ({ state }) => { + // The tokens were the run's alone, and go with it. let runs = join(state, "runs"); - expect((await readdir(runs)).filter(name => name.includes("github"))).toEqual([]); - expect(id).toBeTruthy(); + let left = await readdir(runs); + for (let name of left.filter(name => name.endsWith(".tokens"))) { + expect(await readdir(join(runs, name))).toEqual([]); + } }); + expect(chopin.asked).toEqual([{ + repository: "org/repo", + purpose: "run", + run: id, + document: documentId, + }]); let github = JSON.parse(await readFile(seen, "utf8")); + let bot = `${BOT.name} <${BOT.email}>`; expect(github).toEqual({ - GH_TOKEN: token, - GITHUB_TOKEN: token, + GH_TOKEN: null, + GITHUB_TOKEN: null, + SSH_AUTH_SOCK: null, + GIT_SSH_COMMAND: "false", + starter: [STARTER.login, STARTER.name, STARTER.email], + githubFile: token, + mcpFile: "chopin_mcp_run", + // Made as the App, and not signed, whatever the machine's Git configuration says. + commit: `${bot}|${bot}|N`, credential: `protocol=https\nhost=github.com\nusername=x-access-token\npassword=${token}\n`, gh: 0, - // The checkout's origin is an SSH address, and the rewrite sends https to SSH too: + // The checkout's origin is an SSH address, and the machine rewrites https to SSH too: // the agent's fetches and pushes still go over https, where the token is used. fetch: "https://github.com/org/repo.git", push: "https://github.com/org/repo.git", @@ -1213,47 +1319,54 @@ test.skipIf(process.platform === "win32")( "Task one", "", ]); + expect(await machine.tripped()).toBe(""); expect(written).not.toContain(token); } finally { - process.env.PATH = path; - if (GIT_CONFIG_GLOBAL === undefined) delete process.env.GIT_CONFIG_GLOBAL; - else process.env.GIT_CONFIG_GLOBAL = GIT_CONFIG_GLOBAL; + machine.restore(); delete process.env.CHOPIN_TEST_GITHUB; - await chopin.stop(true); + chopin.stop(); await rm(scratch, { recursive: true, force: true }); } }, ); test.skipIf(process.platform === "win32")( - "without a GitHub token from Chopin, a build's agent keeps its own login", + "a run Chopin refuses a GitHub token never starts its agent, says why naming the repository, and never falls back to the machine's own login", async () => { - let chopin = Bun.serve({ - hostname: "127.0.0.1", - port: 0, - fetch: () => Response.json({ error: "not-found" }, { status: 404 }), - }); - let scratch = await mkdtemp(join(tmpdir(), "chopin-github-none-")); - let seen = join(scratch, "github.json"); - process.env.CHOPIN_TEST_GITHUB = seen; - let { GH_TOKEN, GITHUB_TOKEN } = process.env; - delete process.env.GH_TOKEN; - delete process.env.GITHUB_TOKEN; - try { - await run({}, ["--github"], undefined, undefined, { - url: `http://127.0.0.1:${chopin.port}`, - machine: `chopin_m_${"M".repeat(43)}`, - }); - expect(JSON.parse(await readFile(seen, "utf8"))).toMatchObject({ - GH_TOKEN: null, - GITHUB_TOKEN: null, - }); - } finally { - if (GH_TOKEN !== undefined) process.env.GH_TOKEN = GH_TOKEN; - if (GITHUB_TOKEN !== undefined) process.env.GITHUB_TOKEN = GITHUB_TOKEN; - delete process.env.CHOPIN_TEST_GITHUB; - await chopin.stop(true); - await rm(scratch, { recursive: true, force: true }); + for (let code of ["repository-forbidden", "app-cannot-push", "starter-cannot-push"]) { + let chopin = issuingChopin( + "unused", + () => Response.json({ error: code, message: "refused" }, { status: 403 }), + ); + let scratch = await mkdtemp(join(tmpdir(), "chopin-github-refused-")); + let machine = await decoyMachine(scratch); + let started = join(scratch, "started"); + let terminal = captured(); + try { + let { id, calls } = await run({}, [], undefined, undefined, { + url: chopin.url, + machine: `chopin_m_${"M".repeat(43)}`, + github: undefined, + output: terminal.output, + command: ["/bin/sh", "-c", `touch '${started}'`], + }); + expect(existsSync(started)).toBe(false); + let failed = calls.find(([name, args]) => + name === "report_implementation_build" && args.state === "failed" + ); + expect(failed).toEqual(["report_implementation_build", { + id, + state: "failed", + error: githubRefusal(code, "org/repo"), + }]); + expect(githubRefusal(code, "org/repo")).toContain("org/repo"); + expect(terminal.err()).toContain(githubRefusal(code, "org/repo")); + expect(await machine.tripped()).toBe(""); + } finally { + machine.restore(); + chopin.stop(); + await rm(scratch, { recursive: true, force: true }); + } } }, ); @@ -1269,8 +1382,8 @@ test.skipIf(process.platform === "win32")( if (new URL(request.url).pathname !== "/api/connector/github-token") { return new Response(null, { status: 404 }); } - expect(await request.json()).toEqual({ repository: "org/repo" }); - return Response.json({ token, expiresAt: Date.now() + 8 * 3_600_000 }); + expect(await request.json()).toMatchObject({ repository: "org/repo", purpose: "run" }); + return Response.json({ token, expiresAt: Date.now() + 8 * 3_600_000, identity: BOT }); }, }); let scratch = await mkdtemp(join(tmpdir(), "chopin-spike-token-")); diff --git a/apps/connector/src/run.ts b/apps/connector/src/run.ts index d98d76295..11dd44d12 100644 --- a/apps/connector/src/run.ts +++ b/apps/connector/src/run.ts @@ -6,7 +6,16 @@ import { ConnectorStopped, failureKind, redacted } from "./connection"; import { errorCode } from "./machine"; import { backoff, delay, forgottenMessage, RUN_GRACE_MS, runReports } from "./reconnect"; import { bridgeCommand } from "./bridge"; -import { gitCredentials, githubEnvironment, type GitHubToken, githubToken } from "./github-token"; +import { + gitCredentials, + githubEnvironment, + GitHubRefused, + type GitHubToken, + githubToken, + withoutGitHub, +} from "./github-token"; +import type { GitHubGrantRequest } from "./machine"; +import { type McpTokenFile, mcpTokenFile } from "./mcp-token"; import { AgentTimedOut, runAgent, timedOut } from "./acp"; import type { AgentInfo } from "./acp"; import { serveLocalBridge } from "./local-bridge"; @@ -66,10 +75,13 @@ type Options = { failures?: StartFailures; /** The `--env` variables connect saved, for the agent. */ env?: Record; - /** The machine token, to ask Chopin for the machine's GitHub token for a run that may push. */ + /** The machine token, to ask Chopin for the GitHub token every run needs. */ machine?: string; - /** Hears, once each time, why Chopin gave no GitHub token when its App can't push. */ - githubRefused?: (message: string, repository: string) => void; + /** + * Asks Chopin for a run's GitHub token, written in `folder`; by default with `machine`. A run + * with neither is refused, as one Chopin gave no token is. + */ + github?: (request: GitHubGrantRequest & { folder: string }) => Promise; /** Where the agent starts: the machine's process sandbox, else plainly. */ sandbox?: Sandbox; /** How long a build's or rebuild's agent may work, for tests; `BUILD_LIMIT_MS` by default. */ @@ -228,34 +240,50 @@ export async function runWork( /** The checkout the run's worktree comes from. */ let root: string | undefined; /** - * The machine's GitHub token for this run, where Chopin gives one: for the agent, though a - * spike never pushes, and to clone or fetch the repository when it has no registered checkout. + * Chopin's GitHub token for this run, which every run needs before anything starts: to clone or + * fetch the repository, and for the agent to push with, though a spike's agent is given none. + * A refusal ends the run with the reason, and the machine never uses its own GitHub login. */ - let issued: Promise | undefined; + let issued: Promise | undefined; let github: GitHubToken | undefined; let tokens = tokenFolder( join(options.directory, "runs", runAttempt(input.id, implementation ? 1 : claim.generation)), ); + let repository = input.source.repository; + let request: GitHubGrantRequest & { folder: string } = { + repository, + purpose: "run", + run: input.id, + ...(claim.documentId ? { document: claim.documentId } : {}), + folder: tokens, + }; let token = () => - issued ??= options.machine - ? mkdir(tokens, { recursive: true, mode: 0o700 }).then(() => - githubToken({ - url: options.url, - machine: options.machine!, - repository: input.source.repository, - file: join(tokens, "github"), - ...(options.githubRefused ? { refused: options.githubRefused } : {}), - }) - ) - : Promise.resolve(undefined); - let secrets = () => [...Object.values(options.env ?? {}), ...github?.secrets() ?? []]; + issued ??= mkdir(tokens, { recursive: true, mode: 0o700 }).then(() => + options.github + ? options.github(request) + : options.machine + ? githubToken({ ...request, url: options.url, machine: options.machine }) + : Promise.reject(new GitHubRefused("no-grant", repository)) + ); + // A token that lapsed without a renewal stops the agent, which could no longer push. + let lapsed = () => { + ended ??= (github?.expired.reason as Error | undefined)?.message; + abort.abort(); + }; + /** The token the agent's backend shows Chopin's own tools, minted from the run's credential. */ + let mcp: McpTokenFile | undefined; + let secrets = () => [ + ...Object.values(options.env ?? {}), + ...github?.secrets() ?? [], + ...mcp?.secrets() ?? [], + ]; let agentStop: string | undefined; /** How the run ended, for its journal; a run cut short before it ends says nothing. */ let outcome: RunOutcome | undefined; // A build's agent works through every task in one turn, so it gets longer than an investigation. let limit = implementation ? options.buildLimitMs ?? BUILD_LIMIT_MS : INVESTIGATION_LIMIT_MS; try { - let checkout = await options.checkouts.resolve(input.source.repository, token); + let checkout = await options.checkouts.resolve(repository, token); root = checkout.root; prepared = await prepareWorkspace( root, @@ -269,13 +297,11 @@ export async function runWork( // environment and no one else's. Object.keys(checkout.env).length > 0 ? checkout.env - : await token().then(value => - value ? gitCredentials(value.file, input.source.repository) : {} - ), + : gitCredentials((await token()).file, repository), ); // A prototype never pushes and its agent is given no token, in its environment or on disk // where it could read it: the one that fetched its commit is gone before the agent starts. - if (claim.spike) await issued?.then(value => value?.stop()).catch(() => {}); + if (claim.spike) await issued?.then(value => value.stop()).catch(() => {}); let path = prepared.path; // A sandboxed agent can't add worktrees, which its prompt says (see `RunWorktrees`). let confined = options.sandbox !== undefined && options.sandbox.report.engine !== "none"; @@ -288,7 +314,19 @@ export async function runWork( spikeWork = { path, branch: `chopin/spike-${input.id.slice(0, 8)}` }; git(path, "checkout", "-b", spikeWork.branch); } - if (!claim.spike) github = await token(); + if (!claim.spike) { + github = await token(); + github.expired.addEventListener("abort", lapsed, { once: true }); + if (options.machine) { + mcp = await mcpTokenFile({ + url: options.url, + machine: options.machine, + file: join(tokens, "mcp"), + credential: { run: claim.runToken }, + scheduled: true, + }); + } + } output.job( rebuild ? "rebuild" : implementation ? "build" : claim.spike ? "spike" : "investigation", implementation @@ -309,11 +347,22 @@ export async function runWork( command: options.command, cwd: path, env: { - ...options.env, + ...withoutGitHub(options.env ?? {}), // Where a build's agent adds any extra worktrees, as its prompt says. ...(implementation ? { CHOPIN_RUN_WORKTREES: prepared.extras } : {}), - ...(github ? githubEnvironment(github, input.source.repository) : {}), - ...(claim.spike ? noPushEnvironment() : {}), + ...(github ? githubEnvironment(github, repository) : {}), + ...(mcp ? { CHOPIN_MCP_TOKEN_FILE: mcp.file } : {}), + // A spike's agent reaches GitHub with nothing: no gh login, credential helper or SSH key + // of the machine's, and it can't push. + ...(claim.spike + ? { + GH_CONFIG_DIR: join(tokens, "no-gh"), + GH_PROMPT_DISABLED: "1", + GIT_TERMINAL_PROMPT: "0", + GIT_SSH_COMMAND: "false", + ...noPushEnvironment([["credential.helper", ""]]), + } + : {}), }, onStart: failure => void (failure @@ -459,6 +508,8 @@ export async function runWork( // journal and in Chopin, never as an unexplained failure. let late = error instanceof AgentTimedOut || error instanceof Error && error.message === timedOut(limit); + // Chopin gave no GitHub token, and its reason names the repository and what fixes it. + let refused = error instanceof GitHubRefused; let message = stopped ? STOPPED : ended @@ -484,7 +535,7 @@ export async function runWork( await send(implementation ? "report_implementation_build" : "fail_experiment", { ...identity, ...(implementation ? { state: "failed", idempotencyKey: crypto.randomUUID() } : {}), - error: stopped || ended || late || !implementation + error: stopped || ended || late || refused || !implementation ? message.slice(0, 2000) : "Local implementation failed. Inspect the connector terminal.", }).catch(() => {}); @@ -501,7 +552,9 @@ export async function runWork( if (spikeWork && root) { await discardWorkspace(root, spikeWork.path, spikeWork.branch).catch(() => {}); } - await issued?.then(value => value?.stop()).catch(() => {}); + github?.expired.removeEventListener("abort", lapsed); + await mcp?.stop().catch(() => {}); + await issued?.then(value => value.stop()).catch(() => {}); // Extra worktrees end with the run; the run's own stays for inspection. if (prepared && root) { await endWorkspace(root, prepared, outcome).catch(() => {}); diff --git a/apps/connector/src/sandbox-linux.test.ts b/apps/connector/src/sandbox-linux.test.ts index 125e903e2..36e23cd20 100644 --- a/apps/connector/src/sandbox-linux.test.ts +++ b/apps/connector/src/sandbox-linux.test.ts @@ -1,4 +1,5 @@ import { afterAll, expect, test } from "bun:test"; +import { answerTokens, testGitHubToken } from "./testing/github-grant"; import { spawnSync } from "node:child_process"; import { createHash } from "node:crypto"; import { @@ -12,6 +13,7 @@ import { realpathSync, renameSync, rmSync, + statSync, symlinkSync, writeFileSync, } from "node:fs"; @@ -21,6 +23,7 @@ import { runAgent } from "./acp"; import { checkouts } from "./checkouts"; import { createOutput } from "./output"; import { servePlanners } from "./planner"; +import { runWork } from "./run"; import { discardWorkspace, preparePlannerWorktree, prepareWorkspace } from "./workspace"; import { gitAsync, gitSync, repositoryAt } from "./git-host"; import { createSandbox, inspectSandbox } from "./sandbox"; @@ -250,7 +253,10 @@ async function plannerBackend(linked: boolean) { let chopin = Bun.serve({ hostname: "127.0.0.1", port: 0, - fetch(request) { + async fetch(request) { + // The GitHub token the backend needs before it starts. + let issued = await answerTokens(request); + if (issued) return issued; if (new URL(request.url).pathname === "/api/connector/planner") { return Response.json({ token: `chopin_p_${"P".repeat(43)}`, @@ -1638,3 +1644,245 @@ curl -sS -m 10 -o /dev/null ${chopin.url} > /dev/null 2>&1 && echo chopin=ok || }, 180_000, ); + +/** The processes, other than this test's own, whose command line holds any of `values`. */ +function onCommandLines(values: string[]) { + let found: string[] = []; + for (let pid of readdirSync("/proc").filter(name => /^\d+$/.test(name))) { + let line: string; + try { + line = readFileSync(`/proc/${pid}/cmdline`, "utf8").replaceAll("\0", " "); + } catch { + continue; + } + if (Number(pid) === process.pid) continue; + for (let value of values) if (line.includes(value)) found.push(`${pid}: ${line.slice(0, 200)}`); + } + return found; +} + +/** Waits for `path` to be written, as a launch says it started. */ +async function written(path: string, ms = 60_000) { + for (let deadline = Date.now() + ms; !existsSync(path); await Bun.sleep(50)) { + if (Date.now() > deadline) throw new Error(`${path} was never written.`); + } + await Bun.sleep(50); + return JSON.parse(readFileSync(path, "utf8")) as Record; +} + +/** The mode of `path`, as octal digits. */ +function mode(path: string) { + return (statSync(path).mode & 0o777).toString(8); +} + +sandboxTest( + "a sandboxed launch has every variable it was given, with none of their values on any process's command line", + async () => { + let made = world("sealed"); + let sandbox = await createSandbox({ + directory: made.state, + home, + server: "http://127.0.0.1:1", + }); + let secret = `secret-${crypto.randomUUID()}`; + let tricky = `it's "${secret}" $HOME \`x\`\nnext line`; + let report = join(made.root, "work"); + mkdirSync(report); + try { + let child = sandbox.spawn({ + command: [ + "/bin/sh", + "-c", + 'printf "%s" "$CHOPIN_TEST_TRICKY" > "$PWD/env.txt"; ' + + 'printf "%s" "${CHOPIN_LAUNCH_ENVIRONMENT-unset}" > "$PWD/file.txt"; ' + + 'ls -A "$TMPDIR" > "$PWD/tmp.txt"; ' + + 'while [ ! -e "$PWD/release" ]; do sleep 0.05; done', + ], + cwd: report, + env: { PATH: process.env.PATH, HOME: home, CHOPIN_TEST_TRICKY: tricky }, + stdin: "ignore", + own: [report], + }); + let closed = new Promise(resolve => child.once("close", resolve)); + for (let deadline = Date.now() + 30_000; !existsSync(join(report, "tmp.txt"));) { + if (Date.now() > deadline) throw new Error("The launch never started."); + await Bun.sleep(50); + } + await Bun.sleep(100); + expect(onCommandLines([secret])).toEqual([]); + writeFileSync(join(report, "release"), ""); + await closed; + // The value as given, quotes, a newline and a `$` included, and the file it came in gone. + expect(readFileSync(join(report, "env.txt"), "utf8")).toBe(tricky); + expect(readFileSync(join(report, "file.txt"), "utf8")).toBe("unset"); + expect(readFileSync(join(report, "tmp.txt"), "utf8")).toBe(""); + } finally { + await sandbox.close(); + } + }, + 60_000, +); + +sandboxTest( + "a sandboxed Planner has its credential and GitHub token as before, from files of mode 0600, and neither is on any process's command line", + async () => { + let made = world("planner-credentials"); + let credential = `chopin_p_${crypto.randomUUID().replaceAll("-", "")}`; + let secret = `opaque-${crypto.randomUUID()}`; + let chopin = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + async fetch(request) { + let issued = await answerTokens(request); + if (issued) return issued; + return Response.json({ + token: credential, + expiresAt: Date.now() + 15 * 60_000, + document: { id: "D_1", repository: "octo-org/score" }, + }); + }, + }); + let origin = chopin.url.origin; + let sandbox = await createSandbox({ directory: made.state, home, server: origin }); + let abort = new AbortController(); + let quiet = { write: () => true }; + let worktree = join(made.state, "planners", "octo-org", "score", "D_1"); + let tokens = `${worktree}.tokens`; + let serving = servePlanners({ + url: origin, + machine: MACHINE, + documents: ["D_1"], + command: [process.execPath, join(import.meta.dir, "testing/credential-planner.ts")], + checkouts: checkouts({ + directory: made.state, + registered: { "octo-org/score": made.checkout }, + }), + directory: made.state, + output: createOutput(quiet, quiet, {}), + signal: abort.signal, + env: { + OPAQUE_API_KEY: secret, + CHOPIN_TEST_HELD_NAMES: "CHOPIN_PLANNER_TOKEN,CHOPIN_GITHUB_TOKEN,OPAQUE_API_KEY", + }, + sandbox, + }); + try { + let held = await written(join(worktree, "held.json")); + let github = readFileSync(join(tokens, "github"), "utf8").trim(); + let mcp = readFileSync(join(tokens, "mcp"), "utf8").trim(); + // A backend that reads the variables still has them. + expect(held).toEqual({ + CHOPIN_PLANNER_TOKEN: credential, + CHOPIN_GITHUB_TOKEN: github, + OPAQUE_API_KEY: secret, + }); + expect(onCommandLines([credential, github, mcp, secret])).toEqual([]); + for (let name of ["github", "planner", "mcp", join("gh", "hosts.yml")]) { + expect(mode(join(tokens, name)), name).toBe("600"); + } + expect(mode(tokens)).toBe("700"); + } finally { + abort.abort(); + await serving; + await sandbox.close(); + await chopin.stop(true); + } + }, + 90_000, +); + +for (let kind of ["build", "investigation", "spike"] as const) { + sandboxTest( + `a sandboxed ${kind}'s agent has its run's variables as before, and no token of the run is on any process's command line`, + async () => { + let made = world(`run-credentials-${kind}`); + let secret = `opaque-${crypto.randomUUID()}`; + let runToken = `chopin_r_${crypto.randomUUID().replaceAll("-", "")}`; + let github = `ghs_${crypto.randomUUID().replaceAll("-", "")}`; + let sandbox = await createSandbox({ + directory: made.state, + home, + server: "http://127.0.0.1:1", + }); + let implementation = kind === "build"; + let id = crypto.randomUUID(); + let source = { + repositoryId: "R_score", + repository: "octo-org/score", + commit: git(made.checkout, "rev-parse", "HEAD"), + }; + let api = { call: async () => ({}) } as unknown as Parameters[0]; + let done = runWork( + api, + implementation ? "implementation" : "experiment", + implementation + ? { documentId: "D_1", build: { id, ...source, checkout: source }, runToken } + : { + spike: kind === "spike", + generation: 1, + runToken, + input: { + id, + documentId: crypto.randomUUID(), + brief: "Look", + source, + context: "", + requester: "a", + authorizer: "a", + }, + }, + { + checkouts: checkouts({ + directory: made.state, + registered: { "octo-org/score": made.checkout }, + }), + directory: made.state, + command: [ + process.execPath, + join(import.meta.dir, "testing/fake-implementer.ts"), + "--hold", + ], + url: "http://127.0.0.1:1", + signal: new AbortController().signal, + output: createOutput({ write: () => true }, { write: () => true }, {}), + heartbeatMs: 60_000, + env: { + OPAQUE_API_KEY: secret, + CHOPIN_TEST_HELD_NAMES: "OPAQUE_API_KEY,CHOPIN_GITHUB_TOKEN_FILE,GH_CONFIG_DIR", + }, + github: request => testGitHubToken(request.folder, github), + sandbox, + }, + ); + let runs = join(made.state, "runs"); + try { + let worktree = ""; + for (let deadline = Date.now() + 60_000; !worktree; await Bun.sleep(50)) { + if (Date.now() > deadline) throw new Error("The agent never started."); + let found = existsSync(runs) + ? readdirSync(runs).find(name => + !name.endsWith(".tokens") && existsSync(join(runs, name, "held.json")) + ) + : undefined; + if (found) worktree = join(runs, found); + } + let held = await written(join(worktree, "held.json")); + expect(held.OPAQUE_API_KEY).toBe(secret); + expect(onCommandLines([secret, runToken, github])).toEqual([]); + if (kind === "spike") { + // A prototype is given no token at all. + expect(held.CHOPIN_GITHUB_TOKEN_FILE).toBeNull(); + } else { + expect(mode(held.CHOPIN_GITHUB_TOKEN_FILE!)).toBe("600"); + expect(mode(join(held.GH_CONFIG_DIR!, "hosts.yml"))).toBe("600"); + expect(mode(dirname(held.CHOPIN_GITHUB_TOKEN_FILE!))).toBe("700"); + } + writeFileSync(join(worktree, "release"), ""); + await done; + } finally { + await sandbox.close(); + } + }, + 90_000, + ); +} diff --git a/apps/connector/src/sandbox-policy.ts b/apps/connector/src/sandbox-policy.ts index 8af7affc5..e0ab0a400 100644 --- a/apps/connector/src/sandbox-policy.ts +++ b/apps/connector/src/sandbox-policy.ts @@ -14,6 +14,11 @@ export type SandboxSettings = { hosts?: string[]; /** Directories agents may write beyond their worktree and own state. */ writable?: string[]; + /** + * Files and folders no agent may read or write, beyond `secretPaths`, even inside a `writable` + * folder: a backend's own sign-in or tokens, say. + */ + hidden?: string[]; }; /** diff --git a/apps/connector/src/sandbox.test.ts b/apps/connector/src/sandbox.test.ts index 54fa12d06..4879d63c9 100644 --- a/apps/connector/src/sandbox.test.ts +++ b/apps/connector/src/sandbox.test.ts @@ -9,6 +9,7 @@ import { realpathSync, renameSync, rmSync, + statSync, symlinkSync, writeFileSync, } from "node:fs"; @@ -16,13 +17,16 @@ import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { sandboxLabel } from "@chopin/protocol/connector"; import type { Host } from "./sandbox-probe"; +import { mxcRequest } from "./sandbox-policy"; import { createSandbox, inspectSandbox, + LAUNCHER, launchPolicy, locateExecutor, SandboxRefused, sdkRoot, + sealedEnvironment, spawnIn, systemHost, } from "./sandbox"; @@ -697,6 +701,40 @@ test("a launch whose working directory holds the connector's state directory, as } }); +test("what the owner hides with --sandbox-hide is in no grant, read or write, even inside a folder they let agents write, and is denied besides", () => { + let root = join(readableScratch, "hidden"); + let home = join(root, "home"); + let base = join(home, ".local", "state", "chopin", "connector"); + let tool = join(home, ".tool"); + mkdirSync(base, { recursive: true }); + mkdirSync(join(tool, "cache"), { recursive: true }); + writeFileSync(join(tool, "github-token"), "a sign-in of the tool's own"); + writeFileSync(join(tool, "settings.json"), "{}"); + mkdirSync(join(root, "work"), { recursive: true }); + for (let platform of ["linux", "darwin"] as const) { + let policy = launchPolicy( + platform === "darwin" ? "seatbelt" : "bubblewrap", + { ...systemHost(), platform }, + { command: ["agent", "planner"], cwd: join(root, "work"), env: {}, stdin: "ignore" }, + { + directory: base, + home, + settings: { writable: [tool], hidden: [join(tool, "github-token")] }, + }, + "http://127.0.0.1:4000", + join(root, "tmp"), + ); + let secret = join(tool, "github-token"); + for (let path of [...policy.readable, ...policy.writable]) { + expect(secret === path || secret.startsWith(`${path}/`), `${platform}: ${path}`).toBe(false); + } + // What is beside it in the folder is still written, and it is denied as a secret is. + expect(policy.writable, platform).toContain(join(tool, "cache")); + expect(policy.writable, platform).toContain(join(tool, "settings.json")); + expect(policy.denied, platform).toContain(secret); + } +}); + test("a working directory inside the state directory is read by name only where it is a clone the connector made, and not at all elsewhere in it", () => { let root = join(readableScratch, "inside"); let base = join(root, "state"); @@ -837,3 +875,45 @@ test("the SDK package is found by walking up node_modules, never by a resolver t mkdirSync(join(scratch, "app", "src", "deep"), { recursive: true }); expect(sdkRoot(join(scratch, "app", "src", "deep"))).toBe(nested); }); + +test("on Linux and macOS a launch's environment goes to the sandbox in a 0600 file its shell reads, never on a command line; Windows passes it as it did", () => { + let folder = mkdtempSync(join(scratch, "sealed-")); + let policy = { + platform: "linux" as NodeJS.Platform, + engine: "bubblewrap" as const, + command: ["agent", "--acp"], + cwd: folder, + env: { + PATH: "/usr/bin", + CHOPIN_PLANNER_TOKEN: "chopin_p_secret", + QUOTED: "it's", + HTTPS_PROXY: "http://person:password@proxy:8080", + "ODD.NAME": "x", + GONE: undefined, + }, + readable: [], + writable: [], + denied: [], + proxy: "http://127.0.0.1:1", + }; + let sealed = sealedEnvironment(policy, folder); + let file = join(folder, ".chopin-launch-environment"); + expect(sealed.command).toEqual(["/bin/sh", "-c", LAUNCHER, "chopin-launch", "agent", "--acp"]); + // The only values left for MXC's command line: the search path, the file, and a name no shell + // can export. + expect(sealed.env).toEqual({ + PATH: "/usr/bin", + CHOPIN_LAUNCH_ENVIRONMENT: file, + "ODD.NAME": "x", + }); + expect(JSON.stringify(mxcRequest(sealed))).not.toContain("chopin_p_secret"); + // The person's proxy neither reaches the command line nor replaces the one MXC sets. + expect(readFileSync(file, "utf8")).toBe( + "PATH='/usr/bin'\nCHOPIN_PLANNER_TOKEN='chopin_p_secret'\nQUOTED='it'\\''s'\n", + ); + expect(statSync(file).mode & 0o777).toBe(0o600); + expect(sealedEnvironment({ ...policy, platform: "win32" }, folder)).toEqual({ + ...policy, + platform: "win32", + }); +}); diff --git a/apps/connector/src/sandbox.ts b/apps/connector/src/sandbox.ts index be3a50d05..78070bfdb 100644 --- a/apps/connector/src/sandbox.ts +++ b/apps/connector/src/sandbox.ts @@ -509,8 +509,16 @@ export function launchPolicy( ...repository ? [repository.commonDir] : [], ...checkout ? [checkout] : [], ]; + // What the owner hides is in no grant, read or write, whatever holds it: the grants that hold + // one are listed around it, as for the state directory, and it is denied as well. + let concealed = (options.settings?.hidden ?? []).map(real); + let around = (paths: readonly string[]) => + concealed.reduce( + (grants, path) => readGrantsAround(grants, path, disk, platform), + readGrantsAround(paths, hidden, disk, platform), + ); // What the owner adds is written around the state directory as well, never in it. - let added = readGrantsAround(options.settings?.writable ?? [], hidden, disk, platform).filter( + let added = around(options.settings?.writable ?? []).filter( path => !theirs.some(kept => inside(real(path), kept, platform)), ); let reopened = theirs.filter(kept => @@ -547,14 +555,14 @@ export function launchPolicy( // MXC takes a path granted both ways as read-only, so a grant that is also writable is dropped. let written = new Set(writable.map(real)); let readable = [ - ...readGrantsAround(roots, hidden, disk, platform), + ...around(roots), ...(launch.own ?? []).filter(exists), ...person ? named(person) : [], // The repository's git directory is read, whole, where only some roots are read (Windows). ...repository ? named(repository.commonDir) : [], ].filter(path => !written.has(real(path))); let readonlyInside = [...git?.readonlyInside ?? [], ...reopened]; - let denied = secretPaths(platform, home).filter(path => exists(path)); + let denied = [...secretPaths(platform, home), ...concealed].filter(path => exists(path)); let env: Record = { ...launch.env }; let pathKey = Object.keys(env).find(key => key.toUpperCase() === "PATH") ?? "PATH"; env[pathKey] = sandboxPath(env[pathKey], platform, real); @@ -574,6 +582,54 @@ export function launchPolicy( }; } +/** The variable the launcher finds the launch's environment file by. */ +const ENVIRONMENT_FILE = "CHOPIN_LAUNCH_ENVIRONMENT"; +/** What a shell can export: other names stay on the command line, and none of ours is one. */ +const SHELL_NAME = /^[A-Za-z_][A-Za-z0-9_]*$/; +/** The proxy MXC points a launch at, which a variable of the person's must not replace. */ +const PROXY = /^(https?|ftp|all)_proxy$/i; + +/** + * The shell that starts every sandboxed launch on Linux and macOS: it exports the environment + * written to the file `CHOPIN_LAUNCH_ENVIRONMENT` names, deletes the file, then becomes the + * launch's own command. The file is in the launch's private temporary directory (mode 0600). + */ +export const LAUNCHER = `f=$${ENVIRONMENT_FILE}; unset ${ENVIRONMENT_FILE}; ` + + `set -a; . "$f" || exit 127; set +a; rm -f "$f"; exec "$@"`; + +/** A value as a POSIX shell reads it, quoted whole. */ +function shellQuote(value: string) { + return `'${value.replaceAll("'", "'\\''")}'`; +} + +/** + * `policy` with its environment handed over in a file instead of on a command line. MXC starts a + * process with each variable on the sandbox tool's command line (`bwrap --setenv NAME VALUE`), + * which every process on the machine can read in `/proc//cmdline` or `ps`; a launch's + * environment holds its credentials, a Planner's and a GitHub token among them, and the person's + * `--env` values. So on Linux and macOS the command line carries only the file's path, and + * `LAUNCHER` exports what is in the file before the command starts: the launch has every variable + * as before, readable by the person's own processes alone. The proxy MXC adds still wins. On + * Windows the policy is as it was: ProcessContainer passes the environment as an environment. + */ +export function sealedEnvironment(policy: Policy, folder: string): Policy { + if (policy.platform === "win32") return policy; + let file = join(folder, ".chopin-launch-environment"); + let lines: string[] = []; + let shown: Record = {}; + for (let [name, value] of Object.entries(policy.env)) { + if (value === undefined || PROXY.test(name)) continue; + if (SHELL_NAME.test(name)) lines.push(`${name}=${shellQuote(value)}`); + else shown[name] = value; + } + writeFileSync(file, lines.join("\n") + "\n", { mode: 0o600 }); + return { + ...policy, + command: ["/bin/sh", "-c", LAUNCHER, "chopin-launch", ...policy.command], + env: { ...shown, PATH: policy.env.PATH, [ENVIRONMENT_FILE]: file }, + }; +} + function spawnConfined( executor: string, engine: SandboxEngine, @@ -594,7 +650,9 @@ function spawnConfined( if (removed.length) options.planted?.(removed); }; cleared(); - let request = mxcRequest(launchPolicy(engine, host, launch, options, proxy.url, temporary)); + let request = mxcRequest( + sealedEnvironment(launchPolicy(engine, host, launch, options, proxy.url, temporary), temporary), + ); writeFileSync(file, JSON.stringify(request), { mode: 0o600 }); let child = spawn(executor, ["--config", file], { stdio: [launch.stdin, "pipe", "pipe"], diff --git a/apps/connector/src/status.test.ts b/apps/connector/src/status.test.ts index 5b69f0dfe..d889ab00c 100644 --- a/apps/connector/src/status.test.ts +++ b/apps/connector/src/status.test.ts @@ -134,6 +134,7 @@ test("status reports a paired machine Chopin accepts, its connectors, a stale lo plannerWorktrees: [], checkouts: {}, clones: [], + github: {}, }); expect(JSON.stringify(report)).not.toContain(TOKEN); let text = formatStatus(report); @@ -265,27 +266,71 @@ test("status shows an agent that failed to start under its connector", async () ); }); -test("status says once a connector's machine uses its own GitHub login because the App can't push", async () => { - let { directory, host } = await machine(); - let lock = "f".repeat(64); - await writeFile( - join(directory, `${lock}.lock`), - JSON.stringify({ - pid: process.pid, - processStart: (await processStart()(process.pid))!.id, - root: "/work/score", - }), - ); - await writeFile( - join(directory, `${lock}.github.json`), - JSON.stringify({ repository: "octo-org/score" }), - ); - let report = await collectStatus({ directory, server: "https://x", source: "config", host }); - expect(report.connectors[0]?.githubCannotPush).toEqual(["octo-org/score"]); - expect(formatStatus(report)).toContain( - " The GitHub App can't push to octo-org/score (it needs Contents and Pull requests " - + "write), so this machine uses its own GitHub login for it.", - ); +test("status asks Chopin, without a token being made, whether each registered checkout's repository can have one, and says why not", async () => { + let asked: unknown[] = []; + let server = Bun.serve({ + hostname: "127.0.0.1", + port: 0, + async fetch(request) { + let { pathname } = new URL(request.url); + if (pathname === "/api/connector/machine") return Response.json({ login: "ada" }); + if (pathname !== "/api/connector/github-token") return new Response(null, { status: 404 }); + let body = await request.json() as { repository: string }; + asked.push(body); + if (body.repository === "octo-org/score") return Response.json({ ok: true }); + if (body.repository === "octo-org/old") { + // A Chopin from before the check refuses what it doesn't know. + return Response.json({ error: "invalid-request" }, { status: 400 }); + } + return Response.json({ error: "app-cannot-push" }, { status: 403 }); + }, + }); + try { + let url = `http://127.0.0.1:${server.port}`; + let { directory, host } = await machine(url); + await writeFile( + join(directory, "config.json"), + JSON.stringify({ + connect: { + server: url, + command: ["agent"], + planners: [], + plannerCommand: [], + checkouts: { + "octo-org/score": "/work/score", + "octo-org/site": "/work/site", + "octo-org/old": "/work/old", + }, + }, + }), + ); + let report = await collectStatus({ directory, server: url, source: "config", host }); + expect(asked).toEqual(expect.arrayContaining([ + { repository: "octo-org/score", purpose: "check" }, + { repository: "octo-org/site", purpose: "check" }, + ])); + expect(report.github).toEqual({ + "octo-org/score": "ok", + "octo-org/site": "app-cannot-push", + "octo-org/old": "unknown", + }); + let text = formatStatus(report).join("\n"); + expect(text).toContain( + "octo-org/site at /work/site; Chopin's GitHub App can't write to octo-org/site", + ); + expect(text).not.toContain("own GitHub login for it"); + // Unpaired, nothing is asked. + let unpaired = await machine(); + let none = await collectStatus({ + directory: unpaired.directory, + server: url, + source: "config", + host: unpaired.host, + }); + expect(none.github).toEqual({}); + } finally { + server.stop(true); + } }); test("status names a built-in agent by its --agent id, with the command it runs", async () => { diff --git a/apps/connector/src/status.ts b/apps/connector/src/status.ts index cc2f06987..25a82714d 100644 --- a/apps/connector/src/status.ts +++ b/apps/connector/src/status.ts @@ -1,10 +1,10 @@ import type { SandboxReport } from "@chopin/protocol/connector"; import { sandboxLabel } from "@chopin/protocol/connector"; import { type ConnectSettings, readConfig } from "./config"; -import { machineLabel, machineToken, SELF } from "./machine"; +import { machineGitHubCheck, machineLabel, machineToken, SELF } from "./machine"; import { CONNECTOR_HEADERS, jsonObject, refusal } from "./mcp"; import { logPath, type ServiceHost, type ServiceStatus, serviceStatus } from "./service"; -import { appCannotPush } from "./github-token"; +import { githubRefusal } from "./github-token"; import { CONNECTOR_PROTOCOL, VERSION } from "./version"; import { join } from "node:path"; import { clones } from "./checkouts"; @@ -83,11 +83,6 @@ export type StatusReport = { failures?: Array< Omit & { at: string; retryAt?: string } >; - /** - * The repositories Chopin's GitHub App can't push to, so the machine's Planner commands and - * runs use its own GitHub login there. - */ - githubCannotPush?: string[]; }>; service: ServiceStatus | { state: "unknown"; detail: string }; /** Whether agents run in the system's process sandbox, and if not, why and the one fix. */ @@ -112,6 +107,13 @@ export type StatusReport = { checkouts: Record; /** The clones made on demand in `/repos//` for every other repository. */ clones: Array<{ repository: string; path: string }>; + /** + * Whether Chopin would give this machine a GitHub token for each registered checkout's + * repository, asked without one being made: `ok`, why not (`repository-forbidden`, + * `app-cannot-push`, `no-grant`, …), or `unknown` when Chopin couldn't be asked. Nothing that + * Chopin refuses runs there: the machine never uses its own GitHub login. + */ + github: Record; }; const CHECK_MS = 3000; @@ -188,6 +190,8 @@ export async function collectStatus(options: { let found = await runs(directory, processStart(host.platform, host.run)); let kept = found.filter(run => !run.active && run.worktree); let bytes = await diskUsage(kept.map(run => join(directory, "runs", run.attempt))); + let checkouts = held.find(item => item.running && item.checkouts)?.checkouts + ?? registered(config.service?.settings ?? config.connect); return { version: VERSION, protocol: CONNECTOR_PROTOCOL, @@ -204,7 +208,6 @@ export async function collectStatus(options: { ...(item.planners ? { planners: item.planners } : {}), ...(item.plannerCommand ? { canRunPlanners: true as const } : {}), ...(item.chosen ? { chosenPlanners: item.chosen } : {}), - ...(item.githubCannotPush ? { githubCannotPush: item.githubCannotPush } : {}), ...(item.failures ? { failures: item.failures.map(({ at, retryAt, ...failure }) => ({ @@ -227,9 +230,25 @@ export async function collectStatus(options: { ...(bytes === undefined ? {} : { bytes }), }, plannerWorktrees: await plannerWorktrees(directory), - checkouts: held.find(item => item.running && item.checkouts)?.checkouts - ?? registered(config.service?.settings ?? config.connect), + checkouts, clones: await clones(directory), + github: Object.fromEntries( + await Promise.all( + Object.keys(checkouts).filter(Boolean).map(async repository => + [ + repository, + token && machine.accepted + ? await machineGitHubCheck( + server, + token, + repository, + AbortSignal.timeout(options.timeoutMs ?? CHECK_MS), + ) + : "unknown", + ] as const + ), + ), + ), }; } @@ -321,7 +340,6 @@ export function formatStatus(report: StatusReport, now = Date.now()) { item.version ? `, chopin ${item.version}` : "" }${item.service ? ", the service" : ""})`, ...(agent ? [` Agent${failing(agent, now)}`] : []), - ...(item.githubCannotPush ?? []).map(repository => ` ${appCannotPush(repository)}`), ...(item.canRunPlanners ? [" Can run Planners for documents that choose this machine"] : []), @@ -351,9 +369,15 @@ export function formatStatus(report: StatusReport, now = Date.now()) { let worktrees = report.plannerWorktrees.map(item => `${item.path} (${item.repository}, document ${item.document})` ); - let checkouts = Object.entries(report.checkouts).map(([repository, path]) => - repository ? `${repository} at ${path}` : path - ); + let checkouts = Object.entries(report.checkouts).map(([repository, path]) => { + let github = report.github?.[repository]; + let refused = github && github !== "ok" && github !== "unknown" + ? `; ${ + githubRefusal(github, repository).replace(", so nothing ran", ", so nothing runs there") + }` + : ""; + return `${repository ? `${repository} at ${path}` : path}${refused}`; + }); let cloned = report.clones.map(item => `${item.repository} at ${item.path}`); return [ `chopin ${report.version} (connector protocol ${report.protocol})`, diff --git a/apps/connector/src/testing/credential-planner.ts b/apps/connector/src/testing/credential-planner.ts new file mode 100644 index 000000000..4688d542b --- /dev/null +++ b/apps/connector/src/testing/credential-planner.ts @@ -0,0 +1,13 @@ +import { writeFileSync } from "node:fs"; + +/** + * A Planner backend that writes what it was given, by name, in its own worktree, and then waits to + * be stopped, so a test can look at every process's command line while it runs. + */ +let names = (process.env.CHOPIN_TEST_HELD_NAMES ?? "").split(",").filter(Boolean); +writeFileSync( + `${process.env.CHOPIN_WORKTREE}/held.json`, + JSON.stringify(Object.fromEntries(names.map(name => [name, process.env[name] ?? null]))), +); +process.on("SIGTERM", () => process.exit(0)); +setInterval(() => {}, 1000); diff --git a/apps/connector/src/testing/fake-implementer.ts b/apps/connector/src/testing/fake-implementer.ts index d73af1ab1..4431094d2 100644 --- a/apps/connector/src/testing/fake-implementer.ts +++ b/apps/connector/src/testing/fake-implementer.ts @@ -1,6 +1,7 @@ import { AgentApp, ndJsonStream, PROTOCOL_VERSION } from "@agentclientprotocol/sdk"; import type { McpServer } from "@agentclientprotocol/sdk"; import { spawn, spawnSync } from "node:child_process"; +import { readFileSync } from "node:fs"; import { access, writeFile } from "node:fs/promises"; import { Readable, Writable } from "node:stream"; import { Client } from "@modelcontextprotocol/sdk/client/index.js"; @@ -48,6 +49,19 @@ let app = new AgentApp() JSON.stringify({ prompt, worktrees: process.env.CHOPIN_RUN_WORKTREES ?? null }), ); } + if (process.argv.includes("--hold")) { + // What it was given, by name, written where its run may write; then it holds its turn + // open until the test, which looks at every process's command line meanwhile, lets it go. + let names = (process.env.CHOPIN_TEST_HELD_NAMES ?? "").split(",").filter(Boolean); + await writeFile( + `${cwd}/held.json`, + JSON.stringify(Object.fromEntries(names.map(name => [name, process.env[name] ?? null]))), + ); + while (!(await access(`${cwd}/release`).then(() => true, () => false))) { + await new Promise(resolve => setTimeout(resolve, 50)); + } + return { stopReason: "end_turn" as const }; + } if (process.argv.includes("--github")) { // What a build's agent pushes and opens pull requests with: gh's token, and Git's // credentials for github.com, asked as `git push` would. @@ -59,11 +73,39 @@ let app = new AgentApp() cwd, encoding: "utf8", }); + // A commit as the agent makes one, and who it says made it, signed or not. + let commit = spawnSync( + "git", + ["-C", cwd, "commit", "-q", "--allow-empty", "-m", "Task one"], + { + encoding: "utf8", + }, + ); + let made = spawnSync("git", ["-C", cwd, "log", "-1", "--format=%an <%ae>|%cn <%ce>|%G?"], { + encoding: "utf8", + }); + let read = (path: string | undefined) => { + try { + return path ? readFileSync(path, "utf8").trim() : null; + } catch { + return null; + } + }; await writeFile( process.env.CHOPIN_TEST_GITHUB!, JSON.stringify({ GH_TOKEN: process.env.GH_TOKEN ?? null, GITHUB_TOKEN: process.env.GITHUB_TOKEN ?? null, + SSH_AUTH_SOCK: process.env.SSH_AUTH_SOCK ?? null, + GIT_SSH_COMMAND: process.env.GIT_SSH_COMMAND ?? null, + starter: [ + process.env.CHOPIN_STARTER_LOGIN, + process.env.CHOPIN_STARTER_NAME, + process.env.CHOPIN_STARTER_EMAIL, + ], + githubFile: read(process.env.CHOPIN_GITHUB_TOKEN_FILE), + mcpFile: read(process.env.CHOPIN_MCP_TOKEN_FILE), + commit: commit.status === 0 ? made.stdout.trim() : commit.stderr.trim(), credential: fill.status === 0 ? fill.stdout : null, gh: gh.status, // Where `git fetch` and `git push` would go for origin, under this environment. diff --git a/apps/connector/src/testing/github-grant.ts b/apps/connector/src/testing/github-grant.ts new file mode 100644 index 000000000..c41ef40a3 --- /dev/null +++ b/apps/connector/src/testing/github-grant.ts @@ -0,0 +1,84 @@ +import { mkdir, rm, writeFile } from "node:fs/promises"; +import { join } from "node:path"; +import type { GitHubToken } from "../github-token"; +import type { GitHubAccount } from "../machine"; + +/** The App bot a test's GitHub token commits as. */ +export const BOT: GitHubAccount = { + login: "chopin-test[bot]", + id: 1001, + name: "chopin-test[bot]", + email: "1001+chopin-test[bot]@users.noreply.github.com", +}; + +/** The person a test's run is for. */ +export const STARTER: GitHubAccount = { + login: "mona", + id: 7, + name: "Mona Lisa", + email: "7+mona@users.noreply.github.com", +}; + +/** + * A GitHub token as Chopin gives one, without asking a server: written to `folder` as `githubToken` + * writes it, the token file and gh's configuration, and gone again on `stop`. + */ +export async function testGitHubToken( + folder: string, + token = `ghs_${"T".repeat(36)}`, +): Promise { + let file = join(folder, "github"); + let config = join(folder, "gh"); + await mkdir(config, { recursive: true, mode: 0o700 }); + await writeFile(file, `${token}\n`, { mode: 0o600 }); + await writeFile( + join(config, "hosts.yml"), + `github.com:\n oauth_token: "${token}"\n user: "${BOT.login}"\n git_protocol: https\n`, + { mode: 0o600 }, + ); + return { + token, + expiresAt: Date.now() + 3_600_000, + file, + config, + identity: BOT, + starter: STARTER, + expired: new AbortController().signal, + secrets: () => [token], + async stop() { + await rm(file, { force: true }); + await rm(config, { recursive: true, force: true }); + }, + }; +} + +/** + * What a Chopin with its GitHub App answers to a connector's asking for a launch's GitHub token or + * its token for Chopin's tools; undefined for any other request. + */ +export async function answerTokens( + request: Request, + /** The refusal each named repository gets instead. */ + refusing: Record = {}, +): Promise { + let { pathname } = new URL(request.url); + if (pathname === "/api/connector/github-token") { + let { repository } = await request.json() as { repository: string }; + let refused = refusing[repository]; + if (refused) return Response.json({ error: refused }, { status: 403 }); + return Response.json({ + token: `ghs_${"G".repeat(36)}`, + expiresAt: Date.now() + 3_600_000, + kind: "installation", + identity: BOT, + starter: STARTER, + }); + } + if (pathname === "/api/connector/mcp-token") { + return Response.json({ + token: `chopin_mcp_${"T".repeat(43)}`, + expiresAt: Date.now() + 900_000, + }); + } + return undefined; +} diff --git a/apps/connector/src/version.ts b/apps/connector/src/version.ts index 84bd6a5cd..98b294073 100644 --- a/apps/connector/src/version.ts +++ b/apps/connector/src/version.ts @@ -4,9 +4,12 @@ declare const CHOPIN_CLI_VERSION: string | undefined; * The connector protocol this CLI speaks, for a server to check compatibility against. Protocol 4 * polls for the Planners its owner chose this machine for. Protocol 5 connects the machine, not * one checkout: it takes work in any repository its owner can push to. Protocol 6 keeps the claimed - * document ID on build renewals and reports after assignment cleanup. + * document ID on build renewals and reports after assignment cleanup. Protocol 7 asks for each + * launch's GitHub token with what it is for (`purpose`, `run`, `document`), gives the launch only + * that token and the identity Chopin names, and starts nothing Chopin refuses one: the machine's own + * GitHub login is never used. */ -export const CONNECTOR_PROTOCOL = 6; +export const CONNECTOR_PROTOCOL = 7; /** Embedded by the binary build with `--define`; a source or `/connector.js` run has none. */ export const VERSION = typeof CHOPIN_CLI_VERSION === "string" ? CHOPIN_CLI_VERSION : "dev"; diff --git a/apps/connector/src/workspace.ts b/apps/connector/src/workspace.ts index 5304c6abb..5c34ccaf2 100644 --- a/apps/connector/src/workspace.ts +++ b/apps/connector/src/workspace.ts @@ -392,29 +392,6 @@ export function chosenPlanners(directory: string, root: string) { export type ChosenPlanners = ReturnType; -/** - * The repositories Chopin said its GitHub App can't push to, so the machine's processes use its - * own GitHub login for them, kept for `chopin status` in a file beside the connector's lock. - * `record` is true only the first time for each, so the connector says it once. - */ -export function githubFallback(directory: string, root: string) { - let path = join(directory, `${lockId(root)}.github.json`); - // A connector that crashed may have left its note behind. - let writing = rm(path, { force: true }).catch(() => {}); - let recorded: string[] = []; - return { - record(repository: string) { - if (recorded.some(value => value.toLowerCase() === repository.toLowerCase())) return false; - recorded.push(repository); - let repositories = [...recorded]; - writing = writing.then(() => - writeFile(path, JSON.stringify({ repositories }), { mode: 0o600 }) - ).catch(() => {}); - return true; - }, - }; -} - /** Whether no process `pid` exists; EPERM means it exists but belongs to someone else. */ function gone(pid: number) { try { @@ -479,8 +456,6 @@ export type RunningConnector = { started?: number; /** How its agent or Planner backends last failed to start, while it runs. */ failures?: StartFailure[]; - /** The repositories its GitHub App can't push to, so its machine uses its own login there. */ - githubCannotPush?: string[]; }; /** @@ -520,17 +495,6 @@ export async function connectors( .then(text => JSON.parse(text)) .catch(() => undefined) : undefined; - let github: unknown = running - ? await readFile(join(directory, name.replace(/\.lock$/, ".github.json")), "utf8") - .then(text => { - let value = JSON.parse(text) as { repository?: unknown; repositories?: unknown }; - return Array.isArray(value.repositories) ? value.repositories : [value.repository]; - }) - .catch(() => undefined) - : undefined; - let refused = Array.isArray(github) - ? github.filter((value): value is string => typeof value === "string") - : []; found.push({ pid: pid as number, ...(typeof root === "string" ? { root } : {}), @@ -550,7 +514,6 @@ export async function connectors( ...(typeof protocol === "number" ? { protocol } : {}), ...(typeof started === "number" ? { started } : {}), ...(Array.isArray(failures) && failures.length ? { failures } : {}), - ...(refused.length ? { githubCannotPush: refused } : {}), }); } return found; @@ -561,12 +524,18 @@ export async function connectors( * tree fails: each remote URL is rewritten to a scheme Git cannot push to. A spike runs under it, * so its throwaway branch never leaves the laptop even if the agent tries. */ -export function noPushEnvironment(): Record { +export function noPushEnvironment(more: Array<[string, string]> = []): Record { let prefixes = ["https://", "http://", "ssh://", "git@", "git://", "file://", "/", "."]; - let env: Record = { GIT_CONFIG_COUNT: String(prefixes.length) }; - prefixes.forEach((prefix, index) => { - env[`GIT_CONFIG_KEY_${index}`] = "url.chopin-spike-never-pushes://.pushInsteadOf"; - env[`GIT_CONFIG_VALUE_${index}`] = prefix; + let entries: Array<[string, string]> = [ + ...prefixes.map(prefix => + ["url.chopin-spike-never-pushes://.pushInsteadOf", prefix] as [string, string] + ), + ...more, + ]; + let env: Record = { GIT_CONFIG_COUNT: String(entries.length) }; + entries.forEach(([key, value], index) => { + env[`GIT_CONFIG_KEY_${index}`] = key; + env[`GIT_CONFIG_VALUE_${index}`] = value; }); return env; } diff --git a/docs/agent-connection.md b/docs/agent-connection.md index 5b907f325..e33346ea4 100644 --- a/docs/agent-connection.md +++ b/docs/agent-connection.md @@ -1496,17 +1496,18 @@ long to mask is left out, with a note in its place. Each launch gets the connector's environment, your `--env` values, and these: -| Variable | Value | -| -------------------------------- | ---------------------------------------------------------------------------------------- | -| `CHOPIN_URL` | The server's origin. | -| `CHOPIN_DOCUMENT` | The document as given to `--planner`, or its URL when Chopin chose this machine. | -| `CHOPIN_PLANNER_TOKEN` | This launch's [credential](#credential). | -| `CHOPIN_PLANNER_TOKEN_FILE` | A file (mode `0600`) that always holds a current credential, replaced before it expires. | -| `CHOPIN_CHECKOUT` | The absolute root of the document's repository's checkout or clone. | -| `CHOPIN_WORKTREE` | The command's own worktree path, `/planners///`. | -| `CHOPIN_GITHUB_TOKEN` | The machine owner's token from Chopin's GitHub App, for pushing and draft pull requests. | -| `CHOPIN_GITHUB_TOKEN_EXPIRES_AT` | When that token expires, in ISO 8601. | -| `CHOPIN_GITHUB_TOKEN_FILE` | A file (mode `0600`) that always holds the current token. | +| Variable | Value | +| -------------------------------- | -------------------------------------------------------------------------------------------------- | +| `CHOPIN_URL` | The server's origin. | +| `CHOPIN_DOCUMENT` | The document as given to `--planner`, or its URL when Chopin chose this machine. | +| `CHOPIN_PLANNER_TOKEN` | This launch's [credential](#credential). | +| `CHOPIN_PLANNER_TOKEN_FILE` | A file (mode `0600`) that always holds a current credential, replaced before it expires. | +| `CHOPIN_CHECKOUT` | The absolute root of the document's repository's checkout or clone. | +| `CHOPIN_WORKTREE` | The command's own worktree path, `/planners///`. | +| `CHOPIN_GITHUB_TOKEN` | A token of Chopin's GitHub App for the document's repository, for pushing and draft pull requests. | +| `CHOPIN_GITHUB_TOKEN_EXPIRES_AT` | When that token expires, in ISO 8601. | +| `CHOPIN_GITHUB_TOKEN_FILE` | A file (mode `0600`) that always holds the current token. | +| `CHOPIN_MCP_TOKEN_FILE` | A file (mode `0600`) holding the token for Chopin's tools at `/mcp`, renewed with the credential. | `CHOPIN_WORKTREE` stays the same across launches. The connector makes its parent folders but never removes the worktree itself. Whether the backend or the @@ -1544,8 +1545,11 @@ launch's credential, never reaches the connector's output. A backend that outliv link, as a `resumable-sessions` one does, reads it again before reattaching. The connector renews the GitHub token in the file 25 minutes before it expires, -so read the file before each use. The three `CHOPIN_GITHUB_TOKEN` variables are -absent when Chopin gives no token; see +so read the file before each use. It acts as the App's bot, so `gh api user` +fails with it; who the work is for is in `CHOPIN_STARTER_LOGIN`. Use the GitHub +token for Git and gh only, and `CHOPIN_MCP_TOKEN_FILE` for Chopin's tools. When +Chopin refuses a token the command doesn't start. The rest of what the launch +gets for GitHub, and what it doesn't inherit, is in [Which GitHub account pushes](chopin-cli.md#which-github-account-pushes). ### Credential diff --git a/docs/chopin-cli.md b/docs/chopin-cli.md index 728224c7e..c0ef0463d 100644 --- a/docs/chopin-cli.md +++ b/docs/chopin-cli.md @@ -714,7 +714,7 @@ instead. | Writes | Only the worktree the connector made for it (a run's, or a Planner's own; never your checkout), the repository's git directory in the few places commits and refs go and that worktree's entry (see [Git in the sandbox](#git-in-the-sandbox)), the agent's own state (`~/.atomic` or `~/.copilot` for the built-in agents), a private temporary directory, and what you add with `--sandbox-write`. Nothing else. | | Reads | The rest of the disk on Linux and macOS, except `~/.ssh`, `~/.aws`, `~/.azure`, `~/.kube`, `~/.docker`, `~/.gnupg`, `~/.config/gh`, `~/.config/gcloud`, `.netrc`, `.git-credentials` and browser profiles, which read as empty or unreadable, and the connector's state directory (`--base-dir`), of which it sees only its own worktree and the folder of its own tokens. The machine token and every other run's and Planner's files, tokens included, are out of its sight, those made after it started too; so is the state directory when a working directory holds it, as a home does. **On Windows a ProcessContainer reads only what it is given, so it is not the rest of the disk:** your profile folder (the same names are unreadable), the directories on `PATH` and those that `PYTHONHOME`, `GOROOT`, `GOPATH`, `CARGO_HOME`, `RUSTUP_HOME`, `JAVA_HOME`, `DOTNET_ROOT`, `NODE_PATH`, `VIRTUAL_ENV`, `CONDA_PREFIX` and the other tool variables in MXC's own baseline name, the programs installed per user in `%LOCALAPPDATA%\Programs`, its working directory and its worktree, the repository's git directory whole (its configuration, hooks, objects and every worktree's entry, since a worktree's `.git` leads into it), and what Windows lets every container read (`C:\Windows`, `C:\Program Files`). A folder on another drive, in another user's profile or anywhere else, a checkout among them, is not readable, but for the checkout a Planner runs in, and for a run's checkout's git directory, which is read without that checkout's files. | | Network | Only through a proxy on your machine that allows the hosts below and refuses the rest. A direct connection fails, IPv6 is blocked, and the proxy resolves names. | -| Environment | Everything it has today, including `GH_TOKEN` and `--env` values. `TMPDIR` points at the private directory, `PATH` names each directory by its real path, and `NODE_USE_ENV_PROXY=1` makes Node's own `fetch` use the proxy; the proxy variables come from MXC. | +| Environment | Everything it has today but GitHub credentials (see [Which GitHub account pushes](#which-github-account-pushes)), and `--env` values. `TMPDIR` points at the private directory, `PATH` names each directory by its real path, and `NODE_USE_ENV_PROXY=1` makes Node's own `fetch` use the proxy; the proxy variables come from MXC. On Linux and macOS it reaches the sandbox in a `0600` file that a shell exports and deletes before the agent starts, so no value is on any process's command line. | Hosts an agent may reach (the full list is `HOST_GROUPS` in [`apps/connector/src/sandbox-policy.ts`](../apps/connector/src/sandbox-policy.ts)): @@ -782,6 +782,7 @@ corporate registry is. | `--no-sandbox` | Never sandboxes. `chopin status` says it is off. | | `--sandbox-host ` | Allows an agent to reach one more host. Repeat it. | | `--sandbox-write ` | Allows an agent to write one more directory, such as a package cache or a local remote, but never your checkout (see below). Repeat it. | +| `--sandbox-hide ` | Hides one more file or folder from every agent, even inside a `--sandbox-write` directory, such as a tool's own sign-in. Repeat it. | ### Git in the sandbox @@ -1100,40 +1101,49 @@ nothing. ## Which GitHub account pushes -Builds and Planner commands push branches and open draft pull requests as you, -with your token from Chopin's GitHub App. Investigations get the token too; -prototypes never do. - -| What runs | Gets your token as | -| ---------------------------------- | ---------------------------------------------------------------------------------------- | -| A build's or investigation's agent | `GH_TOKEN` and `GITHUB_TOKEN`. Git uses it for github.com. | -| A Planner command | `CHOPIN_GITHUB_TOKEN`; see [the Planner's environment](agent-connection.md#environment). | - -The machine uses your own `gh` and Git login instead when: - -- the server runs `AUTH_MODE=local`; -- you can't push to the repository; or -- the App can only read the repository. The connector and `chopin status` then - say so: +Chopin's GitHub App does, never your own login. Before each Planner command, +build or investigation starts, the connector asks Chopin for a token of the App +for that one repository. Chopin checks that the person it is for can push there: +whoever started the run, or you for a Planner on your machine. Commits are made +as the App's bot (`[bot]`), and a build's pull request names and credits +the person who asked. A prototype's token only fetches its commit and is gone +before its agent starts. Setting the App up is in +[Chopin's GitHub App identity](github-app-identity.md#set-it-up). + +| Variable | What it is | +| --------------------------------------------------------------------- | ----------------------------------------------------------------------------------------- | +| `GH_CONFIG_DIR` | A gh configuration of the launch's own, holding the current token. gh never reads yours. | +| `CHOPIN_GITHUB_TOKEN_FILE` | A file (mode `0600`) holding the current token, renewed before it expires. | +| `CHOPIN_MCP_TOKEN_FILE` | A file holding a Chopin token for Chopin's own tools at `/mcp`. It is not a GitHub token. | +| `GIT_AUTHOR_NAME`, `GIT_AUTHOR_EMAIL` and the committer's | The App's bot. Your Git identity and signing settings are not used. | +| `CHOPIN_STARTER_LOGIN`, `CHOPIN_STARTER_NAME`, `CHOPIN_STARTER_EMAIL` | Who the work is for, for "Requested by" and a `Co-authored-by` trailer. | + +Nothing a launch inherits reaches GitHub another way: `GH_TOKEN`, +`GITHUB_TOKEN`, other `GH_*` variables, `GIT_ASKPASS`, `SSH_AUTH_SOCK`, +`GIT_SSH*` and `GIT_CONFIG_*` are removed from its environment and from your +`--env` values, and Git never uses SSH (`GIT_SSH_COMMAND=false`). + +When Chopin refuses a token, nothing starts. The connector says why, naming the +repository, and the run or Planner fails with that reason; see +[Refusals](github-app-identity.md#refusals). `chopin status` asks Chopin, without +a token being made, about each registered checkout's repository: ```text -The GitHub App can't push to octo-org/score (it needs Contents and Pull requests write), so this machine uses its own GitHub login for it. +Checkouts: octo-org/score at /work/score; Chopin's GitHub App can't write to octo-org/score; it needs Contents and Pull requests write, which an admin of the repository must grant. This machine won't use its own GitHub login, so nothing runs there. ``` -### Your Git configuration +`chopin status --json` has the same as `github`: `"ok"`, the refusal's code, +or `"unknown"` when Chopin couldn't be asked. -Where Chopin gives the machine a token, the connector's Git commands for that -repository, and those of the Planner and agent it starts, use the repository's -`https://github.com/` address and the token, whatever your own Git -configuration says. A `url..insteadOf` or `pushInsteadOf` rule that sends -GitHub to SSH, which a background service has no agent or key for, doesn't -reach them, and a checkout whose `origin` is an SSH address is fetched and -pushed over https. The rest of your configuration (identity, signing, other -rules and repositories) applies as it is, and nothing is written to your global -configuration or the checkout's own. +### Your Git configuration -If a Git command still reaches GitHub over SSH and fails, the error names the -rule that rewrites the address. +The connector's Git commands for a repository, and those of the Planner and +agent it starts, use the repository's `https://github.com/` address and Chopin's +token, whatever your Git configuration says. A `url..insteadOf` or +`pushInsteadOf` rule that sends GitHub to SSH doesn't reach them, a checkout +whose `origin` is an SSH address is fetched and pushed over https, and your +credential helpers are not run. Nothing is written to your global configuration +or the checkout's own. ## Update @@ -1173,6 +1183,7 @@ pairs again. To remove the service as well, run `chopin service uninstall`. | `chopin connect … --no-sandbox` | Never sandboxes agents and Planner commands. | | `chopin connect … --sandbox-host ` | Lets agents reach one more host through the sandbox. Repeat it. | | `chopin connect … --sandbox-write ` | Lets agents write one more directory inside the sandbox. Repeat it. | +| `chopin connect … --sandbox-hide ` | Hides one more file or folder from agents in the sandbox. Repeat it. | | `chopin pair [--label ] [--key ]` | Pairs this machine, or pairs it again. The label defaults to the hostname. | | `chopin status [--json]` | Shows the server, pairing, the connector, the service and what it runs, the sandbox, your checkouts and clones, and the log. | | `chopin service install [--allow-downgrade] [--skip-check]` | Copies what `chopin connect` last ran and runs it as a background service. | diff --git a/e2e/build-restart.e2e.ts b/e2e/build-restart.e2e.ts index 84a0b9bfc..35636f4b9 100644 --- a/e2e/build-restart.e2e.ts +++ b/e2e/build-restart.e2e.ts @@ -13,6 +13,7 @@ import { SQL } from "bun"; import { createChannel, registerTestDatabase, seedChannel, testChannelPath } from "./database"; import { authenticate, expect, test } from "./room"; import { e2eDatabaseName, e2ePort } from "./ports"; +import { GITHUB_APP_ENV } from "./github-app"; import { HOST, ROOT } from "./servers"; import type { Graph } from "../apps/server/src/tasks/graphs"; @@ -52,6 +53,7 @@ const ENV = { GITHUB_APP_SLUG: "chopin-e2e", GITHUB_APP_CLIENT_ID: "e2e", GITHUB_APP_CLIENT_SECRET: "e2e", + ...GITHUB_APP_ENV, GITHUB_ALLOWED_USERS: "", GITHUB_ALLOWED_ORGANIZATIONS: "githubnext", SESSION_ENCRYPTION_KEY: process.env.SESSION_ENCRYPTION_KEY!, diff --git a/e2e/github-app.ts b/e2e/github-app.ts new file mode 100644 index 000000000..cd19a6d1f --- /dev/null +++ b/e2e/github-app.ts @@ -0,0 +1,14 @@ +/** + * The e2e GitHub App's id and key, which a server under test mints a paired machine's GitHub + * tokens with; e2e/github.ts plays GitHub for it and checks only the id. The key is made for each + * run, as nothing outside it ever checks the signature. + */ +import { generateKeyPairSync } from "node:crypto"; + +export const GITHUB_APP_ENV = { + GITHUB_APP_ID: "4242", + GITHUB_APP_PRIVATE_KEY: generateKeyPairSync("rsa", { modulusLength: 2048 }).privateKey.export({ + type: "pkcs8", + format: "pem", + }).toString(), +}; diff --git a/e2e/github.ts b/e2e/github.ts index 7cd3051c3..2b78d806f 100644 --- a/e2e/github.ts +++ b/e2e/github.ts @@ -229,6 +229,8 @@ let fake = async (input: Parameters[0], init?: Parameters[0], init?: Parameters sum * 31 + character.charCodeAt(0) | 0, 7) >>> 0, + node_id: `U_${login}`, + name: null, + }); + } + let permission = /^\/repos\/octo-org\/([\w.-]+)\/collaborators\/([^/]+)\/permission$/.exec( + url.pathname, + ); + if (permission) { + let login = decodeURIComponent(permission[2]!); + let push = !login.startsWith(SCORE_READER) && !login.startsWith(SCORE_OUTSIDER) + && login !== "readonly"; + return json({ permission: push ? "write" : "read", user: { login, permissions: { push } } }); + } + return json({ message: "Not Found" }, { status: 404 }); +} diff --git a/e2e/implementation-launcher.e2e.ts b/e2e/implementation-launcher.e2e.ts index 36410f473..1d8a61e5a 100644 --- a/e2e/implementation-launcher.e2e.ts +++ b/e2e/implementation-launcher.e2e.ts @@ -517,18 +517,20 @@ for (let mode of ["block", "complete", "startup-failure", "live"] as const) { }); } -test("a build's agent opens draft pull requests with the machine's own GitHub token", async ({ seed, join: enter, room, baseURL }) => { +test("a build's agent opens draft pull requests with a token of Chopin's GitHub App, commits as its bot and credits whoever started the build", async ({ seed, join: enter, room, baseURL }) => { test.setTimeout(60_000); - await seed("# Draft pull requests\n\nOpen them as the machine's owner.\n", { + await seed("# Draft pull requests\n\nOpen them with Chopin's GitHub App.\n", { graph: preparedGraph(), }); let scratch = await mkdtemp(join(tmpdir(), "chopin-implementation-github-")); let bin = join(scratch, "bin"); await mkdir(bin); - // gh, as the agent finds it: it records the token it was given and its arguments. + // gh, as the agent finds it: it records the token its configuration holds and its arguments. await writeFile( join(bin, "gh"), - `#!/bin/sh\nprintf '%s\\n' "$GH_TOKEN" "$@" > "${join(scratch, "gh")}"\n`, + `#!/bin/sh\nprintf '%s\\n' "$(sed -n 's/^ oauth_token: "\\(.*\\)"$/\\1/p' "$GH_CONFIG_DIR/hosts.yml")" "$@" > "${ + join(scratch, "gh") + }"\n`, ); await chmod(join(bin, "gh"), 0o755); let handle = person(); @@ -554,15 +556,26 @@ test("a build's agent opens draft pull requests with the machine's own GitHub to { timeout: 30_000 }, ).not.toBe(""); let [token, ...args] = (await readFile(join(scratch, "gh"), "utf8")).split("\n"); - // The machine's own GitHub authorization, which its owner approved when it paired. - expect(token).toMatch(new RegExp(`^ghu_e2e_${handle}_\\d+$`)); + // A token Chopin's GitHub App minted for the one repository, never the machine's own login. + expect(token).toMatch(/^ghs_e2e_app_\d+$/); expect(args).toEqual(["pr", "create", "--draft", "--title", "Task one", ""]); await expect.poll( () => readFile(join(scratch, "github.json"), "utf8").catch(() => ""), { timeout: 30_000 }, ).not.toBe(""); let seen = JSON.parse(await readFile(join(scratch, "github.json"), "utf8")); - expect(seen).toMatchObject({ GH_TOKEN: token, GITHUB_TOKEN: token }); + let bot = "chopin-e2e[bot] <4242000+chopin-e2e[bot]@users.noreply.github.com>"; + expect(seen).toMatchObject({ + GH_TOKEN: null, + GITHUB_TOKEN: null, + githubFile: token, + commit: `${bot}|${bot}|N`, + starter: [ + handle, + handle, + expect.stringMatching(new RegExp(`\\+${handle}@users\\.noreply\\.github\\.com$`)), + ], + }); expect(seen.credential).toContain(`password=${token}\n`); expect(local.output()).not.toContain(token); } finally { diff --git a/e2e/playwright.config.ts b/e2e/playwright.config.ts index ec969a77b..04b606a97 100644 --- a/e2e/playwright.config.ts +++ b/e2e/playwright.config.ts @@ -3,6 +3,7 @@ import { join } from "node:path"; import { defineConfig, devices } from "@playwright/test"; +import { GITHUB_APP_ENV } from "./github-app"; import { e2ePort } from "./ports"; import { FIXTURES, HARNESS, HOST, PLAIN, ROOT, SETUP_AGENTS } from "./servers"; @@ -50,6 +51,7 @@ function server(port: number, database: string, extra: Record) { GITHUB_APP_SLUG: "chopin-e2e", GITHUB_APP_CLIENT_ID: "e2e", GITHUB_APP_CLIENT_SECRET: "e2e", + ...GITHUB_APP_ENV, GITHUB_ALLOWED_USERS: "", GITHUB_ALLOWED_ORGANIZATIONS: "githubnext", SESSION_ENCRYPTION_KEY: process.env.SESSION_ENCRYPTION_KEY!,