fix(brainstorming): prevent path traversal in resolve_detail() - #2627
fix(brainstorming): prevent path traversal in resolve_detail()#2627santhiprakash wants to merge 4 commits into
Conversation
- Problem: resolve_detail() joined an unvalidated detail path onto csv_dir and read the result with no containment check, allowing --extra overlays (or a crafted CSV) to read arbitrary files (e.g. ../.env, /etc/passwd) into the LLM context during a normal bmad-brainstorming session. - Fix: resolve both the base directory and the target path, then reject the read if the resolved path escapes the base via is_relative_to(). This also closes the symlink-escape variant. - Verification: all 31 pytest tests pass, including 2 new traversal tests (relative and absolute). Full npm test suite also passes.
📝 WalkthroughWalkthrough
ChangesDetail path security
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core-skills/bmad-brainstorming/scripts/tests/test_brain.py (1)
77-89: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a regression test for symlink escapes.
The implementation claims symlink protection, but this test only covers relative traversal and absolute paths. Add an in-catalog symlink targeting
secret, then assertresolve_detail()returnsNoneand reportsescapes.Proposed test addition
assert brain.resolve_detail(rows[1], lib.parent) is None assert "escapes" in capsys.readouterr().err + link = lib.parent / "techniques" / "escape.md" + link.symlink_to(secret) + rows[1]["detail"] = "techniques/escape.md" + assert brain.resolve_detail(rows[1], lib.parent) is None + assert "escapes" in capsys.readouterr().err🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core-skills/bmad-brainstorming/scripts/tests/test_brain.py` around lines 77 - 89, Extend test_resolve_detail_refuses_path_traversal with an in-catalog symlink pointing to the existing secret file outside the catalog, then set the row detail to that symlink and assert resolve_detail() returns None and stderr contains “escapes”.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/core-skills/bmad-brainstorming/scripts/tests/test_brain.py`:
- Around line 77-89: Extend test_resolve_detail_refuses_path_traversal with an
in-catalog symlink pointing to the existing secret file outside the catalog,
then set the row detail to that symlink and assert resolve_detail() returns None
and stderr contains “escapes”.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b3318f9-b291-4dfe-90a0-fbeb93114756
📒 Files selected for processing (2)
src/core-skills/bmad-brainstorming/scripts/brain.pysrc/core-skills/bmad-brainstorming/scripts/tests/test_brain.py
…tail - Problem: CodeRabbit review flagged that resolve_detail() claims symlink protection but the test only covers relative traversal and absolute paths. - Fix: Add in-catalog symlink test case that creates a symlink inside the catalog directory pointing to a file outside it, then asserts resolve_detail() returns None and stderr contains 'escapes'. - Verification: all 31 pytest tests pass. Co-Authored-By: Paperclip <noreply@paperclip.ing>
Status noteThe CodeRabbit suggestion (a symlink-escape regression test) is now covered in One heads-up: the Quality & Validation workflows currently show Thanks for taking a look. |
- Problem: PR bmad-code-org#2627 branch drifted behind upstream after bmad-code-org#2631 merge work. - Fix: merge current upstream/main to restore MERGEABLE/CLEAN state. - Verification: targeted pytest on brainstorming tests pending.
Greptile SummaryThis PR fixes a path traversal vulnerability in
Confidence Score: 5/5Safe to merge — the change is a minimal, targeted containment check that closes a real file-read vulnerability without altering any other behaviour. The fix is correct: resolving both paths before comparing with is_relative_to() covers all three documented escape vectors (relative traversal, absolute-path injection via pathlib's / operator, symlink escape). The Python 3.10+ runtime requirement satisfies is_relative_to's 3.9+ availability. No existing shipped rows use detail, so there is nothing to regress. The new tests exercise all three attack vectors and confirm the happy path still works. Files Needing Attention: No files require special attention.
|
| Filename | Overview |
|---|---|
| src/core-skills/bmad-brainstorming/scripts/brain.py | Adds path containment check in resolve_detail() — resolves both base and target before comparing with is_relative_to(), correctly blocking relative traversal, absolute-path injection, and symlink escapes |
| src/core-skills/bmad-brainstorming/scripts/tests/test_brain.py | Adds two new tests: a traversal test covering relative, absolute, and symlink escape vectors; and a regression test confirming valid in-directory detail files still resolve correctly |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["resolve_detail(row, csv_dir)"] --> B{detail present?}
B -- No --> C[return None]
B -- Yes --> D["base = csv_dir.resolve()"]
D --> E["path = base / row detail .resolve()"]
E --> F{is_relative_to base?}
F -- No --> G["warn to stderr: path escapes catalog"]
G --> C
F -- Yes --> H{path.is_file?}
H -- No --> I["warn to stderr: file not found"]
I --> C
H -- Yes --> J["return path.read_text()"]
Reviews (2): Last reviewed commit: "chore: merge upstream/main into fix/reso..." | Re-trigger Greptile
Greptile gate triagedGreptile summary (2026-07-30, 5/5) reviewed — no author-side code changes required. The containment guard and all three traversal vectors (relative, absolute, symlink) remain covered in the current branch head. Also merged latest Awaiting human maintainer review/merge. |
|
Current head already includes the requested symlink regression coverage in . The test now creates an in-catalog symlink that points outside the catalog and asserts refuses it. Verified locally with ============================= test session starts ============================== ============================ no tests ran in 0.00s ============================= (31/31 passed). |
|
Current head already includes the requested symlink regression coverage in |
sanmaxdev
left a comment
sanmaxdev
left a comment
There was a problem hiding this comment.
The containment check covers relative, absolute, and symlink escapes while preserving valid in-directory detail files.
Verified with uv run --with pytest -m pytest src/core-skills/bmad-brainstorming/scripts/tests/test_brain.py -v (31 passed) and git diff --check.
2 participants
What
Add a containment check to
resolve_detail()inbrain.pyso that adetailpath cannot escape the catalog directory.Why
Fixes #2625. The function joined an unvalidated
detailvalue (from a CSV row or--extraoverlay) ontocsv_dirand read the result with no check that the resolved path stayed inside the catalog. A crafted--extraJSON file (or a project-local CSV pointed at viacustomize.toml) could read arbitrary files (e.g.../.env,/etc/passwd) into the LLM context during a normalbmad-brainstormingsession. The repo'sSECURITY.mdexplicitly lists "Path traversal or file system access issues" as in scope.How
path.is_relative_to(base)is false.Path.resolve()follows symlinks, so this also closes the symlink-escape variant.Path.is_relative_torequires 3.9+; the script already declaresrequires-python = ">=3.10".brain-methods.csvusesdetail(0 of 108), so the check has no existing behaviour to regress.../.env), absolute-path, and in-catalog symlink escape vectors, plus a regression test confirming legitimate in-directory detail files still resolve.Testing
uv run --with pytest -m pytest tests/test_brain.py -v— all 31 tests pass (29 existing + 2 new). The symlink-escape case (suggested by CodeRabbit) is covered in the traversal test. Fullnpm testsuite also passes (566 tests + lint + format).