Merge pull request 'fix(security): improve TeapotController header sanitization' (#21) from security/improve-teapot-sanitization into main

src/Mod/Hub/Controllers/TeapotController.php

@@ -44,7 +44,7 @@ class TeapotController
             HoneypotHit::create([
                 'ip_address' => $ip,
                 'user_agent' => substr($userAgent ?? '', 0, 1000),
-                'referer' => substr($request->header('Referer', ''), 0, 2000),
+                'referer' => $this->sanitizeReferer($request->header('Referer', '')),
                 'path' => $path,
                 'method' => $request->method(),
                 'headers' => $this->sanitizeHeaders($request->headers->all()),
@@ -59,7 +59,7 @@ class TeapotController
         // Auto-block critical hits (active probing) if enabled in config.
         // Skip localhost in dev to avoid blocking yourself.
         $autoBlockEnabled = config('core.bouncer.honeypot.auto_block_critical', true);
-        $isLocalhost = in_array($ip, ['127.0.0.1', '::1'], true);
+        $isLocalhost = $this->isPrivateIp($ip);
         $isCritical = $severity === HoneypotHit::getSeverityCritical();
 
         if ($autoBlockEnabled && $isCritical && ! $isLocalhost) {
@@ -75,17 +75,56 @@ class TeapotController
     }
 
     /**
-     * Remove sensitive headers before storing.
+     * Validate and truncate the referer header.
+     */
+    protected function sanitizeReferer(string $referer): string
+    {
+        if ($referer === '') {
+            return '';
+        }
+
+        if (filter_var($referer, FILTER_VALIDATE_URL) === false) {
+            return 'invalid-url';
+        }
+
+        return substr($referer, 0, 2000);
+    }
+
+    /**
+     * Whitelist headers useful for bot detection before storing.
      */
     protected function sanitizeHeaders(array $headers): array
     {
-        $sensitive = ['cookie', 'authorization', 'x-csrf-token', 'x-xsrf-token'];
+        $allowed = [
+            'user-agent',
+            'accept',
+            'accept-language',
+            'accept-encoding',
+            'referer',
+            'origin',
+            'x-requested-with',
+            'x-forwarded-for',
+            'x-real-ip',
+            'cf-connecting-ip',
+            'x-client-ip',
+        ];
 
-        foreach ($sensitive as $key) {
+        return array_intersect_key($headers, array_flip($allowed));
-            unset($headers[$key]);
+    }
-        }
 
-        return $headers;
+    /**
+     * Check whether an IP address is private or reserved.
+     */
+    protected function isPrivateIp(string $ip): bool
+    {
+        // Normalise IPv4-mapped IPv6 addresses
+        $ip = preg_replace('/^::ffff:/i', '', $ip);
+
+        return filter_var(
+            $ip,
+            FILTER_VALIDATE_IP,
+            FILTER_FLAG_NO_PRIV_RANGE | FILTER_FLAG_NO_RES_RANGE
+        ) === false;
     }
 
     /**

tests/Feature/Honeypot/TeapotTest.php

@@ -49,6 +49,8 @@ beforeEach(function () {
 
     // Clear rate limiter between tests
     RateLimiter::clear('honeypot:log:192.168.1.100');
+    RateLimiter::clear('honeypot:log:203.0.113.50');
+    RateLimiter::clear('honeypot:log:127.0.0.1');
 });
 
 afterEach(function () {
@@ -472,13 +474,14 @@ describe('TeapotController', function () {
     it('records critical severity for admin paths', function () {
         $controller = new TeapotController();
         $request = Request::create('/admin', 'GET');
+        $request->server->set('REMOTE_ADDR', '203.0.113.50');
 
         $mockGeoIp = Mockery::mock(DetectLocation::class);
         $mockGeoIp->shouldReceive('getCountryCode')->andReturn(null);
         $mockGeoIp->shouldReceive('getCity')->andReturn(null);
         app()->instance(DetectLocation::class, $mockGeoIp);
 
-        // Critical path should trigger auto-block for non-localhost
+        // Critical path should trigger auto-block for non-private IPs
         $mockBlocklist = Mockery::mock(BlocklistService::class);
         $mockBlocklist->shouldReceive('block')->once();
         app()->instance(BlocklistService::class, $mockBlocklist);
@@ -490,14 +493,16 @@ describe('TeapotController', function () {
         expect($response->headers->get('X-Severity'))->toBe('critical');
     });
 
-    it('sanitizes sensitive headers before storing', function () {
+    it('uses whitelist to only store allowed headers', function () {
         $controller = new TeapotController();
         $request = Request::create('/teapot', 'GET');
         $request->headers->set('User-Agent', 'TestBot/1.0');
+        $request->headers->set('Accept-Language', 'en-GB');
         $request->headers->set('Cookie', 'session=secret123');
         $request->headers->set('Authorization', 'Bearer token123');
         $request->headers->set('X-CSRF-Token', 'csrf123');
         $request->headers->set('X-Custom-Header', 'safe-value');
+        $request->headers->set('X-Forwarded-For', '203.0.113.50');
 
         $mockGeoIp = Mockery::mock(DetectLocation::class);
         $mockGeoIp->shouldReceive('getCountryCode')->andReturn(null);
@@ -513,13 +518,18 @@ describe('TeapotController', function () {
         $hit = HoneypotHit::first();
         $headers = $hit->headers;
 
-        // Sensitive headers should be removed
+        // Sensitive headers must not be stored
         expect($headers)->not->toHaveKey('cookie');
         expect($headers)->not->toHaveKey('authorization');
         expect($headers)->not->toHaveKey('x-csrf-token');
 
-        // Safe headers should be preserved
+        // Non-whitelisted headers must not be stored
-        expect($headers)->toHaveKey('x-custom-header');
+        expect($headers)->not->toHaveKey('x-custom-header');
+
+        // Whitelisted headers should be preserved
+        expect($headers)->toHaveKey('user-agent');
+        expect($headers)->toHaveKey('accept-language');
+        expect($headers)->toHaveKey('x-forwarded-for');
     });
 
     it('truncates long user agent strings', function () {
@@ -573,6 +583,140 @@ describe('TeapotController', function () {
     });
 });
 
+// =============================================================================
+// Private IP Detection Tests
+// =============================================================================
+
+describe('Private IP detection', function () {
+    it('detects IPv4 loopback as private', function () {
+        $controller = new TeapotController();
+        $reflection = new ReflectionMethod($controller, 'isPrivateIp');
+        $reflection->setAccessible(true);
+
+        expect($reflection->invoke($controller, '127.0.0.1'))->toBeTrue();
+    });
+
+    it('detects IPv6 loopback as private', function () {
+        $controller = new TeapotController();
+        $reflection = new ReflectionMethod($controller, 'isPrivateIp');
+        $reflection->setAccessible(true);
+
+        expect($reflection->invoke($controller, '::1'))->toBeTrue();
+    });
+
+    it('detects IPv4-mapped IPv6 loopback as private', function () {
+        $controller = new TeapotController();
+        $reflection = new ReflectionMethod($controller, 'isPrivateIp');
+        $reflection->setAccessible(true);
+
+        expect($reflection->invoke($controller, '::ffff:127.0.0.1'))->toBeTrue();
+    });
+
+    it('detects private network ranges as private', function () {
+        $controller = new TeapotController();
+        $reflection = new ReflectionMethod($controller, 'isPrivateIp');
+        $reflection->setAccessible(true);
+
+        expect($reflection->invoke($controller, '10.0.0.1'))->toBeTrue();
+        expect($reflection->invoke($controller, '172.17.0.1'))->toBeTrue();
+        expect($reflection->invoke($controller, '192.168.1.1'))->toBeTrue();
+    });
+
+    it('allows public IP addresses', function () {
+        $controller = new TeapotController();
+        $reflection = new ReflectionMethod($controller, 'isPrivateIp');
+        $reflection->setAccessible(true);
+
+        expect($reflection->invoke($controller, '8.8.8.8'))->toBeFalse();
+        expect($reflection->invoke($controller, '203.0.113.50'))->toBeFalse();
+    });
+
+    it('skips auto-block for private IPs on critical paths', function () {
+        $controller = new TeapotController();
+        $request = Request::create('/admin', 'GET');
+        $request->server->set('REMOTE_ADDR', '192.168.1.100');
+
+        $mockGeoIp = Mockery::mock(DetectLocation::class);
+        $mockGeoIp->shouldReceive('getCountryCode')->andReturn(null);
+        $mockGeoIp->shouldReceive('getCity')->andReturn(null);
+        app()->instance(DetectLocation::class, $mockGeoIp);
+
+        $mockBlocklist = Mockery::mock(BlocklistService::class);
+        $mockBlocklist->shouldNotReceive('block');
+        app()->instance(BlocklistService::class, $mockBlocklist);
+
+        $response = $controller($request);
+
+        expect($response->getStatusCode())->toBe(418);
+    });
+});
+
+// =============================================================================
+// Referer Validation Tests
+// =============================================================================
+
+describe('Referer validation', function () {
+    it('stores valid URL referers', function () {
+        $controller = new TeapotController();
+        $request = Request::create('/teapot', 'GET');
+        $request->headers->set('Referer', 'https://example.com/page');
+
+        $mockGeoIp = Mockery::mock(DetectLocation::class);
+        $mockGeoIp->shouldReceive('getCountryCode')->andReturn(null);
+        $mockGeoIp->shouldReceive('getCity')->andReturn(null);
+        app()->instance(DetectLocation::class, $mockGeoIp);
+
+        $mockBlocklist = Mockery::mock(BlocklistService::class);
+        $mockBlocklist->shouldReceive('block')->andReturn(null);
+        app()->instance(BlocklistService::class, $mockBlocklist);
+
+        $controller($request);
+
+        $hit = HoneypotHit::first();
+        expect($hit->referer)->toBe('https://example.com/page');
+    });
+
+    it('replaces invalid referers with marker', function () {
+        $controller = new TeapotController();
+        $request = Request::create('/teapot', 'GET');
+        $request->headers->set('Referer', '<script>alert("xss")</script>');
+
+        $mockGeoIp = Mockery::mock(DetectLocation::class);
+        $mockGeoIp->shouldReceive('getCountryCode')->andReturn(null);
+        $mockGeoIp->shouldReceive('getCity')->andReturn(null);
+        app()->instance(DetectLocation::class, $mockGeoIp);
+
+        $mockBlocklist = Mockery::mock(BlocklistService::class);
+        $mockBlocklist->shouldReceive('block')->andReturn(null);
+        app()->instance(BlocklistService::class, $mockBlocklist);
+
+        $controller($request);
+
+        $hit = HoneypotHit::first();
+        expect($hit->referer)->toBe('invalid-url');
+    });
+
+    it('stores empty string for missing referer', function () {
+        $controller = new TeapotController();
+        $request = Request::create('/teapot', 'GET');
+        // No Referer header set
+
+        $mockGeoIp = Mockery::mock(DetectLocation::class);
+        $mockGeoIp->shouldReceive('getCountryCode')->andReturn(null);
+        $mockGeoIp->shouldReceive('getCity')->andReturn(null);
+        app()->instance(DetectLocation::class, $mockGeoIp);
+
+        $mockBlocklist = Mockery::mock(BlocklistService::class);
+        $mockBlocklist->shouldReceive('block')->andReturn(null);
+        app()->instance(BlocklistService::class, $mockBlocklist);
+
+        $controller($request);
+
+        $hit = HoneypotHit::first();
+        expect($hit->referer)->toBe('');
+    });
+});
+
 // =============================================================================
 // Integration Tests
 // =============================================================================
@@ -581,6 +725,7 @@ describe('Honeypot integration', function () {
     it('creates hit record with all fields populated', function () {
         $controller = new TeapotController();
         $request = Request::create('/wp-admin/admin.php', 'POST');
+        $request->server->set('REMOTE_ADDR', '203.0.113.50');
         $request->headers->set('User-Agent', 'python-requests/2.28.1');
         $request->headers->set('Referer', 'https://malicious-site.com/scanner');
         $request->headers->set('Accept-Language', 'en-US,en;q=0.9');