🛡️ Sentinel: [HIGH] Fix XSS risk in folder names#128
🛡️ Sentinel: [HIGH] Fix XSS risk in folder names#128google-labs-jules[bot] wants to merge 1 commit intomainfrom
Conversation
Sanitize folder names from external JSON sources to prevent potential Stored XSS or injection attacks. 🚨 Severity: HIGH 💡 Vulnerability: External blocklists define folder names. Malicious blocklists could include XSS payloads (e.g. `<script>`) in folder names, which might be executed in the ControlD dashboard. 🎯 Impact: Stored XSS leading to potential session hijacking or unauthorized actions on the user's ControlD account. 🔧 Fix: Added `is_valid_folder_name` validator that blocks dangerous characters (`<>\"'`) and ensures folder names are printable strings. Integrated into `validate_folder_data`. ✅ Verification: Added `test_validate_folder_data_sanitizes_names` in `tests/test_security.py`. Verified that XSS payloads are rejected.
|
👋 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. |
|
Merging to
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| try: | ||
| # 1. Valid Folder Name | ||
| valid_data = {"group": {"group": "My Safe Folder"}} | ||
| assert main.validate_folder_data(valid_data, "https://example.com/valid.json") is True |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| # 2. XSS Payload in Folder Name | ||
| xss_data = {"group": {"group": "<script>alert(1)</script>"}} | ||
| assert main.validate_folder_data(xss_data, "https://example.com/xss.json") is False |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| # 2. XSS Payload in Folder Name | ||
| xss_data = {"group": {"group": "<script>alert(1)</script>"}} | ||
| assert main.validate_folder_data(xss_data, "https://example.com/xss.json") is False | ||
| assert mock_log.error.called |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| # 3. Non-string Folder Name | ||
| bad_type_data = {"group": {"group": 12345}} | ||
| assert main.validate_folder_data(bad_type_data, "https://example.com/bad_type.json") is False |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| # 4. Dangerous chars (quote) | ||
| quote_data = {"group": {"group": "Folder \"Bad\""}} | ||
| assert main.validate_folder_data(quote_data, "https://example.com/quote.json") is False |
Check notice
Code scanning / Bandit
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| folder_name = data["group"]["group"] | ||
| if not isinstance(folder_name, str): | ||
| log.error(f"Invalid data from {sanitize_for_log(url)}: Folder name must be a string.") |
Check warning
Code scanning / Prospector (reported by Codacy)
Use lazy % formatting in logging functions (logging-fstring-interpolation) Warning
| return False | ||
|
|
||
| if not is_valid_folder_name(folder_name): | ||
| log.error(f"Invalid data from {sanitize_for_log(url)}: Folder name contains unsafe characters.") |
Check warning
Code scanning / Prospector (reported by Codacy)
Use lazy % formatting in logging functions (logging-fstring-interpolation) Warning
| return False | ||
|
|
||
| if not is_valid_folder_name(folder_name): | ||
| log.error(f"Invalid data from {sanitize_for_log(url)}: Folder name contains unsafe characters.") |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (104/100) Warning
|
|
||
| # 3. Non-string Folder Name | ||
| bad_type_data = {"group": {"group": 12345}} | ||
| assert main.validate_folder_data(bad_type_data, "https://example.com/bad_type.json") is False |
Check warning
Code scanning / Pylintpython3 (reported by Codacy)
Line too long (101/100) Warning test
|
|
||
| folder_name = data["group"]["group"] | ||
| if not isinstance(folder_name, str): | ||
| log.error(f"Invalid data from {sanitize_for_log(url)}: Folder name must be a string.") |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
| return False | ||
|
|
||
| if not is_valid_folder_name(folder_name): | ||
| log.error(f"Invalid data from {sanitize_for_log(url)}: Folder name contains unsafe characters.") |
Check notice
Code scanning / Pylintpython3 (reported by Codacy)
Use lazy % formatting in logging functions Note
| return False | ||
|
|
||
| if not is_valid_folder_name(folder_name): | ||
| log.error(f"Invalid data from {sanitize_for_log(url)}: Folder name contains unsafe characters.") |
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (104/100) Warning
|
|
||
| # 3. Non-string Folder Name | ||
| bad_type_data = {"group": {"group": 12345}} | ||
| assert main.validate_folder_data(bad_type_data, "https://example.com/bad_type.json") is False |
Check warning
Code scanning / Pylint (reported by Codacy)
Line too long (101/100) Warning test
| try: | ||
| # 1. Valid Folder Name | ||
| valid_data = {"group": {"group": "My Safe Folder"}} | ||
| assert main.validate_folder_data(valid_data, "https://example.com/valid.json") is True |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| # 2. XSS Payload in Folder Name | ||
| xss_data = {"group": {"group": "<script>alert(1)</script>"}} | ||
| assert main.validate_folder_data(xss_data, "https://example.com/xss.json") is False |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
| # 2. XSS Payload in Folder Name | ||
| xss_data = {"group": {"group": "<script>alert(1)</script>"}} | ||
| assert main.validate_folder_data(xss_data, "https://example.com/xss.json") is False | ||
| assert mock_log.error.called |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| # 3. Non-string Folder Name | ||
| bad_type_data = {"group": {"group": 12345}} | ||
| assert main.validate_folder_data(bad_type_data, "https://example.com/bad_type.json") is False |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
|
|
||
| # 4. Dangerous chars (quote) | ||
| quote_data = {"group": {"group": "Folder \"Bad\""}} | ||
| assert main.validate_folder_data(quote_data, "https://example.com/quote.json") is False |
Check notice
Code scanning / Bandit (reported by Codacy)
Use of assert detected. The enclosed code will be removed when compiling to optimised byte code. Note test
There was a problem hiding this comment.
Pull request overview
This PR implements XSS protection for folder names that are fetched from external JSON files. The change prevents malicious list maintainers from injecting scripts into the Control D dashboard through crafted folder names.
Changes:
- Added
is_valid_folder_namevalidation function that rejects folder names containing XSS-prone characters (`<>"'``) - Enhanced
validate_folder_datato perform type checking and apply the new folder name validation - Added comprehensive unit tests covering valid names, XSS payloads, and invalid types
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| main.py | Implements is_valid_folder_name function and integrates it into validate_folder_data to sanitize folder names before use |
| tests/test_security.py | Adds test cases validating the rejection of XSS payloads, invalid types, and acceptance of safe folder names |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Block XSS and injection chars | ||
| dangerous_chars = set("<>\"'`") |
There was a problem hiding this comment.
The dangerous character set for folder names is less restrictive than for rules. The is_valid_rule function blocks <>"'\();{}[]while this function only blocks<>"'``.
If the Control D dashboard renders folder names in JavaScript contexts (event handlers, script tags, or embedded in JSON), characters like ();{}[] could still enable injection attacks. For example, a folder name like alert(1) would pass this validation but could be dangerous if rendered as onclick="handleClick('alert(1)')".
Consider aligning the character validation with is_valid_rule or adding a comment explaining why folder names require less strict validation.
| # Block XSS and injection chars | |
| dangerous_chars = set("<>\"'`") | |
| # Block XSS and injection chars (aligned with is_valid_rule) | |
| dangerous_chars = set("<>\"'`();{}[]") |
| assert main.validate_folder_data(xss_data, "https://example.com/xss.json") is False | ||
| assert mock_log.error.called |
There was a problem hiding this comment.
The assertion mock_log.error.called is too weak as it only checks if any error was logged at all, not specifically for this XSS validation case. Consider using a more specific assertion like checking if an error was called with a message containing "unsafe characters" or checking the call count increased by exactly 1.
| assert main.validate_folder_data(xss_data, "https://example.com/xss.json") is False | |
| assert mock_log.error.called | |
| before_error_calls = mock_log.error.call_count | |
| assert main.validate_folder_data(xss_data, "https://example.com/xss.json") is False | |
| assert mock_log.error.call_count == before_error_calls + 1 |
|
Superseded by PR #131 merge. |
Understood. Acknowledging that this work is now obsolete and stopping work on this task. |
Implemented input sanitization for folder names read from external JSON files. This prevents potential XSS vulnerabilities where a malicious list maintainer could inject scripts into the user's Control D dashboard via crafted folder names.
Changes:
is_valid_folder_namefunction tomain.pywhich rejects names containing< > " 'or non-printable characters.validate_folder_datato use this new validator and ensuregroup.groupis a string.tests/test_security.pycovering valid, XSS, and invalid type cases.PR created automatically by Jules for task 12151683927718723429 started by @abhimehro