fix(daemon): reject path-traversal ids in readPromptTemplate - #5027
Conversation
|
🧪 Added a QA gate for this one because it hardens a live daemon read path that user flows depend on. Nothing needed from you right now — we'll update here once it goes through validation before merge. Thanks for the focused fix. |
mrcfps
left a comment
There was a problem hiding this comment.
@EthanGuo-coder I reviewed the daemon prompt-template containment fix and the added regression coverage. The changed helper now rejects ids whose resolved path would leave the surface directory before attempting the read, while valid direct-child ids continue through the existing JSON validation path. I also checked the route caller and daemon ownership guidance; this stays scoped to the daemon helper and its tests. Thanks for the focused security hardening here.
🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.
AmyShang-alt
left a comment
There was a problem hiding this comment.
QA 验收记录
验收范围:
- PR head: 3ed33c4
- daemon prompt-template path traversal fix:
readPromptTemplaterejects ids that resolve outside the surface directory while preserving valid direct-child template reads.
已验证:
pnpm --dir apps/daemon exec vitest run -c vitest.config.ts tests/prompt-templates.test.ts-> 14/14 passed.pnpm --dir apps/daemon exec vitest run -c vitest.config.ts tests/static-resource-routes.test.ts tests/server-bootstrap-regression.test.ts-> 19/19 passed.pnpm --filter @open-design/daemon typecheckpassed.- GitHub CI is green for Workspace unit tests, E2E Vitest, Preflight, UI P0 smoke, and related UI P0 groups.
未验证:
- No Electron manual validation run; this PR is daemon helper/API hardening with no UI behavior change.
风险/关注点:
- Security-sensitive daemon file-read boundary, but the diff is tightly scoped and has direct regression coverage for the traversal case plus route/bootstrap regression coverage.
结论:
- 验收通过 / 可接受。
Why
readPromptTemplatejoins the caller-suppliedidstraight into a filesystem path without checking that the result stays inside the surface directory. The daemon exposes it atGET /api/prompt-templates/:surface/:id, so a percent-encoded id such as..%2f..%2ffoodecodes to../../fooand escapes the templates directory — an arbitrary.jsonfile read against the machine running the daemon. I noticed this while adding the prompt-template scanner tests in #2447 and am fixing it on its own so the containment is explicit rather than resting on the incidentalid === filenamevalidation.What users will see
Nothing changes for correctly-authored templates — every committed template id equals its filename, so all 104 still resolve. Only ids that would escape the surface directory (those containing a path separator or a
..segment, which are never valid template ids) now return 404 instead of reading outside the directory.Surface area
/api/prompt-templates/:surface/:idresponse is unchanged for every valid id; only traversal ids that never matched a real template are now rejected before the read.Bug fix verification
readPromptTemplatejoinedidinto the file path (<root>/<surface>/<id>.json) with no containment check, so a traversal id like../secretresolved outside the surface directory and returned its contents.apps/daemon/tests/prompt-templates.test.ts→ "returns null for a path-traversal id instead of reading outside the surface dir" plants<root>/secret.jsonone level above theimagesurface directory and assertsreadPromptTemplate(root, 'image', '../secret')is null.path.dirname(filePath) !== dirguard it returns null.Validation
pnpm --filter @open-design/daemon test—tests/prompt-templates.test.ts14/14, plustests/static-resource-routes.test.tsandtests/server-bootstrap-regression.test.ts(which exercise the endpoint) 19/19.tsc --noEmitfor bothtsconfig.jsonandtsconfig.tests.json.