From caa130ef1f3632ba6948c5edbf0735038f544d7d Mon Sep 17 00:00:00 2001 From: Tom Vaillant Date: Fri, 17 Jul 2026 17:42:01 +0200 Subject: [PATCH] Protect skill projections from foreign collisions --- .changeset/safe-skill-projections.md | 5 ++ packages/server/src/skill-projection.test.ts | 32 +++++++--- packages/server/src/skill-projection.ts | 62 +++++++++++++++++--- 3 files changed, 83 insertions(+), 16 deletions(-) create mode 100644 .changeset/safe-skill-projections.md diff --git a/.changeset/safe-skill-projections.md b/.changeset/safe-skill-projections.md new file mode 100644 index 000000000..8a655ffcb --- /dev/null +++ b/.changeset/safe-skill-projections.md @@ -0,0 +1,5 @@ +--- +'@inkeep/open-knowledge': patch +--- + +Protect skill projections from accidental replacement. Installing a project skill now refuses a conflicting editor-skill path before writing any target, while existing links to the same source remain idempotent. diff --git a/packages/server/src/skill-projection.test.ts b/packages/server/src/skill-projection.test.ts index 16b18fa07..92448a154 100644 --- a/packages/server/src/skill-projection.test.ts +++ b/packages/server/src/skill-projection.test.ts @@ -149,19 +149,37 @@ describe('projectSkill / reverseProjectSkill', () => { } }); - test('install is authoritative — replaces a legacy real-dir copy with a symlink', () => { + test('refuses a foreign real-dir collision without mutating it', () => { const dir = makeSkill('s', '# v1'); const dest = skillHostDir(root, 'claude', 's') as string; - // Simulate a legacy copy-install: a real directory at the host path. + // A legacy copy may contain user edits; projection cannot prove ownership. mkdirSync(dest, { recursive: true }); writeFileSync(join(dest, 'stale.md'), 'leftover', 'utf-8'); expect(lstatSync(dest).isSymbolicLink()).toBe(false); - projectSkill(dir, 's', root, ['claude']); - // Now a symlink to the source; the stale real-dir contents are gone. - expect(lstatSync(dest).isSymbolicLink()).toBe(true); - expect(existsSync(join(dest, 'stale.md'))).toBe(false); - expect(existsSync(join(dest, 'SKILL.md'))).toBe(true); + expect(() => projectSkill(dir, 's', root, ['claude'])).toThrow('occupied by a non-link'); + expect(lstatSync(dest).isSymbolicLink()).toBe(false); + expect(existsSync(join(dest, 'stale.md'))).toBe(true); + }); + + test('preflights every target before creating any projection', () => { + const dir = makeSkill('atomic', '# v1'); + const foreign = skillHostDir(root, 'cursor', 'atomic') as string; + mkdirSync(foreign, { recursive: true }); + writeFileSync(join(foreign, 'keep.md'), 'user-owned', 'utf-8'); + + expect(() => projectSkill(dir, 'atomic', root, ['claude', 'cursor'])).toThrow( + 'occupied by a non-link', + ); + expect(existsSync(skillHostDir(root, 'claude', 'atomic') as string)).toBe(false); + expect(existsSync(join(foreign, 'keep.md'))).toBe(true); + }); + + test('keeps an exact existing projection idempotently', () => { + const dir = makeSkill('stable', '# v1'); + expect(projectSkill(dir, 'stable', root, ['codex'])).toEqual(['codex']); + expect(projectSkill(dir, 'stable', root, ['codex'])).toEqual(['codex']); + expect(lstatSync(skillHostDir(root, 'codex', 'stable') as string).isSymbolicLink()).toBe(true); }); test('skillHostDir returns null for claude-desktop', () => { diff --git a/packages/server/src/skill-projection.ts b/packages/server/src/skill-projection.ts index 41ac0ac4c..fddda15c1 100644 --- a/packages/server/src/skill-projection.ts +++ b/packages/server/src/skill-projection.ts @@ -26,7 +26,15 @@ * in lock-step. */ -import { existsSync, lstatSync, readdirSync, readFileSync, realpathSync, statSync } from 'node:fs'; +import { + existsSync, + lstatSync, + readdirSync, + readFileSync, + readlinkSync, + realpathSync, + statSync, +} from 'node:fs'; import { dirname, isAbsolute, join, relative, resolve } from 'node:path'; import { containsXmlTag, @@ -228,11 +236,36 @@ function skillLinkTarget(cwd: string, hostRoot: string, skillDir: string): strin return insideProject ? relative(hostRoot, absSkill) : absSkill; } +/** + * Inspect one projection destination without following a link. Only an absent + * path or an exact existing link to this source is safe: a real directory, + * file, dangling link, or link to another source belongs to a user or another + * installer and must never be replaced implicitly. + */ +function projectionStatus(dest: string, skillDir: string): 'absent' | 'owned' { + try { + const stat = lstatSync(dest); + if (!stat.isSymbolicLink()) { + throw new Error(`skill projection destination is occupied by a non-link: ${dest}`); + } + const target = resolve(dirname(dest), readlinkSync(dest)); + if (target !== resolve(skillDir)) { + throw new Error(`skill projection destination points at a different source: ${dest}`); + } + return 'owned'; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + if (code === 'ENOENT') return 'absent'; + throw error; + } +} + /** * Install a skill source dir into each target editor's host dir by SYMLINK. - * Removes any existing entry first (authoritative replace — a stale/broken - * link or a legacy real-dir copy is dropped before the link is made). Returns - * the editor ids actually written (skipping editors with no skill surface). + * Every destination is preflighted before the first write. An existing exact + * projection is idempotent; every foreign destination is refused untouched, + * so callers never get a partial multi-editor install or a silent overwrite. + * Returns the editor ids with a valid projection (created or already owned). */ export function projectSkill( skillDir: string, @@ -240,17 +273,28 @@ export function projectSkill( cwd: string, targets: readonly EditorId[], ): EditorId[] { - const written: EditorId[] = []; + const candidates: Array<{ + editor: EditorId; + dest: string; + hostRoot: string; + status: 'absent' | 'owned'; + }> = []; for (const editor of targets) { const dest = skillHostDir(cwd, editor, name); if (dest === null) continue; const hostRoot = dirname(dest); // Refuse to write through a host root that symlink-escapes the project. if (hostSkillsRootEscapes(cwd, hostRoot)) continue; - tracedRmSync(dest, { recursive: true, force: true }); - tracedMkdirSync(hostRoot, { recursive: true }); - tracedSymlinkSync(skillLinkTarget(cwd, hostRoot, skillDir), dest, 'dir'); - written.push(editor); + candidates.push({ editor, dest, hostRoot, status: projectionStatus(dest, skillDir) }); + } + + const written: EditorId[] = []; + for (const candidate of candidates) { + if (candidate.status === 'absent') { + tracedMkdirSync(candidate.hostRoot, { recursive: true }); + tracedSymlinkSync(skillLinkTarget(cwd, candidate.hostRoot, skillDir), candidate.dest, 'dir'); + } + written.push(candidate.editor); } return written; }