Conversation
🚨 Severity: CRITICAL 💡 Vulnerability: Path Traversal via Folder ID (PK) injection from API response. 🎯 Impact: An attacker controlling the API or MITM could inject path traversal payloads (e.g. ../../etc/passwd) into file operations. 🔧 Fix: Validated Folder IDs against a strict whitelist (^[a-zA-Z0-9_.-]+$). ✅ Verification: Added tests/test_folder_id_validation.py proving invalid IDs are rejected. Co-authored-by: abhimehro <84992105+abhimehro@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. |
|
😎 Merged manually by @abhimehro - details. |
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
Summary of ChangesHello @abhimehro, 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 by implementing robust validation for folder IDs received from external APIs. By whitelisting acceptable characters and integrating this validation into key file operation functions, the application is now protected against malicious API responses or Man-in-the-Middle attacks attempting to inject path traversal sequences. 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
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # Function should filter out malicious IDs | ||
| result = m.verify_access_and_get_folders(mock_client, "valid_profile") | ||
|
|
||
| assert result is not None |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert result is not None | ||
|
|
||
| # Check that valid IDs are preserved | ||
| assert "Safe Folder" in result |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| # Check that valid IDs are preserved | ||
| assert "Safe Folder" in result | ||
| assert result["Safe Folder"] == "safe_id_123" |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| # Check that valid IDs are preserved | ||
| assert "Safe Folder" in result | ||
| assert result["Safe Folder"] == "safe_id_123" | ||
| assert "Safe Folder 2" in result |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert "Safe Folder" in result | ||
| assert result["Safe Folder"] == "safe_id_123" | ||
| assert "Safe Folder 2" in result | ||
| assert result["Safe Folder 2"] == "safe-id-456_789" |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| assert result["Safe Folder 2"] == "safe-id-456_789" | ||
|
|
||
| # Check that malicious IDs are removed | ||
| assert "Malicious Folder" not in result |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| # Check that malicious IDs are removed | ||
| assert "Malicious Folder" not in result | ||
| assert "Malicious Folder 2" not in result |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
![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 effectively addresses a path traversal vulnerability by implementing whitelist-based validation for folder IDs (PKs) through the introduction of validate_folder_id. For further refinement, ensure that dot-only IDs like .. are explicitly blocked and consider reducing redundant logging in the folder creation logic.
There was a problem hiding this comment.
Pull request overview
This PR hardens handling of API-provided folder IDs (“PK”) to mitigate traversal/injection risks by validating IDs before they’re used in API path construction and related operations.
Changes:
- Added
FOLDER_ID_PATTERN+validate_folder_id()inmain.pyand applied validation when listing/using folder PKs. - Updated folder listing/creation flows to filter or reject invalid PKs.
- Added a regression test for malicious PKs and documented the security learning in
.jules/sentinel.md(plus pyproject/lock metadata updates).
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
main.py |
Introduces folder-ID validation and applies it in folder list/create flows. |
tests/test_folder_id_validation.py |
Adds regression coverage for filtering malicious PKs in verify_access_and_get_folders. |
.jules/sentinel.md |
Documents the vulnerability/learning/prevention guidance. |
pyproject.toml |
Adds a dependency-groups stanza (currently empty). |
uv.lock |
Records additional metadata for dev requirements (currently empty). |
| pk = str(body["group"]["PK"]) | ||
| if not validate_folder_id(pk): | ||
| log.error(f"API returned invalid folder ID: {sanitize_for_log(pk)}") | ||
| return None |
There was a problem hiding this comment.
The PR changes create_folder() behavior to return None when the API returns an invalid folder ID, but there is no regression test asserting this new security behavior. Add a test that mocks the POST response to return an invalid PK (e.g., .. or ../../etc/passwd) and asserts create_folder() returns None and logs an error.
| pk = str(body["group"]["PK"]) | ||
| if not validate_folder_id(pk): | ||
| log.error(f"API returned invalid folder ID: {sanitize_for_log(pk)}") | ||
| return None |
There was a problem hiding this comment.
create_folder() logs an error after validate_folder_id(pk) fails, but validate_folder_id() already logs an error by default. This results in duplicate error lines for the same bad PK. Consider calling validate_folder_id(pk, log_errors=False) here (and keeping the more specific message), or removing the extra log.error to avoid noisy logs.
| def list_existing_folders(client: httpx.Client, profile_id: str) -> Dict[str, str]: | ||
| try: | ||
| data = _api_get(client, f"{API_BASE}/{profile_id}/groups").json() | ||
| folders = data.get("body", {}).get("groups", []) | ||
| return { | ||
| f["group"].strip(): f["PK"] | ||
| for f in folders | ||
| if f.get("group") and f.get("PK") | ||
| } | ||
| result = {} | ||
| for f in folders: | ||
| if not f.get("group") or not f.get("PK"): | ||
| continue | ||
| pk = str(f["PK"]) | ||
| if validate_folder_id(pk): | ||
| result[f["group"].strip()] = pk | ||
| return result |
There was a problem hiding this comment.
list_existing_folders() now filters out invalid PK values via validate_folder_id(), but there is no test covering this new behavior. Add a regression test that mocks the /groups API response with a mix of valid and invalid PKs and asserts the invalid ones are excluded (mirroring the new security intent).
| def validate_folder_id(folder_id: str, log_errors: bool = True) -> bool: | ||
| """Validates folder ID (PK) format to prevent path traversal.""" | ||
| if not folder_id: | ||
| return False | ||
| if not FOLDER_ID_PATTERN.match(folder_id): | ||
| if log_errors: | ||
| log.error(f"Invalid folder ID format: {sanitize_for_log(folder_id)}") | ||
| return False | ||
| return True |
There was a problem hiding this comment.
validate_folder_id() currently allows . and .. because FOLDER_ID_PATTERN permits dots. If a malicious API response returns .., it will pass validation and later be interpolated into URL path segments (e.g., /groups/{folder_id}), reintroducing a dot-segment traversal vector on servers/proxies that normalize paths. Add an explicit block for folder_id.strip() in (., ..) (similar to is_valid_folder_name) before the regex check.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
2 similar comments
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
1 similar comment
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
abhimehro
deleted the
sentinel/fix-path-traversal-folder-id-2150753986649535080
branch
This PR addresses a critical path traversal vulnerability where the application trusted "PK" (ID) fields from the external API and used them directly in file operations.
Changes:
FOLDER_ID_PATTERNregex (^[a-zA-Z0-9_.-]+$) to whitelist safe folder IDs.validate_folder_idhelper function.verify_access_and_get_folders,list_existing_folders, andcreate_folderinmain.pyto validate and reject invalid IDs.tests/test_folder_id_validation.py..jules/sentinel.mdwith security learning.This prevents attackers from exploiting the application via malicious API responses or MITM attacks injecting path traversal sequences.
PR created automatically by Jules for task 2150753986649535080 started by @abhimehro