From a82fe9c6536e943bef07121cab706873b6833c56 Mon Sep 17 00:00:00 2001 From: Praveen Mittal Date: Fri, 31 Jul 2026 11:07:56 +0200 Subject: [PATCH] fix: verify automation re-export path stays inside the automations root exportAutomation() derived the re-export directory from a persisted automation's exportedPath (path.dirname(prior.exportedPath)) with no check that it still resolves inside automationsRoot(), then mkdirSync + writeFileSync(automation.json) into it. A relocated or tampered exportedPath in the persisted built-automation.json (which lives under the sessions root, a separate directory) causes automation.json to be written outside the automations root. The sibling Skill Builder already guards the equivalent re-install path via an isInside(root, dir) containment check (electron/skillbuilder/builder.ts); this defense-in-depth guard was missing for the Automation Builder's re-export path. Fixed by exporting isInside from skillbuilder/builder.ts and reusing it here: only reuse the prior directory when it's still inside the automations root, otherwise fall back to a fresh, non-colliding directory (matching the exact reuse-decision pattern already used by exportSkill()). Fixes #17. --- electron/automationbuilder/builder.test.ts | 136 +++++++++++++++++++++ electron/automationbuilder/builder.ts | 12 +- electron/skillbuilder/builder.ts | 2 +- package.json | 2 +- 4 files changed, 146 insertions(+), 6 deletions(-) create mode 100644 electron/automationbuilder/builder.test.ts diff --git a/electron/automationbuilder/builder.test.ts b/electron/automationbuilder/builder.test.ts new file mode 100644 index 0000000..da583e6 --- /dev/null +++ b/electron/automationbuilder/builder.test.ts @@ -0,0 +1,136 @@ +import assert from "node:assert/strict"; +import { mkdir, mkdtemp, readFile, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import path from "node:path"; +import test from "node:test"; + +import type { AutomationPlan, BuiltAutomation } from "../../common/automation"; +import { sessionDir } from "../recorder/session-store"; +import { AutomationBuilder } from "./builder"; + +function samplePlan(overrides: Partial = {}): AutomationPlan { + return { + architecture: "scout", + name: "daily-digest", + title: "Daily digest", + description: "Sends a daily digest.", + summary: "", + generalization: "", + trigger: { + type: "schedule", + schedule: { kind: "single", naturalLanguage: "", days: [], time: { hour: 9, minute: 0 } }, + condition: "", + }, + values: [], + steps: [{ label: "Send digest", prompt: "Send the daily digest email." }], + model: "", + skillNames: [], + ...overrides, + }; +} + +test("exportAutomation refuses to reuse a re-exported path outside the automations root", async () => { + const automationsRoot = await mkdtemp(path.join(tmpdir(), "skill-recorder-automations-")); + const sessionsRoot = await mkdtemp(path.join(tmpdir(), "skill-recorder-sessions-")); + const outsideRoot = await mkdtemp(path.join(tmpdir(), "skill-recorder-outside-")); + + const previousAutomationsDir = process.env.SKILL_RECORDER_AUTOMATIONS_DIR; + const previousSessionsDir = process.env.SKILL_RECORDER_SESSIONS_DIR; + process.env.SKILL_RECORDER_AUTOMATIONS_DIR = automationsRoot; + process.env.SKILL_RECORDER_SESSIONS_DIR = sessionsRoot; + + try { + const sessionId = "attack-session"; + + // Seed a persisted automation whose exportedPath was relocated outside the + // automations root (e.g. a tampered/relocated built-automation.json). + const maliciousExportedPath = path.join(outsideRoot, "automation.json"); + const priorAutomation: BuiltAutomation = { + version: 1, + sessionId, + kind: "automation", + architecture: "scout", + name: "daily-digest", + description: "", + triggerType: "schedule", + schedule: { kind: "single", naturalLanguage: "", days: [], time: { hour: 9, minute: 0 } }, + condition: "", + model: "", + steps: [{ label: "Send digest", prompt: "Send the daily digest email." }], + values: [], + plan: null, + createdAt: Date.now(), + exportedPath: maliciousExportedPath, + exportedAt: Date.now(), + }; + await mkdir(sessionDir(sessionId), { recursive: true }); + await writeFile(path.join(sessionDir(sessionId), "built-automation.json"), JSON.stringify(priorAutomation)); + + const builder = new AutomationBuilder(() => undefined); + const { path: exportedPath } = await builder.create(sessionId, samplePlan()); + + // The export must land inside the automations root, never inside the + // attacker-controlled outside directory the prior exportedPath pointed to. + const relative = path.relative(automationsRoot, exportedPath); + assert.ok( + relative !== "" && !relative.startsWith("..") && !path.isAbsolute(relative), + `expected export path inside ${automationsRoot}, got ${exportedPath}`, + ); + assert.notEqual(path.dirname(exportedPath), outsideRoot); + + // Nothing should have been written into the attacker-controlled directory. + await assert.rejects(readFile(maliciousExportedPath)); + } finally { + if (previousAutomationsDir === undefined) delete process.env.SKILL_RECORDER_AUTOMATIONS_DIR; + else process.env.SKILL_RECORDER_AUTOMATIONS_DIR = previousAutomationsDir; + if (previousSessionsDir === undefined) delete process.env.SKILL_RECORDER_SESSIONS_DIR; + else process.env.SKILL_RECORDER_SESSIONS_DIR = previousSessionsDir; + } +}); + +test("exportAutomation reuses a prior export path when it is inside the automations root", async () => { + const automationsRoot = await mkdtemp(path.join(tmpdir(), "skill-recorder-automations-")); + const sessionsRoot = await mkdtemp(path.join(tmpdir(), "skill-recorder-sessions-")); + + const previousAutomationsDir = process.env.SKILL_RECORDER_AUTOMATIONS_DIR; + const previousSessionsDir = process.env.SKILL_RECORDER_SESSIONS_DIR; + process.env.SKILL_RECORDER_AUTOMATIONS_DIR = automationsRoot; + process.env.SKILL_RECORDER_SESSIONS_DIR = sessionsRoot; + + try { + const sessionId = "reuse-session"; + const priorDir = path.join(automationsRoot, "daily-digest"); + const priorExportedPath = path.join(priorDir, "automation.json"); + + const priorAutomation: BuiltAutomation = { + version: 1, + sessionId, + kind: "automation", + architecture: "scout", + name: "daily-digest", + description: "", + triggerType: "schedule", + schedule: { kind: "single", naturalLanguage: "", days: [], time: { hour: 9, minute: 0 } }, + condition: "", + model: "", + steps: [{ label: "Send digest", prompt: "Send the daily digest email." }], + values: [], + plan: null, + createdAt: Date.now(), + exportedPath: priorExportedPath, + exportedAt: Date.now(), + }; + await mkdir(sessionDir(sessionId), { recursive: true }); + await writeFile(path.join(sessionDir(sessionId), "built-automation.json"), JSON.stringify(priorAutomation)); + + const builder = new AutomationBuilder(() => undefined); + const { path: exportedPath } = await builder.create(sessionId, samplePlan()); + + assert.equal(exportedPath, priorExportedPath); + } finally { + if (previousAutomationsDir === undefined) delete process.env.SKILL_RECORDER_AUTOMATIONS_DIR; + else process.env.SKILL_RECORDER_AUTOMATIONS_DIR = previousAutomationsDir; + if (previousSessionsDir === undefined) delete process.env.SKILL_RECORDER_SESSIONS_DIR; + else process.env.SKILL_RECORDER_SESSIONS_DIR = previousSessionsDir; + } +}); diff --git a/electron/automationbuilder/builder.ts b/electron/automationbuilder/builder.ts index 66a3d4d..7f538da 100644 --- a/electron/automationbuilder/builder.ts +++ b/electron/automationbuilder/builder.ts @@ -21,6 +21,7 @@ import { createReadTools } from "../builders/read-tools"; import { loadPersistedAnalysis } from "../describer/describer"; import { createLogger } from "../logger"; import { isValidSessionId, sessionDir } from "../recorder/session-store"; +import { isInside } from "../skillbuilder/builder"; import { AUTOMATION_BUILDER_INSTRUCTIONS } from "./instructions"; import { automationCatalogueFor } from "./scout-automation-catalog"; import { createAutomationBuilderTools } from "./tools"; @@ -219,10 +220,13 @@ export class AutomationBuilder extends AgentBuilder { const root = automationsRoot(); const name = slugifySkillName(automation.name); const prior = loadPersistedAutomation(automation.sessionId); - // Re-export to the same folder if this session already exported one; otherwise pick - // a fresh, non-colliding directory so we never clobber an unrelated automation. - let dir = prior?.exportedPath ? path.dirname(prior.exportedPath) : path.join(root, name); - if (!prior?.exportedPath && existsSync(dir)) { + const priorDir = prior?.exportedPath ? path.dirname(prior.exportedPath) : null; + // Re-export to the same folder only when it already lives under the automations root; + // a relocated/tampered `exportedPath` must not be reused here. Otherwise pick a fresh, + // non-colliding directory so we never clobber an unrelated automation. + const reuse = priorDir !== null && isInside(root, priorDir); + let dir = reuse ? priorDir : path.join(root, name); + if (!reuse && existsSync(dir)) { let n = 2; while (existsSync(path.join(root, `${name}-${n}`))) n++; dir = path.join(root, `${name}-${n}`); diff --git a/electron/skillbuilder/builder.ts b/electron/skillbuilder/builder.ts index 6041f0f..95658c0 100644 --- a/electron/skillbuilder/builder.ts +++ b/electron/skillbuilder/builder.ts @@ -53,7 +53,7 @@ function skillsRoot(): string { } /** True when `dir` is `root` or nested inside it (so we can safely re-use it). */ -function isInside(root: string, dir: string): boolean { +export function isInside(root: string, dir: string): boolean { const rel = path.relative(root, dir); return rel === "" || (!rel.startsWith("..") && !path.isAbsolute(rel)); } diff --git a/package.json b/package.json index dc77de2..c2dbab4 100644 --- a/package.json +++ b/package.json @@ -14,7 +14,7 @@ "build": "tsc --noEmit && vite build", "typecheck": "tsc --noEmit", "typecheck:evals": "tsc --noEmit -p evals/tsconfig.json", - "test": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs --test common/audio.test.ts common/microphone.test.ts common/narration.test.ts electron/recording-controls-bounds.test.ts electron/recording-privacy.test.ts electron/recorder/controller.test.ts electron/frames/extractor.test.ts electron/narration/audio-analysis.test.ts electron/narration/analyze-gate.test.ts electron/narration/transcribe.test.ts electron/narration/whisper.test.ts electron/sessions.test.ts electron/debug-bundle.test.ts scripts/compliance.test.mjs", + "test": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs --test common/audio.test.ts common/microphone.test.ts common/narration.test.ts electron/recording-controls-bounds.test.ts electron/recording-privacy.test.ts electron/recorder/controller.test.ts electron/frames/extractor.test.ts electron/narration/audio-analysis.test.ts electron/narration/analyze-gate.test.ts electron/narration/transcribe.test.ts electron/narration/whisper.test.ts electron/sessions.test.ts electron/debug-bundle.test.ts electron/automationbuilder/builder.test.ts scripts/compliance.test.mjs", "eval": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/run.ts", "eval:builder": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/builder/run.ts", "eval:skill": "node --experimental-transform-types --no-warnings --import ./evals/register.mjs evals/skillbuilder/run.ts",