mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-28 20:21:50 +01:00
fix(server): keep the API base path prefix and scope the launcher profile
The paperclip-api helper dropped any path prefix from PAPERCLIP_API_URL by taking only the URL origin, so a reverse-proxied deployment sent every request to the wrong path. It now keeps the normalized base, including its prefix, and rejects a path that resolves outside that prefix. A generated property test with 200 cases and a named table cover this. A host-credentials run now stages the token-free paperclip-api helper, but it also picked up a forced PATH/ZDOTDIR/BASH_ENV and six shell profile files meant for a managed Git credential run. That silently narrowed the host's own shell setup. The launcher now applies the profile redirection only when it also stages git or gh, and a host-credentials run keeps only its PATH entry and the launcher directory. A login shell can still reorder PATH ahead of the launcher directory on a host-credentials run; nothing here restores it, since doing so would again override the host's own profile. This is a known, accepted residual. The API access note rendered from a launcher directory alone, so a future caller that stages only git and gh, without paperclip-api, could still make the note teach a command that is not on PATH. The launcher now reports its staged program list, and the note reads that list instead of the directory. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
a2d014d2ef
commit
27bdb45fae
7 files changed
+218
-24
No files matched your search
@@ -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" },
|
||||
);
|
||||
|
||||
|
||||
@@ -2904,13 +2904,14 @@ function renderPaperclipEnvNote(env: Record<string, string>): 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<string, string>): boolean {
|
||||
return Boolean(env.PAPERCLIP_GITHUB_LAUNCHER_DIR);
|
||||
return (env.PAPERCLIP_GITHUB_LAUNCHER_PROGRAMS ?? "").split(",").includes("paperclip-api");
|
||||
}
|
||||
|
||||
function renderApiAccessNote(env: Record<string, string>): string {
|
||||
|
||||
@@ -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);
|
||||
}
|
||||
|
||||
@@ -1720,6 +1720,11 @@ export async function prepareGitHubOperationLaunchers(input: {
|
||||
}): Promise<Record<string, string>> {
|
||||
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<string, string> = 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 } : {}),
|
||||
};
|
||||
}
|
||||
|
||||
@@ -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<void>((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<void>((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<void>((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<void>((resolve) => echo.server.close(() => resolve()));
|
||||
}
|
||||
},
|
||||
30_000,
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -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';
|
||||
|
||||
@@ -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",
|
||||
|
||||
Reference in new issue
Block a user