From 06f5ece00dd5ebb5d95b7c5acc568b77dc286e0e Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 11:46:45 +0100 Subject: [PATCH 01/13] Fix orphan tempnam base file in Archiver tempnam() created a zero-byte placeholder, then ZipArchive opened ${base}.zip, leaking the base file. Unlink the placeholder before opening the archive and throw ArchiveNotCreatedException when tempnam returns false. Add a focused regression test asserting no non-.zip temporary file remains after construction and generation. --- src/PHPDocker/Zip/Archiver.php | 10 ++++++++++ tests/Unit/PHPDocker/Zip/ArchiverTest.php | 15 +++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/PHPDocker/Zip/Archiver.php b/src/PHPDocker/Zip/Archiver.php index 34ee572..cff55b0 100644 --- a/src/PHPDocker/Zip/Archiver.php +++ b/src/PHPDocker/Zip/Archiver.php @@ -32,12 +32,22 @@ final class Archiver /** * Initialise Zip File via the zip PECL extension into a temporary file on local storage. + * + * @throws Exception\ArchiveNotCreatedException */ public function __construct(private readonly string $baseFolder = '') { $this->zipFile = new ZipArchive(); $zipFilename = tempnam(sys_get_temp_dir(), str_replace('\\', '_', self::class)); + + if ($zipFilename === false) { + throw new Exception\ArchiveNotCreatedException('Could not create temporary file for archive'); + } + + // tempnam() creates a zero-byte placeholder, but the archive lives at ${base}.zip, so remove it. + unlink($zipFilename); + $this->zipFile->open(sprintf('%s.zip', $zipFilename), ZipArchive::CREATE); } diff --git a/tests/Unit/PHPDocker/Zip/ArchiverTest.php b/tests/Unit/PHPDocker/Zip/ArchiverTest.php index 9e6e63f..29eeb1d 100644 --- a/tests/Unit/PHPDocker/Zip/ArchiverTest.php +++ b/tests/Unit/PHPDocker/Zip/ArchiverTest.php @@ -42,6 +42,21 @@ public function generateArchiveReturnsArchiveWithCorrectFilename(): void self::assertSame('test.zip', $archive->getFilename()); } + #[Test] + public function constructorDoesNotLeaveOrphanTemporaryBaseFile(): void + { + $tempPrefix = sprintf('%s/%s*', sys_get_temp_dir(), str_replace('\\', '_', Archiver::class)); + $before = glob($tempPrefix) ?: []; + + $archiver = new Archiver('phpdocker'); + $archiver->generateArchive('test.zip'); + + $created = array_diff(glob($tempPrefix) ?: [], $before); + $orphans = array_filter($created, static fn (string $path): bool => !str_ends_with($path, '.zip')); + + self::assertSame([], $orphans); + } + #[Test] public function generateArchiveReturnsTmpFilenamePointingToActualFile(): void { From 14154354f53b97fe2c24e214c853dad2c48c99d6 Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 11:53:06 +0100 Subject: [PATCH 02/13] Add bounded path validation and POST generation rate limiting Validate generated paths against strict grammars and reject traversal, and rate limit POST generation using a Redis-backed production pool with isolated test state. --- composer.json | 1 + composer.lock | 78 +++++++++++++++++++- config/packages/cache.yaml | 11 +++ config/packages/rate_limiter.yaml | 13 ++++ src/Assert/Path.php | 58 +++++++++++++++ src/Assert/PathValidator.php | 50 +++++++++++++ src/Controller/GeneratorController.php | 2 + src/Form/Generator/GlobalOptionsType.php | 20 ++++- src/Form/Generator/PhpType.php | 2 + tests/Functional/GeneratorTest.php | 80 ++++++++++++++++++++ tests/Unit/Assert/PathValidatorTest.php | 93 ++++++++++++++++++++++++ 11 files changed, 402 insertions(+), 6 deletions(-) create mode 100644 config/packages/rate_limiter.yaml create mode 100644 src/Assert/Path.php create mode 100644 src/Assert/PathValidator.php create mode 100644 tests/Unit/Assert/PathValidatorTest.php diff --git a/composer.json b/composer.json index 8829307..8ff51c7 100644 --- a/composer.json +++ b/composer.json @@ -17,6 +17,7 @@ "symfony/framework-bundle": "^8.0", "symfony/mime": "^8.0", "symfony/monolog-bundle": "^4.0", + "symfony/rate-limiter": "^8.0", "symfony/runtime": "^8.0", "symfony/security-csrf": "^8.0", "symfony/twig-bundle": "^8.0", diff --git a/composer.lock b/composer.lock index b9bf1bc..c2d438f 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "283de4b0f26712d589b3e88c11e08508", + "content-hash": "8293553ccaaaaf59037df5ee070cadb8", "packages": [ { "name": "michelf/php-markdown", @@ -3071,6 +3071,80 @@ ], "time": "2026-09-04T10:14:04+00:00" }, + { + "name": "symfony/rate-limiter", + "version": "v8.1.6", + "source": { + "type": "git", + "url": "https://github.com/symfony/rate-limiter.git", + "reference": "dee2fc983ca9944ac37af105a4940507ff5fc42c" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/symfony/rate-limiter/zipball/dee2fc983ca9944ac37af105a4940507ff5fc42c", + "reference": "dee2fc983ca9944ac37af105a4940507ff5fc42c", + "shasum": "" + }, + "require": { + "php": ">=8.4.1", + "symfony/options-resolver": "^7.4|^8.0" + }, + "require-dev": { + "psr/cache": "^1.0|^2.0|^3.0", + "symfony/lock": "^7.4|^8.0" + }, + "type": "library", + "autoload": { + "psr-4": { + "Symfony\\Component\\RateLimiter\\": "" + }, + "exclude-from-classmap": [ + "/Tests/" + ] + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "Wouter de Jong", + "email": "wouter@wouterj.nl" + }, + { + "name": "Symfony Community", + "homepage": "https://symfony.com/contributors" + } + ], + "description": "Provides a Token Bucket implementation to rate limit input and output in your application", + "homepage": "https://symfony.com", + "keywords": [ + "limiter", + "rate-limiter" + ], + "support": { + "source": "https://github.com/symfony/rate-limiter/tree/v8.1.6" + }, + "funding": [ + { + "url": "https://symfony.com/sponsor", + "type": "custom" + }, + { + "url": "https://github.com/fabpot", + "type": "github" + }, + { + "url": "https://github.com/nicolas-grekas", + "type": "github" + }, + { + "url": "https://tidelift.com/funding/github/packagist/symfony/symfony", + "type": "tidelift" + } + ], + "time": "2026-08-07T15:35:35+00:00" + }, { "name": "symfony/routing", "version": "v8.1.6", @@ -6855,5 +6929,5 @@ "ext-zip": "*" }, "platform-dev": {}, - "plugin-api-version": "2.9.0" + "plugin-api-version": "2.6.0" } diff --git a/config/packages/cache.yaml b/config/packages/cache.yaml index 9e407cc..20874f5 100644 --- a/config/packages/cache.yaml +++ b/config/packages/cache.yaml @@ -1,3 +1,14 @@ framework: cache: app: cache.adapter.apcu + default_redis_provider: 'redis://%env(REDIS_HOST)%:%env(REDIS_PORT)%' + pools: + rate_limiter.cache: + adapter: cache.adapter.redis + +when@test: + framework: + cache: + pools: + rate_limiter.cache: + adapter: cache.adapter.filesystem diff --git a/config/packages/rate_limiter.yaml b/config/packages/rate_limiter.yaml new file mode 100644 index 0000000..02a78c2 --- /dev/null +++ b/config/packages/rate_limiter.yaml @@ -0,0 +1,13 @@ +framework: + rate_limiter: + generator: + policy: 'fixed_window' + limit: 30 + interval: '1 minute' + cache_pool: 'rate_limiter.cache' + +when@test: + framework: + rate_limiter: + generator: + limit: 3 diff --git a/src/Assert/Path.php b/src/Assert/Path.php new file mode 100644 index 0000000..953ee29 --- /dev/null +++ b/src/Assert/Path.php @@ -0,0 +1,58 @@ + + */ + public static function getRegexes(): array + { + return [ + self::ABSOLUTE_DIR => '\A/[A-Za-z0-9._-]+(?:/[A-Za-z0-9._-]+)*\z', + self::RELATIVE_PHP => '\A[A-Za-z0-9._-]+(?:/[A-Za-z0-9._-]+)*\.php\z', + self::HOST_PATH => '\A[A-Za-z0-9._/-]+\z', + ]; + } +} diff --git a/src/Assert/PathValidator.php b/src/Assert/PathValidator.php new file mode 100644 index 0000000..f511d93 --- /dev/null +++ b/src/Assert/PathValidator.php @@ -0,0 +1,50 @@ +type] ?? null; + + if ($regex === null || str_contains($value, '..') || preg_match('#' . $regex . '#', $value) !== 1) { + $this + ->context + ->buildViolation($constraint->message) + ->setParameter('{{ value }}', $this->formatValue($value)) + ->addViolation(); + } + } +} diff --git a/src/Controller/GeneratorController.php b/src/Controller/GeneratorController.php index 8be26dc..9e31056 100644 --- a/src/Controller/GeneratorController.php +++ b/src/Controller/GeneratorController.php @@ -27,6 +27,7 @@ use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpFoundation\ResponseHeaderBag; +use Symfony\Component\HttpKernel\Attribute\RateLimit; /** * Docker environment generator controller. @@ -43,6 +44,7 @@ public function __construct( /** * Form and form processor for creating a project. */ + #[RateLimit('generator', methods: ['POST'])] public function create(Request $request): BinaryFileResponse|Response { $form = $this->createForm(type: ProjectType::class, options: ['method' => Request::METHOD_POST]); diff --git a/src/Form/Generator/GlobalOptionsType.php b/src/Form/Generator/GlobalOptionsType.php index b639b6e..69e2259 100644 --- a/src/Form/Generator/GlobalOptionsType.php +++ b/src/Form/Generator/GlobalOptionsType.php @@ -3,9 +3,11 @@ namespace App\Form\Generator; +use App\Assert\Path; use Symfony\Component\Form\Extension\Core\Type\IntegerType; use Symfony\Component\Form\Extension\Core\Type\TextType; use Symfony\Component\Form\FormBuilderInterface; +use Symfony\Component\Validator\Constraints\Length; use Symfony\Component\Validator\Constraints\NotBlank; use Symfony\Component\Validator\Constraints\Range; use Symfony\Component\Validator\Constraints\Type; @@ -26,12 +28,22 @@ public function buildForm(FormBuilderInterface $builder, array $options): void ], ]) ->add('appPath', TextType::class, [ - 'label' => 'Your source code\'s path', - 'data' => '.', + 'label' => 'Your source code\'s path', + 'data' => '.', + 'constraints' => [ + new NotBlank(), + new Length(max: 255), + new Path(type: Path::HOST_PATH), + ], ]) ->add('dockerWorkingDir', TextType::class, [ - 'label' => 'Containers workdir', - 'data' => '/application', + 'label' => 'Containers workdir', + 'data' => '/application', + 'constraints' => [ + new NotBlank(), + new Length(max: 255), + new Path(type: Path::ABSOLUTE_DIR), + ], ]); } diff --git a/src/Form/Generator/PhpType.php b/src/Form/Generator/PhpType.php index 462c6e0..0e196e6 100644 --- a/src/Form/Generator/PhpType.php +++ b/src/Form/Generator/PhpType.php @@ -19,6 +19,7 @@ namespace App\Form\Generator; +use App\Assert\Path; use App\PHPDocker\PhpExtension\AvailableExtensionsFactory; use App\PHPDocker\PhpExtension\PhpExtension; use App\PHPDocker\Project\ServiceOptions\Php; @@ -53,6 +54,7 @@ public function buildForm(FormBuilderInterface $builder, array $options): void 'constraints' => [ new NotBlank(), new Length(min: 2, max: 128), + new Path(type: Path::RELATIVE_PHP), ], ]) ->add('hasGit', CheckboxType::class, [ diff --git a/tests/Functional/GeneratorTest.php b/tests/Functional/GeneratorTest.php index 8860639..e822529 100644 --- a/tests/Functional/GeneratorTest.php +++ b/tests/Functional/GeneratorTest.php @@ -37,6 +37,10 @@ public function setUp(): void 'environment' => 'test', 'debug' => false, ]); + + // The rate limiter pool survives kernel resets in tests, so clear it to + // keep limiter state from leaking between tests. + $this->client->getContainer()->get('rate_limiter.cache')->clear(); } #[Test] @@ -185,6 +189,82 @@ public function testPortOffsetsRespectCustomBasePort(): void self::assertStringContainsString('3004', $dockerCompose); // Postgres offset +4 } + #[Test] + public function testInvalidAppPathIsRejected(): void + { + $this->client->request('GET', '/'); + $this->client->submitForm('Generate project archive', [ + 'project[globalOptions][basePort]' => '8000', + 'project[globalOptions][appPath]' => '/var/www/my app', + ]); + + self::assertResponseIsSuccessful(); + self::assertStringContainsString('This value is not a valid path', (string) $this->client->getResponse()->getContent()); + } + + #[Test] + public function testInvalidDockerWorkingDirIsRejected(): void + { + $this->client->request('GET', '/'); + $this->client->submitForm('Generate project archive', [ + 'project[globalOptions][basePort]' => '8000', + 'project[globalOptions][dockerWorkingDir]' => 'relative/dir', + ]); + + self::assertResponseIsSuccessful(); + self::assertStringContainsString('This value is not a valid path', (string) $this->client->getResponse()->getContent()); + } + + #[Test] + public function testInvalidFrontControllerPathIsRejected(): void + { + $this->client->request('GET', '/'); + $this->client->submitForm('Generate project archive', [ + 'project[globalOptions][basePort]' => '8000', + 'project[phpOptions][frontControllerPath]' => 'public/index', + ]); + + self::assertResponseIsSuccessful(); + self::assertStringContainsString('This value is not a valid path', (string) $this->client->getResponse()->getContent()); + } + + #[Test] + public function testOversizedAppPathIsRejected(): void + { + $this->client->request('GET', '/'); + $this->client->submitForm('Generate project archive', [ + 'project[globalOptions][basePort]' => '8000', + 'project[globalOptions][appPath]' => str_repeat('a', 256), + ]); + + self::assertResponseIsSuccessful(); + self::assertStringContainsString('This value is too long', (string) $this->client->getResponse()->getContent()); + } + + #[Test] + public function testPostGenerationIsRateLimited(): void + { + $this->client->disableReboot(); + + // The limiter counts every POST, valid or not, so keep the first three + // submissions invalid to avoid generating multiple archives with the + // shared, single-use Archiver instance. + for ($i = 1; $i <= 3; ++$i) { + $this->client->request('GET', '/'); + $this->client->submitForm('Generate project archive', [ + 'project[globalOptions][basePort]' => '', + ]); + self::assertResponseIsSuccessful(); + } + + $this->client->request('GET', '/'); + $this->client->submitForm('Generate project archive', [ + 'project[globalOptions][basePort]' => '8000', + ]); + + self::assertResponseStatusCodeSame(429); + } + private function generateAndGetZip(array $formData): void { $this->client->request('GET', '/'); diff --git a/tests/Unit/Assert/PathValidatorTest.php b/tests/Unit/Assert/PathValidatorTest.php new file mode 100644 index 0000000..d6e02b0 --- /dev/null +++ b/tests/Unit/Assert/PathValidatorTest.php @@ -0,0 +1,93 @@ + + */ +class PathValidatorTest extends ConstraintValidatorTestCase +{ + protected function createValidator(): ConstraintValidatorInterface + { + return new PathValidator(); + } + + /** + * @return array + */ + public static function validPathsProvider(): array + { + return [ + 'absolute dir' => [Path::ABSOLUTE_DIR, '/application'], + 'absolute nested' => [Path::ABSOLUTE_DIR, '/var/www/myapp'], + 'relative php' => [Path::RELATIVE_PHP, 'public/index.php'], + 'relative php nested' => [Path::RELATIVE_PHP, 'app/src/index.php'], + 'host path dot' => [Path::HOST_PATH, '.'], + 'host path relative' => [Path::HOST_PATH, 'src/app'], + 'host path absolute' => [Path::HOST_PATH, '/var/www'], + ]; + } + + #[Test] + #[DataProvider('validPathsProvider')] + public function validPathsProduceNoViolations(string $type, string $value): void + { + $this->validator->validate($value, new Path(type: $type)); + $this->assertNoViolation(); + } + + /** + * @return array + */ + public static function invalidPathsProvider(): array + { + return [ + 'space in host path' => [Path::HOST_PATH, '/var/www/my app'], + 'traversal host path' => [Path::HOST_PATH, '../etc'], + 'traversal absolute' => [Path::ABSOLUTE_DIR, '/var/../etc'], + 'space absolute' => [Path::ABSOLUTE_DIR, '/var/www myapp'], + 'absolute without leading slash' => [Path::ABSOLUTE_DIR, 'var/www'], + 'php missing extension' => [Path::RELATIVE_PHP, 'public/index'], + 'php space' => [Path::RELATIVE_PHP, 'public/my index.php'], + 'php traversal' => [Path::RELATIVE_PHP, 'public/../index.php'], + ]; + } + + #[Test] + #[DataProvider('invalidPathsProvider')] + public function invalidPathsProduceOneViolation(string $type, string $value): void + { + $constraint = new Path(type: $type); + $this->validator->validate($value, $constraint); + + $this->buildViolation($constraint->message) + ->setParameter('{{ value }}', '"' . $value . '"') + ->assertRaised(); + } + + #[Test] + public function nonStringValueProducesNoViolations(): void + { + $this->validator->validate(null, new Path(type: Path::HOST_PATH)); + $this->assertNoViolation(); + } + + #[Test] + public function emptyStringProducesOneViolation(): void + { + $constraint = new Path(type: Path::HOST_PATH); + $this->validator->validate('', $constraint); + + $this->buildViolation($constraint->message) + ->setParameter('{{ value }}', '""') + ->assertRaised(); + } +} From 2a4a478275a6cd3c24e09c98f8866b073fce3fb6 Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 11:45:33 +0100 Subject: [PATCH 03/13] Pin workflow actions to immutable SHAs and set least-privilege permissions Pin every uses: in build-containers.yaml and cleanup-repo.yaml to full commit SHAs (version comments kept). Add explicit job-level GITHUB_TOKEN permissions: build needs only contents:read (pushes to DockerHub, not GH packages); cleanup needs contents/issues/pull-requests write for stale handling and branch deletion. --- .github/workflows/build-containers.yaml | 9 ++++++--- .github/workflows/cleanup-repo.yaml | 9 +++++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/.github/workflows/build-containers.yaml b/.github/workflows/build-containers.yaml index 2a087e7..904ff37 100644 --- a/.github/workflows/build-containers.yaml +++ b/.github/workflows/build-containers.yaml @@ -25,15 +25,18 @@ jobs: if: ${{ github.ref == 'refs/heads/master' }} runs-on: ubuntu-latest + permissions: + contents: read + steps: - name: Checkout - uses: actions/checkout@v6 + uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v4 + uses: docker/setup-buildx-action@f87e5991a6d7451dcb8d9637bfbc97413f497069 # v4 - name: Login to DockerHub - uses: docker/login-action@v4 + uses: docker/login-action@dbcb813823bdd20940b903addbd779551569679f # v4 with: username: ${{ secrets.DOCKERHUB_USERNAME }} password: ${{ secrets.DOCKERHUB_TOKEN }} diff --git a/.github/workflows/cleanup-repo.yaml b/.github/workflows/cleanup-repo.yaml index 1a44be6..1bf2b70 100644 --- a/.github/workflows/cleanup-repo.yaml +++ b/.github/workflows/cleanup-repo.yaml @@ -11,10 +11,15 @@ jobs: cleanup-repository: runs-on: ubuntu-latest + permissions: + contents: write + issues: write + pull-requests: write + steps: # Mark issues and PRs with no activity as stale after a while, and close them after a while longer - - uses: actions/stale@v10 + - uses: actions/stale@1e223db275d687790206a7acac4d1a11bd6fe629 # v10 with: stale-issue-message: 'Marking issue as stale' stale-pr-message: 'Marking PR as stale' @@ -26,7 +31,7 @@ jobs: # Delete old, abandoned branches # See what constitutes an abandoned branch here: https://github.com/phpdocker-io/github-actions-delete-abandoned-branches - - uses: phpdocker-io/github-actions-delete-abandoned-branches@v1 + - uses: phpdocker-io/github-actions-delete-abandoned-branches@d8e3635360bd492315571df30f15f55c98a28434 # v1 with: github_token: ${{ github.token }} last_commit_age_days: 30 From 395e69177d21126eb98452506730b0921b5725a1 Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 11:49:22 +0100 Subject: [PATCH 04/13] Harden Makefile mkcert/hosts downloads Verify every downloaded executable before it is chmod'ed or run: - curl -fsSL --retry 3: HTTP/network errors now abort the target instead of falling through to chmod + execution. Downloads go to a temp file and are moved into place, so a failed fetch leaves no partial cache. - Pin the xwmx/hosts source to the commit the 3.6.4 tag points at (9a929dc70fa11bfe6dc5b0f1d53aea442395edd3) instead of a mutable tag. - Add verify-mkcert/verify-hosts targets that check the SHA-256 of both cached and freshly downloaded files. verify-mkcert precedes install-mkcert and create-certs; verify-hosts precedes install-hosts, clean-hosts and init-hosts (the sudo paths). Upstream publishes no checksum for the mkcert v1.4.3 assets, so the values were computed from the assets downloaded over HTTPS from the official release URLs on 2026-09-25; windows-amd64.exe is corroborated by the mkcert 1.4.3 Chocolatey package. Provenance is documented next to the values. An unsupported BINARY_SUFFIX fails closed instead of skipping verification. --- Makefile | 78 ++++++++++++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 68 insertions(+), 10 deletions(-) diff --git a/Makefile b/Makefile index fbfa7ea..d4952b6 100644 --- a/Makefile +++ b/Makefile @@ -6,15 +6,39 @@ HOSTS_LOCATION=bin/hosts SITE_HOST=phpdocker.local PHP_RUN=docker compose run -e XDEBUG_MODE=coverage --rm php-fpm +# Integrity data for the binaries downloaded below. Upstream publishes no checksum +# for the mkcert v1.4.3 release assets (the GitHub release API reports +# "digest": null for each of them), so these SHA-256 values were computed locally +# from the assets downloaded over HTTPS from the official release URLs on +# 2026-09-25. The windows-amd64.exe value is independently corroborated by the +# mkcert 1.4.3 Chocolatey package (tools/mkcert.exe, published 2020-11-26), which +# contains the same bytes. hosts is fetched from the commit the 3.6.4 tag points at +# (9a929dc70fa11bfe6dc5b0f1d53aea442395edd3) and was hashed on 2026-09-25. If an +# asset is ever republished, update its URL and its hash in the same commit. +MKCERT_SHA256_linux-amd64=c2b0746528588d2a5dabe7c4394a848909da07e23ca3f2393375e9baa3931649 +MKCERT_SHA256_darwin-amd64=0b5bd40ea69ec34c567707249938bcd0502d2c3efc0137143a076a2b80d5e882 +MKCERT_SHA256_linux-arm=b982ade61b6781f17afc210914116d8078af3ebb631facd28a944033018d41d2 +MKCERT_SHA256_linux-arm64=43c4e3b9e7e6466d397b3d6e221788f83b5b91f826f1040240dbaddfc101ce33 +MKCERT_SHA256_windows-amd64.exe=9dc25f7d1ae0be93db81aa42f3abfd62d13725dfd48969c9fe94b6af57e5573c +HOSTS_COMMIT=9a929dc70fa11bfe6dc5b0f1d53aea442395edd3 +HOSTS_SHA256=eee51960ec8dd30e00090779ba79f11410396e69ac7812b0ad99f5b597c8c36e + +# sha256sum on Linux, shasum on macOS +SHA256_CMD=$(shell command -v sha256sum >/dev/null 2>&1 && echo sha256sum || echo 'shasum -a 256') + INFECTION_THREADS?=8 BUILD_TAG?:=$(shell date +'%Y-%m-%d-%H-%M-%S')-$(shell git rev-parse --short HEAD) -# linux-amd64, darwin-amd64, linux-arm +# linux-amd64, darwin-amd64, linux-arm, linux-arm64 # On windows, override with windows-amd64.exe ifndef BINARY_SUFFIX BINARY_SUFFIX:=$(shell [[ "`uname -s`" == "Linux" ]] && echo linux || echo darwin)-amd64 endif +# Resolved from BINARY_SUFFIX; an unsupported suffix yields an empty value, which +# fails verification instead of skipping it. +MKCERT_SHA256=$(MKCERT_SHA256_$(BINARY_SUFFIX)) + ifndef BUILD_TAG BUILD_TAG:=$(shell date +'%Y-%m-%d-%H-%M-%S')-$(shell git rev-parse --short HEAD) endif @@ -85,19 +109,53 @@ composer-update: $(PHP_RUN) composer update --no-scripts make composer-install -install-mkcert: - @echo "Installing mkcert for OS type ${BINARY_SUFFIX}" - @if [[ ! -f '$(MKCERT_LOCATION)' ]]; then curl -sL 'https://github.com/FiloSottile/mkcert/releases/download/$(MKCERT_VERSION)/mkcert-$(MKCERT_VERSION)-$(BINARY_SUFFIX)' -o $(MKCERT_LOCATION); chmod +x $(MKCERT_LOCATION); fi; +install-mkcert: download-mkcert verify-mkcert + chmod +x $(MKCERT_LOCATION) bin/mkcert -install -create-certs: +download-mkcert: + @echo "Installing mkcert for OS type ${BINARY_SUFFIX}" + @if [[ ! -f '$(MKCERT_LOCATION)' ]]; then \ + curl -fsSL --retry 3 -o '$(MKCERT_LOCATION).tmp' 'https://github.com/FiloSottile/mkcert/releases/download/$(MKCERT_VERSION)/mkcert-$(MKCERT_VERSION)-$(BINARY_SUFFIX)' || { rm -f '$(MKCERT_LOCATION).tmp'; exit 1; }; \ + mv '$(MKCERT_LOCATION).tmp' '$(MKCERT_LOCATION)'; \ + fi + +verify-mkcert: + @if [[ -f '$(MKCERT_LOCATION)' ]]; then \ + if [[ -z '$(MKCERT_SHA256)' ]]; then \ + echo "No pinned SHA-256 for BINARY_SUFFIX '$(BINARY_SUFFIX)'; refusing to run $(MKCERT_LOCATION)"; \ + exit 1; \ + fi; \ + actual="$$($(SHA256_CMD) '$(MKCERT_LOCATION)' | awk '{print $$1}')"; \ + if [[ "$$actual" != '$(MKCERT_SHA256)' ]]; then \ + echo "SHA-256 mismatch for $(MKCERT_LOCATION): expected '$(MKCERT_SHA256)', got '$$actual'"; \ + exit 1; \ + fi; \ + fi + +create-certs: verify-mkcert bin/mkcert -cert-file=infrastructure/local/localhost.pem -key-file=infrastructure/local/localhost-key.pem $(SITE_HOST) -install-hosts: - @echo "Installing hosts script" - @if [[ ! -f '$(HOSTS_LOCATION)' ]]; then curl -sL 'https://raw.githubusercontent.com/xwmx/hosts/$(HOSTS_VERSION)/hosts' -o $(HOSTS_LOCATION); chmod +x $(HOSTS_LOCATION); fi; - -clean-hosts: +install-hosts: download-hosts verify-hosts + chmod +x $(HOSTS_LOCATION) + +download-hosts: + @echo "Installing hosts script ($(HOSTS_VERSION))" + @if [[ ! -f '$(HOSTS_LOCATION)' ]]; then \ + curl -fsSL --retry 3 -o '$(HOSTS_LOCATION).tmp' 'https://raw.githubusercontent.com/xwmx/hosts/$(HOSTS_COMMIT)/hosts' || { rm -f '$(HOSTS_LOCATION).tmp'; exit 1; }; \ + mv '$(HOSTS_LOCATION).tmp' '$(HOSTS_LOCATION)'; \ + fi + +verify-hosts: + @if [[ -f '$(HOSTS_LOCATION)' ]]; then \ + actual="$$($(SHA256_CMD) '$(HOSTS_LOCATION)' | awk '{print $$1}')"; \ + if [[ "$$actual" != '$(HOSTS_SHA256)' ]]; then \ + echo "SHA-256 mismatch for $(HOSTS_LOCATION): expected '$(HOSTS_SHA256)', got '$$actual'"; \ + exit 1; \ + fi; \ + fi + +clean-hosts: verify-hosts sudo bin/hosts remove --force *$(SITE_HOST) > /dev/null 2>&1 || exit 0 init-hosts: clean-hosts From e15d1af1533ee0eb1d182682c2d1aec179cd8ec5 Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 11:58:43 +0100 Subject: [PATCH 05/13] Handle unlink and ZipArchive open failures in Archiver Throw ArchiveNotCreatedException when the tempnam placeholder cannot be unlinked or when ZipArchive::open fails, removing any partially created ${base}.zip first so no temporary artefact is left behind. The regression test now cleans up the zip it generates and asserts both that the archive file exists and that no non-.zip orphan remains. --- src/PHPDocker/Zip/Archiver.php | 14 +++++++++++-- tests/Unit/PHPDocker/Zip/ArchiverTest.php | 24 ++++++++++++++++------- 2 files changed, 29 insertions(+), 9 deletions(-) diff --git a/src/PHPDocker/Zip/Archiver.php b/src/PHPDocker/Zip/Archiver.php index cff55b0..1c318d9 100644 --- a/src/PHPDocker/Zip/Archiver.php +++ b/src/PHPDocker/Zip/Archiver.php @@ -46,9 +46,19 @@ public function __construct(private readonly string $baseFolder = '') } // tempnam() creates a zero-byte placeholder, but the archive lives at ${base}.zip, so remove it. - unlink($zipFilename); + if (unlink($zipFilename) === false) { + throw new Exception\ArchiveNotCreatedException('Could not remove temporary file placeholder for archive'); + } + + $zipPath = sprintf('%s.zip', $zipFilename); + + if ($this->zipFile->open($zipPath, ZipArchive::CREATE) !== true) { + if (file_exists($zipPath)) { + unlink($zipPath); + } - $this->zipFile->open(sprintf('%s.zip', $zipFilename), ZipArchive::CREATE); + throw new Exception\ArchiveNotCreatedException('Archive creation failed for an unknown reason'); + } } /** diff --git a/tests/Unit/PHPDocker/Zip/ArchiverTest.php b/tests/Unit/PHPDocker/Zip/ArchiverTest.php index 29eeb1d..1b62372 100644 --- a/tests/Unit/PHPDocker/Zip/ArchiverTest.php +++ b/tests/Unit/PHPDocker/Zip/ArchiverTest.php @@ -48,13 +48,23 @@ public function constructorDoesNotLeaveOrphanTemporaryBaseFile(): void $tempPrefix = sprintf('%s/%s*', sys_get_temp_dir(), str_replace('\\', '_', Archiver::class)); $before = glob($tempPrefix) ?: []; - $archiver = new Archiver('phpdocker'); - $archiver->generateArchive('test.zip'); - - $created = array_diff(glob($tempPrefix) ?: [], $before); - $orphans = array_filter($created, static fn (string $path): bool => !str_ends_with($path, '.zip')); - - self::assertSame([], $orphans); + try { + $archiver = new Archiver('phpdocker'); + $archiver->addFile($this->makeFile('placeholder.txt', 'content')); + $archive = $archiver->generateArchive('test.zip'); + + $created = array_diff(glob($tempPrefix) ?: [], $before); + $orphans = array_filter($created, static fn (string $path): bool => !str_ends_with($path, '.zip')); + + self::assertFileExists($archive->getTmpFilename()); + self::assertSame([], $orphans); + } finally { + foreach (array_diff(glob($tempPrefix) ?: [], $before) as $path) { + if (is_file($path)) { + unlink($path); + } + } + } } #[Test] From 721aacfba13d70a70a419c3b302ac69b0ee6cf25 Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 11:58:19 +0100 Subject: [PATCH 06/13] Tighten path grammar and rate limiter dependency floor Allow a single optional trailing slash for absolute dirs, reject only real .. path segments, and require framework-bundle ^8.1 for the RateLimit attribute wiring. --- composer.json | 2 +- composer.lock | 2 +- src/Assert/Path.php | 2 +- src/Assert/PathValidator.php | 5 +++-- tests/Unit/Assert/PathValidatorTest.php | 9 ++++++--- 5 files changed, 12 insertions(+), 8 deletions(-) diff --git a/composer.json b/composer.json index 8ff51c7..5014e7a 100644 --- a/composer.json +++ b/composer.json @@ -14,7 +14,7 @@ "symfony/dotenv": "^8.0", "symfony/flex": "^2.4", "symfony/form": "^8.0", - "symfony/framework-bundle": "^8.0", + "symfony/framework-bundle": "^8.1", "symfony/mime": "^8.0", "symfony/monolog-bundle": "^4.0", "symfony/rate-limiter": "^8.0", diff --git a/composer.lock b/composer.lock index c2d438f..dd3b76b 100644 --- a/composer.lock +++ b/composer.lock @@ -4,7 +4,7 @@ "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", "This file is @generated automatically" ], - "content-hash": "8293553ccaaaaf59037df5ee070cadb8", + "content-hash": "da92f28a7de2e1aa75b04e145ce31632", "packages": [ { "name": "michelf/php-markdown", diff --git a/src/Assert/Path.php b/src/Assert/Path.php index 953ee29..c538b8c 100644 --- a/src/Assert/Path.php +++ b/src/Assert/Path.php @@ -50,7 +50,7 @@ public function __construct( public static function getRegexes(): array { return [ - self::ABSOLUTE_DIR => '\A/[A-Za-z0-9._-]+(?:/[A-Za-z0-9._-]+)*\z', + self::ABSOLUTE_DIR => '\A/[A-Za-z0-9._-]+(?:/[A-Za-z0-9._-]+)*/?\z', self::RELATIVE_PHP => '\A[A-Za-z0-9._-]+(?:/[A-Za-z0-9._-]+)*\.php\z', self::HOST_PATH => '\A[A-Za-z0-9._/-]+\z', ]; diff --git a/src/Assert/PathValidator.php b/src/Assert/PathValidator.php index f511d93..7072b0d 100644 --- a/src/Assert/PathValidator.php +++ b/src/Assert/PathValidator.php @@ -37,9 +37,10 @@ public function validate(mixed $value, Constraint $constraint): void assert($constraint instanceof Path); - $regex = Path::getRegexes()[$constraint->type] ?? null; + $regex = Path::getRegexes()[$constraint->type] ?? null; + $segments = explode('/', $value); - if ($regex === null || str_contains($value, '..') || preg_match('#' . $regex . '#', $value) !== 1) { + if ($regex === null || in_array('..', $segments, true) || preg_match('#' . $regex . '#', $value) !== 1) { $this ->context ->buildViolation($constraint->message) diff --git a/tests/Unit/Assert/PathValidatorTest.php b/tests/Unit/Assert/PathValidatorTest.php index d6e02b0..d8133e4 100644 --- a/tests/Unit/Assert/PathValidatorTest.php +++ b/tests/Unit/Assert/PathValidatorTest.php @@ -30,9 +30,11 @@ public static function validPathsProvider(): array 'absolute nested' => [Path::ABSOLUTE_DIR, '/var/www/myapp'], 'relative php' => [Path::RELATIVE_PHP, 'public/index.php'], 'relative php nested' => [Path::RELATIVE_PHP, 'app/src/index.php'], - 'host path dot' => [Path::HOST_PATH, '.'], - 'host path relative' => [Path::HOST_PATH, 'src/app'], - 'host path absolute' => [Path::HOST_PATH, '/var/www'], + 'host path dot' => [Path::HOST_PATH, '.'], + 'host path relative' => [Path::HOST_PATH, 'src/app'], + 'host path absolute' => [Path::HOST_PATH, '/var/www'], + 'host path dots in name' => [Path::HOST_PATH, 'my..app'], + 'absolute trailing slash' => [Path::ABSOLUTE_DIR, '/srv/'], ]; } @@ -55,6 +57,7 @@ public static function invalidPathsProvider(): array 'traversal absolute' => [Path::ABSOLUTE_DIR, '/var/../etc'], 'space absolute' => [Path::ABSOLUTE_DIR, '/var/www myapp'], 'absolute without leading slash' => [Path::ABSOLUTE_DIR, 'var/www'], + 'absolute double trailing slash' => [Path::ABSOLUTE_DIR, '/srv//'], 'php missing extension' => [Path::RELATIVE_PHP, 'public/index'], 'php space' => [Path::RELATIVE_PHP, 'public/my index.php'], 'php traversal' => [Path::RELATIVE_PHP, 'public/../index.php'], From 439ba15263a5166e2456224c494e0a48ec3ee54e Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 11:49:22 +0100 Subject: [PATCH 07/13] Harden Makefile mkcert/hosts downloads Verify every downloaded executable before it is chmod'ed or run: - curl -fsSL --retry 3: HTTP/network errors now abort the target instead of falling through to chmod + execution. Downloads go to a temp file and are moved into place, so a failed fetch leaves no partial cache. - Pin the xwmx/hosts source to the commit the 3.6.4 tag points at (9a929dc70fa11bfe6dc5b0f1d53aea442395edd3) instead of a mutable tag. - verify-mkcert/verify-hosts check the SHA-256 of cached and freshly downloaded files, refuse to pass when the expected executable is missing, and fail closed on an unsupported BINARY_SUFFIX. Each verify target depends on its download target, so make -j still downloads before verifying. verify-mkcert precedes install-mkcert and create-certs (which therefore downloads and verifies on a clean tree); verify-hosts precedes install-hosts, clean-hosts and init-hosts (the sudo paths). Upstream publishes no checksum for the mkcert v1.4.3 assets, so the values were computed from the assets downloaded over HTTPS from the official release URLs on 2026-09-25; windows-amd64.exe is corroborated by the mkcert 1.4.3 Chocolatey package. Provenance is documented next to the values. --- Makefile | 44 ++++++++++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 20 deletions(-) diff --git a/Makefile b/Makefile index d4952b6..22a8634 100644 --- a/Makefile +++ b/Makefile @@ -109,7 +109,7 @@ composer-update: $(PHP_RUN) composer update --no-scripts make composer-install -install-mkcert: download-mkcert verify-mkcert +install-mkcert: verify-mkcert chmod +x $(MKCERT_LOCATION) bin/mkcert -install @@ -120,23 +120,25 @@ download-mkcert: mv '$(MKCERT_LOCATION).tmp' '$(MKCERT_LOCATION)'; \ fi -verify-mkcert: - @if [[ -f '$(MKCERT_LOCATION)' ]]; then \ - if [[ -z '$(MKCERT_SHA256)' ]]; then \ - echo "No pinned SHA-256 for BINARY_SUFFIX '$(BINARY_SUFFIX)'; refusing to run $(MKCERT_LOCATION)"; \ - exit 1; \ - fi; \ - actual="$$($(SHA256_CMD) '$(MKCERT_LOCATION)' | awk '{print $$1}')"; \ - if [[ "$$actual" != '$(MKCERT_SHA256)' ]]; then \ - echo "SHA-256 mismatch for $(MKCERT_LOCATION): expected '$(MKCERT_SHA256)', got '$$actual'"; \ - exit 1; \ - fi; \ +verify-mkcert: download-mkcert + @if [[ ! -f '$(MKCERT_LOCATION)' ]]; then \ + echo "Missing $(MKCERT_LOCATION); refusing to run it"; \ + exit 1; \ + fi; \ + if [[ -z '$(MKCERT_SHA256)' ]]; then \ + echo "No pinned SHA-256 for BINARY_SUFFIX '$(BINARY_SUFFIX)'; refusing to run $(MKCERT_LOCATION)"; \ + exit 1; \ + fi; \ + actual="$$($(SHA256_CMD) '$(MKCERT_LOCATION)' | awk '{print $$1}')"; \ + if [[ "$$actual" != '$(MKCERT_SHA256)' ]]; then \ + echo "SHA-256 mismatch for $(MKCERT_LOCATION): expected '$(MKCERT_SHA256)', got '$$actual'"; \ + exit 1; \ fi create-certs: verify-mkcert bin/mkcert -cert-file=infrastructure/local/localhost.pem -key-file=infrastructure/local/localhost-key.pem $(SITE_HOST) -install-hosts: download-hosts verify-hosts +install-hosts: verify-hosts chmod +x $(HOSTS_LOCATION) download-hosts: @@ -146,13 +148,15 @@ download-hosts: mv '$(HOSTS_LOCATION).tmp' '$(HOSTS_LOCATION)'; \ fi -verify-hosts: - @if [[ -f '$(HOSTS_LOCATION)' ]]; then \ - actual="$$($(SHA256_CMD) '$(HOSTS_LOCATION)' | awk '{print $$1}')"; \ - if [[ "$$actual" != '$(HOSTS_SHA256)' ]]; then \ - echo "SHA-256 mismatch for $(HOSTS_LOCATION): expected '$(HOSTS_SHA256)', got '$$actual'"; \ - exit 1; \ - fi; \ +verify-hosts: download-hosts + @if [[ ! -f '$(HOSTS_LOCATION)' ]]; then \ + echo "Missing $(HOSTS_LOCATION); refusing to run it"; \ + exit 1; \ + fi; \ + actual="$$($(SHA256_CMD) '$(HOSTS_LOCATION)' | awk '{print $$1}')"; \ + if [[ "$$actual" != '$(HOSTS_SHA256)' ]]; then \ + echo "SHA-256 mismatch for $(HOSTS_LOCATION): expected '$(HOSTS_SHA256)', got '$$actual'"; \ + exit 1; \ fi clean-hosts: verify-hosts From b862b3e05da396e7d7dac9af3d4ad95c6483de19 Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 12:05:42 +0100 Subject: [PATCH 08/13] Isolate rate limiter in functional tests via unique client IP GeneratorTest::setUp() retrieved the rate_limiter.cache pool directly to clear limiter state between tests. That service is only present in a freshly compiled test container; with debug=false and a stale var/cache/test it is missing, so every GeneratorTest test errored in setUp before issuing a request. The limiter keys requests by client IP (plus method and path). Give each test a unique, per-run IP instead, which isolates limiter buckets between tests and test runs without ever touching the cache pool. Production Redis limiter and the test filesystem pool override are unchanged. --- tests/Functional/GeneratorTest.php | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/tests/Functional/GeneratorTest.php b/tests/Functional/GeneratorTest.php index e822529..edece84 100644 --- a/tests/Functional/GeneratorTest.php +++ b/tests/Functional/GeneratorTest.php @@ -33,14 +33,24 @@ public function setUp(): void { parent::setUp(); - $this->client = static::createClient(options: [ - 'environment' => 'test', - 'debug' => false, - ]); - - // The rate limiter pool survives kernel resets in tests, so clear it to - // keep limiter state from leaking between tests. - $this->client->getContainer()->get('rate_limiter.cache')->clear(); + // The rate limiter keys requests by client IP, and its cache pool + // survives kernel resets. Give each test a unique, per-run IP so limiter + // state cannot leak between tests or test runs, and so tests never need + // to reach into the limiter's cache pool directly. + $clientIp = sprintf( + '10.%d.%d.%d', + random_int(0, 255), + random_int(0, 255), + random_int(1, 254), + ); + + $this->client = static::createClient( + options: [ + 'environment' => 'test', + 'debug' => false, + ], + server: ['REMOTE_ADDR' => $clientIp], + ); } #[Test] From 9e7c56980374a2e20dbb14c78a53a5adede3bea6 Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 12:47:13 +0100 Subject: [PATCH 09/13] Pin GitHub Actions refs in tests workflow to full SHAs Audit SA-004: actions/checkout and actions/cache were referenced by mutable major-version tags. Pin both to immutable 40-char commit SHAs with version comments, matching build-containers.yaml. --- .github/workflows/tests.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 9a5e4f7..7e0a994 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -18,14 +18,14 @@ jobs: steps: - name: Checkout - uses: actions/checkout@v6 + uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6 - name: Get Composer Cache Directory id: composer-cache run: | echo "dir=$(make composer-cache-dir)" >> $GITHUB_OUTPUT - - uses: actions/cache@v5 + - uses: actions/cache@caa296126883cff596d87d8935842f9db880ef25 # v5 with: path: ${{ steps.composer-cache.outputs.dir }} key: ${{ runner.os }}-composer-${{ hashFiles('**/composer.lock') }} From b59dae754549a270e8ab1166560e242c7c0330f5 Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 12:47:20 +0100 Subject: [PATCH 10/13] Fix Makefile verify targets to chmod verified binaries verify-mkcert/verify-hosts now chmod +x the binary immediately after the SHA-256 comparison succeeds, so every consumer (create-certs, init-hosts, install-mkcert, clean-hosts) gets an executable binary. Removes the redundant chmod from install-mkcert/install-hosts that left create-certs and init-hosts failing standalone on fresh checkouts and raced under make -j. Mismatch still exits before any chmod. --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 22a8634..9e1fe6c 100644 --- a/Makefile +++ b/Makefile @@ -110,7 +110,6 @@ composer-update: make composer-install install-mkcert: verify-mkcert - chmod +x $(MKCERT_LOCATION) bin/mkcert -install download-mkcert: @@ -134,12 +133,12 @@ verify-mkcert: download-mkcert echo "SHA-256 mismatch for $(MKCERT_LOCATION): expected '$(MKCERT_SHA256)', got '$$actual'"; \ exit 1; \ fi + chmod +x $(MKCERT_LOCATION) create-certs: verify-mkcert bin/mkcert -cert-file=infrastructure/local/localhost.pem -key-file=infrastructure/local/localhost-key.pem $(SITE_HOST) install-hosts: verify-hosts - chmod +x $(HOSTS_LOCATION) download-hosts: @echo "Installing hosts script ($(HOSTS_VERSION))" @@ -158,6 +157,7 @@ verify-hosts: download-hosts echo "SHA-256 mismatch for $(HOSTS_LOCATION): expected '$(HOSTS_SHA256)', got '$$actual'"; \ exit 1; \ fi + chmod +x $(HOSTS_LOCATION) clean-hosts: verify-hosts sudo bin/hosts remove --force *$(SITE_HOST) > /dev/null 2>&1 || exit 0 From 40732d5560b240c7a25557640caa5eb93ff47641 Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 12:54:01 +0100 Subject: [PATCH 11/13] Reject slash-only host paths and cover path injection/length regressions HOST_PATH now requires at least one non-slash character so values like '/', '//' no longer validate and cannot collapse to an empty DockerCompose volume. Adds unit cases for slash-only and nginx/newline/semicolon/#/$ payloads, plus functional length boundaries for appPath/dockerWorkingDir (255/256) and frontControllerPath (128/129). --- src/Assert/Path.php | 2 +- tests/Functional/GeneratorTest.php | 66 +++++++++++++++++++++++++ tests/Unit/Assert/PathValidatorTest.php | 11 +++++ 3 files changed, 78 insertions(+), 1 deletion(-) diff --git a/src/Assert/Path.php b/src/Assert/Path.php index c538b8c..16ca3b5 100644 --- a/src/Assert/Path.php +++ b/src/Assert/Path.php @@ -52,7 +52,7 @@ public static function getRegexes(): array return [ self::ABSOLUTE_DIR => '\A/[A-Za-z0-9._-]+(?:/[A-Za-z0-9._-]+)*/?\z', self::RELATIVE_PHP => '\A[A-Za-z0-9._-]+(?:/[A-Za-z0-9._-]+)*\.php\z', - self::HOST_PATH => '\A[A-Za-z0-9._/-]+\z', + self::HOST_PATH => '\A[A-Za-z0-9._/-]*[A-Za-z0-9._-][A-Za-z0-9._/-]*\z', ]; } } diff --git a/tests/Functional/GeneratorTest.php b/tests/Functional/GeneratorTest.php index edece84..194bbef 100644 --- a/tests/Functional/GeneratorTest.php +++ b/tests/Functional/GeneratorTest.php @@ -212,6 +212,19 @@ public function testInvalidAppPathIsRejected(): void self::assertStringContainsString('This value is not a valid path', (string) $this->client->getResponse()->getContent()); } + #[Test] + public function testSlashOnlyAppPathIsRejected(): void + { + $this->client->request('GET', '/'); + $this->client->submitForm('Generate project archive', [ + 'project[globalOptions][basePort]' => '8000', + 'project[globalOptions][appPath]' => '/', + ]); + + self::assertResponseIsSuccessful(); + self::assertStringContainsString('This value is not a valid path', (string) $this->client->getResponse()->getContent()); + } + #[Test] public function testInvalidDockerWorkingDirIsRejected(): void { @@ -251,6 +264,59 @@ public function testOversizedAppPathIsRejected(): void self::assertStringContainsString('This value is too long', (string) $this->client->getResponse()->getContent()); } + #[Test] + public function testMaxLengthAppPathIsAccepted(): void + { + $this->generateAndGetZip([ + 'project[globalOptions][basePort]' => '8000', + 'project[globalOptions][appPath]' => str_repeat('a', 255), + ]); + } + + #[Test] + public function testMaxLengthDockerWorkingDirIsAccepted(): void + { + $this->generateAndGetZip([ + 'project[globalOptions][basePort]' => '8000', + 'project[globalOptions][dockerWorkingDir]' => '/' . str_repeat('a', 254), + ]); + } + + #[Test] + public function testOversizedDockerWorkingDirIsRejected(): void + { + $this->client->request('GET', '/'); + $this->client->submitForm('Generate project archive', [ + 'project[globalOptions][basePort]' => '8000', + 'project[globalOptions][dockerWorkingDir]' => '/' . str_repeat('a', 255), + ]); + + self::assertResponseIsSuccessful(); + self::assertStringContainsString('This value is too long', (string) $this->client->getResponse()->getContent()); + } + + #[Test] + public function testMaxLengthFrontControllerPathIsAccepted(): void + { + $this->generateAndGetZip([ + 'project[globalOptions][basePort]' => '8000', + 'project[phpOptions][frontControllerPath]' => str_repeat('a', 124) . '.php', + ]); + } + + #[Test] + public function testOversizedFrontControllerPathIsRejected(): void + { + $this->client->request('GET', '/'); + $this->client->submitForm('Generate project archive', [ + 'project[globalOptions][basePort]' => '8000', + 'project[phpOptions][frontControllerPath]' => str_repeat('a', 125) . '.php', + ]); + + self::assertResponseIsSuccessful(); + self::assertStringContainsString('This value is too long', (string) $this->client->getResponse()->getContent()); + } + #[Test] public function testPostGenerationIsRateLimited(): void { diff --git a/tests/Unit/Assert/PathValidatorTest.php b/tests/Unit/Assert/PathValidatorTest.php index d8133e4..94d4f9c 100644 --- a/tests/Unit/Assert/PathValidatorTest.php +++ b/tests/Unit/Assert/PathValidatorTest.php @@ -61,6 +61,17 @@ public static function invalidPathsProvider(): array 'php missing extension' => [Path::RELATIVE_PHP, 'public/index'], 'php space' => [Path::RELATIVE_PHP, 'public/my index.php'], 'php traversal' => [Path::RELATIVE_PHP, 'public/../index.php'], + 'slash only host path' => [Path::HOST_PATH, '/'], + 'double slash host path' => [Path::HOST_PATH, '//'], + 'triple slash host path' => [Path::HOST_PATH, '///'], + 'absolute newline injection' => [Path::ABSOLUTE_DIR, "/var/www\nfastcgi_pass evil;"], + 'absolute semicolon injection' => [Path::ABSOLUTE_DIR, '/var/www;'], + 'absolute hash injection' => [Path::ABSOLUTE_DIR, '/var/www#'], + 'absolute dollar injection' => [Path::ABSOLUTE_DIR, '/var/www$foo'], + 'php newline injection' => [Path::RELATIVE_PHP, "public/index.php\n"], + 'php semicolon injection' => [Path::RELATIVE_PHP, 'public/index.php;'], + 'php hash injection' => [Path::RELATIVE_PHP, 'public/index.php#'], + 'php dollar injection' => [Path::RELATIVE_PHP, 'public/index.php$'], ]; } From 5e6a8c630c9040f75ba15236ef0652bae58c788a Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 13:00:24 +0100 Subject: [PATCH 12/13] Trust forwarded headers only from configured proxies Configure framework.trusted_headers to x-forwarded-for/proto/port so no other forwarded header is honoured, and rely on Symfony's default SYMFONY_TRUSTED_PROXIES env var (unset in .env) for a no-trust fallback. Remove the obsolete TRUSTED_PROXIES line that no code read. .env.test points SYMFONY_TRUSTED_PROXIES at a documentation-only range and TrustedProxyTest proves the rate limiter buckets forwarded clients separately through a trusted peer while an untrusted peer cannot spoof X-Forwarded-For. --- .env | 3 - .env.test | 3 + config/packages/framework.yaml | 8 ++ tests/Functional/TrustedProxyTest.php | 138 ++++++++++++++++++++++++++ 4 files changed, 149 insertions(+), 3 deletions(-) create mode 100644 tests/Functional/TrustedProxyTest.php diff --git a/.env b/.env index c0b38c1..796b445 100644 --- a/.env +++ b/.env @@ -23,6 +23,3 @@ GOOGLE_ANALYTICS=foo REDIS_HOST=redis REDIS_PORT=6379 - -# Required for https redirects to correctly work behind a reverse proxy / load balancer -TRUSTED_PROXIES=127.0.0.1,10.0.0.0/8,172.16.0.0/12 diff --git a/.env.test b/.env.test index 9e7162f..9f7baf3 100644 --- a/.env.test +++ b/.env.test @@ -2,5 +2,8 @@ KERNEL_CLASS='App\Kernel' APP_SECRET='$ecretf0rt3st' SYMFONY_DEPRECATIONS_HELPER=999999 + +# Trusted proxy used by functional tests exercising forwarded client IPs. +SYMFONY_TRUSTED_PROXIES=192.0.2.0/24 PANTHER_APP_ENV=panther PANTHER_ERROR_SCREENSHOT_DIR=./var/error-screenshots diff --git a/config/packages/framework.yaml b/config/packages/framework.yaml index bbd6107..549331f 100644 --- a/config/packages/framework.yaml +++ b/config/packages/framework.yaml @@ -13,6 +13,14 @@ framework: cookie_secure: auto cookie_samesite: strict + # Only these forwarded headers are honoured, and only when the request + # arrives from a proxy listed in SYMFONY_TRUSTED_PROXIES (Symfony's default + # env var, left unset here so no proxy is trusted by default). + trusted_headers: + - x-forwarded-for + - x-forwarded-proto + - x-forwarded-port + #esi: true #fragments: true php_errors: diff --git a/tests/Functional/TrustedProxyTest.php b/tests/Functional/TrustedProxyTest.php new file mode 100644 index 0000000..a4f2851 --- /dev/null +++ b/tests/Functional/TrustedProxyTest.php @@ -0,0 +1,138 @@ +createClientWithRemoteAddr(self::TRUSTED_PEER); + + // The limiter cache pool survives kernel resets and test runs, so use + // unique client IPs to avoid collisions with other tests or runs. + $clientA = $this->randomClientIp(); + $clientB = $this->randomClientIp(); + self::assertNotSame($clientA, $clientB); + + for ($i = 1; $i <= 3; ++$i) { + $this->submitGeneration(clientIp: $clientA); + self::assertResponseIsSuccessful(); + } + + // Fourth request from the same forwarded client exceeds the limit. + $this->submitGeneration(clientIp: $clientA); + self::assertResponseStatusCodeSame(429); + + // A different forwarded client through the same trusted peer has its + // own bucket and is still allowed. + $this->submitGeneration(clientIp: $clientB); + self::assertResponseIsSuccessful(); + } + + #[Test] + public function untrustedPeerCannotSpoofForwardedClientToEvadeLimit(): void + { + $this->createClientWithRemoteAddr($this->randomUntrustedIp()); + + // Spoof a different X-Forwarded-For on every request. As the peer is not + // trusted, the limiter must key on REMOTE_ADDR, so all four requests + // share one bucket and the fourth is rejected. + for ($i = 1; $i <= 3; ++$i) { + $this->submitGeneration(clientIp: $this->randomClientIp()); + self::assertResponseIsSuccessful(); + } + + $this->submitGeneration(clientIp: $this->randomClientIp()); + self::assertResponseStatusCodeSame(429); + } + + #[Test] + public function onlyForwardedForProtoAndPortAreTrusted(): void + { + $this->createClientWithRemoteAddr(self::TRUSTED_PEER); + + self::assertSame(['192.0.2.0/24'], Request::getTrustedProxies()); + self::assertSame( + Request::HEADER_X_FORWARDED_FOR | Request::HEADER_X_FORWARDED_PROTO | Request::HEADER_X_FORWARDED_PORT, + Request::getTrustedHeaderSet(), + ); + } + + private function createClientWithRemoteAddr(string $remoteAddr): void + { + $this->client = static::createClient( + options: [ + 'environment' => 'test', + 'debug' => false, + ], + server: ['REMOTE_ADDR' => $remoteAddr], + ); + } + + /** + * Performs a valid, CSRF-protected generation POST claiming the given client IP. + */ + private function submitGeneration(string $clientIp): void + { + $this->client->request('GET', '/'); + $this->client->submitForm( + 'Generate project archive', + ['project[globalOptions][basePort]' => '8000'], + serverParameters: ['HTTP_X_FORWARDED_FOR' => $clientIp], + ); + } + + private function randomClientIp(): string + { + return sprintf('198.51.100.%d', random_int(1, 254)); + } + + private function randomUntrustedIp(): string + { + return sprintf('203.0.113.%d', random_int(1, 254)); + } +} From 9282f7e94c2c7d98b0bd4d05b6b7c318f47fcfa0 Mon Sep 17 00:00:00 2001 From: Luis Pabon Date: Fri, 25 Sep 2026 13:05:09 +0100 Subject: [PATCH 13/13] Add least-privilege permissions block to tests workflow Audit SA-004: tests.yaml had no explicit permissions, so the job ran with default token permissions. Scope the job to contents: read. --- .github/workflows/tests.yaml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/tests.yaml b/.github/workflows/tests.yaml index 7e0a994..3ffef61 100644 --- a/.github/workflows/tests.yaml +++ b/.github/workflows/tests.yaml @@ -15,6 +15,8 @@ jobs: tests: timeout-minutes: 10 runs-on: ubuntu-latest + permissions: + contents: read steps: - name: Checkout