mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-28 20:21:50 +01:00
fix(runner): keep OpenRouter upstream keys in the parent proxy (#17949)
## Thinking Path > - Paperclip manages agents and their work. > - Paperclip Runner starts OpenCode with the selected provider and model. > - The native OpenRouter route passed the reusable upstream key into the child environment. > - A persisted-state scan found that key in an OpenCode database WAL. The exact write operation is not known. > - This pull request reuses the existing parent-held provider proxy for that route. > - The child receives a temporary loopback credential instead of the upstream key. ## Linked Issues or Issue Description **What happened?** The native OpenRouter path passed `OPENROUTER_API_KEY` into the OpenCode child. A real Product E2E persisted-state scan detected the key in the child database WAL. The scan does not establish which operation wrote it. **Expected behavior** The runner keeps the reusable upstream key in its own process. OpenCode can call only the selected model through the session proxy. **Steps to reproduce** 1. Start the OpenCode driver with an explicit or ambient synthetic OpenRouter key. 2. Inspect the fake child configuration, environment, and persisted provider values. 3. Send a request through the real loopback proxy and verify the upstream authorization and exact model. **Paperclip version or commit** The regression is present on merged master `813e808af462b6a6f9db38b6791e3bfe8465b51e`. This PR is rebased onto merged master `d40f9ac377`. **Deployment mode** Built from source. The finding came from an AWS Product E2E job. The regression tests use local synthetic servers and a fake child, with no provider calls. Related issue search found #16643 about general inherited environment scope. This change addresses the narrower native OpenRouter path. No duplicate custody PR was found. This PR must remain unmerged until separately approved; it does not change the eval baseline or qualify a new live result. ## What Changed - Reuse the existing parent-held proxy for native `openrouter/` models. - Remove the upstream key from the child environment and retain it in diagnostic redaction. - Keep the selected provider and exact model. Reject alternate-routing fields before the upstream request. Use a session-lived loopback token. - Preserve the effective explicit or ambient proxy bypass list when adding loopback entries. - Add explicit and ambient key regressions, upstream routing checks, and child persistence checks. - Document the boundary and its limits. ## Verification - Final rebased head: full OpenCode driver suite, 64 synthetic tests passed, and runner source TypeScript check passed. Original author run had 61 passing tests before the new master tests. - The two new explicit and ambient regressions fail on the old implementation. - Independent source review passed. The two new synthetic regressions also passed independently. A separately filtered legacy pair hit fixture selection/chmod ordering and is not claimed as a passing run. - Tests use the real driver and proxy with a fake OpenCode child and synthetic upstream transport. They check key exclusion, exact URL/model/authentication, wrong-model and alternate-routing rejection, proxy bypass preservation, redirect rejection, and proxy shutdown. - The two advisory regressions failed before the correction: fallback routing reached upstream, and ambient NO_PROXY was lost. The full 64-test driver suite and source TypeScript check pass after the correction. - No real OpenCode SDK/provider qualification was run. The original WAL write mechanism remains unknown. - Repo-wide typecheck, test:run, and build were not run locally because local native/DB/model execution is excluded. Normal PR CI passed on head `3e1903272ce65b92055ea70f8dc586d9aa5e0205`: typecheck/release registry, general and serialized suites, runner static/Rust/Vitest checks, E2E shards, build, and canary dry run. Greptile is 5/5 on that head. Paperclip Review is neutral with no blocking findings; its two earlier findings are fixed and resolved. ## Risks The native OpenRouter route now uses the existing OpenAI-compatible proxy transport. Synthetic tests do not prove every live SDK response shape. The loopback token remains a credential, is redacted, and expires with the session. This change does not claim isolation of every child secret or persisted value. ## Model Used OpenAI GPT-6.1 Sol (`gpt-6.1-sol`) assisted source investigation, implementation, synthetic tests, and review with tool use. No provider model call was used for verification. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
e4b30163bb
commit
38d08847b8
4 files changed
+116
-11
No files matched your search
@@ -388,6 +388,13 @@ support disabling reasoning; other runner providers reject an explicit selection
|
||||
before starting work. Native sessions expose support through the
|
||||
`perTurnReasoning` capability.
|
||||
|
||||
For the native `openrouter/` route, the parent-held provider proxy retains the
|
||||
upstream OpenRouter key. The child config keeps the selected `openrouter` provider
|
||||
and exact model, using a random loopback token restricted to that model's chat
|
||||
completions endpoint. The proxy and token expire when the session closes. The
|
||||
local token remains a credential and is redacted from diagnostics; this does not
|
||||
claim isolation for every child credential or persisted value.
|
||||
|
||||
OpenCode's proxy sends agent instructions once per turn through the system prompt. The
|
||||
task envelope retains task-specific constraints and the completion contract,
|
||||
without repeating the system instructions. This applies to all users of the
|
||||
|
||||
@@ -19,7 +19,7 @@ import { EventEmitter } from "node:events";
|
||||
import { promisify } from "node:util";
|
||||
import { getCACertificates } from "node:tls";
|
||||
import { basename, dirname, join, resolve } from "node:path";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest";
|
||||
import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from "vitest";
|
||||
|
||||
import {
|
||||
NATIVE_RUNTIME_ASSET_SCHEMA,
|
||||
@@ -37,6 +37,18 @@ import {
|
||||
openCodeServerDriverInternals,
|
||||
} from "./opencode-server-driver.js";
|
||||
|
||||
const upstreamTransport = vi.hoisted(() => ({ endpoint: "", urls: [] as string[] }));
|
||||
vi.mock("node:https", async importOriginal => {
|
||||
const actual = await importOriginal<typeof import("node:https")>();
|
||||
const http = await import("node:http");
|
||||
return { ...actual, request: (...args: Parameters<typeof actual.request>) => {
|
||||
if (!upstreamTransport.endpoint) return actual.request(...args);
|
||||
upstreamTransport.urls.push(String(args[0]));
|
||||
const options = args[1] as import("node:http").RequestOptions;
|
||||
return http.request(upstreamTransport.endpoint, { ...options, agent: undefined }, args[2]);
|
||||
} };
|
||||
});
|
||||
|
||||
const roots: string[] = [];
|
||||
let fixtureRoot: string | undefined;
|
||||
let fixture: string;
|
||||
@@ -704,6 +716,81 @@ describe("OpenCodeServerDriver", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it.each(["explicit", "ambient"])("keeps OpenRouter upstream credentials out of the child while preserving exact provider/model routing (%s)", async source => {
|
||||
await chmod(fixture, 0o755);
|
||||
const key = "synthetic-upstream-openrouter-key";
|
||||
const model = "deepseek/deepseek-v4.1-flash-1017";
|
||||
const received: Array<{ authorization?: string; body: unknown; referer?: string }> = [];
|
||||
let redirect = false;
|
||||
const upstream = createHttpServer((request, response) => {
|
||||
void (async () => {
|
||||
const chunks = [];
|
||||
for await (const chunk of request) chunks.push(Buffer.from(chunk));
|
||||
received.push({ authorization: request.headers.authorization, body: JSON.parse(Buffer.concat(chunks).toString("utf8")), referer: request.headers["http-referer"] as string });
|
||||
if (redirect) response.writeHead(302, { location: "https://must-not-follow.invalid/" }).end();
|
||||
else response.writeHead(200, { "content-type": "text/event-stream" }).end("data: [DONE]\n\n");
|
||||
})().catch(() => response.destroy());
|
||||
});
|
||||
await new Promise<void>(resolve => upstream.listen(0, "127.0.0.1", resolve));
|
||||
const address = upstream.address();
|
||||
if (!address || typeof address === "string") throw new Error("Missing fixture address");
|
||||
upstreamTransport.endpoint = `http://127.0.0.1:${address.port}/chat/completions`;
|
||||
upstreamTransport.urls = [];
|
||||
const root = await mkdtemp(join(tmpdir(), "paperclip-openrouter-custody-"));
|
||||
roots.push(root);
|
||||
let session: Awaited<ReturnType<OpenCodeServerDriver["openSession"]>> | undefined;
|
||||
try {
|
||||
if (source === "ambient") {
|
||||
vi.stubEnv("OPENROUTER_API_KEY", key);
|
||||
vi.stubEnv("no_proxy", undefined);
|
||||
vi.stubEnv("NO_PROXY", "custody-bypass.invalid");
|
||||
}
|
||||
const driver = new OpenCodeServerDriver({ model: `openrouter/${model}`, runtimeDirectory: root, command: fixture,
|
||||
...(source === "explicit" ? { environment: { PATH: process.env.PATH, OPENROUTER_API_KEY: key, NO_PROXY: "custody-bypass.invalid" } } : {}) });
|
||||
session = await driver.openSession({ runId: "custody", normalizedSessionId: "custody", workingDirectory: root });
|
||||
const configText = await readFile(join(root, "custody", "config", "opencode", "opencode.json"), "utf8");
|
||||
const config = JSON.parse(configText);
|
||||
expect(config.model).toBe(`openrouter/${model}`);
|
||||
expect(config.small_model).toBe(`openrouter/${model}`);
|
||||
expect(Object.keys(config.provider)).toEqual(["openrouter"]);
|
||||
expect(Object.keys(config.provider.openrouter.models)).toEqual([model]);
|
||||
const { baseURL, apiKey } = config.provider.openrouter.options;
|
||||
expect(configText).not.toContain(key);
|
||||
const environmentText = await readFile(join(root, "custody", "data", "fake-environment.json"), "utf8");
|
||||
expect(JSON.parse(environmentText).keys).not.toContain("OPENROUTER_API_KEY");
|
||||
expect(environmentText).not.toContain(key);
|
||||
expect(JSON.parse(environmentText).noProxy).toBe("custody-bypass.invalid,127.0.0.1,localhost");
|
||||
const persisted = await readFile(join(root, "custody", "data", "fake-provider-custody.json"), "utf8");
|
||||
expect(persisted).not.toContain(key);
|
||||
expect(JSON.parse(persisted)).toMatchObject({ providerOptions: { baseURL, apiKey } });
|
||||
expect(baseURL).toMatch(/^http:\/\/127\.0\.0\.1:\d+\/v1$/);
|
||||
expect(apiKey).not.toBe(key);
|
||||
const url = `${baseURL}/chat/completions`;
|
||||
const headers = { Authorization: `Bearer ${apiKey}`, "Content-Type": "application/json" };
|
||||
const payload = { model, messages: [{ role: "user", content: "synthetic" }] };
|
||||
expect((await fetch(url, { method: "POST", headers, body: JSON.stringify({ ...payload, model: "wrong-model" }) })).status).toBe(400);
|
||||
expect(received).toHaveLength(0);
|
||||
for (const [field, value] of Object.entries({ models: ["wrong-model"], route: "fallback", plugins: [{ id: "web" }], transforms: ["middle-out"] })) {
|
||||
expect((await fetch(url, { method: "POST", headers, body: JSON.stringify({ ...payload, [field]: value }) })).status).toBe(400);
|
||||
expect(received).toHaveLength(0);
|
||||
}
|
||||
expect((await fetch(url, { method: "POST", headers, body: JSON.stringify(payload) })).status).toBe(200);
|
||||
expect(received).toEqual([{ authorization: `Bearer ${key}`, body: payload, referer: "https://paperclip.ing" }]);
|
||||
expect(upstreamTransport.urls).toEqual(["https://openrouter.ai/api/v1/chat/completions"]);
|
||||
redirect = true;
|
||||
expect((await fetch(url, { method: "POST", headers, body: JSON.stringify(payload) })).status).toBe(502);
|
||||
expect(upstreamTransport.urls).toHaveLength(2);
|
||||
await session.close({ reason: "test" });
|
||||
session = undefined;
|
||||
await expect(fetch(url, { method: "POST", headers, body: JSON.stringify(payload) })).rejects.toThrow();
|
||||
} finally {
|
||||
await session?.close({ reason: "test" });
|
||||
upstreamTransport.endpoint = "";
|
||||
vi.unstubAllEnvs();
|
||||
await new Promise<void>(resolve => { upstream.close(() => resolve()); upstream.closeAllConnections(); });
|
||||
}
|
||||
});
|
||||
|
||||
it.each([["HTTP_PROXY", "file"], ["ALL_PROXY", "directory"]] as const)("uses the runtime's %s, %s trust, and NO_PROXY without changing global transport", async (proxySetting, trust) => {
|
||||
const requests: string[] = [];
|
||||
const outgoingProxy = createHttpServer((request, response) => {
|
||||
@@ -910,7 +997,7 @@ describe("OpenCodeServerDriver", () => {
|
||||
"utf8",
|
||||
),
|
||||
);
|
||||
expect(environment.keys).toContain("OPENROUTER_API_KEY");
|
||||
expect(environment.keys).not.toContain("OPENROUTER_API_KEY");
|
||||
expect(environment.keys).not.toContain("PAPERCLIP_API_KEY");
|
||||
expect(environment.keys).not.toContain("UNRELATED_SECRET");
|
||||
expect(environment.keys).not.toContain("PAPERCLIP_PROVIDER_TRACE_PATH");
|
||||
|
||||
@@ -2216,7 +2216,8 @@ async function startOpenCodeProviderProxy(baseUrl: string, key: string, model: s
|
||||
let payload: Record<string, unknown>;
|
||||
try { payload = JSON.parse(body.toString("utf8")); }
|
||||
catch { response.writeHead(400).end(); return; }
|
||||
if (!payload || typeof payload !== "object" || payload.model !== model) {
|
||||
if (!payload || typeof payload !== "object" || payload.model !== model ||
|
||||
["models", "route", "plugins", "transforms"].some(field => Object.hasOwn(payload, field))) {
|
||||
response.writeHead(400).end();
|
||||
return;
|
||||
}
|
||||
@@ -2323,14 +2324,15 @@ async function startRuntime(input: {
|
||||
const identityKey = input.options.environment?.PAPERCLIP_AGENT_PRIVATE_KEY;
|
||||
const identityValues = identityKey ? [identityKey, JSON.stringify(identityKey).slice(1, -1),
|
||||
...identityKey.split(/\r?\n/).filter(line => line && !line.startsWith("-----"))] : [];
|
||||
const providerEnvironment = input.options.environment ?? process.env;
|
||||
const sensitiveValues = [
|
||||
...identityValues,
|
||||
password,
|
||||
authHeader,
|
||||
bridge.secret,
|
||||
assignedMcp?.token,
|
||||
input.options.environment?.OPENROUTER_API_KEY,
|
||||
input.options.environment?.PAPERCLIP_AI_PROVIDER_KEY,
|
||||
providerEnvironment.OPENROUTER_API_KEY,
|
||||
providerEnvironment.PAPERCLIP_AI_PROVIDER_KEY,
|
||||
].filter((value): value is string => Boolean(value));
|
||||
input.trace?.addSensitiveValues(sensitiveValues);
|
||||
const instructionRoot =
|
||||
@@ -2346,8 +2348,13 @@ async function startRuntime(input: {
|
||||
if (instructionRoot) externalDirectories[`${instructionRoot}/**`] = "allow";
|
||||
const [modelProvider, ...modelIdParts] = input.options.model.split("/");
|
||||
const providerModelId = modelIdParts.join("/");
|
||||
const providerProxy = modelProvider === "paperclip" && input.options.environment?.PAPERCLIP_AI_PROVIDER_URL
|
||||
? await startOpenCodeProviderProxy(input.options.environment.PAPERCLIP_AI_PROVIDER_URL, input.options.environment.PAPERCLIP_AI_PROVIDER_KEY ?? "", providerModelId, input.options.environment).catch(async error => {
|
||||
const providerRoute = modelProvider === "openrouter"
|
||||
? { baseUrl: "https://openrouter.ai/api/v1", key: providerEnvironment.OPENROUTER_API_KEY ?? "" }
|
||||
: modelProvider === "paperclip" && input.options.environment?.PAPERCLIP_AI_PROVIDER_URL
|
||||
? { baseUrl: input.options.environment.PAPERCLIP_AI_PROVIDER_URL, key: input.options.environment.PAPERCLIP_AI_PROVIDER_KEY ?? "" }
|
||||
: null;
|
||||
const providerProxy = providerRoute
|
||||
? await startOpenCodeProviderProxy(providerRoute.baseUrl, providerRoute.key, providerModelId, providerEnvironment).catch(async error => {
|
||||
await bridge.close().catch(() => {});
|
||||
await removeIsolatedHome(isolatedHome).catch(() => {});
|
||||
throw error;
|
||||
@@ -2397,8 +2404,9 @@ async function startRuntime(input: {
|
||||
...(modelProvider === "openrouter" ? { options: { headers: OPENROUTER_ATTRIBUTION_HEADERS } } : {}),
|
||||
...(providerProxy ? {
|
||||
npm: "@ai-sdk/openai-compatible",
|
||||
name: "Paperclip connection",
|
||||
name: modelProvider === "openrouter" ? "OpenRouter" : "Paperclip connection",
|
||||
options: {
|
||||
...(modelProvider === "openrouter" ? { headers: OPENROUTER_ATTRIBUTION_HEADERS } : {}),
|
||||
baseURL: providerProxy.baseURL,
|
||||
apiKey: providerProxy.token,
|
||||
},
|
||||
@@ -2470,7 +2478,7 @@ async function startRuntime(input: {
|
||||
OPENCODE_SERVER_USERNAME: username,
|
||||
OPENCODE_SERVER_PASSWORD: password,
|
||||
...performanceDiagnostics?.environment,
|
||||
...(providerProxy ? { NO_PROXY: [input.options.environment?.no_proxy ?? input.options.environment?.NO_PROXY, "127.0.0.1", "localhost"].filter(Boolean).join(",") } : {}),
|
||||
...(providerProxy ? { NO_PROXY: [providerEnvironment.no_proxy ?? providerEnvironment.NO_PROXY, "127.0.0.1", "localhost"].filter(Boolean).join(",") } : {}),
|
||||
},
|
||||
);
|
||||
const isolateProcessGroup = input.options.isolateProcessGroup ?? true;
|
||||
@@ -2960,7 +2968,6 @@ function sanitizedEnvironment(
|
||||
"ALL_PROXY",
|
||||
"SSL_CERT_FILE",
|
||||
"SSL_CERT_DIR",
|
||||
"OPENROUTER_API_KEY",
|
||||
];
|
||||
const result: NodeJS.ProcessEnv = {};
|
||||
const candidate = source ?? process.env;
|
||||
@@ -2982,7 +2989,6 @@ function sanitizedEnvironmentKeys(): string[] {
|
||||
"ALL_PROXY",
|
||||
"SSL_CERT_FILE",
|
||||
"SSL_CERT_DIR",
|
||||
"OPENROUTER_API_KEY",
|
||||
];
|
||||
}
|
||||
|
||||
|
||||
@@ -93,10 +93,15 @@ let pendingPermission = await readFile(
|
||||
let permissionStyle = "v2";
|
||||
|
||||
await mkdir(process.env.XDG_DATA_HOME, { recursive: true });
|
||||
await writeFile(join(process.env.XDG_DATA_HOME, "fake-provider-custody.json"), JSON.stringify({
|
||||
openrouterKey: process.env.OPENROUTER_API_KEY,
|
||||
providerOptions: runtimeConfig.provider?.openrouter?.options,
|
||||
}));
|
||||
await writeFile(
|
||||
join(process.env.XDG_DATA_HOME, "fake-environment.json"),
|
||||
JSON.stringify({
|
||||
keys: Object.keys(process.env).sort(),
|
||||
noProxy: process.env.NO_PROXY,
|
||||
home: process.env.HOME,
|
||||
configHome: process.env.XDG_CONFIG_HOME,
|
||||
projectConfigDisabled: process.env.OPENCODE_DISABLE_PROJECT_CONFIG,
|
||||
|
||||
Reference in new issue
Block a user