🛡️ Sentinel: [CRITICAL] Fix Path Traversal in Session Loading#12
🛡️ Sentinel: [CRITICAL] Fix Path Traversal in Session Loading#12badMade wants to merge 10 commits into
Conversation
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Code Review
This pull request addresses a path traversal vulnerability in session ID handling by implementing centralized validation logic in both Python and Rust. While the core security fix is sound, several issues need to be addressed: the Rust implementation of create_handle panics on invalid input instead of returning a Result as intended, and numerous temporary artifacts, redundant test scripts, and local session data files were accidentally committed. Additionally, several test files are incorrectly placed in the root or source directories rather than being integrated into a proper test suite.
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
Co-authored-by: google-labs-jules[bot] <161369871+google-labs-jules[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Addresses a path traversal vulnerability in session persistence by validating user-provided session identifiers before converting them into filesystem paths (Python .port_sessions and Rust .claw/sessions).
Changes:
- Added centralized session ID validation in Python and applied it to
save_session/load_session. - Added centralized session ID validation in Rust, applied it to managed session path creation/resolution, and changed
SessionStore::create_handleto returnResult. - Added a Sentinel journal entry documenting the vulnerability and mitigation.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/session_store.py | Introduces validate_session_id and enforces it for session save/load. |
| rust/crates/runtime/src/session_control.rs | Adds validate_session_id, applies it in several path-building functions, and hardens create_handle API. |
| .jules/sentinel.md | Documents the security issue and mitigation steps taken. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ion for backslash/drive-prefix, add unit tests Agent-Logs-Url: https://github.com/badMade/claw-code/sessions/8f0030f2-8109-43a2-aa5b-791c5be3eafe Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>
|
@copilot code review |
|
@claude code review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…session_store unit tests Agent-Logs-Url: https://github.com/badMade/claw-code/sessions/70662ee3-a33e-47e0-ad37-04a229c38569 Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>
… .gitignore Agent-Logs-Url: https://github.com/badMade/claw-code/sessions/70662ee3-a33e-47e0-ad37-04a229c38569 Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>
|
@codex Code Review |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex code review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4811ce60e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Windows drive prefixes such as `C:` or UNC paths. | ||
| for path_cls in (PurePosixPath, PureWindowsPath): | ||
| p = path_cls(session_id) | ||
| if p.drive or p.root or len(p.parts) != 1 or p.parts[0] in (".", ".."): |
There was a problem hiding this comment.
Reject trailing separators in Python session IDs
This check can be bypassed by IDs like foo/, foo//, and foo/. because PurePosixPath/PureWindowsPath normalize those inputs to a single part, so len(p.parts) == 1 still passes. As a result, save_session will accept separator-bearing IDs and build paths like <session_dir>/foo/.json, which undermines the traversal hardening and can redirect IO into nested or symlinked directories if present.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@copilot, make changes based on the above suggestion.
There was a problem hiding this comment.
@copilot, make changes based on the above suggestion.
There was a problem hiding this comment.
@codex, make changes based on:
@copilot, make changes based on:
There was a problem hiding this comment.
@codex, make changes based on:
@copilot, make changes based on:
There was a problem hiding this comment.
Summary
- Consolidated
src/session_store.pyto a singlevalidate_session_idimplementation (removing the accidental duplicate function definition) and kept the cross-platformPurePosixPath/PureWindowsPathchecks in one place. - Hardened Python validation to explicitly reject trailing separators and terminal dot segments (e.g.
foo/,foo//,foo/.,foo\\,foo\\.), addressing the bypass noted in review comments. - Added focused unit tests that assert those trailing separator/dot-segment IDs are rejected, preventing regression of this security edge case.
Testing
- ✅
python -m unittest -q tests/test_session_store.py
Committed on the current branch:
8027892— Harden Python session ID validation for trailing separators
Also created the follow-up PR via make_pr with title:
- Follow-up: tighten Python session ID validation edge cases
There was a problem hiding this comment.
Summary
- Removed the duplicate Python
validate_session_iddefinition so there is a single canonical validator insrc/session_store.py, then hardened it to explicitly reject raw/and\characters before path normalization can hide trailing separator forms likefoo/,foo//, orfoo/.. src/session_store.pyL19-L37 - Added/extended Python unit tests to assert those trailing-separator bypass variants are rejected (
foo/,foo//,foo/.), alongside existing traversal checks. tests/test_session_store.pyL23-L35 - Committed the changes on the current branch with commit
6d291ed. - Created a follow-up PR via the
make_prtool with title: “Follow-up: tighten Python session ID validation for trailing separators”.
Testing
- ✅
python -m unittest -q tests/test_session_store.py
| match (components.next(), components.next()) { | ||
| (Some(std::path::Component::Normal(_)), None) => Ok(()), |
There was a problem hiding this comment.
Reject trailing '/' and '/.' in Rust session IDs
Path::components() normalizes trailing separators and terminal . segments, so values like foo/ or foo/. still satisfy the (Normal, None) pattern here. That means validate_session_id accepts separator-bearing IDs and create_handle can generate paths such as .../foo/.jsonl, leaving a path-injection gap in the new validation (especially when subdirectories or symlinks exist under the session root).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@copilot, make changes based on the above suggestion.
There was a problem hiding this comment.
@copilot, make changes based on the above suggestion.
There was a problem hiding this comment.
@codex, make changes based on:
@copilot, make changes based on:
There was a problem hiding this comment.
@codex, make changes based on:
@copilot, make changes based on:
There was a problem hiding this comment.
Summary
- Addressed the follow-up review concern about trailing-separator bypass in Python session ID validation by adding explicit
/and\rejection beforePurePathnormalization. This prevents inputs likefoo/,foo//, andfoo/.from being accepted after path normalization. Also removed the duplicate earliervalidate_session_iddefinition so only the hardened implementation remains. src/session_store.pyL19-L38 - Expanded unit tests to explicitly assert rejection of trailing-separator/trailing-dot variants (
foo/,foo//,foo/.) in addition to existing traversal cases. tests/test_session_store.pyL23-L35
Testing
- ✅
python -m unittest -q tests.test_session_store
Also completed the requested workflow actions:
- Committed on the current branch:
20b0ef4 - Created a follow-up PR via
make_prwith a title/body summarizing changes on top of the original PR.
There was a problem hiding this comment.
Summary
- Updated Python session ID validation to close the trailing-separator bypass by explicitly rejecting any raw
/or\insession_idbeforePurePathnormalization, while preserving the existing cross-platformPurePosixPath/PureWindowsPathchecks for drive/root/dot segments. This also removes the duplicatevalidate_session_iddefinition so there is a single canonical validator. src/session_store.pyL19-L37 - Added focused regression tests for the reported bypass patterns (
foo/,foo//,foo/.) to ensure separator-bearing IDs are rejected consistently. tests/test_session_store.pyL31-L38
Testing
- ✅
python -m unittest -q tests.test_session_store
|
@copilot, resolve the merge conflicts in this pull request. |
- session_control.rs: keep validate_session_id() (stronger path-component-based validation) over is_valid_session_id() at all 4 conflict sites - .jules/sentinel.md: combine both journal entries Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>
Resolved in
All 41 Python tests and Rust fmt/clippy/tests pass after the merge. |
|
@copilot, make changes based on:
See: #12 (comment) |
|
@copilot, make changes based on:
See: #12 (comment) |
Agent-Logs-Url: https://github.com/badMade/claw-code/sessions/4c6b85d3-0326-430a-8781-c8d8e8865712 Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>
The referenced message was a Codex usage-limit error with no actionable code feedback, so I performed the code review directly. During review I found that
Same as above — Codex's usage-limit message had no actionable feedback, so I did the review myself. Found and removed a duplicate |
🚨 Severity: CRITICAL
💡 Vulnerability: Path traversal in session file loading (
load_sessioninsrc/session_store.pyandresolve_managed_session_path_forinrust/crates/runtime/src/session_control.rs). User-provided session IDs were read directly from disk without sanitization, allowing attackers to specify identifiers like../../../../tmp/fileto read or overwrite arbitrary JSON files on the system under the disguise of a session file.🎯 Impact: An attacker could potentially read sensitive data outside of the intended
.port_sessionsor.claw/sessionsdirectories, or potentially overwrite other files.🔧 Fix: Implemented a centralized
validate_session_idfunction in both Python and Rust that strictly forbids path separators (/,\) and specific filesystem traversal segments (.,..). This validation is now applied uniformly wherever session IDs are handled before converting them into file paths.SessionStore::create_handlein Rust was also updated to returnResultrather than panicking on bad inputs to fail securely.✅ Verification: Ran
cargo test --workspaceandpytestlocally to ensure no regressions were introduced. Evaluated manually against path traversal strings.PR created automatically by Jules for task 9304634400431962093 started by @badMade