diff --git a/.jules/sentinel.md b/.jules/sentinel.md index fe79fb0..5c49015 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -33,3 +33,8 @@ **Learning:** Path-based checks (`os.path.islink`, `os.stat`) followed by path-based operations (`os.chmod`) are inherently racy. File descriptors are the only way to pin the resource. **Prevention:** Use `os.open` with `O_NOFOLLOW` to open the file securely (failing if it's a symlink). Then use file-descriptor-based operations: `os.fstat(fd)` to check modes and `os.fchmod(fd, mode)` to modify permissions. This ensures operations apply to the exact inode opened. + +## 2026-02-14 - CSV Injection via Log Sanitization +**Vulnerability:** `sanitize_for_log` in `main.py` was stripping quotes from `repr()` output for aesthetic reasons, inadvertently exposing strings starting with `=`, `+`, `-`, `@` to formula injection if logs are viewed in Excel. +**Learning:** Security controls (like `repr()`) can be weakened by subsequent "cleanup" steps. Always consider downstream consumption of logs (e.g., CSV export). +**Prevention:** Check for dangerous prefixes before stripping quotes. If detected, retain the quotes to force string literal interpretation. diff --git a/main.py b/main.py index 3a75f9b..6b9e731 100644 --- a/main.py +++ b/main.py @@ -241,6 +241,13 @@ def sanitize_for_log(text: Any) -> str: # repr() safely escapes control characters (e.g., \n -> \\n, \x1b -> \\x1b) # This prevents log injection and terminal hijacking. safe = repr(s) + + # Security: Prevent CSV Injection (Formula Injection) + # If the string starts with =, +, -, or @, we keep the quotes from repr() + # to force spreadsheet software to treat it as a string literal. + if s and s.startswith(("=", "+", "-", "@")): + return safe + if len(safe) >= 2 and safe[0] == safe[-1] and safe[0] in ("'", '"'): return safe[1:-1] return safe diff --git a/tests/test_csv_injection.py b/tests/test_csv_injection.py new file mode 100644 index 0000000..557d4a0 --- /dev/null +++ b/tests/test_csv_injection.py @@ -0,0 +1,66 @@ + +import unittest +import sys +import os + +# Add root to path to import main +sys.path.append(os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +import main + +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", + "", # 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(("'", '"')), + 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") + +if __name__ == '__main__': + unittest.main() diff --git a/tests/test_push_rules_perf.py b/tests/test_push_rules_perf.py index 63a89c8..18dcd63 100644 --- a/tests/test_push_rules_perf.py +++ b/tests/test_push_rules_perf.py @@ -98,31 +98,31 @@ def test_push_rules_multi_batch(self, mock_executor, mock_as_completed): # This should ALWAYS be True self.assertTrue(mock_executor.called, "ThreadPoolExecutor should be called for multi-batch") - @patch("main.is_valid_rule") - def test_push_rules_skips_validation_for_existing(self, mock_is_valid): + 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") if __name__ == '__main__': unittest.main()