bug #17595 [HttpKernel] Remove _path from query parameters when fragment is a subrequest (cmenning)

This PR was merged into the 2.3 branch.

Discussion
----------

[HttpKernel] Remove _path from query parameters when fragment is a subrequest

| Q             | A
| ------------- | ---
| Bug fix?      | Yes
| New feature?  | No
| BC breaks?    | No
| Deprecations? | No
| Tests pass?   | Yes
| Fixed tickets |
| License       | MIT
| Doc PR        |

Prior to 2.3.29, all requests to the ESI fragment path ("/_fragment" by default) would have the "_path" query parameter removed. This held true whether an external proxy (such as Varnish) handled the request as true ESI, or whether the Symfony kernel was mocking ESI behavior and inlining the subrequest.

Once the "_controller" check was added in 2.3.29, the "_path" query parameter was only removed on master requests (such as those coming from an external proxy) and not subrequests, leading to differing behavior in production and development settings.

Commits
-------

c374420 Remove _path from query parameters when fragment is a subrequest and request attributes are already set Added tests for _path removal in FragmentListener

src/Symfony/Component/HttpKernel/EventListener/FragmentListener.php

@@ -58,7 +58,14 @@ class FragmentListener implements EventSubscriberInterface
     {
         $request = $event->getRequest();
 
-        if ($request->attributes->has('_controller') || $this->fragmentPath !== rawurldecode($request->getPathInfo())) {
+        if ($this->fragmentPath !== rawurldecode($request->getPathInfo())) {
+            return;
+        }
+
+        if ($request->attributes->has('_controller')) {
+            // Is a sub-request: no need to parse _path but it should still be removed from query parameters as below.
+            $request->query->remove('_path');
+
             return;
         }
 

src/Symfony/Component/HttpKernel/Tests/EventListener/FragmentListenerTest.php

@@ -89,6 +89,31 @@ class FragmentListenerTest extends \PHPUnit_Framework_TestCase
         $this->assertFalse($request->query->has('_path'));
     }
 
+    public function testRemovesPathWithControllerDefined()
+    {
+        $request = Request::create('http://example.com/_fragment?_path=foo%3Dbar%26_controller%3Dfoo');
+
+        $listener = new FragmentListener(new UriSigner('foo'));
+        $event = $this->createGetResponseEvent($request, HttpKernelInterface::SUB_REQUEST);
+
+        $listener->onKernelRequest($event);
+
+        $this->assertFalse($request->query->has('_path'));
+    }
+
+    public function testRemovesPathWithControllerNotDefined()
+    {
+        $signer = new UriSigner('foo');
+        $request = Request::create($signer->sign('http://example.com/_fragment?_path=foo%3Dbar'), 'GET', array(), array(), array(), array('REMOTE_ADDR' => '10.0.0.1'));
+
+        $listener = new FragmentListener($signer);
+        $event = $this->createGetResponseEvent($request);
+
+        $listener->onKernelRequest($event);
+
+        $this->assertFalse($request->query->has('_path'));
+    }
+
     private function createGetResponseEvent(Request $request, $requestType = HttpKernelInterface::MASTER_REQUEST)
     {
         return new GetResponseEvent($this->getMock('Symfony\Component\HttpKernel\HttpKernelInterface'), $request, $requestType);