[HttpFoundation] Do not set X-Accel-Redirect for paths outside of X-Accel-Mapping

vilius-g · vilius-g · commit a662f61e0898 · 2019-05-28T15:48:21.000+03:00
Currently BinaryFileResponse, when configured with X-Accel-Redirect sendfile type,
will only substitute file paths specified in X-Accel-Mapping. But if the provided
file path does not have a defined prefix, then the resulting header will include
the absolute path. Nginx expects a valid URI, therefore this will result in an
issue that is very hard to detect and debug as it will not show up in error logs
and instead the request would just hang for some time and then be re-served
without query parameters(?).
diff --git a/src/Symfony/Component/HttpFoundation/BinaryFileResponse.php b/src/Symfony/Component/HttpFoundation/BinaryFileResponse.php
@@ -227,13 +227,18 @@ public function prepare(Request $request)
 
                         if (substr($path, 0, \strlen($pathPrefix)) === $pathPrefix) {
                             $path = $location.substr($path, \strlen($pathPrefix));
+                            // Only set X-Accel-Redirect header if a valid URI can be produced
+                            // as nginx does not serve arbitrary file paths.
+                            $this->headers->set($type, $path);
+                            $this->maxlen = 0;
                             break;
                         }
                     }
                 }
+            } else {
+                $this->headers->set($type, $path);
+                $this->maxlen = 0;
             }
-            $this->headers->set($type, $path);
-            $this->maxlen = 0;
         } elseif ($request->headers->has('Range')) {
             // Process the range headers.
             if (!$request->headers->has('If-Range') || $this->hasValidIfRangeHeader($request->headers->get('If-Range'))) {
diff --git a/src/Symfony/Component/HttpFoundation/Tests/BinaryFileResponseTest.php b/src/Symfony/Component/HttpFoundation/Tests/BinaryFileResponseTest.php
@@ -338,6 +338,7 @@ public function getSampleXAccelMappings()
         return [
             ['/var/www/var/www/files/foo.txt', '/var/www/=/files/', '/files/var/www/files/foo.txt'],
             ['/home/foo/bar.txt', '/var/www/=/files/,/home/foo/=/baz/', '/baz/bar.txt'],
+            ['/tmp/bar.txt', '"/var/www/"="/files/", "/home/Foo/"="/baz/"', null],
         ];
     }
 

Original file line number	Diff line number	Diff line change
`@@ -227,13 +227,18 @@ public function prepare(Request $request)`
`227`	`227`
`228`	`228`	`if (substr($path, 0, \strlen($pathPrefix)) === $pathPrefix) {`
`229`	`229`	`$path = $location.substr($path, \strlen($pathPrefix));`
	`230`	`+ // Only set X-Accel-Redirect header if a valid URI can be produced`
	`231`	`+ // as nginx does not serve arbitrary file paths.`
	`232`	`+ $this->headers->set($type, $path);`
	`233`	`+ $this->maxlen = 0;`
`230`	`234`	`break;`
`231`	`235`	`}`
`232`	`236`	`}`
`233`	`237`	`}`
	`238`	`+ } else {`
	`239`	`+ $this->headers->set($type, $path);`
	`240`	`+ $this->maxlen = 0;`
`234`	`241`	`}`
`235`		`- $this->headers->set($type, $path);`
`236`		`- $this->maxlen = 0;`
`237`	`242`	`} elseif ($request->headers->has('Range')) {`
`238`	`243`	`// Process the range headers.`
`239`	`244`	`if (!$request->headers->has('If-Range') \|\| $this->hasValidIfRangeHeader($request->headers->get('If-Range'))) {`
Original file line number	Diff line number	Diff line change
`@@ -338,6 +338,7 @@ public function getSampleXAccelMappings()`
`338`	`338`	`return [`
`339`	`339`	`['/var/www/var/www/files/foo.txt', '/var/www/=/files/', '/files/var/www/files/foo.txt'],`
`340`	`340`	`['/home/foo/bar.txt', '/var/www/=/files/,/home/foo/=/baz/', '/baz/bar.txt'],`
	`341`	`+ ['/tmp/bar.txt', '"/var/www/"="/files/", "/home/Foo/"="/baz/"', null],`
`341`	`342`	`];`
`342`	`343`	`}`
`343`	`344`