diff --git a/.github/workflows/windows-recovery.yml b/.github/workflows/windows-recovery.yml index dc9184186f..22a2f2d443 100644 --- a/.github/workflows/windows-recovery.yml +++ b/.github/workflows/windows-recovery.yml @@ -158,8 +158,8 @@ jobs: $exitCode = $LASTEXITCODE if ($exitCode -ne 0) { exit $exitCode } $output = Get-Content "$env:RUNNER_TEMP/skill-catalog.tap" - if ($output -notcontains '# tests 90' -or $output -notcontains '# pass 90' -or $output -notcontains '# skipped 0') { - Write-Error 'Skill catalog gate did not run exactly 90 passing Windows tests' + if ($output -notcontains '# tests 91' -or $output -notcontains '# pass 91' -or $output -notcontains '# skipped 0') { + Write-Error 'Skill catalog gate did not run exactly 91 passing Windows tests' exit 1 } diff --git a/packages/runtime-host/src/__tests__/skill-catalog-repository.test.ts b/packages/runtime-host/src/__tests__/skill-catalog-repository.test.ts index 919cfbc3d9..d2e6367dd3 100644 --- a/packages/runtime-host/src/__tests__/skill-catalog-repository.test.ts +++ b/packages/runtime-host/src/__tests__/skill-catalog-repository.test.ts @@ -21,7 +21,7 @@ import { RuntimeHostProtocolError } from '../protocol/errors.js'; import assert from 'node:assert/strict'; import { execFile } from 'node:child_process'; import { createHash } from 'node:crypto'; -import { chmod, mkdir, mkdtemp, readFile, rm, writeFile } from 'node:fs/promises'; +import { chmod, mkdir, mkdtemp, readFile, rm, symlink, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; import { afterEach, test } from 'node:test'; @@ -903,6 +903,78 @@ test('non-force managed update preserves a local edit made after its snapshot', assert.equal(await readFile(installedPath, 'utf8'), localEdit); }); +test('managed update blocks a symlink redirected outside its discovery root after scanning', async () => { + const fixture = await createFixture(); + const sourceId = 'managed-symlink-race'; + const installedContent = skillBody('Managed Symlink Race', 'installed'); + const updateContent = skillBody('Managed Symlink Race', 'source update'); + const outsideContent = skillBody('Managed Symlink Race', 'outside replacement'); + await createSkill(fixture.sources, sourceId, updateContent); + + const containedSkill = await createSkill( + join(fixture.root, 'linked-skill-sources'), + sourceId, + installedContent, + ); + await mkdir(join(containedSkill, '.maka', 'baseline'), { recursive: true }); + await writeFile( + join(containedSkill, 'skill.lock.json'), + `${JSON.stringify( + createManagedSkillLock(sourceId, sha256(installedContent), sha256(installedContent)), + null, + 2, + )}\n`, + ); + await writeFile(join(containedSkill, '.maka', 'baseline', 'SKILL.md'), installedContent); + + const skillsDirectory = join(fixture.root, 'skills'); + const linkedSkill = join(skillsDirectory, sourceId); + await mkdir(skillsDirectory, { recursive: true }); + await symlink(containedSkill, linkedSkill, 'dir'); + + const outsideSkill = await createSkill( + await tempDirectory('maka-skill-symlink-race-outside-'), + sourceId, + outsideContent, + ); + await mkdir(join(outsideSkill, '.maka', 'baseline'), { recursive: true }); + await writeFile( + join(outsideSkill, 'skill.lock.json'), + `${JSON.stringify( + createManagedSkillLock(sourceId, sha256(outsideContent), sha256(outsideContent)), + null, + 2, + )}\n`, + ); + await writeFile(join(outsideSkill, '.maka', 'baseline', 'SKILL.md'), outsideContent); + + let redirectAfterScan = false; + const repository = fixture.repository(undefined, { + beforeManagedInstalledArtifactsRead: async () => { + if (!redirectAfterScan) return; + redirectAfterScan = false; + await rm(linkedSkill); + await symlink(outsideSkill, linkedSkill, 'dir'); + }, + }); + const snapshot = await start(repository, fixture.project, 'governance'); + + redirectAfterScan = true; + const result = await repository.mutate({ + expectedRevision: snapshot.revision, + mutation: { + kind: 'update_managed', + ref: `workspace:legacy:${sourceId}`, + force: false, + expectedCurrentSha256: null, + expectedSourceSha256: null, + }, + }); + + assert.deepEqual(result, { kind: 'rejected', reason: 'metadata_error' }); + assert.equal(await readFile(join(outsideSkill, 'SKILL.md'), 'utf8'), outsideContent); +}); + test('revision covers exact managed lock and baseline bytes and rejects post-snapshot edits', async () => { const fixture = await createFixture(); const sourceId = 'managed-artifact-race'; diff --git a/packages/runtime-host/src/server/skill-catalog-repository.ts b/packages/runtime-host/src/server/skill-catalog-repository.ts index e4f85f3e96..65a1b7fd91 100644 --- a/packages/runtime-host/src/server/skill-catalog-repository.ts +++ b/packages/runtime-host/src/server/skill-catalog-repository.ts @@ -403,7 +403,10 @@ export class SkillCatalogRepository { if (!validateSkillMetadata(source.content).valid) { return { kind: 'rejected', reason: 'source_invalid' }; } - const current = await readContainedArtifact(fact.skill.path, join(fact.skill.path, SKILL_FILE)); + const current = await readContainedArtifact( + fact.skill.discoveryRoot, + join(fact.skill.path, SKILL_FILE), + ); if (current.status !== 'available') { return { kind: 'rejected', reason: 'metadata_error' }; } @@ -1012,7 +1015,10 @@ async function governanceForSkill( return { governance: missingSkillLockStatus(), baselineAvailable: false }; } const lockPath = join(skill.path, LOCK_FILE); - const baselineRead = await readContainedArtifact(skill.path, join(skill.path, BASELINE_FILE)); + const baselineRead = await readContainedArtifact( + skill.discoveryRoot, + join(skill.path, BASELINE_FILE), + ); const baselineAvailable = baselineRead.status === 'available'; const baselineSha256 = baselineRead.status === 'unavailable' ? undefined : baselineRead.sha256; const lockStat = await lstat(lockPath).catch((error: NodeJS.ErrnoException) => @@ -1039,7 +1045,7 @@ async function governanceForSkill( ...(baselineSha256 === undefined ? {} : { baselineSha256 }), }; } - const lockRead = await readContainedArtifact(skill.path, lockPath); + const lockRead = await readContainedArtifact(skill.discoveryRoot, lockPath); if (lockRead.status === 'unavailable') { return { governance: invalidSkillLockStatus('invalid_json', 'Skill lock could not be read safely.'), @@ -1681,9 +1687,9 @@ async function readManagedArtifacts(skill: ScannedSkill): Promise<{ baselineSha256: string; } | null> { const [skillFile, lock, baseline] = await Promise.all([ - readContainedArtifact(skill.path, join(skill.path, SKILL_FILE)), - readContainedArtifact(skill.path, join(skill.path, LOCK_FILE)), - readContainedArtifact(skill.path, join(skill.path, BASELINE_FILE)), + readContainedArtifact(skill.discoveryRoot, join(skill.path, SKILL_FILE)), + readContainedArtifact(skill.discoveryRoot, join(skill.path, LOCK_FILE)), + readContainedArtifact(skill.discoveryRoot, join(skill.path, BASELINE_FILE)), ]); if ( skillFile.status !== 'available' || diff --git a/packages/runtime/src/__tests__/skills.test.ts b/packages/runtime/src/__tests__/skills.test.ts index 2e92539dc6..8410e473d8 100644 --- a/packages/runtime/src/__tests__/skills.test.ts +++ b/packages/runtime/src/__tests__/skills.test.ts @@ -520,6 +520,106 @@ Open local targets carefully.`, }); }); + it('discovers contained symlinked skill directories using the link identity', async () => { + await withWorkspace(async (workspaceRoot) => { + const skillsDir = join(workspaceRoot, 'skills'); + const sourceDir = join(workspaceRoot, 'linked-skill-sources'); + const linkedSkill = join(skillsDir, 'linked-alias'); + await mkdir(skillsDir, { recursive: true }); + await writeSkillInDirectory( + sourceDir, + 'source-name', + 'Linked Skill', + 'A contained symlinked skill.', + ); + await symlink(join(sourceDir, 'source-name'), linkedSkill, 'dir'); + + const scan = await scanSkillsWithDiagnostics(workspaceRoot); + + assert.deepEqual( + scan.skills.map((skill) => ({ id: skill.id, ref: skill.ref, path: skill.path })), + [ + { + id: 'linked-alias', + ref: 'workspace:legacy:linked-alias', + path: linkedSkill, + }, + ], + ); + assert.deepEqual(scan.discoveryDiagnostics, []); + }); + }); + + it('diagnoses unusable symlinked skill directories without hiding valid skills', async () => { + await withWorkspace(async (workspaceRoot) => { + const outside = await mkdtemp(join(tmpdir(), 'maka-skill-entry-outside-')); + const skillsDir = join(workspaceRoot, 'skills'); + const containedFile = join(workspaceRoot, 'contained-file'); + const containedFileLink = join(skillsDir, 'contained-file-link'); + const cyclicSkill = join(skillsDir, 'cyclic-link'); + const danglingSkill = join(skillsDir, 'dangling-link'); + const notDirectorySkill = join(skillsDir, 'not-directory-link'); + const escapingSkill = join(skillsDir, 'escaping-link'); + try { + await writeSkill( + workspaceRoot, + 'valid', + `--- +name: Valid +description: A valid local skill. +--- +# Valid`, + ); + await writeSkillInDirectory(outside, 'source-name', 'Outside', 'An escaping skill.'); + await writeFile(containedFile, 'not a directory', 'utf8'); + await symlink(containedFile, containedFileLink, 'file'); + await symlink(cyclicSkill, cyclicSkill, 'dir'); + await symlink(join(workspaceRoot, 'missing-target'), danglingSkill, 'dir'); + await symlink(join(containedFile, 'child'), notDirectorySkill, 'dir'); + await symlink(join(outside, 'source-name'), escapingSkill, 'dir'); + + const scan = await scanSkillsWithDiagnostics(workspaceRoot); + + assert.deepEqual( + scan.skills.map((skill) => skill.id), + ['valid'], + ); + assert.deepEqual(scan.discoveryDiagnostics, [ + { + path: cyclicSkill, + scope: 'workspace', + source: 'legacy', + precedence: 0, + reason: 'read_failed', + }, + { + path: danglingSkill, + scope: 'workspace', + source: 'legacy', + precedence: 0, + reason: 'blocked_path', + }, + { + path: escapingSkill, + scope: 'workspace', + source: 'legacy', + precedence: 0, + reason: 'blocked_path', + }, + { + path: notDirectorySkill, + scope: 'workspace', + source: 'legacy', + precedence: 0, + reason: 'blocked_path', + }, + ]); + } finally { + await rm(outside, { recursive: true, force: true }); + } + }); + }); + it('readSkillRuntimeState does not read through a symlinked state file', async () => { await withWorkspace(async (workspaceRoot) => { const outside = await mkdtemp(join(tmpdir(), 'maka-skill-state-file-outside-')); diff --git a/packages/runtime/src/skills-discovery.ts b/packages/runtime/src/skills-discovery.ts index d971fc6c1e..f2ba5b73cd 100644 --- a/packages/runtime/src/skills-discovery.ts +++ b/packages/runtime/src/skills-discovery.ts @@ -19,7 +19,7 @@ import { createHash } from 'node:crypto'; import { homedir } from 'node:os'; -import { lstat, readdir, realpath } from 'node:fs/promises'; +import { lstat, readdir, realpath, stat } from 'node:fs/promises'; import { join } from 'node:path'; import { isPathInside, readContainedRegularFile } from './path-containment.js'; import { validateSkillMetadata } from './skills-metadata.js'; @@ -109,9 +109,10 @@ export interface ScannedSkill extends RuntimeSkillDefinition { contentSha256: string; /** * The containment root this skill was discovered under (e.g. workspace root, - * home dir). Used to compute `relativePath` in `loadSkillInstructions` so - * legacy callers see `skills//SKILL.md` while multi-path callers see - * the actual subpath. + * home dir). Remains the authority for later contained reads even when + * `path` is a mutable symlink, and is used to compute `relativePath` in + * `loadSkillInstructions` so legacy callers see `skills//SKILL.md` + * while multi-path callers see the actual subpath. */ discoveryRoot: string; } @@ -361,10 +362,11 @@ function normalizeSkillSource(source: SkillSource): { * `SKILL.md` is parsed. Metadata validation errors exclude only the malformed * skill and are returned as structured diagnostics. * - * The directory itself must be a real directory (not a symlink) and its - * realpath must be contained within the realpath of its parent directory. - * This prevents ancestor-level symlinks (e.g. `repo/.agents -> /outside`) - * from escaping the expected boundary. + * The discovery directory itself must be a real directory (not a symlink) and + * its realpath must be contained within its configured containment root. This + * prevents ancestor-level symlinks (e.g. `repo/.agents -> /outside`) from + * escaping the expected boundary. Immediate symlinked skill entries are + * followed only when their targets remain inside that same containment root. */ async function scanSkillDir( discovery: SkillDiscoveryEntry, @@ -382,26 +384,29 @@ async function scanSkillDir( discoveryDiagnostics, runtimeState, }); - const sourceDiagnostic = ( + const discoveryDiagnostic = ( + path: string, reason: SkillDiscoveryDiagnostic['reason'], ): SkillDiscoveryDiagnostic => ({ - path: dir, + path, scope, source, precedence, reason, }); let entries: import('node:fs').Dirent[]; + let rootReal: string; try { const dirStat = await lstat(dir); if (!dirStat.isDirectory() || dirStat.isSymbolicLink()) { - return empty([sourceDiagnostic('blocked_path')]); + return empty([discoveryDiagnostic(dir, 'blocked_path')]); } // Verify the resolved directory has not escaped its containment root via // an ancestor symlink (e.g. `repo/.agents -> /outside`). - const [rootReal, dirReal] = await Promise.all([realpath(containmentRoot), realpath(dir)]); + const [resolvedRoot, dirReal] = await Promise.all([realpath(containmentRoot), realpath(dir)]); + rootReal = resolvedRoot; if (!isPathInside(rootReal, dirReal)) { - return empty([sourceDiagnostic('blocked_path')]); + return empty([discoveryDiagnostic(dir, 'blocked_path')]); } entries = await readdir(dir, { withFileTypes: true }); entries.sort((a, b) => a.name.localeCompare(b.name)); @@ -409,18 +414,42 @@ async function scanSkillDir( // An absent optional discovery root is normal. A configured path that // exists but cannot be inspected is not: expose it to governance clients. if ((error as NodeJS.ErrnoException).code === 'ENOENT') return empty(); - return empty([sourceDiagnostic('read_failed')]); + return empty([discoveryDiagnostic(dir, 'read_failed')]); } const out: ScannedSkill[] = []; const rejected: RejectedSkillDefinition[] = []; const diagnostics: SkillScanDiagnostic[] = []; + const discoveryDiagnostics: SkillDiscoveryDiagnostic[] = []; for (const dirEntry of entries) { - if (!dirEntry.isDirectory()) continue; const skillPath = join(dir, dirEntry.name); - const skillFile = join(skillPath, 'SKILL.md'); + let skillReadRoot = skillPath; + let skillDirectory = skillPath; + if (!dirEntry.isDirectory()) { + if (!dirEntry.isSymbolicLink()) continue; + try { + const skillReal = await realpath(skillPath); + if (!isPathInside(rootReal, skillReal)) { + discoveryDiagnostics.push(discoveryDiagnostic(skillPath, 'blocked_path')); + continue; + } + if (!(await stat(skillReal)).isDirectory()) continue; + skillReadRoot = rootReal; + skillDirectory = skillReal; + } catch (error) { + const code = (error as NodeJS.ErrnoException).code; + discoveryDiagnostics.push( + discoveryDiagnostic( + skillPath, + code === 'ENOENT' || code === 'ENOTDIR' ? 'blocked_path' : 'read_failed', + ), + ); + continue; + } + } + const skillFile = join(skillDirectory, 'SKILL.md'); try { - const read = await readContainedRegularFile(skillPath, skillFile); + const read = await readContainedRegularFile(skillReadRoot, skillFile); if (!read.ok) continue; const bytes = read.bytes; const text = bytes.toString('utf8'); @@ -485,7 +514,7 @@ async function scanSkillDir( inventory: out, rejected, diagnostics, - discoveryDiagnostics: [], + discoveryDiagnostics, runtimeState, }; } diff --git a/scripts/ci-test-plan.test.mjs b/scripts/ci-test-plan.test.mjs index 2ee7e3b524..dce0577a6a 100644 --- a/scripts/ci-test-plan.test.mjs +++ b/scripts/ci-test-plan.test.mjs @@ -630,8 +630,8 @@ test('Windows recovery executes the complete Skill catalog suite', () => { assert.match(recovery, /skill-catalog-repository\.test\.js/u); assert.match(recovery, /skill-catalog-transaction\.test\.js/u); assert.match(recovery, /skill-catalog-two-client-uds\.test\.js/u); - assert.match(recovery, /# tests 90/u); - assert.match(recovery, /# pass 90/u); + assert.match(recovery, /# tests 91/u); + assert.match(recovery, /# pass 91/u); assert.match(recovery, /# skipped 0/u); });