diff --git a/CHANGELOG.md b/CHANGELOG.md index d727572..c8450a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- `StaticFilesMiddleware` now decodes the request path once with `rawurldecode()` before lookup, so percent-encoded file names (`/hello%20world.txt`, percent-encoded UTF-8 names) are served instead of falling through to the application with a 404; `%2e%2e`, `%00` and double-encoded sequences are still blocked by the existing guards (#905) - `SfxDownloader::fetch()` now also fails fast with the directory-permissions message when a cached zip artifact sits in a destination directory that is no longer writable, instead of failing late per entry inside the extraction (a read-only directory that already holds every entry still re-extracts in place); a cached archive that does not open still reaches the poisoned-artifact cleanup path (#1052) - `StaticFilesMiddleware` now answers `HEAD` on a static file with headers only (`Content-Length`, `Content-Type`, `Accept-Ranges`, `ETag`, `Last-Modified`, `Cache-Control`, as for `GET`) and no body, and still returns 304 on a matching `If-None-Match`; methods other than `GET` and `HEAD` (`POST`, `PUT`, `DELETE`, ...) are passed to the application instead of returning the file, so the denylist and allowlist 404 answers now apply to `GET` and `HEAD` only (#903) - The CI matrix `sed` rewrite now matches digits in package names (`[a-z0-9-]+` instead of `[a-z-]+`), so a future `symfony/foo2bar`-style package is not silently skipped by the "Update Symfony constraints" step; `GithubWorkflowsTest` now replays the rewrite character class against every `symfony/*` key in `composer.json` (#1060) diff --git a/src/Middleware/StaticFilesMiddleware.php b/src/Middleware/StaticFilesMiddleware.php index 30c0f08..7d17679 100644 --- a/src/Middleware/StaticFilesMiddleware.php +++ b/src/Middleware/StaticFilesMiddleware.php @@ -106,11 +106,19 @@ public function __invoke(Request $request, callable $next): Response return $next($request); } - if ($this->isAllowlistExtensionMismatch($request->path())) { + // Workerman Request::path() returns the raw path without decoding + // percent-encoding (issue #905), so decode once here and use the + // decoded path for both the allowlist check and the resolver. + // Decoding twice would turn `%252e%252e` into `..`, so this is + // the only decode; the NUL, backslash and root guards in + // getPublicPathFile() run on the decoded path. + $path = rawurldecode($request->path()); + + if ($this->isAllowlistExtensionMismatch($path)) { return new Response(404); } - $filePath = $this->getPublicPathFile($request); + $filePath = $this->getPublicPathFile($path); if ($filePath === false || !is_file($filePath)) { return $next($request); } @@ -288,10 +296,8 @@ private function isComponentBlocked(string $name, bool $isFile = false): bool return $isFile && $this->allowedExtensions !== [] && !in_array($ext, $this->allowedExtensions, true); } - private function getPublicPathFile(Request $request): string|false + private function getPublicPathFile(string $path): string|false { - $path = $request->path(); - if (str_contains($path, "\0") || str_contains($path, '%00') || str_contains($path, '\\')) { return false; } diff --git a/tests/StaticFilesMiddlewareTest.php b/tests/StaticFilesMiddlewareTest.php index d516c80..c0cfa51 100644 --- a/tests/StaticFilesMiddlewareTest.php +++ b/tests/StaticFilesMiddlewareTest.php @@ -333,6 +333,78 @@ public function testValidFileServed(): void $this->assertFalse($called, "Next should not be called for valid file"); } + /** + * Issue #905: Workerman Request::path() returns the raw path, so a + * percent-encoded file name must be decoded once before lookup. + * The UTF-8 name is built from its percent-encoded form so this file + * stays ASCII-only. + */ + public function testPercentEncodedSpaceAndUtf8NamesAreServed(): void + { + $middleware = new StaticFilesMiddleware($this->rootDirectory); + + $spaceFile = $this->rootDirectory . '/hello world.txt'; + $utf8File = $this->rootDirectory . '/' . rawurldecode('za%C5%BC%C3%B3%C5%82%C4%87.txt'); + file_put_contents($spaceFile, 'space content'); + file_put_contents($utf8File, 'utf8 content'); + + try { + $next = static function (Request $request): Response { + return new Response(404); + }; + + $spaceResponse = $middleware($this->createRequest('/hello%20world.txt'), $next); + $this->assertSame(200, $spaceResponse->getStatusCode(), 'A %20-encoded space name must be served'); + + $utf8Response = $middleware($this->createRequest('/za%C5%BC%C3%B3%C5%82%C4%87.txt'), $next); + $this->assertSame(200, $utf8Response->getStatusCode(), 'A percent-encoded UTF-8 name must be served'); + } finally { + if (file_exists($spaceFile)) { + unlink($spaceFile); + } + if (file_exists($utf8File)) { + unlink($utf8File); + } + } + } + + /** + * Issue #905: decoding once must not open traversal holes — an + * encoded `..` or NUL is still blocked, and a double-encoded name + * stays a literal (missing) file name instead of becoming `..`. + * + * @dataProvider encodedTraversalProvider + */ + public function testEncodedTraversalAndDoubleEncodingPassToNext(string $path): void + { + $middleware = new StaticFilesMiddleware($this->rootDirectory); + + $request = $this->createRequest($path); + $called = false; + $next = function (Request $req) use (&$called): Response { + $called = true; + return new Response(200); + }; + + $response = $middleware($request, $next); + + $this->assertTrue($called, "Next should be called for encoded path: $path"); + $this->assertEquals(200, $response->getStatusCode()); + } + + /** + * @return array + */ + public static function encodedTraversalProvider(): array + { + return [ + 'encoded dot-dot slash' => ['/%2e%2e/test.txt'], + 'encoded NUL' => ['/%00/test.txt'], + 'double-encoded dots stay a literal name' => ['/%252e%252e/test.txt'], + 'encoded backslash' => ['/%5C..%5Ctest.txt'], + ]; + } + public function testNonExistentFilePassesToNext(): void { $middleware = new StaticFilesMiddleware($this->rootDirectory);