diff --git a/.jules/sentinel.md b/.jules/sentinel.md index 038829ea..ef7a79bf 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -61,3 +61,11 @@ **Learning:** Logic errors in security controls often lead to "fail-closed" states that break functionality entirely, or "fail-open" states that bypass security. Implicit returns in Python (`None`) can be dangerous when boolean validation is expected. **Prevention:** Always use explicit return statements for both success and failure paths in validation functions. Use static analysis (linting) to catch unreachable code and implicit returns. Ensure unit tests cover positive cases (valid inputs) as rigorously as negative cases (attack vectors). + +## 2026-10-25 - Security Control Bypass via Optimization + +**Vulnerability:** The `push_rules` function bypassed the standard `is_valid_rule` validation logic by using an optimized inline regex check (`match_rule = RULE_PATTERN.match`) for performance. This created a discrepancy where future enhancements to `is_valid_rule` (like length limits) would not apply to the hot path, effectively bypassing the new security control. + +**Learning:** Performance optimizations that inline or duplicate security logic create maintenance hazards ("drift") where security updates are applied to one path but missed in another. + +**Prevention:** Avoid duplicating validation logic for performance unless strictly necessary. If inlining is required, ensure the inlined logic stays synchronized with the canonical validation function, or add comments explicitly linking them. Ideally, improve the performance of the canonical function instead. diff --git a/main.py b/main.py index 7e758616..2d311013 100644 --- a/main.py +++ b/main.py @@ -1019,6 +1019,10 @@ 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 len(folder_id) > 64: + if log_errors: + log.error(f"Invalid folder ID length (max 64): {len(folder_id)}") + return False if folder_id in (".", "..") or not FOLDER_ID_PATTERN.match(folder_id): if log_errors: log.error(f"Invalid folder ID format: {sanitize_for_log(folder_id)}") @@ -1035,6 +1039,10 @@ def is_valid_rule(rule: str) -> bool: if not rule: return False + # Enforce max length to prevent DoS + if len(rule) > 255: + return False + # Strict whitelist to prevent injection if not RULE_PATTERN.match(rule): return False @@ -1056,6 +1064,10 @@ def is_valid_folder_name(name: str) -> bool: if not name or not name.strip() or not name.isprintable(): return False + # Enforce max length to prevent DoS (matching profile ID limit) + if len(name) > 64: + return False + # Check for dangerous characters (pre-compiled at module level for performance) if any(c in _DANGEROUS_FOLDER_CHARS or c in _BIDI_CONTROL_CHARS for c in name): return False @@ -1131,19 +1143,22 @@ def validate_folder_data(data: Dict[str, Any], url: str) -> bool: ) return False if "rules" in rg: - if not isinstance (rg["rules"], list): - log. error ( - f"Invalid data from {sanitize_for_log(url)} : rule_groups[fil].rules must be a list." + if not isinstance(rg["rules"], list): + log.error( + f"Invalid data from {sanitize_for_log(url)}: rule_groups[{i}].rules must be a list." ) return False -# Ensure each rule within the group is an object (dict), -# because later code treats each rule as a mapping (e.g., rule.get(...)). -for j, rule in enumerate (rgi"rules"1): -if not isinstance (rule, dict): - log. error ( - f"Invalid data from {sanitize_for_log(u rl)}: rule_groups[fiłl.rules[kił] must be an object." - ) - return False + # Ensure each rule within the group is an object (dict), + # because later code treats each rule as a mapping (e.g., rule.get(...)). + for j, rule in enumerate(rg["rules"]): + if not isinstance(rule, dict): + log.error( + f"Invalid data from {sanitize_for_log(url)}: rule_groups[{i}].rules[{j}] must be an object." + ) + return False + + return True + # Lock to protect updates to _api_stats in multi-threaded contexts. # Without this, concurrent increments can lose updates because `+=` is not atomic. @@ -1983,7 +1998,8 @@ def push_rules( if h in existing_rules: continue - if not match_rule(h): + # Enforce max length (255) and regex pattern + if len(h) > 255 or not match_rule(h): log.warning( f"Skipping unsafe rule in {sanitize_for_log(folder_name)}: {sanitize_for_log(h)}" ) diff --git a/tests/test_security_limits.py b/tests/test_security_limits.py new file mode 100644 index 00000000..2c15fd46 --- /dev/null +++ b/tests/test_security_limits.py @@ -0,0 +1,27 @@ +import pytest +from unittest.mock import MagicMock, patch +import main + +def test_is_valid_folder_name_limits(): + # Test length limit: 64 characters (pass) + assert main.is_valid_folder_name("a" * 64) is True + + # Test length limit: 65 characters (fail) + assert main.is_valid_folder_name("a" * 65) is False + +def test_is_valid_rule_limits(): + # Test length limit: 255 characters (pass) + # Using 'a' which matches the regex ^[a-zA-Z0-9.\-_:*/@]+$ + assert main.is_valid_rule("a" * 255) is True + + # Test length limit: 256 characters (fail) + assert main.is_valid_rule("a" * 256) is False + +def test_validate_folder_id_limits(): + # Test length limit: 64 characters (pass) + # Using 'a' which matches ^[a-zA-Z0-9_.-]+$ + assert main.validate_folder_id("a" * 64) is True + + # Test length limit: 65 characters (fail) + # We pass log_errors=False to avoid cluttering output + assert main.validate_folder_id("a" * 65, log_errors=False) is False