🛡️ Sentinel: [CRITICAL] Fix path traversal in session stores#6
Conversation
🚨 Severity: CRITICAL
💡 Vulnerability: Python `session_store.py` and Rust `session_control.rs` use the `session_id` to read and write local files (`{session_id}.json`) without validation. A malicious input string containing `../../` allows an attacker to read or overwrite arbitrary `.json` files on the filesystem.
🎯 Impact: This path traversal vulnerability could lead to information disclosure or data destruction if a manipulated session reference is processed.
🔧 Fix: Added strict string validation checks (`validate_session_id`) inside the data access patterns in Python (`save_session`, `load_session`) and Rust (`create_handle`, `resolve_managed_path`) to reject inputs containing path separators (`/`, `\`) or traversal elements (`.`, `..`).
✅ Verification: Python and Rust tests continue to pass and specifically test scripts constructed for `../../../tmp/file` inputs are now safely rejected by both data planes.
|
👋 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. |
🚨 Severity: CRITICAL
💡 Vulnerability: Python `session_store.py` and Rust `session_control.rs` use the `session_id` to read and write local files (`{session_id}.json`) without validation. A malicious input string containing `../../` allows an attacker to read or overwrite arbitrary `.json` files on the filesystem.
🎯 Impact: This path traversal vulnerability could lead to information disclosure or data destruction if a manipulated session reference is processed.
🔧 Fix: Added strict string validation checks (`validate_session_id`) inside the data access patterns in Python (`save_session`, `load_session`) and Rust (`create_handle`, `resolve_managed_path`) to reject inputs containing path separators (`/`, `\`) or traversal elements (`.`, `..`).
✅ Verification: Python and Rust tests continue to pass and specifically test scripts constructed for `../../../tmp/file` inputs are now safely rejected by both data planes.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a path traversal vulnerability by implementing session ID validation in both the Rust and Python components. The changes introduce a validate_session_id function that checks for illegal characters and sequences before performing filesystem operations. Feedback suggests strengthening the validation logic by using a whitelist of allowed characters (alphanumeric, hyphens, and underscores) instead of the current blacklist approach to ensure better security and robustness.
|
@copilot code review |
|
@claude code review |
Agent-Logs-Url: https://github.com/badMade/claw-code/sessions/0444deeb-aa4f-4249-9b5b-b31621571087 Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>
Agent-Logs-Url: https://github.com/badMade/claw-code/sessions/0444deeb-aa4f-4249-9b5b-b31621571087 Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>
Completed review pass on this PR: checked recent CI (including the earlier failed run), re-ran local validation ( |
There was a problem hiding this comment.
Pull request overview
Addresses a path traversal vulnerability where unvalidated session_id values were used to construct on-disk session file paths in both the Python and Rust session stores.
Changes:
- Added
validate_session_idinsrc/session_store.pyand enforced it on save/load. - Added
SessionStore::validate_session_idin Rust and propagated validation/error handling throughcreate_handleandresolve_managed_path. - Added a Sentinel note documenting the vulnerability and prevention guidance.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
src/session_store.py |
Adds session ID validation before forming <dir>/<session_id>.json paths. |
rust/crates/runtime/src/session_control.rs |
Adds session ID validation and switches create_handle to return Result with validation enforced. |
.jules/sentinel.md |
Documents the vulnerability and recommended prevention approach. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@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 |
|
@codex code review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c63af0755a
ℹ️ 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".
|
@copilot, resolve the merge conflicts in this pull request. |
…path-traversal-8720343771462187537 # Conflicts: # .jules/sentinel.md # rust/crates/runtime/src/session_control.rs Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>
Agent-Logs-Url: https://github.com/badMade/claw-code/sessions/4f913af6-1b3d-48c5-ac25-c30b4b994868 Co-authored-by: badMade <106821302+badMade@users.noreply.github.com>
Resolved and pushed. I merged |
|
@copilot, make changes based on:
See: #6 (comment) |
Completed a fresh review pass on commit b59843a: checked Actions runs (including the older failed run and newer action_required/no-job runs), verified no merge conflicts in branch state, and re-ran local validation ( |
The linked note is only a Codex quota/error message, not code-review feedback, so it doesn’t provide actionable code changes to apply. I rechecked this PR state at |
This PR fixes a CRITICAL path traversal vulnerability in how session IDs are stored on disk. By failing to validate the
session_idstring before forming the persistence path, it was possible to read or overwrite arbitrary JSON files across the filesystem.validate_session_id) insrc/session_store.pythat raises aValueErrorfor directory traversal markers.validate_session_idcheck inrust/crates/runtime/src/session_control.rsreturning aSessionControlError::Format.Resultreturn types in the Rust code up throughcreate_handleand into local usages within the workspace codebase..jules/sentinel.mdas part of Sentinel's defensive practices.Verification:
PYTHONPATH=. python3 -m pytest tests/andcargo test --workspaceon the rust package all passing.PR created automatically by Jules for task 8720343771462187537 started by @badMade