Request / response ip filter (#28)

bram123 · web-flow · commit 26bcd92cfbbe · 2024-07-04T10:26:13.000+02:00
* New trusted_ips setting for request and response config
- to only trust request headers from specific IPs
- to only send response headers for requests from specific IPs

Turned config trust_request_header into request-&gt;trust_header
Turned config trust_request_header into response-&gt;send_header
diff --git a/docs/configuration/tracecontext.md b/docs/configuration/tracecontext.md
@@ -18,10 +18,14 @@ return static function (SymfonyTraceConfig $config): void {
     // If true a value in the `traceparent` header in the request
     // will be used and parsed to get the trace ID for the rest of the request. If false
     // those values are ignored and new trace ID's are generated.
-    $config->trustRequestHeader(true);
+    $config->request()
+        ->trustHeader(true)
+        ->trustedIps(env('TRUSTED_IPS')); // Only trust the header from these IP's
 
     // Whether to send the trace details in the response headers. This is turned on by default.
-    $config->sendResponseHeader(true);
+    $config->response()
+        ->sendHeader(true)
+        ->trustedIps(env('TRUSTED_IPS')); // Only send the header to these IP's
 
     // The service key of an object that implements
     // DR\SymfonyTraceBundle\TraceStorageInterface
diff --git a/docs/configuration/traceid.md b/docs/configuration/traceid.md
@@ -28,10 +28,14 @@ return static function (SymfonyTraceConfig $config): void {
     // on by default. If true a value in the `X-Trace-Id` header in the request
     // will be used as the trace ID for the rest of the request. If false
     // those values are ignored.
-    $config->trustRequestHeader(true);
+    $config->request()
+        ->trustHeader(true)
+        ->trustedIps(env('TRUSTED_IPS')); // Only trust the header from these IP's
 
     // Whether to send the trace details in the response headers. This is turned on by default.
-    $config->sendResponseHeader(true);
+    $config->response()
+        ->sendHeader(true)
+        ->trustedIps(env('TRUSTED_IPS')); // Only send the header to these IP's
 
     $config->traceid()
         // The header which the bundle inspects for the incoming trace ID
@@ -72,3 +76,4 @@ return static function (SymfonyTraceConfig $config): void {
         ->enabled(true)
         ->hubService(HubInterface::class);
 };
+```
diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
@@ -1,7 +1,7 @@
 parameters:
 	ignoreErrors:
 		-
-			message: "#^Parameter \\#1 \\$mergedConfig \\(array\\{traceMode\\: 'tracecontext'\\|'traceid', traceid\\: array\\{request_header\\: string, response_header\\: string, generator_service\\: string\\|null\\}, trust_request_header\\: bool, send_response_header\\: bool, storage_service\\: string\\|null, enable_monolog\\: bool, enable_console\\: bool, console\\: array\\{enabled\\: bool, trace_id\\: string\\|null\\}, \\.\\.\\.\\}\\) of method DR\\\\SymfonyTraceBundle\\\\DependencyInjection\\\\SymfonyTraceExtension\\:\\:loadInternal\\(\\) should be contravariant with parameter \\$mergedConfig \\(array\\) of method Symfony\\\\Component\\\\HttpKernel\\\\DependencyInjection\\\\ConfigurableExtension\\:\\:loadInternal\\(\\)$#"
+			message: "#^Parameter \\#1 \\$mergedConfig \\(array\\{traceMode\\: 'tracecontext'\\|'traceid', traceid\\: array\\{request_header\\: string, response_header\\: string, generator_service\\: string\\|null\\}, request\\: array\\{trust_header\\: bool, trusted_ips\\: array\\<string\\>\\|string\\|null\\}, response\\: array\\{send_header\\: bool, trusted_ips\\: array\\<string\\>\\|string\\|null\\}, storage_service\\: string\\|null, enable_monolog\\: bool, enable_console\\: bool, console\\: array\\{enabled\\: bool, trace_id\\: string\\|null\\}, \\.\\.\\.\\}\\) of method DR\\\\SymfonyTraceBundle\\\\DependencyInjection\\\\SymfonyTraceExtension\\:\\:loadInternal\\(\\) should be contravariant with parameter \\$mergedConfig \\(array\\) of method Symfony\\\\Component\\\\HttpKernel\\\\DependencyInjection\\\\ConfigurableExtension\\:\\:loadInternal\\(\\)$#"
 			count: 1
 			path: src/DependencyInjection/SymfonyTraceExtension.php
 
diff --git a/src/DependencyInjection/Configuration.php b/src/DependencyInjection/Configuration.php
@@ -53,14 +53,8 @@ public function getConfigTreeBuilder(): TreeBuilder
                     ->end()
                 ->end()
             ->end()
-            ->booleanNode('trust_request_header')
-                ->defaultTrue()
-                ->info("Whether or not to trust the incoming request's `Trace-Id` header as a real ID")
-            ->end()
-            ->booleanNode('send_response_header')
-                ->defaultTrue()
-                ->info("Whether or not to send a response header with the trace ID. Defaults to true")
-            ->end()
+            ->append($this->createRequestConfiguration())
+            ->append($this->createResponseConfiguration())
             ->scalarNode('storage_service')
                 ->info('The service name for trace ID storage. Defaults to `TraceStorage`')
             ->end()
@@ -101,6 +95,50 @@ public function getConfigTreeBuilder(): TreeBuilder
         return $tree;
     }
 
+    private function createRequestConfiguration(): NodeDefinition
+    {
+        $node = (new TreeBuilder('request'))->getRootNode();
+        $node
+            ->addDefaultsIfNotSet()
+            ->children()
+                ->booleanNode('trust_header')
+                    ->defaultTrue()
+                    ->info("Whether or not to trust the incoming request's headers as a real TraceID")
+                ->end()
+                ->scalarNode('trusted_ips')
+                    ->info(
+                        "Only trust incoming request's headers if the request comes from one of these IPs (supports ranges/masks). " .
+                        "Accepts a string-array, comma separated string or null. Defaults to null, accepting all request IPs. "
+                    )
+                    ->defaultNull()
+                ->end()
+            ->end();
+
+        return $node;
+    }
+
+    private function createResponseConfiguration(): NodeDefinition
+    {
+        $node = (new TreeBuilder('response'))->getRootNode();
+        $node
+            ->addDefaultsIfNotSet()
+            ->children()
+                ->booleanNode('send_header')
+                    ->defaultTrue()
+                    ->info("Whether or not to send a response header with the trace ID. Defaults to true")
+                ->end()
+                ->scalarNode('trusted_ips')
+                    ->info(
+                        "Only send response if the request comes from one of these IPs (supports ranges/masks) " .
+                        "Accepts a string-array, comma separated string or null. Defaults to null, accepting all request IPs. "
+                    )
+                    ->defaultNull()
+                ->end()
+            ->end();
+
+        return $node;
+    }
+
     private function createHttpClientConfiguration(): NodeDefinition
     {
         $node = (new TreeBuilder('http_client'))->getRootNode();
diff --git a/src/DependencyInjection/SymfonyTraceExtension.php b/src/DependencyInjection/SymfonyTraceExtension.php
@@ -38,8 +38,14 @@
  *          response_header: string,
  *          generator_service: ?string,
  *      },
- *      trust_request_header: bool,
- *      send_response_header: bool,
+ *      request: array{
+ *          trust_header: bool,
+ *          trusted_ips: string[]|string|null,
+ *      },
+ *      response: array{
+ *          send_header: bool,
+ *          trusted_ips: string[]|string|null,
+ *      },
  *      storage_service: ?string,
  *      enable_monolog: bool,
  *      enable_console: bool,
@@ -84,8 +90,10 @@ protected function loadInternal(array $mergedConfig, ContainerBuilder $container
         $container->register(TraceSubscriber::class)
             ->setArguments(
                 [
-                    $mergedConfig['trust_request_header'],
-                    $mergedConfig['send_response_header'],
+                    $mergedConfig['request']['trust_header'],
+                    $mergedConfig['request']['trusted_ips'],
+                    $mergedConfig['response']['send_header'],
+                    $mergedConfig['response']['trusted_ips'],
                     new Reference(TraceServiceInterface::class),
                     new Reference(TraceStorageInterface::class)
                 ]
diff --git a/src/EventSubscriber/TraceSubscriber.php b/src/EventSubscriber/TraceSubscriber.php
@@ -7,6 +7,8 @@
 use DR\SymfonyTraceBundle\Service\TraceServiceInterface;
 use DR\SymfonyTraceBundle\TraceStorageInterface;
 use Symfony\Component\EventDispatcher\EventSubscriberInterface;
+use Symfony\Component\HttpFoundation\IpUtils;
+use Symfony\Component\HttpFoundation\Request;
 use Symfony\Component\HttpKernel\Event\RequestEvent;
 use Symfony\Component\HttpKernel\Event\ResponseEvent;
 use Symfony\Component\HttpKernel\KernelEvents;
@@ -18,12 +20,16 @@
 final class TraceSubscriber implements EventSubscriberInterface
 {
     /**
-     * @param bool                      $trustRequest Trust the value from the request? Or generate?
-     * @param TraceStorageInterface     $traceStorage The trace ID storage, used to store the ID from the request or a newly generated ID.
+     * @param bool $trustRequest                       Trust the value from the request? Or generate?
+     * @param string[]|string|null $trustedIpsRequest  The IPs to trust the request from.
+     * @param string[]|string|null $trustedIpsResponse The IPs to send the response header to.
+     * @param TraceStorageInterface $traceStorage      The trace ID storage, used to store the ID from the request or a newly generated ID.
      */
     public function __construct(
         private readonly bool                  $trustRequest,
+        private readonly array|string|null     $trustedIpsRequest,
         private readonly bool                  $sendResponseHeader,
+        private readonly array|string|null     $trustedIpsResponse,
         private readonly TraceServiceInterface $traceService,
         private readonly TraceStorageInterface $traceStorage,
     ) {
@@ -35,7 +41,7 @@ public function __construct(
     public static function getSubscribedEvents(): array
     {
         return [
-            KernelEvents::REQUEST  => ['onRequest', 100],
+            KernelEvents::REQUEST => ['onRequest', 100],
             KernelEvents::RESPONSE => ['onResponse', -99],
         ];
     }
@@ -46,10 +52,11 @@ public function onRequest(RequestEvent $event): void
             return;
         }
 
-        $request = $event->getRequest();
+        $request      = $event->getRequest();
+        $trustRequest = $this->trustRequest && $this->isTrustedRequest($request, $this->trustedIpsRequest);
 
         // If we trust the request, check if the traceService supports it and use the request data
-        if ($this->trustRequest && $this->traceService->supports($request)) {
+        if ($trustRequest && $this->traceService->supports($request)) {
             $this->traceStorage->setTrace($this->traceService->getRequestTrace($request));
 
             return;
@@ -69,8 +76,26 @@ public function onResponse(ResponseEvent $event): void
             return;
         }
 
-        if ($this->sendResponseHeader) {
+        $request   = $event->getRequest();
+        $sendHeader = $this->sendResponseHeader && $this->isTrustedRequest($request, $this->trustedIpsResponse);
+        if ($sendHeader) {
             $this->traceService->handleResponse($event->getResponse(), $this->traceStorage->getTrace());
         }
     }
+
+    /**
+     * @param string[]|string|null $trustedIps
+     */
+    private function isTrustedRequest(Request $request, array|string|null $trustedIps): bool
+    {
+        if ($trustedIps === null) {
+            return true;
+        }
+
+        if (is_string($trustedIps)) {
+            $trustedIps = array_map('trim', explode(',', $trustedIps));
+        }
+
+        return IpUtils::checkIp((string)$request->getClientIp(), $trustedIps);
+    }
 }
diff --git a/tests/Functional/App/config/tracecontext.yml b/tests/Functional/App/config/tracecontext.yml
@@ -1,6 +1,7 @@
 symfony_trace:
   traceMode: 'tracecontext'
-  trust_request_header: true
+  request:
+    trust_header: true
   storage_service: request.id.storage
   enable_messenger: true
   http_client:
diff --git a/tests/Functional/App/config/traceid.yml b/tests/Functional/App/config/traceid.yml
@@ -3,7 +3,8 @@ symfony_trace:
   traceid:
     request_header: Trace-Id
     response_header: Trace-Id
-  trust_request_header: true
+  request:
+    trust_header: true
   storage_service: request.id.storage
   enable_messenger: true
   console:
diff --git a/tests/Unit/EventSubscriber/TraceSubscriberTest.php b/tests/Unit/EventSubscriber/TraceSubscriberTest.php
@@ -34,7 +34,7 @@ protected function setUp(): void
     {
         $this->service   = $this->createMock(TraceServiceInterface::class);
         $this->storage   = $this->createMock(TraceStorageInterface::class);
-        $this->listener  = new TraceSubscriber(true, true, $this->service, $this->storage);
+        $this->listener  = new TraceSubscriber(true, null, true, null, $this->service, $this->storage);
 
         $this->dispatcher = new EventDispatcher();
         $this->dispatcher->addSubscriber($this->listener);
@@ -74,7 +74,7 @@ public function testListenerSetsTheTraceToStorageWhenFoundInRequestHeaders(): vo
     public function testListenerIgnoresIncomingRequestHeadersWhenTrustRequestIsFalse(): void
     {
         $this->dispatcher->removeSubscriber($this->listener);
-        $this->dispatcher->addSubscriber(new TraceSubscriber(false, true, $this->service, $this->storage));
+        $this->dispatcher->addSubscriber(new TraceSubscriber(false, null, true, null, $this->service, $this->storage));
 
         $trace = new TraceContext();
         $this->service->expects(static::never())->method('supports');
@@ -138,7 +138,7 @@ public function testRequestSetsIdOnResponse(): void
     public function testListenerDoesNothingToResponseWithoutMasterRequestWhenSendResponseHeaderIsFalse(): void
     {
         $this->dispatcher->removeSubscriber($this->listener);
-        $this->dispatcher->addSubscriber(new TraceSubscriber(false, false, $this->service, $this->storage));
+        $this->dispatcher->addSubscriber(new TraceSubscriber(false, null, false, null, $this->service, $this->storage));
 
         $this->storage->expects(static::never())->method('getTrace');
         $this->service->expects(static::never())->method('handleResponse');
@@ -148,4 +148,66 @@ public function testListenerDoesNothingToResponseWithoutMasterRequestWhenSendRes
             KernelEvents::RESPONSE
         );
     }
+
+    public function testListenerSetsTheTraceToStorageWhenFoundInTrustedRequestHeaders(): void
+    {
+        $listener   = new TraceSubscriber(true, '127.0.0.1', true, '127.0.0.1', $this->service, $this->storage);
+        $dispatcher = new EventDispatcher();
+        $dispatcher->addSubscriber($listener);
+
+        $this->service->expects(static::once())->method('getRequestTrace')->with($this->request);
+        $this->service->expects(static::once())->method('supports')->with($this->request)->willReturn(true);
+        $this->storage->expects(static::once())->method('setTrace');
+
+        $dispatcher->dispatch(
+            new RequestEvent($this->kernel, $this->request, HttpKernelInterface::MAIN_REQUEST),
+            KernelEvents::REQUEST
+        );
+    }
+
+    public function testListenerIgnoresRequestHeadersOnNonTrustedRequest(): void
+    {
+        $listener   = new TraceSubscriber(true, '127.0.0.2', true, '127.0.0.2', $this->service, $this->storage);
+        $dispatcher = new EventDispatcher();
+        $dispatcher->addSubscriber($listener);
+
+        $this->service->expects(static::never())->method('supports');
+        $this->service->expects(static::never())->method('getRequestTrace');
+
+        $dispatcher->dispatch(
+            new RequestEvent($this->kernel, $this->request, HttpKernelInterface::MAIN_REQUEST),
+            KernelEvents::REQUEST
+        );
+    }
+
+    public function testRequestSetsIdOnResponseOnTrustedIp(): void
+    {
+        $listener   = new TraceSubscriber(true, '127.0.0.1', true, '127.0.0.1', $this->service, $this->storage);
+        $dispatcher = new EventDispatcher();
+        $dispatcher->addSubscriber($listener);
+
+        $trace = new TraceContext();
+        $this->storage->expects(static::once())->method('getTrace')->willReturn($trace);
+        $this->service->expects(static::once())->method('handleResponse')->with($this->response, $trace);
+
+        $dispatcher->dispatch(
+            new ResponseEvent($this->kernel, $this->request, HttpKernelInterface::MAIN_REQUEST, $this->response),
+            KernelEvents::RESPONSE
+        );
+    }
+
+    public function testRequestDoesNotSetIdOnResponseOnNonTrustedIp(): void
+    {
+        $listener   = new TraceSubscriber(true, '127.0.0.2', true, '127.0.0.2', $this->service, $this->storage);
+        $dispatcher = new EventDispatcher();
+        $dispatcher->addSubscriber($listener);
+
+        $this->storage->expects(static::never())->method('getTrace');
+        $this->service->expects(static::never())->method('handleResponse');
+
+        $dispatcher->dispatch(
+            new ResponseEvent($this->kernel, $this->request, HttpKernelInterface::MAIN_REQUEST, $this->response),
+            KernelEvents::RESPONSE
+        );
+    }
 }

Original file line number	Diff line number	Diff line change
`@@ -1,7 +1,7 @@`
`1`	`1`	`parameters:`
`2`	`2`	`ignoreErrors:`
`3`	`3`	`-`
`4`		- message: "#^Parameter \\#1 \\$mergedConfig \\(array\\{traceMode\\: 'tracecontext'\\\|'traceid', traceid\\: array\\{request_header\\: string, response_header\\: string, generator_service\\: string\\\|null\\}, trust_request_header\\: bool, send_response_header\\: bool, storage_service\\: string\\\|null, enable_monolog\\: bool, enable_console\\: bool, console\\: array\\{enabled\\: bool, trace_id\\: string\\\|null\\}, \\.\\.\\.\\}\\) of method DR\\\\SymfonyTraceBundle\\\\DependencyInjection\\\\SymfonyTraceExtension\\:\\:loadInternal\\(\\) should be contravariant with parameter \\$mergedConfig \\(array\\) of method Symfony\\\\Component\\\\HttpKernel\\\\DependencyInjection\\\\ConfigurableExtension\\:\\:loadInternal\\(\\)$#"
	`4`	+ message: "#^Parameter \\#1 \\$mergedConfig \\(array\\{traceMode\\: 'tracecontext'\\\|'traceid', traceid\\: array\\{request_header\\: string, response_header\\: string, generator_service\\: string\\\|null\\}, request\\: array\\{trust_header\\: bool, trusted_ips\\: array\\<string\\>\\\|string\\\|null\\}, response\\: array\\{send_header\\: bool, trusted_ips\\: array\\<string\\>\\\|string\\\|null\\}, storage_service\\: string\\\|null, enable_monolog\\: bool, enable_console\\: bool, console\\: array\\{enabled\\: bool, trace_id\\: string\\\|null\\}, \\.\\.\\.\\}\\) of method DR\\\\SymfonyTraceBundle\\\\DependencyInjection\\\\SymfonyTraceExtension\\:\\:loadInternal\\(\\) should be contravariant with parameter \\$mergedConfig \\(array\\) of method Symfony\\\\Component\\\\HttpKernel\\\\DependencyInjection\\\\ConfigurableExtension\\:\\:loadInternal\\(\\)$#"
`5`	`5`	`count: 1`
`6`	`6`	`path: src/DependencyInjection/SymfonyTraceExtension.php`
`7`	`7`