diff --git a/packages/adapter-utils/src/acpx-engine/execute.test.ts b/packages/adapter-utils/src/acpx-engine/execute.test.ts index d6e549880b8..46e1592bf3d 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.test.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.test.ts @@ -561,7 +561,7 @@ describe("shared ACPX engine runtime behavior", () => { it("includes Paperclip env and API access notes in the ACPX prompt without leaking the token", async () => { const { meta } = await runExecutor( - { agent: "custom", agentCommand: "node ./fake-acp.js", env: { PAPERCLIP_GITHUB_LAUNCHER_DIR: "/tmp/paperclip-op-launchers" } }, + { agent: "custom", agentCommand: "node ./fake-acp.js", env: { PAPERCLIP_GITHUB_LAUNCHER_DIR: "/tmp/paperclip-op-launchers", PAPERCLIP_GITHUB_LAUNCHER_PROGRAMS: "paperclip-api" } }, { authToken: "runtime-secret-token", context: { @@ -617,6 +617,35 @@ describe("shared ACPX engine runtime behavior", () => { expect(prompt).not.toContain("paperclip-api"); }); + it("does not teach the paperclip-api helper when a call stages git and gh only", async () => { + // The launcher directory exists (a call staged git/gh into it), but the + // staged program list does not name paperclip-api. The note must read + // that list, not only the directory, or it would teach a command that + // this run's PATH does not actually have. + const { meta } = await runExecutor( + { + agent: "custom", + agentCommand: "node ./fake-acp.js", + env: { PAPERCLIP_GITHUB_LAUNCHER_DIR: "/tmp/paperclip-op-launchers", PAPERCLIP_GITHUB_LAUNCHER_PROGRAMS: "git,gh" }, + }, + { + authToken: "runtime-secret-token", + context: { + taskId: "issue-1", + wakeReason: "issue_assigned", + paperclipWake: { + reason: "issue_assigned", + issue: { id: "issue-1", identifier: "TEST-1" }, + }, + }, + }, + ); + + const prompt = String(meta[0]?.prompt ?? ""); + expect(prompt).not.toContain("Paperclip API access note:"); + expect(prompt).not.toContain("paperclip-api"); + }); + it.each([ ["claude", false], ["codex", false], ["claude", true], ["codex", true], ] as const)("keeps %s ACP conversation policy on fresh, resumed, and reset turns (custom=%s)", async (agent, custom) => { @@ -720,7 +749,7 @@ describe("shared ACPX engine runtime behavior", () => { it("keeps the authenticated API fallback when ACPX has no native wake reader", async () => { const { meta } = await runExecutor( - { agent: "custom", agentCommand: "node ./fake-acp.js", env: { PAPERCLIP_GITHUB_LAUNCHER_DIR: "/tmp/paperclip-op-launchers" } }, + { agent: "custom", agentCommand: "node ./fake-acp.js", env: { PAPERCLIP_GITHUB_LAUNCHER_DIR: "/tmp/paperclip-op-launchers", PAPERCLIP_GITHUB_LAUNCHER_PROGRAMS: "paperclip-api" } }, { authToken: "runtime-secret-token", context: { @@ -807,7 +836,7 @@ describe("shared ACPX engine runtime behavior", () => { it("does not show a scoped issue API command when the task id is unavailable", async () => { const { meta } = await runExecutor( - { agent: "custom", agentCommand: "node ./fake-acp.js", env: { PAPERCLIP_GITHUB_LAUNCHER_DIR: "/tmp/paperclip-op-launchers" } }, + { agent: "custom", agentCommand: "node ./fake-acp.js", env: { PAPERCLIP_GITHUB_LAUNCHER_DIR: "/tmp/paperclip-op-launchers", PAPERCLIP_GITHUB_LAUNCHER_PROGRAMS: "paperclip-api" } }, { authToken: "runtime-secret-token" }, ); diff --git a/packages/adapter-utils/src/acpx-engine/execute.ts b/packages/adapter-utils/src/acpx-engine/execute.ts index c87a3e77230..d1feddce264 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.ts @@ -2904,13 +2904,14 @@ function renderPaperclipEnvNote(env: Record): string { } /** - * Set by the same run-preparation step that stages the `paperclip-api` helper - * onto `PATH` (see `prepareGitHubOperationLaunchers` in `execution-target.ts`). - * Its presence is the one signal this note needs, so the note and the staging - * step read the same fact and cannot drift apart. + * Read the staged program list, not only the launcher directory: a directory + * can exist for a call that staged `git`/`gh` alone. `PAPERCLIP_GITHUB_LAUNCHER_PROGRAMS` + * is set by the same run-preparation step that stages programs onto `PATH` + * (see `prepareGitHubOperationLaunchers` in `execution-target.ts`), so the + * note and the staging step read the same fact and cannot drift apart. */ function apiAccessHelperIsStaged(env: Record): boolean { - return Boolean(env.PAPERCLIP_GITHUB_LAUNCHER_DIR); + return (env.PAPERCLIP_GITHUB_LAUNCHER_PROGRAMS ?? "").split(",").includes("paperclip-api"); } function renderApiAccessNote(env: Record): string { diff --git a/packages/adapter-utils/src/execution-target.test.ts b/packages/adapter-utils/src/execution-target.test.ts index 994999afa52..21b1e9ba088 100644 --- a/packages/adapter-utils/src/execution-target.test.ts +++ b/packages/adapter-utils/src/execution-target.test.ts @@ -462,6 +462,33 @@ describe("GitHub launcher lifecycle", () => { // A host-GitHub run must keep its own GH_CONFIG_DIR: overwriting it with // an empty staged directory would silently drop the host's gh login. expect(staged.GH_CONFIG_DIR).toBe("/host/.config/gh"); + // A host-credentials run must keep its own shell profile: it must not + // get the ZDOTDIR/BASH_ENV redirection, or the six profile files, that + // a managed run applies to restore the launcher directory onto PATH. + expect(staged.ZDOTDIR).toBeUndefined(); + expect(staged.BASH_ENV).toBeUndefined(); + for (const profile of [".profile", ".bash_profile", ".bashrc", ".zshenv", ".zprofile", ".zshrc"]) { + await expect(access(`${staged.PAPERCLIP_GITHUB_LAUNCHER_DIR}/${profile}`)).rejects.toMatchObject({ code: "ENOENT" }); + } + expect(staged.PATH?.split(":")[0]).toBe(staged.PAPERCLIP_GITHUB_LAUNCHER_DIR); + expect(staged.PAPERCLIP_GITHUB_LAUNCHER_PROGRAMS).toBe("paperclip-api"); + } finally { + await cleanupGitHubOperationLaunchers(run); + } + }); + + it("stages the profile files and ZDOTDIR/BASH_ENV redirection for a managed run", async () => { + const run = { runId: randomUUID(), target: null }; + try { + const staged = await prepareGitHubOperationLaunchers({ + ...run, cwd: "/tmp", env: {}, programs: ["git", "gh", "paperclip-api"], + }); + expect(staged.ZDOTDIR).toBe(staged.PAPERCLIP_GITHUB_LAUNCHER_DIR); + expect(staged.BASH_ENV).toBe(`${staged.PAPERCLIP_GITHUB_LAUNCHER_DIR}/.bashrc`); + expect(staged.PAPERCLIP_GITHUB_LAUNCHER_PROGRAMS).toBe("git,gh,paperclip-api"); + for (const profile of [".profile", ".bash_profile", ".bashrc", ".zshenv", ".zprofile", ".zshrc"]) { + expect(await readFile(`${staged.PAPERCLIP_GITHUB_LAUNCHER_DIR}/${profile}`, "utf8")).toContain("PATH="); + } } finally { await cleanupGitHubOperationLaunchers(run); } diff --git a/packages/adapter-utils/src/execution-target.ts b/packages/adapter-utils/src/execution-target.ts index 23f5d7b168d..395af4856f7 100644 --- a/packages/adapter-utils/src/execution-target.ts +++ b/packages/adapter-utils/src/execution-target.ts @@ -1720,6 +1720,11 @@ export async function prepareGitHubOperationLaunchers(input: { }): Promise> { const programs = input.programs ?? (["git", "gh"] as const); const stageGh = programs.includes("gh"); + // A host-credentials run stages only `paperclip-api`. It must keep the + // shell profile behaviour it had before this function ran unconditionally, + // so only a call that also stages `git` or `gh` writes the profile files + // and redirects ZDOTDIR/BASH_ENV to them. + const stageGitOrGh = stageGh || programs.includes("git"); const remote = input.target?.kind === "remote" ? input.target : null; const directory = githubOperationLauncherDirectory(input); const configDirectory = path.posix.join(directory, "gh-config"); @@ -1730,7 +1735,9 @@ export async function prepareGitHubOperationLaunchers(input: { const profile = `export PATH=${shellQuote(managedPath)}\n`; const files: Record = Object.fromEntries([ ...programs.map((name) => [name, OPERATION_LAUNCHER_SOURCE[name]()] as const), - ...[".zshenv", ".zprofile", ".zshrc", ".bash_profile", ".bashrc", ".profile"].map((name) => [name, profile] as const), + ...(stageGitOrGh + ? [".zshenv", ".zprofile", ".zshrc", ".bash_profile", ".bashrc", ".profile"].map((name) => [name, profile] as const) + : []), ]); if (remote) { const runner = adapterExecutionTargetCommandRunner(remote); @@ -1752,8 +1759,12 @@ export async function prepareGitHubOperationLaunchers(input: { if (stageGh) await fs.mkdir(configDirectory, { recursive: true, mode: 0o700 }); for (const [program, body] of Object.entries(files)) await fs.writeFile(path.join(directory, program), body, { mode: 0o700 }); } - return { ...input.env, PATH: managedPath, ZDOTDIR: directory, BASH_ENV: `${directory}/.bashrc`, + return { ...input.env, PATH: managedPath, PAPERCLIP_GITHUB_LAUNCHER_DIR: directory, + // The staged program list, not only the directory: a caller that stages + // just `paperclip-api` must not make a reader think `git`/`gh` are here. + PAPERCLIP_GITHUB_LAUNCHER_PROGRAMS: programs.join(","), + ...(stageGitOrGh ? { ZDOTDIR: directory, BASH_ENV: `${directory}/.bashrc` } : {}), ...(stageGh ? { GH_CONFIG_DIR: configDirectory } : {}), }; } diff --git a/packages/adapter-utils/src/paperclip-api-launcher.test.ts b/packages/adapter-utils/src/paperclip-api-launcher.test.ts index f3e89a3516b..7db5694d87a 100644 --- a/packages/adapter-utils/src/paperclip-api-launcher.test.ts +++ b/packages/adapter-utils/src/paperclip-api-launcher.test.ts @@ -129,4 +129,123 @@ describe("paperclip-api helper", () => { await expect(exec(bin, ["GET", "/api/agents/me"], { env })).rejects.toMatchObject({ code: 2 }); expect(configured.requests).toHaveLength(0); }); + + describe("keeps the configured base URL's path prefix", () => { + async function startEchoingServer(): Promise<{ server: Server; port: number }> { + const server = createServer((req, res) => { + const chunks: Buffer[] = []; + req.on("data", (chunk) => chunks.push(chunk)); + req.on("end", () => { + res.writeHead(200, { "content-type": "application/json" }); + res.end(JSON.stringify({ receivedPath: req.url })); + }); + }); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address() as { port: number }; + return { server, port }; + } + + it.each<[string, string]>([ + ["http://host", "/api/agents/me"], + ["http://host/", "/api/agents/me"], + ["http://host/api", "/api/agents/me"], + ["http://host/paperclip", "/paperclip/api/agents/me"], + ["http://host/paperclip/api", "/paperclip/api/agents/me"], + ["http://host/paperclip/", "/paperclip/api/agents/me"], + ])("sends PAPERCLIP_API_URL=%s to %s for GET /api/agents/me", async (configuredForm, expectedPath) => { + const echo = await startEchoingServer(); + try { + const apiUrl = configuredForm.replace("http://host", `http://127.0.0.1:${echo.port}`); + const result = await exec(bin, ["GET", "/api/agents/me"], { env: runEnv({ PAPERCLIP_API_URL: apiUrl }) }); + expect(JSON.parse(result.stdout)).toEqual({ receivedPath: expectedPath }); + } finally { + await new Promise((resolve) => echo.server.close(() => resolve())); + } + }); + + const PREFIX_ESCAPE_NAMED_CASES: Array<{ name: string; relativePath: string }> = [ + { name: "a single ../ segment", relativePath: "/api/../agents/me" }, + { name: "a doubled ../../ segment", relativePath: "/api/agents/../../me" }, + { name: "a percent-encoded ../ segment", relativePath: "/api/%2e%2e/agents/me" }, + { name: "an uppercase percent-encoded ../ segment", relativePath: "/api/%2E%2E/agents/me" }, + { name: "a leading // form", relativePath: "//api/agents/me" }, + { name: "a backslash form", relativePath: "/api\\agents\\me" }, + ]; + + it.each(PREFIX_ESCAPE_NAMED_CASES)( + "rejects $name that would escape the configured prefix", + async ({ relativePath }) => { + const echo = await startEchoingServer(); + try { + const apiUrl = `http://127.0.0.1:${echo.port}/paperclip`; + await expect( + exec(bin, ["GET", relativePath], { env: runEnv({ PAPERCLIP_API_URL: apiUrl }) }), + ).rejects.toMatchObject({ code: 2 }); + } finally { + await new Promise((resolve) => echo.server.close(() => resolve())); + } + }, + ); + + const PATH_SEGMENT_POOL = ["agents", "me", "issues", "PAP-1", "comments", "status", "42"]; + + function randomSegment(): string { + return PATH_SEGMENT_POOL[Math.floor(Math.random() * PATH_SEGMENT_POOL.length)]!; + } + + // Generates one of several input shapes at random: an ordinary path under + // /api/, a dot-segment escape, a percent-encoded dot-segment escape, a + // percent-encoded separator, a leading // form, a backslash form, or a + // path carrying a scheme. 200 calls cover many combinations of these + // shapes, instead of one example per shape. + function randomPathCase(): string { + const depth = 1 + Math.floor(Math.random() * 4); + const segments = Array.from({ length: depth }, randomSegment); + const ordinary = "/api/" + segments.join("/"); + switch (Math.floor(Math.random() * 7)) { + case 0: return ordinary; + case 1: return "/api/" + "../".repeat(1 + Math.floor(Math.random() * 3)) + segments.join("/"); + case 2: return "/api/" + "%2e%2e/".repeat(1 + Math.floor(Math.random() * 3)) + segments.join("/"); + case 3: return "/api%2f" + segments.join("%2f"); + case 4: return "//" + segments.join("/"); + case 5: return "/api\\" + segments.join("\\"); + case 6: return "http://attacker.example" + ordinary; + default: return ordinary; + } + } + + it( + "either rejects, or keeps the configured prefix, for 200 generated path inputs (property test)", + async () => { + const echo = await startEchoingServer(); + try { + const apiUrl = `http://127.0.0.1:${echo.port}/paperclip`; + const expectedPrefix = "/paperclip/api/"; + const trials = Array.from({ length: 200 }, randomPathCase); + const outcomes = await Promise.all( + trials.map((relativePath) => + exec(bin, ["GET", relativePath], { env: runEnv({ PAPERCLIP_API_URL: apiUrl }) }) + .then((result) => ({ accepted: true as const, body: result.stdout })) + .catch((error: { code?: number }) => ({ accepted: false as const, code: error.code })), + ), + ); + outcomes.forEach((outcome, index) => { + const input = trials[index]; + if (!outcome.accepted) { + expect(outcome.code, `input ${JSON.stringify(input)} rejected with an unexpected exit code`).toBe(2); + return; + } + const receivedPath = (JSON.parse(outcome.body) as { receivedPath: string }).receivedPath; + expect( + receivedPath.startsWith(expectedPrefix), + `input ${JSON.stringify(input)} produced ${receivedPath}, outside ${expectedPrefix}`, + ).toBe(true); + }); + } finally { + await new Promise((resolve) => echo.server.close(() => resolve())); + } + }, + 30_000, + ); + }); }); diff --git a/packages/adapter-utils/src/paperclip-api-launcher.ts b/packages/adapter-utils/src/paperclip-api-launcher.ts index a958d876c3e..55cf1a3694a 100644 --- a/packages/adapter-utils/src/paperclip-api-launcher.ts +++ b/packages/adapter-utils/src/paperclip-api-launcher.ts @@ -52,34 +52,40 @@ function assertSafeRelativeApiPath(relativePath) { if (!relativePath.startsWith('/api/')) usageError('the path must start with /api/'); } -function readConfiguredOrigin() { +// Read the configured Paperclip API base: the origin plus any path prefix, +// for example "/paperclip" when the API sits behind a reverse proxy at that +// path. Every other base-URL reader in this codebase keeps this prefix; this +// program must too, or a proxied deployment gets every request 404ed. +function readConfiguredBase() { const rawBase = process.env.PAPERCLIP_API_URL; if (!rawBase) usageError('PAPERCLIP_API_URL is not set'); const normalized = rawBase.replace(/\/+$/, '').replace(/\/api$/, ''); - let origin; - try { origin = new URL(normalized).origin; } + let parsed; + try { parsed = new URL(normalized); } catch { usageError('PAPERCLIP_API_URL is not a valid URL'); } - return origin; + const pathPrefix = parsed.pathname === '/' ? '' : parsed.pathname.replace(/\/+$/, ''); + return { origin: parsed.origin, pathPrefix }; } -// Build the final request URL against the normalized configured origin, then -// verify the built URL actually resolved to that same origin. This check -// stands even if a future change to the path check above lets an unexpected -// form through. -function buildRequestUrl(origin, relativePath) { +// Build the final request URL against the normalized configured base, +// including its path prefix, then verify the built URL actually resolved to +// that same origin and under that same prefix. This check stands even if a +// future change to the path check above lets an unexpected form through. +function buildRequestUrl(base, relativePath) { let url; - try { url = new URL(relativePath, origin + '/'); } + try { url = new URL(base.pathPrefix + relativePath, base.origin + '/'); } catch { usageError('could not build a request URL from the given path'); } - if (url.origin !== origin) usageError('the given path did not resolve to the configured Paperclip API origin'); - if (!url.pathname.startsWith('/api/')) usageError('the given path did not resolve under /api/'); + if (url.origin !== base.origin) usageError('the given path did not resolve to the configured Paperclip API origin'); + const expectedPrefix = base.pathPrefix + '/api/'; + if (!url.pathname.startsWith(expectedPrefix)) usageError('the given path did not resolve under the configured Paperclip API base'); return url; } async function main() { const { method, relativePath, headers, body } = parseArgs(process.argv.slice(2)); assertSafeRelativeApiPath(relativePath); - const origin = readConfiguredOrigin(); - const url = buildRequestUrl(origin, relativePath); + const base = readConfiguredBase(); + const url = buildRequestUrl(base, relativePath); const bearer = process.env.PAPERCLIP_API_KEY; if (!bearer) usageError('PAPERCLIP_API_KEY is not set'); if (body !== undefined && !('Content-Type' in headers)) headers['Content-Type'] = 'application/json'; diff --git a/server/src/services/heartbeat.ts b/server/src/services/heartbeat.ts index cdd69904298..e607612f1d6 100644 --- a/server/src/services/heartbeat.ts +++ b/server/src/services/heartbeat.ts @@ -1441,6 +1441,7 @@ const FORBIDDEN_ENV_BINDING_KEYS = new Set([ "PAPERCLIP_GITHUB_BROKER_URL", "PAPERCLIP_GITHUB_BRIDGE_TOKEN", "PAPERCLIP_GITHUB_LAUNCHER_DIR", + "PAPERCLIP_GITHUB_LAUNCHER_PROGRAMS", ]); const MANAGED_GITHUB_TOKEN_KEYS = new Set([ "GH_TOKEN",