🛡️ Sentinel: Fix CSV Injection in log sanitization#222
Conversation
**Vulnerability:** The `sanitize_for_log` function in `main.py` was stripping quotes from `repr()` output for aesthetic reasons. This allowed strings starting with `=`, `+`, `-`, or `@` to be interpreted as formulas if the logs were exported to CSV and opened in spreadsheet software (CSV Injection / Formula Injection). **Fix:** Modified `sanitize_for_log` to detect strings starting with these dangerous characters. If detected, the function now retains the surrounding quotes provided by `repr()`, forcing spreadsheet software to treat the value as a string literal. **Verification:** - Added `tests/test_csv_injection.py` to verify the fix and ensure regression handling for normal strings. - Fixed a flaky test in `tests/test_push_rules_perf.py` that was failing due to module reloading issues in the test suite. - Ran all tests successfully with `pytest`. **Impact:** Prevents potential formula execution on administrative machines analyzing logs. 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 security vulnerability by implementing a fix for CSV Injection in 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 effectively addresses a CSV injection vulnerability within the log sanitization logic. The core change in main.py correctly preserves quotes for strings that could be misinterpreted as formulas, and the addition of dedicated unit tests in tests/test_csv_injection.py is excellent for ensuring this security fix is robust. I have one minor suggestion to improve the readability and style of the implementation.
There was a problem hiding this comment.
Pull request overview
This PR fixes a CSV injection vulnerability in the sanitize_for_log() function by preserving quotes around strings that start with formula-trigger characters (=, +, -, @). When logs are exported to CSV and opened in spreadsheet applications like Excel, these characters can trigger formula execution, potentially leading to code execution or data exfiltration.
Changes:
- Enhanced
sanitize_for_log()to detect and preserverepr()quotes for strings starting with CSV injection characters - Added comprehensive test coverage for CSV injection prevention scenarios
- Documented the vulnerability and prevention strategy in
.jules/sentinel.md
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| main.py | Added CSV injection prevention logic to sanitize_for_log() that preserves quotes for dangerous strings |
| tests/test_csv_injection.py | New test file with comprehensive coverage for CSV injection prevention and normal string behavior |
| .jules/sentinel.md | Added documentation of the CSV injection vulnerability, learning points, and prevention approach |
| tests/test_push_rules_perf.py | Refactored test to use context manager pattern instead of decorator-based mocking (unrelated to CSV fix) |
| def test_push_rules_skips_validation_for_existing(self): | ||
| """ | ||
| Test that is_valid_rule is NOT called for rules that are already in existing_rules. | ||
| """ | ||
| mock_is_valid.return_value = True | ||
| hostnames = ["h1", "h2"] | ||
| # h1 is already known, h2 is new | ||
| existing_rules = {"h1"} | ||
|
|
||
| main.push_rules( | ||
| self.profile_id, | ||
| self.folder_name, | ||
| self.folder_id, | ||
| self.do, | ||
| self.status, | ||
| hostnames, | ||
| existing_rules, | ||
| self.client | ||
| ) | ||
|
|
||
| # h1 is in existing_rules, so we should skip validation for it. | ||
| # h2 is NOT in existing_rules, so we should validate it. | ||
| # So is_valid_rule should be called EXACTLY once, with "h2". | ||
| mock_is_valid.assert_called_once_with("h2") | ||
| with patch.object(main, "is_valid_rule") as mock_is_valid: | ||
| mock_is_valid.return_value = True | ||
| hostnames = ["h1", "h2"] | ||
| # h1 is already known, h2 is new | ||
| existing_rules = {"h1"} | ||
|
|
||
| main.push_rules( | ||
| self.profile_id, | ||
| self.folder_name, | ||
| self.folder_id, | ||
| self.do, | ||
| self.status, | ||
| hostnames, | ||
| existing_rules, | ||
| self.client | ||
| ) | ||
|
|
||
| # h1 is in existing_rules, so we should skip validation for it. | ||
| # h2 is NOT in existing_rules, so we should validate it. | ||
| # So is_valid_rule should be called EXACTLY once, with "h2". | ||
| mock_is_valid.assert_called_once_with("h2") |
There was a problem hiding this comment.
This test refactoring appears unrelated to the CSV injection fix described in the PR title and description. The change from decorator-based @patch("main.is_valid_rule") to context manager with patch.object(main, "is_valid_rule") is a valid refactoring, but it's unclear why it's included in this PR focused on security fixes. Consider moving unrelated refactorings to a separate PR to maintain clear separation of concerns and make reviews easier.
| class TestCSVInjection(unittest.TestCase): | ||
| def test_csv_injection_prevention(self): | ||
| """ | ||
| Verify that sanitize_for_log correctly keeps quotes around strings | ||
| that start with characters known to trigger formula execution in spreadsheets | ||
| (CSV Injection). | ||
| """ | ||
| # Test cases for CSV injection characters | ||
| dangerous_inputs = [ | ||
| "=cmd|' /C calc'!A0", | ||
| "+cmd|' /C calc'!A0", | ||
| "-cmd|' /C calc'!A0", | ||
| "@cmd|' /C calc'!A0", | ||
| ] | ||
|
|
||
| for inp in dangerous_inputs: | ||
| sanitized = main.sanitize_for_log(inp) | ||
| # Should keep quotes (repr adds them) | ||
| # repr("=...") -> "'=...'" | ||
| # So sanitized should start with ' or " | ||
| self.assertTrue(sanitized.startswith("'") or sanitized.startswith('"'), | ||
| f"Input '{inp}' should be quoted to prevent CSV injection. Got: {sanitized}") | ||
|
|
||
| # Should contain the input | ||
| self.assertIn(inp, sanitized) | ||
|
|
||
| def test_normal_string_behavior(self): | ||
| """ | ||
| Verify that normal strings (not starting with =, +, -, @) still have | ||
| their outer quotes stripped, preserving existing behavior. | ||
| """ | ||
| safe_inputs = [ | ||
| "NormalString", | ||
| "Folder Name", | ||
| "12345", | ||
| "<script>alert(1)</script>", # XSS attempt (handled by repr escaping but checked here for quote stripping) | ||
| ] | ||
|
|
||
| for inp in safe_inputs: | ||
| sanitized = main.sanitize_for_log(inp) | ||
| # Should NOT start with quote (unless repr escaped something inside and used different quotes, but for simple strings it shouldn't) | ||
| # Actually, repr("NormalString") is 'NormalString'. Stripped -> NormalString. | ||
| # repr("Folder Name") is 'Folder Name'. Stripped -> Folder Name. | ||
| self.assertFalse(sanitized.startswith("'") and sanitized.endswith("'"), | ||
| f"Input '{inp}' should have outer quotes stripped. Got: {sanitized}") | ||
|
|
||
| # For strict check: | ||
| self.assertEqual(sanitized, repr(inp)[1:-1]) | ||
|
|
||
| def test_empty_input(self): | ||
| """Verify empty input handling.""" | ||
| self.assertEqual(main.sanitize_for_log(""), "") | ||
| self.assertEqual(main.sanitize_for_log(None), "None") | ||
|
|
There was a problem hiding this comment.
Missing test coverage for edge cases: The tests don't cover scenarios where dangerous CSV injection characters might appear after sanitization steps like token redaction or basic auth removal. For example, if a TOKEN value is "=secret" and appears at the start of a string, after redaction it becomes "[REDACTED]" which starts with a safe character. However, if a string like "prefix=secret" has "prefix" redacted to become "=secret", the dangerous character is exposed. Consider adding tests that verify the interaction between redaction and CSV injection prevention.
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: 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. |
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
|
👋 Development Partner is reviewing this PR. Will provide feedback shortly. |
Fix CSV Injection in log sanitization by preserving quotes for dangerous strings.
PR created automatically by Jules for task 4975783286757750034 started by @abhimehro