diff --git a/docs/architecture/agent-reporting-governance-plugin.md b/docs/architecture/agent-reporting-governance-plugin.md index 9e40862..5a0cf7e 100644 --- a/docs/architecture/agent-reporting-governance-plugin.md +++ b/docs/architecture/agent-reporting-governance-plugin.md @@ -227,6 +227,7 @@ Architectural meaning: - storage layer owns loading/package-artifact interpretation - runtime binding can be derived from the artifact rather than hardcoded entirely in docs - tests prove the artifact resolves into concrete script and runtime-artifact paths +- `artifact_roots` enforcement is now two-layered for this slice: lexical boundary rejection plus realpath-level symlink escape rejection This is intentionally still a **minimal verifiable slice**, not the full deployment system. It proves the package boundary can own profile artifacts and bind them into runtime execution inputs. diff --git a/plugins/reporting-governance/README.md b/plugins/reporting-governance/README.md index 8dce7c8..e673ac6 100644 --- a/plugins/reporting-governance/README.md +++ b/plugins/reporting-governance/README.md @@ -158,8 +158,9 @@ What this slice does: 2. loader resolves that artifact from package-local path 3. validator fail-closes minimal boundary drift on `kind`, `apiVersion`, `spec.bindings.entrypoint`, `scripts`, `artifact_roots`, and `spec.package.pluginVersion` 4. binding contract translates profile-declared script/artifact roots into concrete repo/runtime paths -5. adapter runtime binding can be instantiated from that contract in tests -6. orchestrator adapter can now bootstrap from package profile artifact input directly +5. validator rejects `artifact_roots` absolute paths, lexical escapes, and symlink escapes that resolve outside repo realpath boundary +6. adapter runtime binding can be instantiated from that contract in tests +7. orchestrator adapter can now bootstrap from package profile artifact input directly What this slice does **not** claim yet: diff --git a/plugins/reporting-governance/src/storage/profile-artifact.mjs b/plugins/reporting-governance/src/storage/profile-artifact.mjs index b49a696..5298656 100644 --- a/plugins/reporting-governance/src/storage/profile-artifact.mjs +++ b/plugins/reporting-governance/src/storage/profile-artifact.mjs @@ -24,7 +24,36 @@ function assertObjectRecord(value, label) { return value; } -function assertRelativePathWithinRoot(relativePath, label, { root }) { +function assertPathWithinRealRoot(candidatePath, label, { root, allowMissingLeaf = false }) { + const realRoot = fs.realpathSync(root); + let realCandidate; + + if (allowMissingLeaf && !fs.existsSync(candidatePath)) { + let cursor = path.dirname(candidatePath); + while (cursor !== root && !fs.existsSync(cursor)) { + cursor = path.dirname(cursor); + } + + if (!fs.existsSync(cursor)) { + realCandidate = realRoot; + } else { + realCandidate = path.resolve(fs.realpathSync(cursor), path.relative(cursor, candidatePath)); + } + } else { + realCandidate = fs.realpathSync(candidatePath); + } + + const relativeToRealRoot = path.relative(realRoot, realCandidate); + if ( + relativeToRealRoot === '..' + || relativeToRealRoot.startsWith(`..${path.sep}`) + || path.isAbsolute(relativeToRealRoot) + ) { + throw new Error(`${label} must stay within repo root: symlink resolution escapes realpath boundary`); + } +} + +function assertRelativePathWithinRoot(relativePath, label, { root, allowMissingLeaf = false }) { const normalizedPath = assertNonEmptyString(relativePath, label); if (path.isAbsolute(normalizedPath)) { throw new Error(`${label} must stay within repo root: absolute paths are not allowed`); @@ -40,6 +69,7 @@ function assertRelativePathWithinRoot(relativePath, label, { root }) { throw new Error(`${label} must stay within repo root: path escapes root boundary`); } + assertPathWithinRealRoot(resolvedPath, label, { root, allowMissingLeaf }); return normalizedPath; } @@ -70,7 +100,7 @@ export function validateDeploymentProfileArtifact(artifact, { repoRootOverride } assertRelativePathWithinRoot(relativePath, `deployment profile artifact spec.bindings.scripts.${key}`, { root }); } for (const [key, relativePath] of Object.entries(artifactRoots)) { - assertRelativePathWithinRoot(relativePath, `deployment profile artifact spec.bindings.artifact_roots.${key}`, { root }); + assertRelativePathWithinRoot(relativePath, `deployment profile artifact spec.bindings.artifact_roots.${key}`, { root, allowMissingLeaf: true }); } return artifact; diff --git a/plugins/reporting-governance/test/profile-artifact.test.mjs b/plugins/reporting-governance/test/profile-artifact.test.mjs index e65f0fc..6b9c5ec 100644 --- a/plugins/reporting-governance/test/profile-artifact.test.mjs +++ b/plugins/reporting-governance/test/profile-artifact.test.mjs @@ -1,6 +1,7 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import fs from 'node:fs'; +import os from 'node:os'; import path from 'node:path'; import { @@ -13,6 +14,23 @@ import { createRuntimeBinding } from '../src/adapters/index.mjs'; const packageRoot = path.resolve(import.meta.dirname, '..'); const repoRoot = path.resolve(packageRoot, '..', '..'); +function createArtifact(overrides = {}) { + return { + kind: 'DeploymentProfileArtifact', + apiVersion: 'reporting-governance/v1alpha1', + spec: { + package: { pluginVersion: '0.1.0-mainline' }, + bindings: { + runtime: 'openclaw', + entrypoint: 'scripts/watchdog_auto_notify_orchestrator.mjs', + scripts: { watchdog: 'scripts/long_task_watchdog.mjs' }, + artifact_roots: { queueItems: 'state/operator-notify-queue' }, + }, + }, + ...overrides, + }; +} + test('deployment profile artifact loads from package profiles and preserves compatibility envelope metadata', () => { const { artifactPath, artifact } = loadDeploymentProfileArtifact({ profileId: 'strict-manager-mode' }); @@ -57,9 +75,7 @@ test('deployment profile artifact validation fails closed on boundary drift', () ); assert.throws( - () => validateDeploymentProfileArtifact({ - kind: 'DeploymentProfileArtifact', - apiVersion: 'reporting-governance/v1alpha1', + () => validateDeploymentProfileArtifact(createArtifact({ spec: { package: { pluginVersion: '0.1.0-mainline' }, bindings: { @@ -68,14 +84,12 @@ test('deployment profile artifact validation fails closed on boundary drift', () artifact_roots: { queueItems: 'state/operator-notify-queue' }, }, }, - }), + })), /spec\.bindings\.entrypoint must be a non-empty string/ ); assert.throws( - () => validateDeploymentProfileArtifact({ - kind: 'DeploymentProfileArtifact', - apiVersion: 'reporting-governance/v1alpha1', + () => validateDeploymentProfileArtifact(createArtifact({ spec: { package: { pluginVersion: '' }, bindings: { @@ -84,14 +98,12 @@ test('deployment profile artifact validation fails closed on boundary drift', () artifact_roots: { queueItems: 'state/operator-notify-queue' }, }, }, - }), + })), /spec\.package\.pluginVersion must be a non-empty string/ ); assert.throws( - () => validateDeploymentProfileArtifact({ - kind: 'DeploymentProfileArtifact', - apiVersion: 'reporting-governance/v1alpha1', + () => validateDeploymentProfileArtifact(createArtifact({ spec: { package: { pluginVersion: '0.1.0-mainline' }, bindings: { @@ -100,16 +112,14 @@ test('deployment profile artifact validation fails closed on boundary drift', () artifact_roots: { queueItems: 'state/operator-notify-queue' }, }, }, - }), + })), /spec\.bindings\.scripts must be an object record/ ); }); test('deployment profile artifact validation rejects absolute binding paths', () => { assert.throws( - () => validateDeploymentProfileArtifact({ - kind: 'DeploymentProfileArtifact', - apiVersion: 'reporting-governance/v1alpha1', + () => validateDeploymentProfileArtifact(createArtifact({ spec: { package: { pluginVersion: '0.1.0-mainline' }, bindings: { @@ -118,14 +128,12 @@ test('deployment profile artifact validation rejects absolute binding paths', () artifact_roots: { queueItems: 'state/operator-notify-queue' }, }, }, - }), + })), /spec\.bindings\.entrypoint must stay within repo root: absolute paths are not allowed/ ); assert.throws( - () => validateDeploymentProfileArtifact({ - kind: 'DeploymentProfileArtifact', - apiVersion: 'reporting-governance/v1alpha1', + () => validateDeploymentProfileArtifact(createArtifact({ spec: { package: { pluginVersion: '0.1.0-mainline' }, bindings: { @@ -134,16 +142,28 @@ test('deployment profile artifact validation rejects absolute binding paths', () artifact_roots: { queueItems: 'state/operator-notify-queue' }, }, }, - }), + })), /spec\.bindings\.scripts\.watchdog must stay within repo root: absolute paths are not allowed/ ); + + assert.throws( + () => validateDeploymentProfileArtifact(createArtifact({ + spec: { + package: { pluginVersion: '0.1.0-mainline' }, + bindings: { + entrypoint: 'scripts/watchdog_auto_notify_orchestrator.mjs', + scripts: { watchdog: 'scripts/long_task_watchdog.mjs' }, + artifact_roots: { queueItems: '/abs/path' }, + }, + }, + })), + /spec\.bindings\.artifact_roots\.queueItems must stay within repo root: absolute paths are not allowed/ + ); }); test('deployment profile artifact validation rejects escape paths after resolution', () => { assert.throws( - () => validateDeploymentProfileArtifact({ - kind: 'DeploymentProfileArtifact', - apiVersion: 'reporting-governance/v1alpha1', + () => validateDeploymentProfileArtifact(createArtifact({ spec: { package: { pluginVersion: '0.1.0-mainline' }, bindings: { @@ -152,15 +172,13 @@ test('deployment profile artifact validation rejects escape paths after resoluti artifact_roots: { queueItems: 'state/operator-notify-queue' }, }, }, - }), + })), /spec\.bindings\.entrypoint must stay within repo root: path escapes root boundary/ ); assert.throws( () => createDeploymentBindingContract({ - artifact: { - kind: 'DeploymentProfileArtifact', - apiVersion: 'reporting-governance/v1alpha1', + artifact: createArtifact({ spec: { package: { pluginVersion: '0.1.0-mainline' }, bindings: { @@ -169,18 +187,30 @@ test('deployment profile artifact validation rejects escape paths after resoluti artifact_roots: { queueItems: 'state/operator-notify-queue' }, }, }, - }, + }), repoRootOverride: repoRoot, }), /spec\.bindings\.scripts\.watchdog must stay within repo root: path escapes root boundary/ ); + + assert.throws( + () => validateDeploymentProfileArtifact(createArtifact({ + spec: { + package: { pluginVersion: '0.1.0-mainline' }, + bindings: { + entrypoint: 'scripts/watchdog_auto_notify_orchestrator.mjs', + scripts: { watchdog: 'scripts/long_task_watchdog.mjs' }, + artifact_roots: { queueItems: '../escape' }, + }, + }, + })), + /spec\.bindings\.artifact_roots\.queueItems must stay within repo root: path escapes root boundary/ + ); }); test('deployment binding contract allows normalized in-root paths that contain dot segments', () => { const binding = createDeploymentBindingContract({ - artifact: { - kind: 'DeploymentProfileArtifact', - apiVersion: 'reporting-governance/v1alpha1', + artifact: createArtifact({ spec: { package: { pluginVersion: '0.1.0-mainline' }, bindings: { @@ -194,7 +224,7 @@ test('deployment binding contract allows normalized in-root paths that contain d }, }, }, - }, + }), repoRootOverride: repoRoot, }); @@ -202,3 +232,30 @@ test('deployment binding contract allows normalized in-root paths that contain d assert.equal(binding.scripts.watchdog, path.resolve(repoRoot, 'scripts/long_task_watchdog.mjs')); assert.equal(binding.artifactRoots.queueItems, path.resolve(repoRoot, 'state/operator-notify-queue')); }); + +test('deployment profile artifact validation rejects artifact_roots symlink escape after realpath resolution', async (t) => { + const sandbox = fs.mkdtempSync(path.join(os.tmpdir(), 'reporting-governance-profile-artifact-')); + t.after(() => fs.rmSync(sandbox, { recursive: true, force: true })); + + const fakeRepoRoot = path.join(sandbox, 'repo'); + const outsideRoot = path.join(sandbox, 'outside'); + fs.mkdirSync(fakeRepoRoot, { recursive: true }); + fs.mkdirSync(outsideRoot, { recursive: true }); + fs.symlinkSync(outsideRoot, path.join(fakeRepoRoot, 'state-link'), 'dir'); + fs.writeFileSync(path.join(fakeRepoRoot, 'entry.mjs'), 'export default true;\n'); + fs.writeFileSync(path.join(fakeRepoRoot, 'watchdog.mjs'), 'export default true;\n'); + + assert.throws( + () => validateDeploymentProfileArtifact(createArtifact({ + spec: { + package: { pluginVersion: '0.1.0-mainline' }, + bindings: { + entrypoint: 'entry.mjs', + scripts: { watchdog: 'watchdog.mjs' }, + artifact_roots: { queueItems: 'state-link/queue' }, + }, + }, + }), { repoRootOverride: fakeRepoRoot }), + /spec\.bindings\.artifact_roots\.queueItems must stay within repo root: symlink resolution escapes realpath boundary/ + ); +});