mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-06 19:35:04 +02:00
fix(company-skills): approve managed-checkout project dirs for local skill import (#10329)
## 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/<skill>`).
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/<skill>`
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 <noreply@anthropic.com>
Co-authored-by: brandonburr <brandonburr@gmail.com>
This commit is contained in:
2 files changed
+64
No files matched your search
@@ -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;
|
||||
}
|
||||
}
|
||||
});
|
||||
});
|
||||
@@ -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)),
|
||||
|
||||
Reference in new issue
Block a user