diff --git a/doc/skill-package-storage.md b/doc/skill-package-storage.md index c5057926cac..dd2565425e1 100644 --- a/doc/skill-package-storage.md +++ b/doc/skill-package-storage.md @@ -131,6 +131,9 @@ pending delete/existence requests; incomplete cleanup retains the receipt for re verification budget per fill. This is a soft limit during a storage outage: cleanup preserves the last recoverable copy. Version projections retain the existing pin-aware cleanup. Abandoned package staging directories receive a one-hour crash grace. +- Internal package caches, their files, and containing directories cannot be + imported as local skill sources. Fork the installed skill to retain its original + access restrictions. Each entrypoint still has a self-contained runtime tree, and remote staging can duplicate those trees. This change does not introduce a new runner transport format diff --git a/server/src/__tests__/project-skill-sources.test.ts b/server/src/__tests__/project-skill-sources.test.ts index 20838033e00..ed8e6c4f4d9 100644 --- a/server/src/__tests__/project-skill-sources.test.ts +++ b/server/src/__tests__/project-skill-sources.test.ts @@ -8,7 +8,7 @@ import os from 'node:os'; import path from 'node:path'; import { afterAll, beforeAll, describe, expect, it, vi } from 'vitest'; import { and, eq, sql } from 'drizzle-orm'; -import { activityLog, agents, authUsers, companies, companyMemberships, companySkillSources, companySkillImportBudgets, companySkills, createDb, projects, +import { activityLog, agents, authUsers, companies, companyMemberships, companySkillSources, companySkillImportBudgets, companySkills, companySkillVersions, createDb, projects, projectWorkspaces, projectSkillSources, projectAccessMembers } from '@paperclipai/db'; import { startEmbeddedPostgresTestDatabase, getEmbeddedPostgresTestSupport } from './helpers/embedded-postgres.js'; import { githubFixture } from './helpers/github-skills.js'; @@ -16,6 +16,7 @@ import { enqueueProjectSkillSource, detachProjectSkillSource, projectSkillSetupI import { adoptProjectSkillRepository, projectSkillRepositories, skillSourceSyncService } from '../services/skill-source-sync.js'; import { skillSourceService, type SkillSourceContext } from '../services/skill-sources.js'; import { companySkillService } from '../services/company-skills.js'; +import { skillPackagePaths } from '../services/skill-package-store.js'; import { projectService } from '../services/projects.js'; import { skillAccessService, withSkillActor } from '../services/skill-access.js'; import type { AuthorizationActor } from '../services/authorization.js'; @@ -446,6 +447,7 @@ describe.skipIf(!support.supported)('automatic project skill sources', () => { const source = await skillSourceService(db).detail(companyId, binding!.sourceId!); const skillId = source.entries[0]!.skillId!; let privateSnapshotPath = ''; + let privatePackagePath = ''; const privateDerivatives: string[] = []; for (const user of ['alice', 'bob']) { await db.insert(authUsers).values({ id: user, name: user, email: `${user}@example.test`, createdAt: new Date(), updatedAt: new Date() }); @@ -459,6 +461,16 @@ describe.skipIf(!support.supported)('automatic project skill sources', () => { const [runtime] = await companySkillService(db).listRuntimeSkillEntries(companyId, { onlyKeys: new Set([skill.key]) }); privateSnapshotPath = runtime!.source; await expect(companySkillService(db).importFromSource(companyId, runtime!.source)).rejects.toThrow(/Project-derived skill/); + const [version] = await db.select({ packageId: companySkillVersions.packageSnapshotId }) + .from(companySkillVersions).where(eq(companySkillVersions.id, skill.currentVersionId!)); + expect(version?.packageId).toBeTruthy(); + privatePackagePath = skillPackagePaths(companyId, version!.packageId!).directory; + expect((await fs.stat(privatePackagePath)).isDirectory()).toBe(true); + for (const candidate of [privatePackagePath, path.join(privatePackagePath, 'private'), path.dirname(privatePackagePath)]) { + await expect(companySkillService(db).importFromSource(companyId, candidate)).rejects.toMatchObject({ + status: 422, details: { code: 'skill_package_cache_import_denied' }, + }); + } const fork = await companySkillService(db).forkSkill(companyId, skillId, { slug: 'private-copy' }); expect(fork.skill.accessSourceId).not.toBe(source.id); expect(fork.skill.accessSourceId).toBeTruthy(); @@ -475,6 +487,9 @@ describe.skipIf(!support.supported)('automatic project skill sources', () => { await expect(skillSourceService(db).branches(companyId, source.id, {}, { ...context, read: () => branchRead })).rejects.toThrow(/not found/); expect(branchRead).not.toHaveBeenCalled(); await expect(companySkillService(db).importFromSource(companyId, privateSnapshotPath)).rejects.toThrow(/not found/); + await expect(companySkillService(db).importFromSource(companyId, privatePackagePath)).rejects.toMatchObject({ + status: 422, details: { code: 'skill_package_cache_import_denied' }, + }); expect(await companySkillService(db).getById(companyId, skillId)).toBeNull(); expect(await companySkillService(db).readFile(companyId, skillId, 'SKILL.md')).toBeNull(); expect((await skillSourceService(db).list(companyId)).some(item => item.id === source.id)).toBe(false); diff --git a/server/src/services/company-skills.ts b/server/src/services/company-skills.ts index ad3820d3627..9ec8933b5ef 100644 --- a/server/src/services/company-skills.ts +++ b/server/src/services/company-skills.ts @@ -7,6 +7,7 @@ import { ensureSkillCacheDirectory, touchSkillVersionCache, withSkillVersionCach import { measureRunPhase } from "./execution-preparation-timing.js"; import { verifyExistingBundledSkillRelease } from "./bundled-skill-release-validation.js"; import { retainProjectSkillAccess, skillAccessService } from "./skill-access.js"; +import { assertLocalImportOutsideSkillPackageCache } from "./skill-local-import-boundary.js"; import type { AuthorizationActor } from "./authorization.js"; import { updateAgentConfigurationInTransaction } from "./agent-configuration-transaction.js"; import { logger } from "../middleware/logger.js"; @@ -2992,6 +2993,7 @@ export function companySkillService(db: Db, options: { actor?: AuthorizationActo code: "skill_source_validation_failed", }); } + await assertLocalImportOutsideSkillPackageCache(sourceRealPath, resolveManagedSkillsRoot(companyId)); const projectRows = await projects.list(companyId); const configuredRoots = [ resolveManagedSkillsRoot(companyId), diff --git a/server/src/services/skill-local-import-boundary.test.ts b/server/src/services/skill-local-import-boundary.test.ts new file mode 100644 index 00000000000..e049452fc07 --- /dev/null +++ b/server/src/services/skill-local-import-boundary.test.ts @@ -0,0 +1,42 @@ +import { promises as fs } from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { assertLocalImportOutsideSkillPackageCache } from "./skill-local-import-boundary.js"; + +describe("local import package-cache boundary", () => { + let root: string; + let managed: string; + let cache: string; + beforeEach(async () => { + root = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), "skill-import-boundary-"))); + managed = path.join(root, "skills", "company"); + cache = path.join(managed, "__packages__"); + await fs.mkdir(path.join(cache, "snapshot", "private"), { recursive: true }); + await fs.writeFile(path.join(cache, "snapshot", "private", "SKILL.md"), "Private instructions"); + }); + afterEach(() => fs.rm(root, { recursive: true, force: true })); + + it.each(["__packages__", "__packages__/snapshot", "__packages__/snapshot/private", + "__packages__/snapshot/private/SKILL.md", ".", ".."])('rejects cache or ancestor source "%s"', async source => { + await expect(assertLocalImportOutsideSkillPackageCache(await fs.realpath(path.join(managed, source)), managed)) + .rejects.toMatchObject({ status: 422, details: { code: "skill_package_cache_import_denied" } }); + }); + + it("rejects a symlink alias of the cache", async () => { + const alias = path.join(managed, "alias"); + await fs.symlink(path.join(cache, "snapshot"), alias, "dir"); + await expect(assertLocalImportOutsideSkillPackageCache(await fs.realpath(alias), managed)).rejects.toThrow("package caches"); + }); + + it("rejects recursive ancestors before a concurrent fill can create the cache", async () => { + await fs.rm(cache, { recursive: true }); + await expect(assertLocalImportOutsideSkillPackageCache(managed, managed)).rejects.toThrow("package caches"); + }); + + it.each(["ordinary-skill", "__packages__-backup"])("allows unrelated sibling %s", async name => { + const source = path.join(managed, name); + await fs.mkdir(source); + await expect(assertLocalImportOutsideSkillPackageCache(await fs.realpath(source), managed)).resolves.toBeUndefined(); + }); +}); diff --git a/server/src/services/skill-local-import-boundary.ts b/server/src/services/skill-local-import-boundary.ts new file mode 100644 index 00000000000..ab5ad833486 --- /dev/null +++ b/server/src/services/skill-local-import-boundary.ts @@ -0,0 +1,22 @@ +import { promises as fs } from "node:fs"; +import path from "node:path"; +import { unprocessable } from "../errors.js"; + +function contains(root: string, candidate: string) { + const relative = path.relative(root, candidate); + return relative === "" || (!path.isAbsolute(relative) && relative !== ".." && !relative.startsWith(`..${path.sep}`)); +} + +/** Package caches have package-level provenance, not local-import authority. */ +export async function assertLocalImportOutsideSkillPackageCache(sourceRealPath: string, managedSkillsRoot: string) { + const managedRoot = await fs.realpath(managedSkillsRoot).catch(() => null); + if (!managedRoot) return; + const reservedRoot = path.join(managedRoot, "__packages__"); + const canonicalRoot = await fs.realpath(reservedRoot).catch(() => reservedRoot); + // Also reject ancestors before a cache exists: a concurrent fill must not + // become visible to a recursive import after its boundary check. + if ([reservedRoot, canonicalRoot].some(root => contains(root, sourceRealPath) || contains(sourceRealPath, root))) { + throw unprocessable("Skill package caches cannot be imported as local sources. Fork the installed skill to retain its access restrictions.", + { code: "skill_package_cache_import_denied" }); + } +}