From 427e0484059c33fdfd98263e2d6f4c3a27a7e156 Mon Sep 17 00:00:00 2001 From: Michael Nguyen <13559011+nguyenm7@users.noreply.github.com> Date: Thu, 1 Oct 2026 17:34:51 -0700 Subject: [PATCH] fix(company-skills): approve managed-checkout project dirs for local skill import (#10329) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work > - Company skills can be imported into a company from a local folder; `companySkillService.importFromSource` guards this with `assertLocalImportSourceAllowed`, which only permits import sources inside an approved set of roots (managed skills root + registered project-workspace cwds), realpath-resolved and `${root}${sep}`-anchored to prevent path traversal/escape > - A project can exist as a `managed_checkout` (server-managed clone) with **no registered `project_workspaces` row** — its `primaryWorkspace` is `null` and `workspaces` is `[]`, so its only real on-disk location is the server-derived `codebase.managedFolder` > - Because `configuredRoots` was built solely from the managed skills root and `workspaces[].cwd`, a `managed_checkout` project's `managedFolder` was never an approved root, so importing a skill from that project's own folder was rejected with `skill_workspace_boundary_denied` > - This PR adds each project's server-derived `codebase.managedFolder` to `configuredRoots` so in-place skill imports from managed-checkout projects are allowed > - The benefit is that managed-checkout projects can re-import their own skills in place, without weakening the traversal/escape protections (the new roots are server-derived and matched exactly like the existing ones) ## Linked Issues or Issue Description No public GitHub issue. Describing in-PR (bug): **What's wrong:** `assertLocalImportSourceAllowed` denies a legitimate local skill import (`skill_workspace_boundary_denied`) for any project that is a `managed_checkout` with no registered workspace row. **Repro:** Create/import a company skill from a `managed_checkout` project's own folder (`codebase.managedFolder/.agents/skills/`). The import is rejected even though the source is inside the project's server-managed checkout. **Expected:** The import from a managed-checkout project's own `managedFolder` subtree should be allowed, while paths outside that subtree remain denied. Related context (already merged): #9564 introduced the open-by-default skill policy / this import boundary. This PR does not change that boundary's matching logic — it only adds a missing, server-derived approved root. ## What Changed - `server/src/services/company-skills.ts`: add `...projectRows.map((project) => project.codebase.managedFolder)` to `configuredRoots` in `assertLocalImportSourceAllowed`. `managedFolder` is server-derived (`resolveManagedProjectWorkspaceDir(companyId, projectId)` → instance root + sanitized ids); it is `fs.realpath`-resolved and `${root}${sep}`-prefix matched exactly like every existing root. `managedFolder` is used rather than `effectiveLocalFolder` because the latter can fall through to a user-registered `localFolder`, which is already covered by the registered workspace cwds. - `server/src/__tests__/company-skill-import-boundary.test.ts`: add a regression test — a managed-checkout project's `managedFolder/` import is **allowed**, and a **prefix-adjacent sibling** (`managedFolder + "-evil"`) stays **denied**. ## Verification - `cd server && ./node_modules/.bin/vitest run src/__tests__/company-skill-import-boundary.test.ts` → **2/2 pass** (embedded-Postgres suite). Covers both the new allow case and the prefix-adjacent deny case, alongside the existing out-of-tree / symlink-escape / non-file-scheme rejections. ## Risks Low risk. The change only **adds** approved roots; it does not alter the realpath + `${root}${sep}`-anchored matching that closes traversal/prefix-adjacency escapes. The added roots are fully server-derived from the instance root + sanitized company/project ids (same trust class as the existing `resolveManagedSkillsRoot`) — no value derived from the import `source` argument reaches them. Sanitization (`[^a-zA-Z0-9._-]+ → -`) prevents separator/level injection, and the only user-influenced segment (repo name) is normalized via `new URL(...)`. The regression test's prefix-adjacent (`-evil`) case asserts the escape class stays closed. ## Model Used Claude Opus 4.8 (`claude-opus-4-8`), extended thinking + tool use (code execution, git). Implementation and security review were produced with Claude; this integration/PR was prepared with `claude-opus-4-8`. ## 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 - [ ] I have updated relevant documentation to reflect my changes (N/A — internal boundary fix, no user-facing docs) - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green (CI in progress) - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups (pending review) - [x] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Claude Opus 4.8 Co-authored-by: brandonburr --- .../company-skill-import-boundary.test.ts | 60 +++++++++++++++++++ server/src/services/company-skills.ts | 4 ++ 2 files changed, 64 insertions(+) diff --git a/server/src/__tests__/company-skill-import-boundary.test.ts b/server/src/__tests__/company-skill-import-boundary.test.ts index 9e096f1771..bc8ce57ca8 100644 --- a/server/src/__tests__/company-skill-import-boundary.test.ts +++ b/server/src/__tests__/company-skill-import-boundary.test.ts @@ -6,6 +6,7 @@ import { afterAll, afterEach, beforeAll, describe, expect, it } from "vitest"; import { companies, companySkills, createDb, projects, projectWorkspaces } from "@paperclipai/db"; import { getEmbeddedPostgresTestSupport, startEmbeddedPostgresTestDatabase } from "./helpers/embedded-postgres.js"; import { companySkillService } from "../services/company-skills.js"; +import { resolveManagedProjectWorkspaceDir } from "../home-paths.js"; const embeddedPostgresSupport = await getEmbeddedPostgresTestSupport(); const describeEmbeddedPostgres = embeddedPostgresSupport.supported ? describe : describe.skip; @@ -83,4 +84,63 @@ describeEmbeddedPostgres("company skill local import boundary", () => { details: { code: "skill_source_validation_failed" }, }); }); + + // Regression for LOOA-850: a `managed_checkout` project (no registered workspace row) exposes + // only a server-derived `codebase.managedFolder`; before the fix that folder was never an + // approved root, so every managed_checkout project's in-place skill re-import was denied. + it("allows managed_checkout project managedFolder imports and rejects prefix-adjacent siblings", async () => { + const companyId = randomUUID(); + const projectId = randomUUID(); + // Redirect the instance root at temp so the server-derived managedFolder is under our control. + const home = await fs.mkdtemp(path.join(os.tmpdir(), "paperclip-managed-home-")); + cleanupDirs.add(home); + const previousHome = process.env.PAPERCLIP_HOME; + process.env.PAPERCLIP_HOME = home; + try { + await db.insert(companies).values({ + id: companyId, + name: "Managed Co", + issuePrefix: `M${companyId.replaceAll("-", "").slice(0, 6).toUpperCase()}`, + requireBoardApprovalForNewAgents: false, + }); + // No projectWorkspaces row => origin "managed_checkout", primaryWorkspace null, + // managedFolder = resolveManagedProjectWorkspaceDir({ companyId, projectId }) (server-derived). + await db.insert(projects).values({ id: projectId, companyId, name: "Managed project" }); + + const managedFolder = resolveManagedProjectWorkspaceDir({ companyId, projectId }); + const allowedSkill = path.join(managedFolder, ".agents", "skills", "managed-allowed"); + await fs.mkdir(allowedSkill, { recursive: true }); + await fs.writeFile( + path.join(allowedSkill, "SKILL.md"), + "---\nname: managed-allowed\ndescription: managed-allowed\n---\n# Managed Allowed\n", + "utf8", + ); + + // Prefix-adjacent sibling: its path is a string-prefix match of managedFolder but it is NOT + // inside the subtree. Confirms the boundary uses `${root}${sep}`-anchored matching, not a bare + // startsWith, so the added root cannot be widened by an adjacent directory name. + const prefixAdjacent = `${managedFolder}-evil`; + await fs.mkdir(prefixAdjacent, { recursive: true }); + await fs.writeFile( + path.join(prefixAdjacent, "SKILL.md"), + "---\nname: managed-evil\ndescription: managed-evil\n---\n# Managed Evil\n", + "utf8", + ); + + const service = companySkillService(db); + await expect(service.importFromSource(companyId, allowedSkill)).resolves.toMatchObject({ + imported: [expect.objectContaining({ slug: "managed-allowed" })], + }); + await expect(service.importFromSource(companyId, prefixAdjacent)).rejects.toMatchObject({ + status: 403, + details: { code: "skill_workspace_boundary_denied" }, + }); + } finally { + if (previousHome === undefined) { + delete process.env.PAPERCLIP_HOME; + } else { + process.env.PAPERCLIP_HOME = previousHome; + } + } + }); }); diff --git a/server/src/services/company-skills.ts b/server/src/services/company-skills.ts index a88ae25e32..1392c69fec 100644 --- a/server/src/services/company-skills.ts +++ b/server/src/services/company-skills.ts @@ -2960,6 +2960,10 @@ export function companySkillService(db: Db) { const configuredRoots = [ resolveManagedSkillsRoot(companyId), ...projectRows.flatMap((project) => project.workspaces.map((workspace) => workspace.cwd)), + // Include server-derived managed-checkout dirs for projects with no registered workspace cwd. + // Uses managedFolder (not effectiveLocalFolder) to stay server-controlled; user-registered + // localFolder paths are already covered by workspaces[].cwd above. + ...projectRows.map((project) => project.codebase.managedFolder), ].filter((root): root is string => typeof root === "string" && root.trim().length > 0); const approvedRoots = (await Promise.all( configuredRoots.map((root) => fs.realpath(path.resolve(root)).catch(() => null)),