fix(ip): let a configured whitelist decide alone in the global and route IP stages

.github/workflows/ci.yml

@@ -175,6 +175,9 @@ jobs:
       - name: test_client_ip
         run: php bin/test_client_ip.php
 
+      - name: test_ip_list_precedence
+        run: php bin/test_ip_list_precedence.php
+
   conformance-pipeline:
     name: Conformance pipeline (php ${{ matrix.php }})
     runs-on: ubuntu-latest
@@ -252,7 +255,7 @@ jobs:
         run: |
           mkdir -p build
           php .github/coverage-runner.php --collect bin/test_state.php build/coverage-test_state.php --integration
-          for suite in test_parity_seam test_ratelimit test_geo_rate_limits test_pipeline test_m3b test_m4 test_behavior test_m3c conformance conformance_safety conformance_events conformance_redis test_nfkc test_binary_noise_gate test_recon_context_gate test_recon_raw_view_scan test_body_form_scan test_json_walk test_exempt_ips test_excluded_headers test_cors test_geo_country test_geo_lifecycle test_event_bus test_security_headers test_large_body test_support test_redos_gates test_performance_monitor test_enrichment test_composite_handlers test_json_logging test_pattern_registry test_config_fields test_events_parity test_decorators test_scan_time_budget test_dynamic_rules test_client_ip; do
+          for suite in test_parity_seam test_ratelimit test_geo_rate_limits test_pipeline test_m3b test_m4 test_behavior test_m3c conformance conformance_safety conformance_events conformance_redis test_nfkc test_binary_noise_gate test_recon_context_gate test_recon_raw_view_scan test_body_form_scan test_json_walk test_exempt_ips test_excluded_headers test_cors test_geo_country test_geo_lifecycle test_event_bus test_security_headers test_large_body test_support test_redos_gates test_performance_monitor test_enrichment test_composite_handlers test_json_logging test_pattern_registry test_config_fields test_events_parity test_decorators test_scan_time_budget test_dynamic_rules test_client_ip test_ip_list_precedence; do
             php .github/coverage-runner.php --collect "bin/${suite}.php" "build/coverage-${suite}.php"
           done
 

Makefile

@@ -11,7 +11,7 @@ REDIS_HOST ?= host.docker.internal
 # Runners mirrored from CI (.github/workflows/ci.yml). Redis-backed
 # runners are listed in REDIS_RUNNERS and get REDIS_HOST passed through.
 REDIS_RUNNERS = bin/test_ratelimit.php bin/test_geo_rate_limits.php bin/test_pipeline.php bin/test_m3b.php bin/test_m4.php bin/test_behavior.php bin/conformance_redis.php
-RUNNERS = bin/test_state.php $(REDIS_RUNNERS) bin/test_m3c.php bin/conformance.php bin/conformance_pipeline.php bin/conformance_safety.php bin/conformance_events.php bin/test_nfkc.php bin/test_binary_noise_gate.php bin/test_recon_context_gate.php bin/test_recon_raw_view_scan.php bin/test_body_form_scan.php bin/test_json_walk.php bin/test_exempt_ips.php bin/test_excluded_headers.php bin/test_cors.php bin/test_geo_country.php bin/test_security_headers.php bin/test_behavior.php bin/test_large_body.php bin/test_redos_gates.php bin/test_performance_monitor.php bin/test_geo_lifecycle.php bin/test_event_bus.php bin/test_decorators.php bin/test_scan_time_budget.php bin/test_dynamic_rules.php bin/test_client_ip.php
+RUNNERS = bin/test_state.php $(REDIS_RUNNERS) bin/test_m3c.php bin/conformance.php bin/conformance_pipeline.php bin/conformance_safety.php bin/conformance_events.php bin/test_nfkc.php bin/test_binary_noise_gate.php bin/test_recon_context_gate.php bin/test_recon_raw_view_scan.php bin/test_body_form_scan.php bin/test_json_walk.php bin/test_exempt_ips.php bin/test_excluded_headers.php bin/test_cors.php bin/test_geo_country.php bin/test_security_headers.php bin/test_behavior.php bin/test_large_body.php bin/test_redos_gates.php bin/test_performance_monitor.php bin/test_geo_lifecycle.php bin/test_event_bus.php bin/test_decorators.php bin/test_scan_time_budget.php bin/test_dynamic_rules.php bin/test_client_ip.php bin/test_ip_list_precedence.php
 
 .PHONY: install test lint bump-version clean
 

bin/test_geo_country.php

@@ -503,14 +503,15 @@ function geoStashFor(GuardEngine $engine, string $ip): ?array
 $response = $engine->execute($request);
 $t->ok($response !== null && $response->statusCode() === 403, 'route ip_blacklist denies its match 403');
 
-// A route whitelist match never relaxes the global lists (the reference
-// keeps the global blacklist enforced afterwards).
+// A configured route whitelist skips the global lists for the request (the
+// reference skip_ip_lists gate), so its match is not denied by the global
+// blacklist.
 $engine = geoEngine(['blacklist' => ['192.0.2.7']]);
 $request = $routeRequest('/private');
 $request->state()->routeConfig = new RouteConfig(ipWhitelist: ['192.0.2.7']);
 $request->state()->clientIp = '192.0.2.7';
 $response = $engine->execute($request);
-$t->ok($response !== null && $response->statusCode() === 403, 'a route whitelist match still hits the global blacklist');
+$t->ok($response === null, 'a route whitelist match skips the global blacklist');
 
 // A route ipWhitelist clears the identity flags for the request (the
 // reference _resolve_is_whitelisted/_resolve_is_exempt skip_ip_lists gate).

bin/test_ip_list_precedence.php

@@ -0,0 +1,80 @@
+<?php
+
+declare(strict_types=1);
+
+use RenzoFranceschini\GuardCore\Config\SecurityConfig;
+use RenzoFranceschini\GuardCore\Engine\GuardEngine;
+use RenzoFranceschini\GuardCore\Request\SimpleGuardRequest;
+use RenzoFranceschini\GuardCore\Routing\RouteConfig;
+
+require __DIR__ . '/../vendor/autoload.php';
+
+// IP list precedence parity with the reference engine (guard-core Python,
+// _utils/access_control.py _check_ip_lists_detail and
+// core/checks/helpers.py check_route_ip_access): a configured whitelist
+// decides alone and the blacklist is consulted only when no whitelist is
+// configured. tests/data/ip_list_precedence.json is generated by running the
+// reference over every row, never hand-written. In-memory, no Redis.
+
+$passed = 0;
+$failed = 0;
+
+$rows = json_decode((string) file_get_contents(__DIR__ . '/../tests/data/ip_list_precedence.json'), true, flags: JSON_THROW_ON_ERROR);
+
+foreach ($rows as $row) {
+    if ($row['ip'] === 'unknown') {
+        continue;
+    }
+    $label = 'wl=' . json_encode($row['whitelist']) . ' bl=' . json_encode($row['blacklist']) . ' ip=' . $row['ip'];
+
+    $engine = new GuardEngine(new SecurityConfig(
+        enableRedis: false,
+        whitelist: $row['whitelist'] === [] ? null : $row['whitelist'],
+        blacklist: $row['blacklist'],
+    ));
+    $status = $engine->execute(new SimpleGuardRequest(urlPath: '/', clientHost: $row['ip']))?->statusCode();
+    $expected = $row['global'] ? null : 403;
+    if ($status === $expected) {
+        $passed++;
+    } else {
+        $failed++;
+        echo "FAIL global {$label}: expected " . var_export($expected, true) . ', got ' . var_export($status, true) . "\n";
+    }
+
+    $engine = new GuardEngine(new SecurityConfig(enableRedis: false));
+    $request = new SimpleGuardRequest(urlPath: '/', clientHost: $row['ip']);
+    $request->state()->routeConfig = new RouteConfig(ipWhitelist: $row['whitelist'], ipBlacklist: $row['blacklist']);
+    $status = $engine->execute($request)?->statusCode();
+    $expected = $row['route'] === false ? 403 : null;
+    if ($status === $expected) {
+        $passed++;
+    } else {
+        $failed++;
+        echo "FAIL route {$label}: expected " . var_export($expected, true) . ', got ' . var_export($status, true) . "\n";
+    }
+}
+
+// A configured route whitelist skips the global lists for the request (the
+// reference skip_ip_lists gate); the route stage still runs first.
+$rows = json_decode((string) file_get_contents(__DIR__ . '/../tests/data/ip_list_route_global.json'), true, flags: JSON_THROW_ON_ERROR);
+foreach ($rows as $row) {
+    $label = 'gwl=' . json_encode($row['globalWhitelist']) . ' gbl=' . json_encode($row['globalBlacklist'])
+        . ' rwl=' . json_encode($row['routeWhitelist']) . ' rbl=' . json_encode($row['routeBlacklist']) . ' ip=' . $row['ip'];
+    $engine = new GuardEngine(new SecurityConfig(
+        enableRedis: false,
+        whitelist: $row['globalWhitelist'] === [] ? null : $row['globalWhitelist'],
+        blacklist: $row['globalBlacklist'],
+    ));
+    $request = new SimpleGuardRequest(urlPath: '/', clientHost: $row['ip']);
+    $request->state()->routeConfig = new RouteConfig(ipWhitelist: $row['routeWhitelist'], ipBlacklist: $row['routeBlacklist']);
+    $allowed = $engine->execute($request) === null;
+    if ($allowed === $row['allowed']) {
+        $passed++;
+    } else {
+        $failed++;
+        echo "FAIL route+global {$label}: expected " . var_export($row['allowed'], true) . ', got ' . var_export($allowed, true) . "\n";
+    }
+}
+
+echo "\ntest_ip_list_precedence: {$passed} passed, {$failed} failed\n";
+exit($failed === 0 ? 0 : 1);

src/Pipeline/Checks/IpSecurityCheck.php

@@ -73,13 +73,12 @@ public function check(GuardRequest $request): ?GuardResponse
         $skipCountries = false;
 
         // The route IP/country stage (check_route_ip_access,
-        // guard_core/core/checks/helpers.py): the route blacklist denies
-        // first, a configured route whitelist takes over the route verdict
-        // (a miss denies, a match passes the route stage), and the route
+        // guard_core/core/checks/helpers.py): a configured route whitelist
+        // decides alone (a miss denies, a match passes the route stage; the
+        // route blacklist is consulted only without a whitelist), and the route
         // country verdict denies its match or a miss under a restrictive
-        // allowlist. The global lists are still enforced afterwards, so a
-        // route whitelist match never relaxes them; it only clears the
-        // identity flags, and a route allow_countries match clears the
+        // allowlist. A configured route whitelist skips the global
+        // lists and clears the identity flags, and a route allow_countries match clears the
         // global country stage (the reference _route_country_whitelist_matched).
         if ($routeConfig !== null) {
             $routeResponse = $this->checkRouteIpAccess($request, $clientIp, $routeConfig);
@@ -96,13 +95,14 @@ public function check(GuardRequest $request): ?GuardResponse
         $whitelist = $this->config->whitelist;
         $whitelistActive = $whitelist !== null && $whitelist !== [];
 
-        foreach ($this->config->blacklist as $entry) {
-            if (CanonicalIp::ipInList($clientIp, [$entry])) {
-                return $this->deny($request, $clientIp, self::GENERIC_LIST_BLOCK_REASON);
-            }
-        }
-
-        if ($whitelistActive) {
+        // The reference _check_ip_lists_detail: a configured route whitelist
+        // skips the global lists for the request (skip_ip_lists); otherwise a
+        // configured whitelist decides alone (the blacklist is not consulted)
+        // and the blacklist applies only when no whitelist is configured.
+        if ($routeOverridesIpLists) {
+            // The global lists, and the whitelist-driven country skip, are
+            // not consulted for this request.
+        } elseif ($whitelistActive) {
             $allowed = false;
             foreach ($whitelist as $entry) {
                 if (CanonicalIp::ipInList($clientIp, [$entry])) {
@@ -113,13 +113,19 @@ public function check(GuardRequest $request): ?GuardResponse
             if (!$allowed) {
                 return $this->deny($request, $clientIp, self::GENERIC_LIST_BLOCK_REASON);
             }
-            $request->state()->isWhitelisted = !$routeOverridesIpLists;
+            $request->state()->isWhitelisted = true;
             // A global whitelist match skips the country stage (the
             // reference sets skip_countries from the whitelist
             // membership), so a whitelisted IP is never country blocked.
             // The exempt resolution below still runs, like the reference
             // whitelist branch resolving the exempt flag too.
             $skipCountries = true;
+        } else {
+            foreach ($this->config->blacklist as $entry) {
+                if (CanonicalIp::ipInList($clientIp, [$entry])) {
+                    return $this->deny($request, $clientIp, self::GENERIC_LIST_BLOCK_REASON);
+                }
+            }
         }
 
         // Country stage, mirroring _resolve_country_verdict plus
@@ -213,18 +219,18 @@ public function check(GuardRequest $request): ?GuardResponse
     private const GENERIC_LIST_BLOCK_REASON = 'IP %s not in global allowlist/blocklist';
 
     /**
-     * check_route_ip_access: the route blacklist denies first, then a
-     * configured route whitelist takes over the route verdict, then the
-     * route country verdict (a blocked-country match denies, a restrictive
+     * check_route_ip_access: a configured route whitelist decides alone
+     * (the route blacklist is consulted only when there is no whitelist),
+     * then the route country verdict (a blocked-country match denies, a restrictive
      * allowlist denies a miss). The deny reason and payload shapes mirror
      * the reference ip_security check.
      */
     private function checkRouteIpAccess(GuardRequest $request, string $clientIp, RouteConfig $routeConfig): ?GuardResponse
     {
         $ipBlocked = false;
-        if ($routeConfig->ipBlacklist !== [] && $this->ipInList($clientIp, $routeConfig->ipBlacklist)) {
-            $ipBlocked = true;
-        } elseif ($routeConfig->ipWhitelist !== [] && !$this->ipInList($clientIp, $routeConfig->ipWhitelist)) {
+        if ($routeConfig->ipWhitelist !== []) {
+            $ipBlocked = !$this->ipInList($clientIp, $routeConfig->ipWhitelist);
+        } elseif ($routeConfig->ipBlacklist !== [] && $this->ipInList($clientIp, $routeConfig->ipBlacklist)) {
             $ipBlocked = true;
         }