Fix path traversal in Doctor evidence-snippet capture - #115
Open
jay79-boop wants to merge 1 commit into
Open
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
readFileSnippet()inlib/server/doctor/service.jsjoined the workspace root with an evidence item'spathfield via plainpath.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 viaPOST /api/doctor/import'srawOutputbody.normalizeEvidenceItem(lib/server/doctor/normalize.js) only trims thepathstring; it never validates that it's relative-and-contained.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'sresolveSafePath) — Doctor's evidence capture just never applied the same guard.Since Doctor findings can originate from the AI's own analysis of the repo, this isn't only reachable via the authenticated
/api/doctor/importendpoint — a prompt-injected or otherwise manipulatedopenclaw doctorrun could in principle produce a finding whose evidence path walks out of the workspace.Fix
Reused
resolveSafePath— the same traversal guard already proven correct for the file browser — so any evidence path that resolves outsideworkspaceRootis silently skipped (no snippet attached) instead of read.Test plan
tests/server/doctor-service.test.jsthat plants a "secret" file outside the workspace, submits a finding whose evidence path traverses out to it via/api/doctor/import(usingimportDoctorResult), and asserts no snippet is captured for it — while a normal in-workspace evidence path (samestartLine/endLineshape) still gets its snippet.doctor-service,doctor-db,doctor-normalize,doctor-prompt, androutes-doctorsuites pass unchanged.