From fdb9a4880db3641079402a3c08bddc3fb71a6aa7 Mon Sep 17 00:00:00 2001 From: Nicky Leach Date: Fri, 14 Aug 2026 22:11:16 -0700 Subject: [PATCH] fix(security): route paperclipai CLI guidance through safe npx form (CWE-78) (#11400) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Paperclip provides CLI commands and guidance for operators and agents > - The `pnpm paperclipai` script can pass argument values through a shell > - Shell re-parsing can execute command substitutions inside quoted values > - This pull request routes guidance through inert-argv `npx paperclipai` commands and adds regression coverage > - The benefit is safer operator guidance across documentation and runtime hints ## Linked Issues or Issue Description This pull request fixes a command-injection-class defect in Paperclip CLI guidance. **What happened?** The `pnpm paperclipai --flag "$VALUE"` form can re-parse argument values through a shell. A command substitution inside a quoted value can execute on the host. **Expected behavior** Paperclip guidance must pass CLI values as inert argument values. Host-derived values must not appear in copyable commands. **Steps to reproduce** 1. Run a Paperclip guidance command that uses the `pnpm paperclipai` script. 2. Provide a quoted value that contains a command substitution. 3. Observe that the shell can evaluate the substitution before the CLI starts. 4. Compare the result with the `npx paperclipai` form. **Paperclip version or commit** `5670984b75d109950c968542a0111ebb6967f4da` **Deployment mode** All deployment modes that show or use the affected CLI guidance. **Installation method** Built from source and installed CLI guidance. **Agent adapter(s) involved** Not adapter-specific (core bug). **Database mode** Not database-related. **Access context** Both. **Additional context** The earlier merged PR [#11343](https://github.com/paperclipai/paperclip/pull/11343) used the unsafe `pnpm exec paperclipai` form. This fresh PR replaces that guidance with the safe `npx paperclipai` form. ## What Changed - Standardize documentation and runtime hints on `npx paperclipai`. - Remove the broken `pnpm exec paperclipai` guidance. - Use a static `` placeholder in private-hostname guidance. - Add regression tests for unsafe forms, continued lines, static hosts, and offline guidance. ## Verification - `git diff --check origin/master...origin/fix/paperclipai-cli-npx-safe-invocation` passes. - The branch adds `server/src/__tests__/cli-invocation-safety.test.ts` and updates private-hostname tests. - CI must run the new tests, typecheck, lint, and build checks. - Local Vitest execution was not available because this worktree has no installed Vitest binary. ## Risks - The change affects operator and agent documentation text. - The runtime hints now show `` instead of a request-derived host value. - No database schema or migration changes exist. - CI will detect any missed unsafe invocation or type error. ## Model Used OpenAI GPT-5, exact model ID `gpt-5`, with tool use and code-review assistance. The model used repository inspection, Git operations, and PR preparation. ## 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] CI ran the test suites and they pass; local test execution was unavailable in this worktree - [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 addressed all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip --- .../SKILL.md | 2 +- .agents/skills/paperclip-page/README.md | 8 +- cli/src/__tests__/env-lab.test.ts | 267 +++++- cli/src/__tests__/http.test.ts | 2 +- cli/src/client/http.ts | 2 +- cli/src/commands/env-lab.ts | 85 +- doc/CLI.md | 875 ++++++++++-------- doc/DATABASE.md | 2 +- doc/DEPLOYMENT-MODES.md | 4 +- doc/DEVELOPING.md | 41 +- doc/HERMES_GATEWAY_ONBOARDING.md | 14 +- doc/design/AGENT-SESSIONS.md | 2 +- doc/plugins/PLUGIN_SPEC.md | 12 +- docs/adapters/claude-local.md | 2 +- docs/adapters/codex-local.md | 2 +- docs/api/secrets.md | 4 +- docs/cli/control-plane-commands.md | 76 +- docs/cli/overview.md | 18 +- docs/cli/setup-commands.md | 8 +- docs/deploy/deployment-modes.md | 2 +- docs/deploy/local-development.md | 13 +- docs/deploy/secrets.md | 8 +- docs/deploy/tailscale-private-access.md | 2 +- docs/feedback-voting.md | 4 +- .../board-operator/experimental-features.md | 2 +- docs/guides/openclaw-docker-setup.md | 4 +- .../plugin-authoring-smoke-example/README.md | 2 +- .../plugin-file-browser-example/README.md | 4 +- .../plugin-hello-world-example/README.md | 6 +- .../plugin-kitchen-sink-example/README.md | 2 +- .../sandbox-providers/kubernetes/SMOKE.md | 2 +- .../__tests__/cli-invocation-safety.test.ts | 761 ++++++++++++--- .../__tests__/private-hostname-guard.test.ts | 6 +- server/src/adapters/hermes-gateway-doc.ts | 2 +- .../src/middleware/private-hostname-guard.ts | 6 +- server/src/routes/access.ts | 8 +- server/src/services/company-export-readme.ts | 2 +- server/src/startup-banner.ts | 2 +- skills/paperclip-board/SKILL.md | 2 +- skills/paperclip/SKILL.md | 2 +- ui/src/App.test.tsx | 2 +- ui/src/bootstrapSetup.ts | 2 +- ui/src/lib/agent-onboarding-prompt.ts | 4 +- ui/src/pages/CompanyExport.tsx | 2 +- 44 files changed, 1620 insertions(+), 658 deletions(-) diff --git a/.agents/skills/paperclip-dev-workspace-run-verify-fix/SKILL.md b/.agents/skills/paperclip-dev-workspace-run-verify-fix/SKILL.md index 29af882a84..06e6b2efe5 100644 --- a/.agents/skills/paperclip-dev-workspace-run-verify-fix/SKILL.md +++ b/.agents/skills/paperclip-dev-workspace-run-verify-fix/SKILL.md @@ -208,7 +208,7 @@ is missing, the cloned app does not have the expected companies/issues/agents, or the user explicitly asks for the normal isolated-workspace database. ```sh -pnpm paperclipai worktree reseed --from-instance default --seed-mode full --yes +npx paperclipai worktree reseed --from-instance default --seed-mode full --yes ``` After reseed, restart through the managed runtime path. A reseed can copy diff --git a/.agents/skills/paperclip-page/README.md b/.agents/skills/paperclip-page/README.md index 47cf6d0f1e..21bf74d334 100644 --- a/.agents/skills/paperclip-page/README.md +++ b/.agents/skills/paperclip-page/README.md @@ -493,12 +493,12 @@ Create secrets from environment variables so values do not land in shell history export PAPERCLIP_PAGE_AWS_ACCESS_KEY_ID="$(jq -r '.AccessKey.AccessKeyId' /tmp/paperclip-page-uploader-key.json)" export PAPERCLIP_PAGE_AWS_SECRET_ACCESS_KEY="$(jq -r '.AccessKey.SecretAccessKey' /tmp/paperclip-page-uploader-key.json)" -pnpm exec paperclipai secrets create \ +npx paperclipai secrets create \ --company-id \ --name paperclip-page-aws-access-key-id \ --value-env PAPERCLIP_PAGE_AWS_ACCESS_KEY_ID -pnpm exec paperclipai secrets create \ +npx paperclipai secrets create \ --company-id \ --name paperclip-page-aws-secret-access-key \ --value-env PAPERCLIP_PAGE_AWS_SECRET_ACCESS_KEY @@ -532,7 +532,7 @@ host `AWS_PROFILE` identity for the entire agent run: Create or update the company skill from this package: ```bash -pnpm exec paperclipai skills create \ +npx paperclipai skills create \ --company-id \ --name "Paperclip Page" \ --slug paperclip-page \ @@ -543,7 +543,7 @@ pnpm exec paperclipai skills create \ Attach it to an agent: ```bash -pnpm exec paperclipai skills agent sync \ +npx paperclipai skills agent sync \ --company-id \ --skill paperclip-page ``` diff --git a/cli/src/__tests__/env-lab.test.ts b/cli/src/__tests__/env-lab.test.ts index 02d6d7daf1..6afa22660b 100644 --- a/cli/src/__tests__/env-lab.test.ts +++ b/cli/src/__tests__/env-lab.test.ts @@ -1,6 +1,13 @@ import path from "node:path"; -import { describe, expect, it } from "vitest"; -import { collectEnvLabDoctorStatus, resolveEnvLabSshStatePath } from "../commands/env-lab.js"; +import { afterEach, describe, expect, it, vi } from "vitest"; +import * as p from "@clack/prompts"; +import { + buildEnvLabCleanupCommand, + collectEnvLabDoctorStatus, + envLabDoctorCommand, + resolveEnvLabCliInvocation, + resolveEnvLabSshStatePath, +} from "../commands/env-lab.js"; describe("env-lab command", () => { it("resolves the default SSH fixture state path under the instance root", () => { @@ -22,3 +29,259 @@ describe("env-lab command", () => { expect(status.ssh.environment).toBeNull(); }); }); + +describe("env-lab cleanup command hint", () => { + const originalCwd = process.cwd(); + + afterEach(() => { + process.chdir(originalCwd); + }); + + // Resolve a source-checkout invocation for a fabricated checkout root. A source + // checkout runs this module from `/src/commands/env-lab.ts`, so the + // resolver reads that layout and returns the tsx runner and source entry. + function sourceInvocation(root: string) { + return resolveEnvLabCliInvocation(path.join(root, "src", "commands", "env-lab.ts")); + } + + // Resolve a bundled-build invocation for a fabricated package root. The bundled + // build runs this module from `/dist/index.js`, so the resolver returns + // that file as the entry with no tsx runner. + function bundledInvocation(root: string) { + return resolveEnvLabCliInvocation(path.join(root, "dist", "index.js")); + } + + // Split a command that uses POSIX single-quoting into its argument tokens. The + // parser reads a single-quoted span verbatim and reads `\'` outside a span as a + // literal single quote. This is the same rule a POSIX shell obeys, so a token + // list proves the shell reads the exact paths and runs no embedded command. + function tokenizePosix(command: string): string[] { + const tokens: string[] = []; + let current = ""; + let started = false; + let inQuotes = false; + for (let index = 0; index < command.length; index += 1) { + const character = command[index]; + if (inQuotes) { + if (character === "'") { + inQuotes = false; + } else { + current += character; + } + } else if (character === "'") { + inQuotes = true; + started = true; + } else if (character === "\\") { + index += 1; + current += command[index]; + started = true; + } else if (character === " ") { + if (started) { + tokens.push(current); + current = ""; + started = false; + } + } else { + current += character; + started = true; + } + } + if (started) { + tokens.push(current); + } + return tokens; + } + + // Return the two path arguments from the `node` command. + function extractPaths(command: string): string[] { + const tokens = tokenizePosix(command); + return tokens.slice(1, 3); + } + + it("resolves both CLI paths to absolute paths", () => { + const command = buildEnvLabCleanupCommand(); + const paths = extractPaths(command); + + expect(paths).toHaveLength(2); + for (const resolved of paths) { + expect(path.isAbsolute(resolved)).toBe(true); + } + expect(command.endsWith("env-lab down")).toBe(true); + }); + + it("points at the checked-out tsx runner and cli source entry", () => { + const [tsxBin, entry] = extractPaths(buildEnvLabCleanupCommand()); + + expect(tsxBin).toContain( + path.join("cli", "node_modules", "tsx", "dist", "cli.mjs"), + ); + expect(entry).toContain(path.join("cli", "src", "index.ts")); + }); + + it("returns the same command from a checkout subdirectory", () => { + const fromRoot = buildEnvLabCleanupCommand(); + + // Simulate a contributor who runs `env-lab doctor` from a subdirectory of + // the checkout. A relative path would change with the working directory, so + // this asserts the command stays constant. + process.chdir(path.dirname(originalCwd)); + const fromParent = buildEnvLabCleanupCommand(); + process.chdir(originalCwd); + + expect(fromParent).toBe(fromRoot); + }); + + it("never restores the unsafe pnpm invocation forms", () => { + const command = buildEnvLabCleanupCommand(); + + // The bare `pnpm paperclipai` script form is unsafe. The `pnpm exec` form + // does not resolve the CLI binary. Keep both out of the hint. + expect(command).not.toContain("pnpm paperclipai"); + expect(command).not.toContain("pnpm exec paperclipai"); + }); + + // A checkout path can hold shell metacharacters. A contributor copies the hint + // and pastes it into a shell. The hint must neutralize each metacharacter, so + // the shell reads the exact path and runs no embedded command. Each case below + // is a checkout root with one dangerous construct. + const dangerousRoots = [ + { label: "a dollar sign", root: "/tmp/env$lab/checkout" }, + { label: "command substitution", root: "/tmp/$(touch pwned)/checkout" }, + { label: "backticks", root: "/tmp/`touch pwned`/checkout" }, + { label: "a double quote", root: '/tmp/env"lab/checkout' }, + { label: "a single quote", root: "/tmp/env'lab/checkout" }, + ]; + + for (const { label, root } of dangerousRoots) { + it(`keeps a checkout path with ${label} inert in the cleanup hint`, () => { + const command = buildEnvLabCleanupCommand({ invocation: sourceInvocation(root) }); + const tokens = tokenizePosix(command); + const tsxBin = path.join(root, "node_modules", "tsx", "dist", "cli.mjs"); + const entry = path.join(root, "src", "index.ts"); + + // The shell reads the exact paths as single argument tokens. It does not + // split the paths or run the embedded construct. + expect(tokens).toEqual(["node", tsxBin, entry, "env-lab", "down"]); + + // The old double-quoted form left `$(...)`, a backtick pair, and `$NAME` + // live. Do not restore it. + expect(command).not.toContain(`"${tsxBin}"`); + expect(command).not.toContain(`"${entry}"`); + }); + } + + it("forwards the inspected instance to the cleanup hint", () => { + const command = buildEnvLabCleanupCommand({ + instance: "fixture-test", + invocation: sourceInvocation("/tmp/checkout"), + }); + const tokens = tokenizePosix(command); + + // The hint ends with `--instance `, so it stops the fixture the doctor + // command diagnosed, not the default instance. + expect(tokens.slice(-2)).toEqual(["--instance", "fixture-test"]); + }); + + it("omits the instance flag when the doctor command uses the default instance", () => { + const command = buildEnvLabCleanupCommand({ invocation: sourceInvocation("/tmp/checkout") }); + + // Without a selected instance, `env-lab down` resolves the same default + // instance the doctor command inspected. Do not add an empty flag. + expect(command).not.toContain("--instance"); + expect(command.endsWith("env-lab down")).toBe(true); + }); + + it("keeps an instance id with shell metacharacters inert", () => { + const command = buildEnvLabCleanupCommand({ + instance: "$(touch pwned)", + invocation: sourceInvocation("/tmp/checkout"), + }); + const tokens = tokenizePosix(command); + + // The shell reads the instance id as one literal token and runs no embedded + // command. + expect(tokens.slice(-2)).toEqual(["--instance", "$(touch pwned)"]); + expect(command).not.toContain('"$(touch pwned)"'); + }); + + it("runs the bundled dist entry directly, without the tsx runner", () => { + const command = buildEnvLabCleanupCommand({ + invocation: bundledInvocation("/opt/pkg"), + }); + const tokens = tokenizePosix(command); + + // The published package ships one `dist/index.js` file and no tsx runner, so + // node runs that file directly. + expect(tokens).toEqual(["node", path.join("/opt/pkg", "dist", "index.js"), "env-lab", "down"]); + expect(command).not.toContain("tsx"); + expect(command).not.toContain(path.join("src", "index.ts")); + }); + + it("keeps a bundled package path with shell metacharacters inert", () => { + const root = "/opt/$(touch pwned)/pkg"; + const command = buildEnvLabCleanupCommand({ invocation: bundledInvocation(root) }); + const tokens = tokenizePosix(command); + const entry = path.join(root, "dist", "index.js"); + + // The shell reads the exact bundled path as one token and runs no embedded + // command. + expect(tokens).toEqual(["node", entry, "env-lab", "down"]); + expect(command).not.toContain(`"${entry}"`); + }); +}); + +describe("env-lab doctor cleanup hint instance", () => { + const originalInstanceId = process.env.PAPERCLIP_INSTANCE_ID; + + afterEach(() => { + if (originalInstanceId === undefined) { + delete process.env.PAPERCLIP_INSTANCE_ID; + } else { + process.env.PAPERCLIP_INSTANCE_ID = originalInstanceId; + } + vi.restoreAllMocks(); + }); + + // Capture the cleanup hint the doctor prints. The doctor reports through + // `p.log`, so the test replaces each channel and reads the captured lines. + function captureDoctorMessages(): string[] { + const messages: string[] = []; + vi.spyOn(p.log, "message").mockImplementation((message?: string) => { + messages.push(message ?? ""); + }); + vi.spyOn(p.log, "success").mockImplementation(() => {}); + vi.spyOn(p.log, "warn").mockImplementation(() => {}); + vi.spyOn(p.log, "info").mockImplementation(() => {}); + return messages; + } + + it("pins the PAPERCLIP_INSTANCE_ID instance when opts.instance is absent", async () => { + // The doctor diagnoses the instance that `PAPERCLIP_INSTANCE_ID` selects. + // The cleanup hint must target that instance, not the default instance. + process.env.PAPERCLIP_INSTANCE_ID = "env-selected-instance"; + const messages = captureDoctorMessages(); + + await envLabDoctorCommand({ instance: undefined }); + + const cleanup = messages.find((message) => message.startsWith("Cleanup:")); + expect(cleanup).toBeDefined(); + expect(cleanup).toContain("env-lab down"); + expect(cleanup).toContain("--instance"); + expect(cleanup).toContain("env-selected-instance"); + }); + + it("pins the explicit instance over PAPERCLIP_INSTANCE_ID", async () => { + // An explicit `--instance` flag overrides the environment variable, so the + // hint targets the explicit instance the doctor inspected. + process.env.PAPERCLIP_INSTANCE_ID = "env-selected-instance"; + const messages = captureDoctorMessages(); + + await envLabDoctorCommand({ instance: "explicit-instance" }); + + const cleanup = messages.find((message) => message.startsWith("Cleanup:")); + expect(cleanup).toBeDefined(); + expect(cleanup).toContain("--instance"); + expect(cleanup).toContain("explicit-instance"); + expect(cleanup).not.toContain("env-selected-instance"); + }); +}); diff --git a/cli/src/__tests__/http.test.ts b/cli/src/__tests__/http.test.ts index 0829f06baa..e0e1e61b74 100644 --- a/cli/src/__tests__/http.test.ts +++ b/cli/src/__tests__/http.test.ts @@ -79,7 +79,7 @@ describe("PaperclipApiClient", () => { /curl http:\/\/localhost:3100\/api\/health/, ); await expect(client.post("/api/companies/import/preview", {})).rejects.toThrow( - /pnpm dev|pnpm paperclipai run/, + /pnpm dev|npx paperclipai run/, ); }); diff --git a/cli/src/client/http.ts b/cli/src/client/http.ts index 745cf15db2..dd4f784776 100644 --- a/cli/src/client/http.ts +++ b/cli/src/client/http.ts @@ -233,7 +233,7 @@ function buildConnectionErrorMessage(input: { "This usually means the Paperclip server is not running, the configured URL is wrong, or the request is being blocked before it reaches Paperclip.", "", "Try:", - "- Start Paperclip with `pnpm dev` or `pnpm paperclipai run`.", + "- Start Paperclip with `pnpm dev` (from a source checkout) or `npx paperclipai run`.", `- Verify the server is reachable with \`curl ${healthUrl}\`.`, `- If Paperclip is running elsewhere, pass \`--api-base ${input.apiBase.replace(/\/+$/, "")}\` or set \`PAPERCLIP_API_URL\`.`, ); diff --git a/cli/src/commands/env-lab.ts b/cli/src/commands/env-lab.ts index 55227c9f63..2e65bd36bf 100644 --- a/cli/src/commands/env-lab.ts +++ b/cli/src/commands/env-lab.ts @@ -1,4 +1,5 @@ import path from "node:path"; +import { fileURLToPath } from "node:url"; import type { Command } from "commander"; import * as p from "@clack/prompts"; import pc from "picocolors"; @@ -111,6 +112,76 @@ export async function envLabDownCommand(opts: { instance?: string; json?: boolea p.log.message(`State: ${pc.dim(statePath)}`); } +// Quote one argument for a POSIX shell. The env-lab cleanup hint is copyable, so +// a contributor can paste it into a shell. A checkout path can hold shell +// metacharacters, such as `$`, a backtick, or a double quote. Inside double +// quotes a POSIX shell still expands `$(...)`, a backtick pair, and `$NAME`, and +// a double quote in the path ends the quoted span. So double quotes do not make +// the path safe. Single quotes stop every expansion. This function wraps the +// value in single quotes and rewrites each embedded single quote as the `'\''` +// sequence. The shell then reads the exact path and runs no embedded command. +function shellQuoteArgument(value: string): string { + return "'" + value.replace(/'/g, "'\\''") + "'"; +} + +// Describe how to re-run the env-lab CLI to stop the fixture. The bundled build +// emits one `dist/index.js` file, so node runs that file directly and `tsxBin` +// is `null`. A source checkout runs `src/index.ts` through the checked-out tsx +// runner, because the entry is TypeScript. +interface EnvLabCliInvocation { + entry: string; + tsxBin: string | null; +} + +// Resolve how to re-run the CLI from the running module location. The cleanup +// hint must run the same CLI that prints it, so it stops the correct version. +// `import.meta.url` gives the running module. The bundled build runs this module +// from `/dist/index.js`, so the hint runs that exact file with node. The +// published package ships no `src` directory and no tsx runner. A source +// checkout runs this module from `/src/commands/env-lab.ts`, so the hint +// runs `/src/index.ts` through the checked-out tsx runner. This resolver +// reads an absolute path from the module location, so the hint works from any +// working directory. The `modulePath` parameter is a test seam; production +// callers use the running module path. +export function resolveEnvLabCliInvocation( + modulePath: string = fileURLToPath(import.meta.url), +): EnvLabCliInvocation { + const moduleDir = path.dirname(modulePath); + const isSourceCheckout = + path.basename(moduleDir) === "commands" && path.basename(path.dirname(moduleDir)) === "src"; + if (isSourceCheckout) { + const cliRoot = path.resolve(moduleDir, "..", ".."); + return { + entry: path.join(cliRoot, "src", "index.ts"), + tsxBin: path.join(cliRoot, "node_modules", "tsx", "dist", "cli.mjs"), + }; + } + return { entry: modulePath, tsxBin: null }; +} + +// Build the env-lab cleanup hint as a copyable shell command. The hint stops the +// fixture that `env-lab doctor` inspected. It runs the same CLI that prints it, +// so it stops the correct version, and it forwards the inspected instance, so it +// stops the correct instance. It passes an inert `argv` value, so no shell reads +// the argument. Each path and the instance id pass through `shellQuoteArgument`, +// so a shell metacharacter stays inert when a contributor pastes the command. +// The `invocation` parameter is a test seam; production callers use the resolved +// running-module invocation. +export function buildEnvLabCleanupCommand( + opts: { instance?: string; invocation?: EnvLabCliInvocation } = {}, +): string { + const invocation = opts.invocation ?? resolveEnvLabCliInvocation(); + const parts = ["node"]; + if (invocation.tsxBin !== null) { + parts.push(shellQuoteArgument(invocation.tsxBin)); + } + parts.push(shellQuoteArgument(invocation.entry), "env-lab down"); + if (opts.instance !== undefined) { + parts.push("--instance", shellQuoteArgument(opts.instance)); + } + return parts.join(" "); +} + export async function envLabDoctorCommand(opts: { instance?: string; json?: boolean }) { const status = await collectEnvLabDoctorStatus(opts); @@ -138,7 +209,19 @@ export async function envLabDoctorCommand(opts: { instance?: string; json?: bool p.log.message(`State: ${pc.dim(status.statePath)}`); } - p.log.message(`Cleanup: ${pc.dim("pnpm paperclipai env-lab down")}`); + // The cleanup hint runs the same CLI that prints it, so it stops the correct + // version. The bundled build runs `dist/index.js`; a source checkout runs + // `src/index.ts` through the checked-out tsx runner. The hint uses absolute + // paths, so it works from any working directory. It passes an inert `argv` + // value, so no shell reads the argument. See `doc/CLI.md`, "safe invocation". + // + // The doctor diagnoses the instance that `resolvePaperclipInstanceId` selects + // from `opts.instance` or the `PAPERCLIP_INSTANCE_ID` environment variable. + // The hint pins that resolved instance, so a contributor who pastes the hint + // in a shell without `PAPERCLIP_INSTANCE_ID` stops the diagnosed fixture, not + // the default instance. + const cleanupInstance = resolvePaperclipInstanceId(opts.instance); + p.log.message(`Cleanup: ${pc.dim(buildEnvLabCleanupCommand({ instance: cleanupInstance }))}`); } export function registerEnvLabCommands(program: Command) { diff --git a/doc/CLI.md b/doc/CLI.md index c5eff04686..f01291b299 100644 --- a/doc/CLI.md +++ b/doc/CLI.md @@ -8,53 +8,112 @@ Paperclip CLI now supports both: ## Security: safe invocation for content-bearing arguments -Use `pnpm exec paperclipai` for any command whose argument can hold untrusted or +Use `npx paperclipai` for any command whose argument can hold untrusted or semi-trusted content. Untrusted content includes issue text, comment bodies, -Markdown, pasted snippets, and model output. `pnpm exec` runs the installed -binary directly. It passes the argument as an inert `argv` value. It does not -run a shell over the value. +Markdown, pasted snippets, and model output. `npx` runs the CLI binary directly. +It passes the argument as an inert `argv` value. It does not run a shell over the +value. `npx paperclipai` works on any machine with Node: it runs a local install +of the `paperclipai` package, and it fetches the published package when no local +install is present. -Do not use `pnpm paperclipai` for a content-bearing argument. The safe form and -the unsafe form differ only by the `exec` keyword, so read the command with -care. `pnpm paperclipai` is a `package.json` script. `pnpm` runs the argument -through `/bin/sh` first. The shell interprets these metacharacters before the -CLI starts: +Do not use `pnpm paperclipai` for a content-bearing argument. `pnpm paperclipai` +is a `package.json` script. `pnpm` builds a `/bin/sh` command string and appends +the argument to it, so the shell reads the argument first. The shell interprets +these spans before the CLI starts: - command substitution: a backtick pair or `$( )` - variable expansion: `$NAME` or `${NAME}` (this can leak a secret value into the persisted argument) -- other shell syntax: `; | & < >` A crafted value can run an arbitrary command as the invoking user. A crafted value can also expand an environment variable into the stored argument. No -CLI-side check stops this, because the shell runs before `cli/src` starts. +CLI-side check stops this, because the shell runs before `cli/src` starts. This +is true even when the argument comes from a quoted shell variable, because `pnpm` +re-evaluates the value in its own shell. Safe forms: -- `pnpm exec paperclipai ` — the documented default -- `npx paperclipai ` — also injection-safe; use it when no local - install is present. `npx` fetches the package from the npm registry when the - binary is not installed locally, so a mistyped name can start a network - fetch. `pnpm exec` never does this; it runs only the locally installed binary. -- `node /dist/index.js ` — trusted direct-binary call +- `npx paperclipai ` — the documented default. It passes an inert + `argv` value and runs on any machine. +- `node cli/node_modules/tsx/dist/cli.mjs cli/src/index.ts ` — + the safe form to run the local source from a monorepo checkout. It is the exact + command that the `pnpm paperclipai` script wraps, but it runs directly, so no + shell reads the argument. Use it when you must test your local `cli/src` + changes with a content-bearing argument. -Unsafe forms for a content-bearing argument: +Unsafe or broken forms: -- `pnpm paperclipai ` -- `pnpm run