Repository navigation
Prevent path traversal in static-file & front-controller serving - #6
Merged
Merged
Conversation
- Add phpstan/phpstan-mockery so Mockery-based test doubles are understood. - Generate phpstan-baseline.neon capturing the 593 pre-existing level-9 errors so the analysis exits clean and flags only new regressions; the baseline is the burn-down backlog. - Add composer scripts (test, stan, cs, cs:fix); stan bakes in --memory-limit=1G to avoid the default 128M OOM.
The fork had zero driver tests. Adopt upstream's suite (platform-agnostic, same Valet\Drivers namespace): - Add tests/Drivers/ (BaseDriverTestCase + 23 driver tests) and the tests/Drivers/projects + tests/files fixtures. Base case namespaced Valet\Tests\Drivers, extends PHPUnit TestCase directly (no container boot), setUp() instead of the Yoast polyfill's set_up(), projects provider via plain scandir(). - Add NetteValetDriver (upstream drop-in) so the driver set matches upstream; auto-discovered by the specific-driver path scan. - Two ContaoValetDriverTest methods are marked skipped: the fork still ships the legacy Contao 3/4 driver while the upstream fixture is modern Contao; re-enable after modernizing the driver (.review U6). - Refresh phpstan-baseline.neon to absorb the new test files' level-9 errors (burn-down backlog); analysis stays green. Full suite: 305 tests, 2 skipped. php-cs-fixer clean.
The repo's .gitignore excludes bin/ and vendor/, which also matched three driver-suite fixtures, so they were never committed: - tests/Drivers/projects/cake/bin/cake - tests/Drivers/projects/magento2/bin/magento - tests/Drivers/projects/contao/vendor/contao/.gitkeep Force-add them so CakeValetDriverTest / Magento2ValetDriverTest (and the Contao test once modernized) see their fixtures on a fresh checkout. Caught by CI (passed locally only because the Docker mount included ignored files).
Replace the legacy Contao 3/4 driver (vendor/contao + web/app.php) with the upstream laravel/valet implementation: - Auto-detect the front-controller directory (web vs public). - Use index.php as the front controller and route contao-manager.phar.php and preview.php. - Re-enable the two ContaoValetDriverTest methods that were skipped while the fork shipped the legacy driver. Full suite: 305 tests, 0 skipped. Baseline refreshed; phpstan green.
server.php resolves the request URI with rawurldecode() and drivers then concatenate it onto the site path, so an encoded "../" (e.g. %2e%2e) could make isStaticFile()/frontControllerPath() resolve a file outside the site root. Combined with the X-Accel-Redirect handoff (nginx alias /), that is an arbitrary-file-read / LFI surface, and sites can be exposed publicly via share. - Add Server::isWithin($path, $root): realpath-based containment check (uses strpos, not str_starts_with, since server.php runs under the site's possibly-isolated, older PHP). - server.php: refuse to serve a static file or execute a front controller that resolves outside the site root (404 instead). - Add tests/Unit/ServerTest covering inside/root/traversal/encoded-traversal/ nonexistent/sibling-prefix cases. Note: upstream laravel/valet has the same latent gap; this hardens beyond it.
Sandersm90
force-pushed
the
fix/modernize-contao-driver
branch
from
October 1, 2026 12:10
f16419b to
c671aef
Compare
Sandersm90
force-pushed
the
fix/path-traversal-static-serving
branch
from
October 1, 2026 12:10
51d3cb2 to
67873bf
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #5. First Phase-1 (request-time security) fix.
Confirmed vulnerability.
server.phpresolves the request URI withrawurldecode(), then drivers concatenate it onto the site path. An encoded../(e.g./%2e%2e/secret.txt, which decodes to../) makesisStaticFile()/frontControllerPath()resolve a path outside the site root. Reproduced at the PHP level:isStaticFile()returned/srv/secret.txtfor a site rooted at/srv/site. Via theX-Accel-Redirecthandoff (nginxalias /) this is arbitrary file read / LFI — and sites can be exposed publicly withvalet share.Fix (at the enforcement point, not per-driver):
Server::isWithin($path, $root)— realpath-based containment check. Usesstrpos(), notstr_starts_with(), becauseserver.php/Server.phprun under the site's (possibly isolated, older) PHP.server.phpnow 404s when a static file or front controller resolves outside the site root.tests/Unit/ServerTest: inside / root / traversal / decoded-encoded-traversal / nonexistent / sibling-prefix.Full suite: 311 tests. phpstan green, php-cs-fixer clean.
Note: upstream laravel/valet shares this latent gap (
serveStaticFile/isActualFileare identical) — this hardens beyond upstream.