mirror of
https://github.com/paperclipai/paperclip.git
synced 2026-10-30 05:16:50 +01:00
fix(skills): reject local imports of package caches
Protect private package provenance across directory, file, ancestor, and symlink imports. Keep installed-skill forks as the path that preserves project access. Co-Authored-By: Paperclip <noreply@paperclip.ing>
This commit is contained in:
1 parent
867c00673b
commit
a2e597d800
5 files changed
+85
-1
No files matched your search
@@ -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
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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),
|
||||
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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" });
|
||||
}
|
||||
}
|
||||
Reference in new issue
Block a user