diff --git a/lib/project-loop.js b/lib/project-loop.js index 1bc1a32..7f04910 100644 --- a/lib/project-loop.js +++ b/lib/project-loop.js @@ -6,6 +6,20 @@ var { createLoopRegistry } = require("./scheduler"); var store = require("./store"); var usersModule = require("./users"); +var LOOP_ID_RE = /^loop_[A-Za-z0-9_-]+$/; + +/** + * Validate a loop ID before using it as a path component. + * + * Returns true if the id passes the allowlist regex. + * Returns false for any value that could enable path traversal or injection. + * All handlers that build a loop directory path from a user-supplied value + * must route through this function before any path.join / path.resolve call. + */ +function validateLoopId(id) { + return LOOP_ID_RE.test(id || ""); +} + /** * Attach loop engine to a project context. * @@ -284,6 +298,13 @@ function attachLoop(ctx) { console.log("[loop-registry] Schedule triggered: " + record.name + " -> linked task " + loopFilesId); } + // Validate the loop ID before using it as a path component (path traversal guard). + // linkedTaskId flows from client-supplied sData.taskId via schedule_create. + if (!validateLoopId(loopFilesId)) { + console.error("[loop-registry] Invalid loop ID rejected: " + loopFilesId); + return; + } + // Verify the loop directory and PROMPT.md exist var recDir = path.join(cwd, ".claude", "loops", loopFilesId); try { @@ -1010,7 +1031,17 @@ function attachLoop(ctx) { if (msg.type === "loop_registry_files") { var recId = msg.id; - var lDir = path.join(cwd, ".claude", "loops", recId); + // Validate ID before using it as a path component (path traversal guard). + if (!validateLoopId(recId)) { + send({ type: "loop_registry_error", text: "invalid_loop_id" }); + return true; + } + var loopBase = path.resolve(cwd, ".claude", "loops"); + var lDir = path.resolve(loopBase, recId); + if (!lDir.startsWith(loopBase + path.sep)) { + send({ type: "loop_registry_error", text: "invalid_loop_id" }); + return true; + } var promptContent = ""; var judgeContent = ""; var loopSettings = null; @@ -1032,7 +1063,17 @@ function attachLoop(ctx) { if (msg.type === "loop_registry_save_files") { var recId2 = msg.id; - var lDir2 = path.join(cwd, ".claude", "loops", recId2); + // Validate ID before using it as a path component (path traversal guard). + if (!validateLoopId(recId2)) { + send({ type: "loop_registry_error", text: "invalid_loop_id" }); + return true; + } + var loopBase2 = path.resolve(cwd, ".claude", "loops"); + var lDir2 = path.resolve(loopBase2, recId2); + if (!lDir2.startsWith(loopBase2 + path.sep)) { + send({ type: "loop_registry_error", text: "invalid_loop_id" }); + return true; + } try { fs.mkdirSync(lDir2, { recursive: true }); if (msg.prompt !== undefined) { @@ -1348,4 +1389,4 @@ function attachLoop(ctx) { }; } -module.exports = { attachLoop: attachLoop }; +module.exports = { attachLoop: attachLoop, validateLoopId: validateLoopId }; diff --git a/lib/scheduler.js b/lib/scheduler.js index 0a734f6..ec0a800 100644 --- a/lib/scheduler.js +++ b/lib/scheduler.js @@ -12,6 +12,11 @@ var path = require("path"); var { CONFIG_DIR } = require("./config"); var { encodeCwd } = require("./utils"); +// NOTE: this regex mirrors validateLoopId() in project-loop.js. A circular +// import prevents sharing the function directly (project-loop requires this +// module). If the allowlist changes, update both. Tracked: lr-d049 nit. +var LOOP_ID_RE = /^loop_[A-Za-z0-9_-]+$/; + // --- Cron parser (5-field: minute hour day-of-month month day-of-week) --- function parseCronField(field, min, max) { @@ -297,8 +302,15 @@ function createLoopRegistry(opts) { // --- CRUD --- function register(data) { + // Validate caller-supplied id before storing it. + // Auto-generated IDs (the default) always pass the regex; this guard + // blocks client-supplied IDs that could later be used in path construction. + var suppliedId = data.id || null; + if (suppliedId !== null && !LOOP_ID_RE.test(suppliedId)) { + throw new Error("invalid_loop_id: " + suppliedId); + } var rec = { - id: data.id || ("loop_" + Date.now() + "_" + require("crypto").randomBytes(3).toString("hex")), + id: suppliedId || ("loop_" + Date.now() + "_" + require("crypto").randomBytes(3).toString("hex")), name: data.name || "Untitled", task: data.task || "", cron: data.cron || null, diff --git a/test/security.test.js b/test/security.test.js index 46d8924..ac5eccf 100644 --- a/test/security.test.js +++ b/test/security.test.js @@ -1324,6 +1324,158 @@ test("server-dm.js dm_typing handler: silently drops traversal dmKey", function assert.strictEqual(forEachClientCalled, false, "forEachClient must not be called for traversal key"); }); +// ============================================================ +// 18. Loop registry path traversal prevention (lr-d049) +// Tests the REAL production validateLoopId exported from project-loop.js +// and drives the real handleLoopMessage handler through a minimal ctx stub +// to confirm the guard fires before any filesystem access. +// ============================================================ + +var { validateLoopId: prodValidateLoopId, attachLoop } = require("../lib/project-loop"); + +// --- helpers --- + +// Build a minimal ctx stub sufficient for attachLoop. +// The invalid-ID guard fires before any sm/sdk/fs access, so most +// ctx fields can be no-ops for the invalid-ID tests. +function makeLoopCtx(cwd, sendSpy) { + var sessions = new Map(); + return { + cwd: cwd, + slug: "test", + sm: { + sessions: sessions, + createSession: function () { return { localId: "s1", history: [], loop: null }; }, + saveSessionFile: function () {}, + appendToSessionFile: function () {}, + switchSession: function () {}, + broadcastSessionList: function () {}, + deleteSessionQuiet: function () {}, + setResolveLoopInfo: function () {}, + }, + sdk: { startQuery: function () {} }, + send: sendSpy, + sendTo: function (_ws, msg) { sendSpy(msg); }, + sendToSession: function () {}, + pushModule: null, + notificationsModule: null, + getHubSchedules: function () { return []; }, + getAllProjectSessions: function () { return []; }, + getStatus: function () { return null; }, + getLinuxUserForSession: function () { return null; }, + onProcessingChanged: function () {}, + hydrateImageRefs: function () {}, + }; +} + +// --- validateLoopId unit tests (call the REAL production export) --- + +test("validateLoopId: rejects path traversal payload", function () { + assert.strictEqual(prodValidateLoopId("../../../etc/passwd"), false, + "../../../etc/passwd must be rejected"); +}); + +test("validateLoopId: rejects null/undefined/empty id", function () { + assert.strictEqual(prodValidateLoopId(null), false, "null must be rejected"); + assert.strictEqual(prodValidateLoopId(undefined), false, "undefined must be rejected"); + assert.strictEqual(prodValidateLoopId(""), false, "empty string must be rejected"); +}); + +test("validateLoopId: rejects id without loop_ prefix", function () { + assert.strictEqual(prodValidateLoopId("abc123"), false, "no prefix must be rejected"); + assert.strictEqual(prodValidateLoopId("loop"), false, "bare 'loop' must be rejected"); +}); + +test("validateLoopId: rejects id with dots or slashes", function () { + assert.strictEqual(prodValidateLoopId("loop_abc/../../../etc/shadow"), false, + "id with ../ must be rejected"); + assert.strictEqual(prodValidateLoopId("loop_abc/subdir"), false, + "id with embedded slash must be rejected"); + assert.strictEqual(prodValidateLoopId("loop_abc.evil"), false, + "id with dot must be rejected"); +}); + +test("validateLoopId: rejects null byte and shell metacharacters", function () { + assert.strictEqual(prodValidateLoopId("loop_abc\x00def"), false, "null byte must be rejected"); + assert.strictEqual(prodValidateLoopId("loop_abc;whoami"), false, "semicolon must be rejected"); + assert.strictEqual(prodValidateLoopId("loop_abc def"), false, "space must be rejected"); +}); + +test("validateLoopId: accepts well-formed loop IDs", function () { + var valid = [ + "loop_abc", + "loop_ABC123", + "loop_my-task", + "loop_task_2025", + "loop_A-b_C-d", + ]; + for (var i = 0; i < valid.length; i++) { + assert.strictEqual(prodValidateLoopId(valid[i]), true, + valid[i] + " should be accepted"); + } +}); + +// --- handleLoopMessage integration tests --- +// Drive the REAL handler dispatch path through a minimal ctx stub. +// For invalid IDs the guard fires before any fs operation; we assert the +// correct error is emitted and no exception propagates. + +test("loop_registry_files handler: sends loop_registry_error for invalid id via real handleLoopMessage", function () { + var tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "loop-h-test-")); + try { + var sent = []; + var ctx = makeLoopCtx(tmpDir, function (msg) { sent.push(msg); }); + var loop = attachLoop(ctx); + + var attackIds = [ + "../../../etc/passwd", + "loop_ok/../../../tmp/evil", + "loop_ok%2F..%2F..%2Fetc%2Fpasswd", + null, + "", + "abc123", + ]; + for (var i = 0; i < attackIds.length; i++) { + sent = []; + loop.handleLoopMessage({}, { type: "loop_registry_files", id: attackIds[i] }); + assert.ok(sent.length >= 1, "handler must emit a message for id=" + attackIds[i]); + assert.strictEqual(sent[0].type, "loop_registry_error", + "must emit loop_registry_error for id=" + attackIds[i]); + assert.strictEqual(sent[0].text, "invalid_loop_id", + "error text must be invalid_loop_id for id=" + attackIds[i]); + } + } finally { + fs.rmSync(tmpDir, { recursive: true }); + } +}); + +test("loop_registry_save_files handler: sends loop_registry_error for invalid id via real handleLoopMessage", function () { + var tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), "loop-h-test-")); + try { + var sent = []; + var ctx = makeLoopCtx(tmpDir, function (msg) { sent.push(msg); }); + var loop = attachLoop(ctx); + + var attackIds = [ + "../../../etc/passwd", + "loop_ok/../../../tmp/evil", + null, + "", + ]; + for (var i = 0; i < attackIds.length; i++) { + sent = []; + loop.handleLoopMessage({}, { type: "loop_registry_save_files", id: attackIds[i] }); + assert.ok(sent.length >= 1, "handler must emit a message for id=" + attackIds[i]); + assert.strictEqual(sent[0].type, "loop_registry_error", + "must emit loop_registry_error for id=" + attackIds[i]); + assert.strictEqual(sent[0].text, "invalid_loop_id", + "error text must be invalid_loop_id for id=" + attackIds[i]); + } + } finally { + fs.rmSync(tmpDir, { recursive: true }); + } +}); + test("server-dm.js dm_send handler: allows valid key and calls dm.sendMessage", function () { var { attachDm } = require("../lib/server-dm");