From 3573d374f0cd6ffff9e0329b926a9061b4755ca7 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 19 Feb 2026 10:52:51 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[CRITICAL/H?= =?UTF-8?q?IGH]=20Fix=20fail-closed=20DNS=20validation=20logic?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 🚨 Severity: HIGH 💡 Vulnerability: The `validate_hostname` function contained a logic error where it implicitly returned `None` (falsy) for valid domains that passed DNS checks, causing the tool to reject all domain-based URLs (DoS) and only accept IP literals. It also contained unreachable code due to a structural error. 🎯 Impact: The tool was functionally broken for its primary use case (fetching blocklists from domains), and the security control (validating hostnames) was effectively failing closed in an unintended way. 🔧 Fix: Removed unreachable code and added an explicit `return True` for the success path in `validate_hostname`. Also fixed unrelated test failures in `tests/test_push_rules_perf.py`, `tests/test_retry_jitter.py`, and `tests/test_rate_limit.py` to ensure a clean CI. ✅ Verification: Ran `tests/test_hostname_validation.py` and the full test suite (`pytest`), verifying that valid domains are now accepted and invalid ones are still rejected. Co-authored-by: abhimehro <84992105+abhimehro@users.noreply.github.com> --- .jules/sentinel.md | 8 ++++++++ main.py | 5 +---- tests/test_push_rules_perf.py | 1 - tests/test_rate_limit.py | 5 +++-- tests/test_retry_jitter.py | 1 + 5 files changed, 13 insertions(+), 7 deletions(-) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index b6ec1219..038829ea 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -53,3 +53,11 @@ **Learning:** Even trusted APIs should be treated as untrusted sources for critical identifiers used in system operations or path construction. **Prevention:** Whitelist valid characters for all identifiers (e.g. `^[a-zA-Z0-9_.-]+$`) and validate them immediately upon receipt, before any use. + +## 2026-10-24 - Fail-Closed Logic Error in DNS Validation (Broken Availability) + +**Vulnerability:** The `validate_hostname` function implicitly returned `None` (falsy) for valid domains because it lacked a `return True` statement after the successful DNS check loop. This caused the tool to reject *all* domain-based URLs (DoS), only accepting IP literals. It also contained unreachable code due to a structural error. + +**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). diff --git a/main.py b/main.py index a9ce7d7c..da9c0d2c 100644 --- a/main.py +++ b/main.py @@ -929,16 +929,13 @@ def validate_hostname(hostname: str) -> bool: f"Skipping unsafe hostname {sanitize_for_log(hostname)} (resolves to non-global/multicast IP {ip})" ) return False + return True except (socket.gaierror, ValueError, OSError) as e: log.warning( f"Failed to resolve/validate domain {sanitize_for_log(hostname)}: {sanitize_for_log(e)}" ) return False - if not addr_info: - return False - for res in addr_info: - @lru_cache(maxsize=128) def validate_folder_url(url: str) -> bool: diff --git a/tests/test_push_rules_perf.py b/tests/test_push_rules_perf.py index afd0bafb..9d42d682 100644 --- a/tests/test_push_rules_perf.py +++ b/tests/test_push_rules_perf.py @@ -25,7 +25,6 @@ def setUp(self): self.do = 1 self.status = 1 self.existing_rules = set() - self.main = main @patch("main.concurrent.futures.ThreadPoolExecutor") def test_push_rules_single_batch_optimization(self, mock_executor): diff --git a/tests/test_rate_limit.py b/tests/test_rate_limit.py index a7b17299..bbf8cec4 100644 --- a/tests/test_rate_limit.py +++ b/tests/test_rate_limit.py @@ -238,7 +238,8 @@ def test_failed_request_parses_headers(self): with main._rate_limit_lock: assert main._rate_limit_info["remaining"] == 50 - def test_429_without_retry_after_uses_exponential_backoff(self): + @patch('random.random', return_value=0.5) + def test_429_without_retry_after_uses_exponential_backoff(self, mock_random): """Test that 429 without Retry-After falls back to exponential backoff.""" mock_request = MagicMock() mock_response = MagicMock(spec=httpx.Response) @@ -260,7 +261,7 @@ def test_429_without_retry_after_uses_exponential_backoff(self): request_func = MagicMock(side_effect=[error, error, success_response]) # With delay=1, backoff should be: 1s, 2s - # Total wait should be >= 3 seconds + # Total wait should be >= 3 seconds (assuming jitter factor 1.0) start_time = time.time() result = main._retry_request(request_func, max_retries=3, delay=1) elapsed = time.time() - start_time diff --git a/tests/test_retry_jitter.py b/tests/test_retry_jitter.py index e1e0435c..414f901b 100644 --- a/tests/test_retry_jitter.py +++ b/tests/test_retry_jitter.py @@ -124,6 +124,7 @@ def test_four_hundred_errors_still_fail_fast(self): def test_429_rate_limit_retries_with_jitter(self): """Verify 429 rate limit errors retry with jittered backoff.""" response = Mock(status_code=429) + response.headers = {} error = httpx.HTTPStatusError( "Too many requests", request=Mock(),