From c4827f68e8164bc3a98382761182b27a812d1d62 Mon Sep 17 00:00:00 2001 From: jay79-boop Date: Mon, 3 Aug 2026 04:41:18 -0500 Subject: [PATCH] Fix path traversal in Doctor evidence-snippet capture readFileSnippet() joined the workspace root with an evidence item's `path` field via plain path.join() and read whatever that resolved to, with no check that the result stayed inside the workspace. Evidence paths aren't trusted input -- they come from Doctor findings, which are either AI-generated (openclaw doctor's own analysis output) or directly user-submitted via POST /api/doctor/import's `rawOutput` body. A finding with `evidence: [{ type: "path", path: "../../../../etc/passwd", startLine: 1, endLine: 50 }]` would have AlphaClaw read that arbitrary host file and store its contents in the doctor card's evidence.snippet, which then surfaces to the operator via the Setup UI (and potentially further, through whatever delivery channel a "request fix" call is configured to use). This bypasses the workspace sandbox boundary the Browse routes already enforce carefully elsewhere in this codebase (routes/browse/path-utils.js's resolveSafePath) -- Doctor's evidence capture just never applied the same guard. Reused resolveSafePath (the same traversal guard already proven correct for the file browser) so any evidence path that resolves outside workspaceRoot is silently skipped (no snippet attached) instead of read. Added a test that plants a "secret" file outside the workspace, submits a finding whose evidence path traverses out to it via /api/doctor/import, and asserts no snippet is captured for it -- while a normal in-workspace evidence path (with the same startLine/endLine shape) still works. --- lib/server/doctor/service.js | 14 ++++++- tests/server/doctor-service.test.js | 64 +++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/lib/server/doctor/service.js b/lib/server/doctor/service.js index 5119cd4e..d4cbf736 100644 --- a/lib/server/doctor/service.js +++ b/lib/server/doctor/service.js @@ -5,6 +5,7 @@ const { analyzeBootstrapContext, buildBootstrapTruncationCards, } = require("./bootstrap-context"); +const { resolveSafePath } = require("../routes/browse/path-utils"); const { buildDoctorPrompt } = require("./prompt"); const { hashDoctorFixToken } = require("./fix-completion"); const { normalizeDoctorResult } = require("./normalize"); @@ -77,7 +78,18 @@ const formatElapsedSince = (isoTime) => { const readFileSnippet = (rootDir, relativePath, startLine, endLine) => { try { - const fullPath = path.join(rootDir, String(relativePath || "")); + // Evidence paths come from Doctor findings (AI-generated, or user-submitted + // via /api/doctor/import) -- never trust them to stay inside rootDir without + // checking, or a `../../etc/passwd`-style path escapes the workspace sandbox. + const rootResolved = path.resolve(rootDir); + const resolved = resolveSafePath( + relativePath, + rootResolved, + `${rootResolved}${path.sep}`, + rootDir, + ); + if (!resolved.ok) return null; + const fullPath = resolved.absolutePath; const content = fs.readFileSync(fullPath, "utf-8"); const lines = content.split("\n"); const start = Math.max(0, (startLine || 1) - 1); diff --git a/tests/server/doctor-service.test.js b/tests/server/doctor-service.test.js index 38186c33..e4cacde4 100644 --- a/tests/server/doctor-service.test.js +++ b/tests/server/doctor-service.test.js @@ -664,4 +664,68 @@ describe("server/doctor-service", () => { ]), ); }); + + it("does not read files outside the workspace root for evidence with a path-traversal path", () => { + const workspaceRoot = fs.mkdtempSync(path.join(os.tmpdir(), "doctor-traversal-workspace-")); + const dbRoot = fs.mkdtempSync(path.join(os.tmpdir(), "doctor-traversal-db-")); + fs.writeFileSync(path.join(workspaceRoot, "AGENTS.md"), "# Guidance\nLine two\n", "utf8"); + + const secretsDir = fs.mkdtempSync(path.join(os.tmpdir(), "doctor-traversal-secret-")); + const secretPath = path.join(secretsDir, "secret.txt"); + fs.writeFileSync(secretPath, "TOP_SECRET_HOST_FILE_CONTENTS\n", "utf8"); + const traversalPath = path + .relative(workspaceRoot, secretPath) + .split(path.sep) + .join("/"); + + const doctorDb = loadManagedDoctorDb(); + doctorDb.initDoctorDb({ rootDir: dbRoot }); + const { createDoctorService } = loadDoctorService(); + const doctorService = createDoctorService({ + clawCmd: vi.fn(), + listDoctorRuns: doctorDb.listDoctorRuns, + listDoctorCards: doctorDb.listDoctorCards, + getInitialWorkspaceBaseline: doctorDb.getInitialWorkspaceBaseline, + setInitialWorkspaceBaseline: doctorDb.setInitialWorkspaceBaseline, + createDoctorRun: doctorDb.createDoctorRun, + completeDoctorRun: doctorDb.completeDoctorRun, + insertDoctorCards: doctorDb.insertDoctorCards, + getDoctorRun: doctorDb.getDoctorRun, + getDoctorCardsByRunId: doctorDb.getDoctorCardsByRunId, + getDoctorCard: doctorDb.getDoctorCard, + updateDoctorCardStatus: doctorDb.updateDoctorCardStatus, + workspaceRoot, + managedRoot: workspaceRoot, + }); + + const imported = doctorService.importDoctorResult({ + rawOutput: JSON.stringify({ + summary: "Findings with a traversal evidence path", + cards: [ + { + priority: "P1", + category: "security", + title: "Traversal attempt", + summary: "Evidence path escapes the workspace", + recommendation: "n/a", + evidence: [ + { type: "path", path: traversalPath, startLine: 1, endLine: 1 }, + { type: "path", path: "AGENTS.md", startLine: 1, endLine: 1 }, + ], + targetPaths: ["AGENTS.md"], + fixPrompt: "n/a", + status: "open", + }, + ], + }), + }); + + const [card] = doctorDb.getDoctorCardsByRunId(imported.runId); + const [traversalEvidence, inWorkspaceEvidence] = card.evidence; + + // The out-of-workspace path must not be read at all. + expect(traversalEvidence.snippet).toBeUndefined(); + // A legitimate in-workspace path with startLine still gets its snippet. + expect(inWorkspaceEvidence.snippet?.text).toBe("# Guidance"); + }); });