mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-09 16:35:27 +02:00
**Builds on** #10058 — managed detection keys off the *presence* of the `PAPERCLIP_MANAGED_CONFIG` env var that PR introduces, deliberately never its parsed body. **Summary.** Two layered hardenings of the plugin install route. (1) For **all** instances: `localPath` installs previously skipped the package-name validation entirely; the path is now null-byte-checked, resolved absolute, `realpath`'d (collapsing `..` traversal and symlinks), and required to be an existing directory before the loader ever sees it. (2) For instances running under a managed hosting control plane (detected by the *presence* of `PAPERCLIP_MANAGED_CONFIG` — deliberately never its body, so a corrupted document cannot widen the surface): registry/npm installs return 403, and `localPath` installs must canonicalize to inside the bundled plugin catalog root (`packages/plugins`) — a positive allowlist enforced in code at the route, independent of any flag value. Self-hosted behavior is otherwise unchanged. ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - The plugin system lets instance admins install plugins from a registry or from a local filesystem path, and plugin installation is code execution on the host > - The `localPath` branch of `POST /plugins/install` skips the validation applied to registry installs; the raw path reaches the plugin loader without canonicalization > - Separately, instances operated by a managed hosting control plane must constrain installs to the bundled plugin catalog, because there the host belongs to the operator, not the tenant > - This pull request canonicalizes and validates `localPath` for all instances, and adds a bundled-only install floor for managed instances > - The benefit is a smaller install-route attack surface everywhere, and a positive code-enforced allowlist where the operator owns the machine ## Linked Issues or Issue Description No public issue exists; `bug_report` template fields for the validation gap this PR fixes: - **What happened:** `POST /plugins/install` with `localPath` set bypasses the package-name validation entirely; the un-canonicalized path (relative segments, symlinks, no existence check) is handed straight to the plugin loader. - **Expected behavior:** path installs are validated like registry installs — null-byte-checked, resolved absolute, `realpath`'d, and required to be an existing directory before the loader sees them. - **Steps to reproduce:** as an instance admin, call `POST /plugins/install` with a `localPath` containing `..` traversal or a symlink pointing outside any plugin directory; observe the loader receives the raw path. Exploitability is bounded (the route already requires instance admin), so this is hardening of an admin-only surface rather than an open exploit. - **Version:** current `master`. The managed-instance bundled-only floor layered on top is new behavior (motivation: on managed hosting, arbitrary plugin install is arbitrary code execution on operator infrastructure), aligned with the in-progress "Cloud deployments" milestone in `ROADMAP.md`. ## What Changed - New `server/src/services/plugin-install-guard.ts` — three pure primitives: managed detection (presence-based), path canonicalization (null-byte check → absolute resolve → `realpath` → must be an existing directory), and segment-based containment in the bundled plugin catalog root. - Route enforcement in `server/src/routes/plugins.ts`: npm/registry installs return 403 on managed instances; `localPath` installs are canonicalized on every instance and, on managed instances, must land inside the bundled catalog root. - The plugin loader now receives the canonical path instead of the raw request string. ## Verification - 15 guard unit tests (`server/src/__tests__/plugin-install-guard.test.ts`): traversal, symlink escape, null byte, file-vs-directory, string-prefix sibling root. - 13 route security tests (`server/src/__tests__/plugin-install-route-security.test.ts`): 403 matrix on managed instances + self-hosted happy paths. - 36 existing plugin route authz tests green (`server/src/__tests__/plugin-routes-authz.test.ts`). - Server `tsc --noEmit` clean. ```bash cd server pnpm vitest run src/__tests__/plugin-install-guard.test.ts src/__tests__/plugin-install-route-security.test.ts src/__tests__/plugin-routes-authz.test.ts pnpm exec tsc --noEmit ``` ## Risks - Managed instances: npm/registry installs and out-of-catalog `localPath` installs now return 403 — intended new behavior, enforced in code rather than configuration. - All instances: `localPath` installs that previously pointed at nonexistent paths or non-directories now fail with 400 before reaching the loader (previously the loader failed later, less safely). Symlinked deployment layouts are handled by canonicalizing both sides of the containment check. - Self-hosted npm install path is unchanged. Low residual risk. ## Model Used Claude Fable 5 (`claude-fable-5`), extended thinking, agentic tool use; independently peer-reviewed by a second AI agent before push. ## 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 - [ ] All Paperclip CI gates are green - [ ] 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>
162 lines
6.6 KiB
TypeScript
162 lines
6.6 KiB
TypeScript
import { mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises";
|
|
import os from "node:os";
|
|
import path from "node:path";
|
|
import { afterEach, describe, expect, it } from "vitest";
|
|
import {
|
|
canonicalizeLocalPluginPath,
|
|
isCloudManagedInstance,
|
|
isWithinBundledPluginRoot,
|
|
} from "../services/plugin-install-guard.js";
|
|
|
|
describe("isCloudManagedInstance", () => {
|
|
it("returns false when PAPERCLIP_MANAGED_CONFIG is absent", () => {
|
|
expect(isCloudManagedInstance({})).toBe(false);
|
|
expect(isCloudManagedInstance({ OTHER_VAR: "x" })).toBe(false);
|
|
});
|
|
|
|
it("returns true when PAPERCLIP_MANAGED_CONFIG is set to a cloud document", () => {
|
|
const doc = JSON.stringify({ v: 1, mode: "cloud", catalogVersion: "1", features: {}, plugins: { autoInstall: [] } });
|
|
expect(isCloudManagedInstance({ PAPERCLIP_MANAGED_CONFIG: doc })).toBe(true);
|
|
});
|
|
|
|
it("fails closed: blank or corrupted documents still count as cloud-managed", () => {
|
|
expect(isCloudManagedInstance({ PAPERCLIP_MANAGED_CONFIG: "" })).toBe(true);
|
|
expect(isCloudManagedInstance({ PAPERCLIP_MANAGED_CONFIG: " " })).toBe(true);
|
|
expect(isCloudManagedInstance({ PAPERCLIP_MANAGED_CONFIG: "{not json" })).toBe(true);
|
|
expect(isCloudManagedInstance({ PAPERCLIP_MANAGED_CONFIG: JSON.stringify({ mode: "self-hosted" }) })).toBe(true);
|
|
});
|
|
});
|
|
|
|
describe("canonicalizeLocalPluginPath", () => {
|
|
const cleanupPaths = new Set<string>();
|
|
|
|
afterEach(async () => {
|
|
for (const cleanupPath of cleanupPaths) {
|
|
await rm(cleanupPath, { recursive: true, force: true });
|
|
}
|
|
cleanupPaths.clear();
|
|
});
|
|
|
|
async function makeTempDir(prefix: string): Promise<string> {
|
|
const dir = await mkdtemp(path.join(os.tmpdir(), prefix));
|
|
cleanupPaths.add(dir);
|
|
return dir;
|
|
}
|
|
|
|
it("accepts an existing absolute directory path", async () => {
|
|
const dir = await makeTempDir("guard-abs-");
|
|
const result = await canonicalizeLocalPluginPath(dir);
|
|
expect(result).toEqual({ ok: true, canonicalPath: await realCanonical(dir) });
|
|
});
|
|
|
|
it("collapses traversal segments to the canonical path", async () => {
|
|
const dir = await makeTempDir("guard-traversal-");
|
|
const nested = path.join(dir, "a", "b");
|
|
await mkdir(nested, { recursive: true });
|
|
const traversal = path.join(dir, "a", "..", "a", "b", "..", "b");
|
|
const result = await canonicalizeLocalPluginPath(traversal);
|
|
expect(result).toEqual({ ok: true, canonicalPath: await realCanonical(nested) });
|
|
});
|
|
|
|
it("resolves symlinks to their target", async () => {
|
|
const dir = await makeTempDir("guard-symlink-");
|
|
const target = path.join(dir, "target");
|
|
await mkdir(target, { recursive: true });
|
|
const link = path.join(dir, "link");
|
|
await symlink(target, link, "dir");
|
|
const result = await canonicalizeLocalPluginPath(link);
|
|
expect(result).toEqual({ ok: true, canonicalPath: await realCanonical(target) });
|
|
});
|
|
|
|
it("rejects paths containing a null byte", async () => {
|
|
const result = await canonicalizeLocalPluginPath("/tmp/foo\0bar");
|
|
expect(result.ok).toBe(false);
|
|
if (!result.ok) expect(result.reason).toContain("null byte");
|
|
});
|
|
|
|
it("rejects nonexistent paths", async () => {
|
|
const dir = await makeTempDir("guard-missing-");
|
|
const result = await canonicalizeLocalPluginPath(path.join(dir, "does-not-exist"));
|
|
expect(result.ok).toBe(false);
|
|
if (!result.ok) expect(result.reason).toContain("does not exist");
|
|
});
|
|
|
|
it("rejects paths that resolve to a file rather than a directory", async () => {
|
|
const dir = await makeTempDir("guard-file-");
|
|
const file = path.join(dir, "plugin.txt");
|
|
await writeFile(file, "not a directory", "utf8");
|
|
const result = await canonicalizeLocalPluginPath(file);
|
|
expect(result.ok).toBe(false);
|
|
if (!result.ok) expect(result.reason).toContain("not a directory");
|
|
});
|
|
});
|
|
|
|
describe("isWithinBundledPluginRoot", () => {
|
|
const cleanupPaths = new Set<string>();
|
|
|
|
afterEach(async () => {
|
|
for (const cleanupPath of cleanupPaths) {
|
|
await rm(cleanupPath, { recursive: true, force: true });
|
|
}
|
|
cleanupPaths.clear();
|
|
});
|
|
|
|
async function makeCatalogFixture(): Promise<{ root: string; inside: string; outside: string }> {
|
|
const base = await mkdtemp(path.join(os.tmpdir(), "guard-catalog-"));
|
|
cleanupPaths.add(base);
|
|
const root = path.join(base, "packages", "plugins");
|
|
const inside = path.join(root, "plugin-good");
|
|
const outside = path.join(base, "elsewhere");
|
|
await mkdir(inside, { recursive: true });
|
|
await mkdir(outside, { recursive: true });
|
|
return { root, inside, outside };
|
|
}
|
|
|
|
it("accepts a directory inside the catalog root", async () => {
|
|
const { root, inside } = await makeCatalogFixture();
|
|
expect(await isWithinBundledPluginRoot(await realCanonical(inside), root)).toBe(true);
|
|
});
|
|
|
|
it("rejects a directory outside the catalog root", async () => {
|
|
const { root, outside } = await makeCatalogFixture();
|
|
expect(await isWithinBundledPluginRoot(await realCanonical(outside), root)).toBe(false);
|
|
});
|
|
|
|
it("rejects the catalog root itself", async () => {
|
|
const { root } = await makeCatalogFixture();
|
|
expect(await isWithinBundledPluginRoot(await realCanonical(root), root)).toBe(false);
|
|
});
|
|
|
|
it("rejects a sibling directory whose name shares the root as a string prefix", async () => {
|
|
const { root } = await makeCatalogFixture();
|
|
const sibling = `${root}-evil`;
|
|
await mkdir(sibling, { recursive: true });
|
|
expect(await isWithinBundledPluginRoot(await realCanonical(sibling), root)).toBe(false);
|
|
});
|
|
|
|
it("rejects a symlink target that escapes the catalog root once canonicalized", async () => {
|
|
const { root, outside } = await makeCatalogFixture();
|
|
const link = path.join(root, "sneaky");
|
|
await symlink(outside, link, "dir");
|
|
// The guard contract is that callers canonicalize first; the symlink's
|
|
// real path lands outside the root and must be rejected.
|
|
const canonical = await canonicalizeLocalPluginPath(link);
|
|
expect(canonical.ok).toBe(true);
|
|
if (canonical.ok) {
|
|
expect(await isWithinBundledPluginRoot(canonical.canonicalPath, root)).toBe(false);
|
|
}
|
|
});
|
|
|
|
it("fails closed when the catalog root does not exist", async () => {
|
|
const { inside } = await makeCatalogFixture();
|
|
expect(await isWithinBundledPluginRoot(await realCanonical(inside), "/nonexistent/catalog/root")).toBe(false);
|
|
});
|
|
});
|
|
|
|
/** realpath through the same lens the guard uses (macOS /tmp is a symlink). */
|
|
async function realCanonical(target: string): Promise<string> {
|
|
const result = await canonicalizeLocalPluginPath(target);
|
|
if (!result.ok) throw new Error(`fixture path did not canonicalize: ${result.reason}`);
|
|
return result.canonicalPath;
|
|
}
|