mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-30 05:16:50 +01:00
## 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 <sub> --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 `<host>` 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 `<host>` 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 <noreply@paperclip.ing>
97 lines
3.1 KiB
TypeScript
97 lines
3.1 KiB
TypeScript
import type { Request, RequestHandler } from "express";
|
|
|
|
function isLoopbackHostname(hostname: string): boolean {
|
|
const normalized = hostname.trim().toLowerCase();
|
|
return normalized === "localhost" || normalized === "127.0.0.1" || normalized === "::1";
|
|
}
|
|
|
|
function extractHostname(req: Request): string | null {
|
|
const forwardedHost = req.header("x-forwarded-host")?.split(",")[0]?.trim();
|
|
const hostHeader = req.header("host")?.trim();
|
|
const raw = forwardedHost || hostHeader;
|
|
if (!raw) return null;
|
|
|
|
try {
|
|
return new URL(`http://${raw}`).hostname.trim().toLowerCase();
|
|
} catch {
|
|
return raw.trim().toLowerCase();
|
|
}
|
|
}
|
|
|
|
function normalizeAllowedHostnames(values: string[]): string[] {
|
|
const unique = new Set<string>();
|
|
for (const value of values) {
|
|
const trimmed = value.trim().toLowerCase();
|
|
if (!trimmed) continue;
|
|
unique.add(trimmed);
|
|
}
|
|
return Array.from(unique);
|
|
}
|
|
|
|
export function resolvePrivateHostnameAllowSet(opts: { allowedHostnames: string[]; bindHost: string }): Set<string> {
|
|
const configuredAllow = normalizeAllowedHostnames(opts.allowedHostnames);
|
|
const bindHost = opts.bindHost.trim().toLowerCase();
|
|
const allowSet = new Set<string>(configuredAllow);
|
|
|
|
if (bindHost && bindHost !== "0.0.0.0") {
|
|
allowSet.add(bindHost);
|
|
}
|
|
allowSet.add("localhost");
|
|
allowSet.add("127.0.0.1");
|
|
allowSet.add("::1");
|
|
return allowSet;
|
|
}
|
|
|
|
// The hostname comes from the request Host header, so an unauthenticated
|
|
// requester controls it. Never put that value into the guidance command. An
|
|
// operator or an agent can paste the guidance into a shell, and that outer
|
|
// shell evaluates a backtick, `$( )`, or `$NAME` span in the host before any
|
|
// CLI receives argv. A direct-exec form such as `npx` does not stop the
|
|
// outer shell. Emit a static `<host>` placeholder and do not echo the raw request
|
|
// value. The operator supplies the real hostname.
|
|
const BLOCKED_HOSTNAME_MESSAGE =
|
|
"This hostname is not allowed for this Paperclip instance. " +
|
|
"If you want to allow a hostname, run npx paperclipai allowed-hostname <host>.";
|
|
|
|
export function privateHostnameGuard(opts: {
|
|
enabled: boolean;
|
|
allowedHostnames: string[];
|
|
bindHost: string;
|
|
}): RequestHandler {
|
|
if (!opts.enabled) {
|
|
return (_req, _res, next) => next();
|
|
}
|
|
|
|
const allowSet = resolvePrivateHostnameAllowSet({
|
|
allowedHostnames: opts.allowedHostnames,
|
|
bindHost: opts.bindHost,
|
|
});
|
|
|
|
return (req, res, next) => {
|
|
const hostname = extractHostname(req);
|
|
const wantsJson = req.path.startsWith("/api") || req.accepts(["json", "html", "text"]) === "json";
|
|
|
|
if (!hostname) {
|
|
const error = "Missing Host header. If you want to allow a hostname, run npx paperclipai allowed-hostname <host>.";
|
|
if (wantsJson) {
|
|
res.status(403).json({ error });
|
|
} else {
|
|
res.status(403).type("text/plain").send(error);
|
|
}
|
|
return;
|
|
}
|
|
|
|
if (isLoopbackHostname(hostname) || allowSet.has(hostname)) {
|
|
next();
|
|
return;
|
|
}
|
|
|
|
const error = BLOCKED_HOSTNAME_MESSAGE;
|
|
if (wantsJson) {
|
|
res.status(403).json({ error });
|
|
} else {
|
|
res.status(403).type("text/plain").send(error);
|
|
}
|
|
};
|
|
}
|