fix(session): prevent path traversal in JSONSession (CWE-22)#1250
fix(session): prevent path traversal in JSONSession (CWE-22)#1250shlokmestry wants to merge 1 commit intoagentscope-ai:mainfrom
Conversation
|
|
|
|
Summary of ChangesHello @shlokmestry, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical path traversal vulnerability (CWE-22) within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces a critical security fix to prevent path traversal vulnerabilities in JSONSession. The changes are robust, implementing multiple layers of validation for session_id and user_id, including checks for absolute paths, traversal patterns, and a final realpath containment check. A regression test has been added to confirm the fix. My review focuses on improving the documentation by ensuring all new and modified methods have complete docstrings that adhere to the repository's style guide, which is a [MUST] requirement. The provided rules did not apply to any of the comments, so all original comments have been kept.
| def _get_save_path(self, session_id: str, user_id: str) -> str: | ||
| """The path to save the session state. | ||
| def _validate_identifier(self, value: str) -> None: | ||
| """Validate session_id and user_id against path traversal.""" |
There was a problem hiding this comment.
The docstring for this new method is incomplete. According to the repository's style guide (line 46), all methods must have a complete docstring including descriptions of arguments and what they raise.
"""Validate session_id and user_id against path traversal.
Args:
value (`str`):
The identifier to validate.
Raises:
ValueError: If the identifier is invalid, e.g., contains
absolute paths, traversal patterns, or path separators.
"""References
- All classes/methods must have complete docstrings, strictly following the provided template which includes Args, Returns, and other sections. (link)
| raise ValueError("Invalid session_id/user_id") | ||
|
|
||
| def _get_save_path(self, session_id: str, user_id: str) -> str: | ||
| """The path to save the session state.""" |
There was a problem hiding this comment.
The docstring for _get_save_path is incomplete. The repository style guide (line 46) requires a full docstring with Args, Returns, and Raises sections.
"""Get the secure path to save the session state.
This function validates session_id and user_id to prevent path
traversal attacks and returns a secure, absolute path for the
session file.
Args:
session_id (`str`):
The session id.
user_id (`str`):
The user ID for the storage.
Returns:
`str`:
The absolute path to save the session state.
Raises:
ValueError: If path traversal is detected in session_id or user_id.
"""References
- All classes/methods must have complete docstrings, strictly following the provided template which includes Args, Returns, and other sections. (link)
| **state_modules_mapping (`dict[str, StateModule]`): | ||
| A dictionary mapping of state module names to their instances. | ||
| """ | ||
| """Save the state dictionary to a JSON file.""" |
There was a problem hiding this comment.
The docstring for save_session_state is incomplete. While the description has been corrected, it's missing the Args section required by the repository's style guide (line 46). Please provide a complete docstring.
"""Save the state dictionary to a JSON file.
Args:
session_id (`str`):
The session id.
user_id (`str`, default to `""`):
The user ID for the storage.
**state_modules_mapping (`dict[str, StateModule]`):
A dictionary mapping of state module names to their instances.
"""References
- All classes/methods must have complete docstrings, strictly following the provided template which includes Args, Returns, and other sections. (link)
| state_modules_mapping (`list[StateModule]`): | ||
| The list of state modules to be loaded. | ||
| """ | ||
| """Load the state dictionary from a JSON file.""" |
There was a problem hiding this comment.
The docstring for load_session_state is incomplete. It should include the Args section as required by the repository's style guide (line 46).
"""Load the state dictionary from a JSON file.
Args:
session_id (`str`):
The session id.
user_id (`str`, default to `""`):
The user ID for the storage.
allow_not_exist (`bool`, defaults to `True`):
Whether to allow the session to not exist. If `False`, raises
an error if the session does not exist.
**state_modules_mapping (`dict[str, StateModule]`):
The mapping of state modules to be loaded.
"""References
- All classes/methods must have complete docstrings, strictly following the provided template which includes Args, Returns, and other sections. (link)
2 participants
AgentScope Version
1.0.16
Description
This PR fixes a path traversal vulnerability in
JSONSession(CWE-22: Improper Limitation of a Pathname to a Restricted Directory).
Background
session_idanduser_idwere used to construct file paths forsession persistence. While
os.path.basename()was applied,malicious traversal inputs (e.g.
"../evil") were silentlysanitized instead of being explicitly rejected.
This could allow unintended file path manipulation under certain
conditions.
Changes
".."traversal patternscannot escape
save_dirtest_jsonsession_rejects_path_traversalSecurity Impact
Prevents directory traversal attacks when saving or loading
session state files.
Tests
pytest tests/session_test.py -qValueErrorChecklist