diff --git a/CHANGELOG.md b/CHANGELOG.md index 088f0aa..beac4e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,7 +14,51 @@ details. ### Changed -- Nothing. +- [#186](https://github.com/zendframework/zend-stratigility/pull/186) adds a safeguard to middleware pipes to prevent them from being called + multiple times within the same middleware. As an example, consider the + following middleware: + + ```php + public function process( + ServerRequestInterface $request, + RequestHandlerInterface $handler + ) : Response Interface { + $session = $request->getAttribute('session'); + if (! $session) { + $response = $handler->handle($request); + } + + // Inject another attribute before handling + $response = $handler->handle($request->withAttribute( + 'sessionWasEnabled', + true + ); + return $response; + } + ``` + + When using Stratigility, the `$handler` is an instance of + `Zend\Stratigility\Next`, which encapsulates the middleware pipeline and + advances through it on each call to `handle()`. + + The example demonstrates a subtle error: the response from the first + conditional should have been returned, but wasn't, which has led to invoking + the handler a second time. This scenario can have unexpected behaviors, + including always returning a "not found" response, or returning a response + from a handler that was not supposed to execute (as an earlier middleware + already returned early in the original call). + + These bugs are hard to locate, as calling `handle()` is a normal part of any + middleware, and multiple conditional calls to it are a standard workflow. + + With this new version, `Next` will pass a **clone** of itself to the next + middleware in the pipeline, and unset its own internal pipeline queue. Any + subsequent requests to `handle()` within the same scope will therefore result + in the exception `Zend\Stratigility\Exception\MiddlewarePipeNextHandlerAlreadyCalledException`. + + If you depended on calling `$handler->handle()` multiple times in succession + within middleware, we recommend that you compose the specific pipeline(s) + and/or handler(s) you wish to call as class dependencies. ### Deprecated diff --git a/src/Exception/MiddlewarePipeNextHandlerAlreadyCalledException.php b/src/Exception/MiddlewarePipeNextHandlerAlreadyCalledException.php new file mode 100644 index 0000000..368c863 --- /dev/null +++ b/src/Exception/MiddlewarePipeNextHandlerAlreadyCalledException.php @@ -0,0 +1,21 @@ +handle() more than once'); + } +} diff --git a/src/Next.php b/src/Next.php index 118fe69..9a28071 100644 --- a/src/Next.php +++ b/src/Next.php @@ -1,7 +1,7 @@ queue === null) { + throw MiddlewarePipeNextHandlerAlreadyCalledException::create(); + } + if ($this->queue->isEmpty()) { + $this->queue = null; return $this->fallbackHandler->handle($request); } $middleware = $this->queue->dequeue(); + $next = clone $this; // deep clone is not used intentionally + $this->queue = null; // mark queue as processed at this nesting level - return $middleware->process($request, $this); + return $middleware->process($request, $next); } } diff --git a/test/NextTest.php b/test/NextTest.php index a1a591f..45e979b 100644 --- a/test/NextTest.php +++ b/test/NextTest.php @@ -1,7 +1,7 @@ assertSame($response, $next->handle($this->request)); } + + public function testNextHandlerCannotBeInvokedTwice() + { + $fallbackHandler = $this->prophesize(RequestHandlerInterface::class); + $fallbackHandler + ->handle(Argument::any()) + ->willReturn(new Response()); + + $this->queue->push(new DelegatingMiddleware()); + + $next = new Next($this->queue, $fallbackHandler->reveal()); + $next->handle($this->request); + + $this->expectException(MiddlewarePipeNextHandlerAlreadyCalledException::class); + $next->handle($this->request); + } + + public function testSecondInvocationAttemptDoesNotInvokeFinalHandler() + { + $fallBackHandler = $this->prophesize(RequestHandlerInterface::class); + $fallBackHandler + ->handle(Argument::any()) + ->willReturn(new Response()) + ->shouldBeCalledTimes(1); + + $this->queue->push(new DelegatingMiddleware()); + + $next = new Next($this->queue, $fallBackHandler->reveal()); + $next->handle($this->request); + + $this->expectException(MiddlewarePipeNextHandlerAlreadyCalledException::class); + $next->handle($this->request); + } + + public function testSecondInvocationAttemptDoesNotInvokeMiddleware() + { + $fallBackHandler = $this->prophesize(RequestHandlerInterface::class); + $fallBackHandler + ->handle(Argument::any()) + ->willReturn(new Response()); + + $middleware = $this->prophesize(MiddlewareInterface::class); + $middleware + ->process( + Argument::type(ServerRequestInterface::class), + Argument::type(RequestHandlerInterface::class) + ) + ->will(function (array $args): ResponseInterface { + return $args[1]->handle($args[0]); + }) + ->shouldBeCalledTimes(1); + + $this->queue->push($middleware->reveal()); + + $next = new Next($this->queue, $fallBackHandler->reveal()); + $next->handle($this->request); + + $this->expectException(MiddlewarePipeNextHandlerAlreadyCalledException::class); + $next->handle($this->request); + } + + public function testShortCircuitingMiddlewareDoesNotEnableSecondInvocation() + { + $fallBackHandler = $this->prophesize(RequestHandlerInterface::class); + $fallBackHandler + ->handle(Argument::any()) + ->willReturn(new Response()) + ->shouldNotBeCalled(); + + $this->queue->push(new ShortCircuitingMiddleware()); + + // The middleware above shorcircuits (when handler is invoked first) + // The middleware below still exists in the queue (when handler is invoked again) + $this->queue->push(new DelegatingMiddleware()); + + $next = new Next($this->queue, $fallBackHandler->reveal()); + $next->handle($this->request); + + $this->expectException(MiddlewarePipeNextHandlerAlreadyCalledException::class); + $next->handle($this->request); + } + + public function testSecondInvocationAttemptWithEmptyQueueDoesNotInvokeFinalHandler() + { + $fallBackHandler = $this->prophesize(RequestHandlerInterface::class); + $fallBackHandler + ->handle(Argument::any()) + ->willReturn(new Response()) + ->shouldBeCalledTimes(1); + + $next = new Next($this->queue, $fallBackHandler->reveal()); + + $next->handle($this->request); + + $this->expectException(MiddlewarePipeNextHandlerAlreadyCalledException::class); + $next->handle($this->request); + } } diff --git a/test/TestAsset/DelegatingMiddleware.php b/test/TestAsset/DelegatingMiddleware.php new file mode 100644 index 0000000..f9e612a --- /dev/null +++ b/test/TestAsset/DelegatingMiddleware.php @@ -0,0 +1,23 @@ +handle($req); + } +} diff --git a/test/TestAsset/ShortCircuitingMiddleware.php b/test/TestAsset/ShortCircuitingMiddleware.php new file mode 100644 index 0000000..85efe09 --- /dev/null +++ b/test/TestAsset/ShortCircuitingMiddleware.php @@ -0,0 +1,24 @@ +