From fe997b93f44b883ea1e64fb15842ce2b0a9703f8 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 24 Aug 2026 15:17:46 +1000 Subject: [PATCH 1/5] Make the suite pass on Windows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `windows` job added alongside the audit fixes ran 658 tests and returned 45 failures. One was the library; the rest were the suite and the checkout assuming POSIX. **The library.** `FileInfo::setNameWithExtension()` and `Filename::sanitizeNameWithExtension()` split a client filename with `pathinfo()`, which treats `\` as a path separator on Windows and as an ordinary character on POSIX. The same name split two ways: `a\b.txt` was stored as `a-b.txt` here and `b.txt` there, `..\..\windows\win.ini` as `windows-win.ini` or `win.ini`. `Filename::rewriteCharacters()` rewrites `\` to `-`, so the rule both layers read from `Filename` is that a backslash stays in the name — and a rule cannot depend on which platform applies it. `Filename::splitNameAndExtension()` owns the split now, with `/` the only separator on every platform. It answers what `pathinfo()`'s two fields answered otherwise, trailing slashes and the all-extension dotfile included, which the provider pins case by case. **The assertions.** `upload()` returns `$this->directory . $filename` and the constructor ends the directory with `DIRECTORY_SEPARATOR`, where these tests build their working directory with `/`. Windows resolves both to the same file, so the storage was correct and the string comparison was not. Four sites and the shared `assertStoredAs()` helper now go through `destinationOf()`. **The line endings.** The callback tests echoed `PHP_EOL` against an expected literal `\n`; they are about hook order, so the literal wins. `CatalogueTest` parses `i18n/upload.pot` line by line and a CRLF checkout left `"$` unmatchable, so no msgid was read and every assertion failed vacuously — `.gitattributes` pins that file to LF, which the `i18n` workflow's byte comparison needs regardless. **The subprocess.** `FilenameTest` builds a `php -r` script containing `"a\nb"`. `escapeshellarg()` quotes with `"` on Windows and cannot escape one inside the argument, so the inner quotes were dropped and PHP read `a\nb` as a constant. The script carries no quote character of its own now. **What cannot run there.** NTFS refuses control characters in a filename, so `touch()` cannot create the colliding file two of the collision-message data sets need. Those two are split into their own `@group posix` test, since a data set cannot carry a group. Co-Authored-By: Claude Opus 5 (1M context) --- .gitattributes | 4 ++ CHANGELOG.md | 1 + CLAUDE.md | 2 + UPGRADE.md | 5 +++ src/Upload/FileInfo.php | 8 +++- src/Upload/Filename.php | 41 ++++++++++++++++++- tests/Upload/FileInfoTest.php | 9 +++++ tests/Upload/FileListTest.php | 2 +- tests/Upload/FileTest.php | 8 ++-- tests/Upload/FilenameTest.php | 54 ++++++++++++++++++++++++- tests/Upload/Storage/FileSystemTest.php | 51 ++++++++++++++++++++--- 11 files changed, 169 insertions(+), 16 deletions(-) diff --git a/.gitattributes b/.gitattributes index e95e208..d897a8e 100644 --- a/.gitattributes +++ b/.gitattributes @@ -7,3 +7,7 @@ /phpunit.xml export-ignore /phpstan.neon export-ignore /CLAUDE.md export-ignore + +# The catalogue's exact bytes are a guarantee: the `i18n` workflow regenerates it and fails on +# a diff, and `CatalogueTest` parses it line by line. A CRLF checkout on Windows breaks both. +/i18n/upload.pot text eol=lf diff --git a/CHANGELOG.md b/CHANGELOG.md index a5c8f16..0ce44ad 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,7 @@ Both still work and neither raises a runtime notice. ## Bug Fixes +* **A backslash in a client filename is a character, not a path separator.** `FileInfo::setNameWithExtension()` and `Filename::sanitizeNameWithExtension()` split through `pathinfo()`, which treats `\` as a separator on Windows and as an ordinary character on POSIX — so `a\b.txt` was stored as `a-b.txt` on one and `b.txt` on the other, and `..\..\windows\win.ini` as `windows-win.ini` or `win.ini`. `Filename::rewriteCharacters()` rewrites `\` to `-`, so the rule both layers read from `Filename` is that it stays in the name; a rule cannot depend on which platform applies it. `Filename::splitNameAndExtension()` owns the split now and treats `/` alone as a separator, on every platform. **On Windows this keeps name content that was previously discarded** * **`$file[] = $fileInfo` appends instead of silently discarding a file.** PHP passes `offsetSet()` a null offset for the append syntax, and assigning it straight through wrote the string key `''` rather than the next integer: the second append overwrote the first, and a key the `ArrayAccess` contract does not admit reached `getUploadedLocators()`, which `store()` keys by collection offset. PHP 8.5 also deprecates the null offset, so every append raised a notice. On `FileList`, the offset the append lands at is read back before the source key is dropped, so `getSourceKeys()[$i]` keeps naming `$list[$i]` * **`Validation\Size` rejects a bound that is not a byte count, at construction.** A float — what a limit read out of JSON or arrived at by division actually is — reached the `int`-typed `scale()` and raised a `TypeError` from inside `validate()`, where `File::runValidations()` absorbed it as `Validation could not be completed`: the developer's misconfiguration shown to whoever submitted the file, with nothing anywhere naming the cause. Both bounds are now checked in the constructor, along with a negative bound and a minimum above the maximum, all as `InvalidArgumentException` * **`Validation\Mimetype` folds its allow-list.** `Extension` and `FileType` both put theirs through `AsciiCase::toLower()`; this one compared what it was given. A media type is case-insensitive and `FileInfo::getMimetype()` always answers lowercase, so `new Mimetype(['IMAGE/PNG'])` rejected every PNG. The sniffed type is folded too, which only a custom `FileInfoInterface` can arrive with in another case diff --git a/CLAUDE.md b/CLAUDE.md index 10e4d97..d90c4c4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -47,6 +47,8 @@ The `i18n` workflow regenerates the catalogue and fails on a diff, which is what The `phpunit` workflow carries a second job, `cross-file-system`, which mounts a tmpfs and points `UPLOAD_TEST_OTHER_FS` at it so `FileSystemTest::testStoresAFileFromAnotherFileSystem()` runs against two real file systems rather than skipping. It runs the **whole suite** with `--fail-on-skipped` rather than filtering to that test: a `--filter` matching nothing exits 0 with "No tests executed!", so it needed a second guard to prove it had run, and renaming the test was enough to trigger exactly that. That test is the suite's only `markTestSkipped()`, which is what makes one guard sufficient — keep it that way, or the job goes quiet. Tests that cannot run in a given configuration are excluded by group instead: `@group mbstring` marks the ones that need the UTF-8 repair, so the `no-mbstring` job drops them with `--exclude-group` rather than adding a second skip, and `@group posix` marks the fourteen that need `symlink()`, `chmod()` or `umask()` to mean something, which the `windows` job drops the same way. Set `UPLOAD_TEST_OTHER_FS` to run it locally: on macOS, `hdiutil attach -nomount ram://8192` then `diskutil erasevolume HFS+ UPLOADTMP `. +`Filename::splitNameAndExtension()` is what `FileInfo::setNameWithExtension()` and `Filename::sanitizeNameWithExtension()` split with, not `pathinfo()`: that treats `\` as a path separator on Windows and as an ordinary character on POSIX, so one client name split two ways. `rewriteCharacters()` rewrites `\` to `-`, so the shared rule is that a backslash stays in the name. `/` is a separator on every platform. Do not put `pathinfo()` back. + The `windows` job is `continue-on-error: true` until it has been green once — nothing in this project had ever run on Windows, so the first runs are discovery rather than a gate. **Remove the flag once it passes**, and the comment above it with it: a job nobody may merge past is why it is there. `phpunit.xml` sets `convertDeprecationsToExceptions`, `failOnWarning` and `failOnRisky`, so a deprecation raised from `src/` fails the suite rather than passing through a green run — which is how `$file[] = $fileInfo` wrote a string key and warned about it under PHP 8.5 while the suite reported OK. `composer phpstan` bootstraps PHPStan from `tools/phpstan/` rather than the root `require-dev`. PHPStan 2.x needs PHP 7.4 to run, and the root manifest has to stay resolvable on 7.3 or the 7.3 test and PHPCS jobs cannot install at all. `phpstan.neon` sets `phpVersion` to the 7.3-8.5 range, so the analysis still covers the whole supported range from whatever version runs it. diff --git a/UPGRADE.md b/UPGRADE.md index c8216e9..55927ba 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -324,6 +324,11 @@ Each of these is listed in full in the [changelog](CHANGELOG.md). sequence no longer reports every error twice. * `$file[0] = $value` throws `InvalidArgumentException` unless the value is a `FileInfoInterface`. +* **On Windows, a backslash in a client filename is no longer treated as a path separator.** + `pathinfo()` splits on it there and not on POSIX, so `a\b.txt` was stored as `b.txt` under + Windows and `a-b.txt` everywhere else. It is now `a-b.txt` on both, which is what + `Filename`'s documented rules always said. A Windows deployment relying on the implicit + basename-ing gets longer names than before; nothing on POSIX changes. * `$file[] = $value` appends. In 3.x it wrote the string key `''`, so a second append overwrote the first and `getUploadedLocators()` came back with a key that is not an offset. * **`Validation\Size` now throws `InvalidArgumentException` for a bound it cannot use**: one diff --git a/src/Upload/FileInfo.php b/src/Upload/FileInfo.php index 2cec2ec..2bf160b 100644 --- a/src/Upload/FileInfo.php +++ b/src/Upload/FileInfo.php @@ -189,11 +189,15 @@ public function getNameWithExtension(): string public function setNameWithExtension(string $name): FileInfo { + /* `Filename` rather than `pathinfo()`, which treats `\` as a separator on Windows and + not on POSIX — the same client name split two ways. See splitNameAndExtension(). */ + list($base, $extension) = Filename::splitNameAndExtension($name); + /* Not setExtension(): that re-fits the name to the new budget, and the setName() below overwrites the result unconditionally. Assign the extension, then let setName() do the one fit that survives. */ - $this->extension = $this->acceptExtension(pathinfo($name, PATHINFO_EXTENSION)); - $this->setName(pathinfo($name, PATHINFO_FILENAME)); + $this->extension = $this->acceptExtension($extension); + $this->setName($base); return $this; } diff --git a/src/Upload/Filename.php b/src/Upload/Filename.php index 26d1460..cb9151e 100644 --- a/src/Upload/Filename.php +++ b/src/Upload/Filename.php @@ -143,12 +143,49 @@ public static function sanitizeName(string $name, string $extension = '', ?array */ public static function sanitizeNameWithExtension(string $filename, ?array $reserved = null): string { - $extension = self::acceptExtension((string) pathinfo($filename, PATHINFO_EXTENSION), $reserved); - $name = self::sanitizeName((string) pathinfo($filename, PATHINFO_FILENAME), $extension, $reserved); + list($name, $extension) = self::splitNameAndExtension($filename); + + $extension = self::acceptExtension($extension, $reserved); + $name = self::sanitizeName($name, $extension, $reserved); return $extension === '' ? $name : sprintf('%s.%s', $name, $extension); } + /** + * Split a client-supplied filename into the name and the extension + * + * What `pathinfo()`'s `PATHINFO_FILENAME` and `PATHINFO_EXTENSION` answer, except that `/` + * is the only separator. `pathinfo()` treats `\` as one on Windows and not on POSIX, so the + * same name split two ways: `a\b.txt` was stored as `a-b.txt` here and `b.txt` there. + * `rewriteCharacters()` rewrites `\` to `-`, so the rule both layers share is that a + * backslash is a character in the name — and a rule cannot depend on which platform is + * applying it. + * + * Trailing slashes go first, as `basename()` drops them, so `photos/` still names `photos`. + * + * @return array The name and the extension, either of which may be `''` + * @phpstan-return array{0: string, 1: string} + * + * @internal Not part of the public API + */ + public static function splitNameAndExtension(string $filename): array + { + $filename = rtrim($filename, '/'); + + $separator = strrpos($filename, '/'); + $basename = $separator === false ? $filename : substr($filename, $separator + 1); + + /* The last dot, wherever it is: `.htaccess` is all extension and no name, which is what + `pathinfo()` answers and what the storage deny-list is then handed. */ + $dot = strrpos($basename, '.'); + + if ($dot === false) { + return [$basename, '']; + } + + return [substr($basename, 0, $dot), substr($basename, $dot + 1)]; + } + /** * Make a string safe to render as one line of prose * diff --git a/tests/Upload/FileInfoTest.php b/tests/Upload/FileInfoTest.php index 4b86b83..9af1e13 100644 --- a/tests/Upload/FileInfoTest.php +++ b/tests/Upload/FileInfoTest.php @@ -277,6 +277,15 @@ public function providerSetNameSanitizing(): array /* Neither is a control character, so both survive */ 75 => ["caf\u{00E9}", 'txt', "caf\u{00E9}.txt"], 76 => ["10\u{20AC}", 'txt', "10\u{20AC}.txt"], + + /* A backslash is a character in the name, not a separator, on every platform. + `pathinfo()` splits on it under Windows and not under POSIX, so these came back + as `b` and `windows-win` respectively until `Filename::splitNameAndExtension()` + took the split over. A forward slash *is* a separator on both. */ + 82 => ['a-b', 'txt', 'a\b.txt'], + 83 => ['windows-win', 'ini', '..\..\windows\win.ini'], + 84 => ['b', 'txt', 'a/b.txt'], + 85 => ['passwd', '', '../../etc/passwd'], ]; } diff --git a/tests/Upload/FileListTest.php b/tests/Upload/FileListTest.php index d569d7e..cd9bcd7 100644 --- a/tests/Upload/FileListTest.php +++ b/tests/Upload/FileListTest.php @@ -503,7 +503,7 @@ public function testTheLifecycleCallbacksFirePerFile(): void foreach (['beforeValidate', 'afterValidate', 'beforeUpload', 'afterUpload'] as $hook) { $list->$hook(static function (FileInfoInterface $fileInfo) use ($hook): void { - echo ucfirst($hook) . ': ' . $fileInfo->getName(), PHP_EOL; + echo ucfirst($hook) . ': ' . $fileInfo->getName(), "\n"; }); } diff --git a/tests/Upload/FileTest.php b/tests/Upload/FileTest.php index 2537fd5..399d934 100644 --- a/tests/Upload/FileTest.php +++ b/tests/Upload/FileTest.php @@ -277,19 +277,19 @@ public function testCallbacks(): void ); $callbackBeforeValidate = function (FileInfoInterface $fileInfo) { - echo 'BeforeValidate: ' . $fileInfo->getName(), PHP_EOL; + echo 'BeforeValidate: ' . $fileInfo->getName(), "\n"; }; $callbackAfterValidate = function (FileInfoInterface $fileInfo) { - echo 'AfterValidate: ' . $fileInfo->getName(), PHP_EOL; + echo 'AfterValidate: ' . $fileInfo->getName(), "\n"; }; $callbackBeforeUpload = function (FileInfoInterface $fileInfo) { - echo 'BeforeUpload: ' . $fileInfo->getName(), PHP_EOL; + echo 'BeforeUpload: ' . $fileInfo->getName(), "\n"; }; $callbackAfterUpload = function (FileInfoInterface $fileInfo) { - echo 'AfterUpload: ' . $fileInfo->getName(), PHP_EOL; + echo 'AfterUpload: ' . $fileInfo->getName(), "\n"; }; $file = new File('multiple', $this->storage); diff --git a/tests/Upload/FilenameTest.php b/tests/Upload/FilenameTest.php index 29620e4..6abf9bf 100644 --- a/tests/Upload/FilenameTest.php +++ b/tests/Upload/FilenameTest.php @@ -124,6 +124,54 @@ public function testFinalizeSurvivesAnExtensionLongerThanTheWholeBudget(): void $this->assertSame(Filename::FALLBACK, Filename::finalize('report', $extension)); } + /** + * `pathinfo()` treats `\` as a path separator on Windows and as an ordinary character on + * POSIX, so the same client name split two ways: `a\b.txt` was stored as `a-b.txt` here + * and `b.txt` there. `rewriteCharacters()` rewrites `\` to `-`, so the rule these layers + * share is that a backslash is part of the name. + * + * Every other case pins the `pathinfo()` behaviour this replaces, including the trailing + * slash `basename()` drops and the dotfile that is all extension and no name. + * + * @dataProvider provideFilenamesToSplit + * + * @param string[] $expected + */ + public function testSplitNameAndExtension(string $filename, array $expected): void + { + $this->assertSame($expected, Filename::splitNameAndExtension($filename)); + } + + /** + * @return array> + */ + public function provideFilenamesToSplit(): array + { + return [ + 'an ordinary name' => ['report.txt', ['report', 'txt']], + 'no extension' => ['report', ['report', '']], + 'a trailing dot' => ['report.', ['report', '']], + 'the last dot wins' => ['archive.tar.gz', ['archive.tar', 'gz']], + 'a dotfile is all extension' => ['.htaccess', ['', 'htaccess']], + 'empty' => ['', ['', '']], + + /* A backslash is a character, on every platform */ + 'a backslash is not a separator' => ['a\\b.txt', ['a\\b', 'txt']], + 'a windows path is not split' => ['..\\..\\windows\\win.ini', ['..\\..\\windows\\win', 'ini']], + + /* A forward slash is one, on every platform */ + 'a slash is a separator' => ['a/b.txt', ['b', 'txt']], + 'traversal is reduced' => ['../../etc/passwd', ['passwd', '']], + 'a trailing slash is dropped' => ['photos/', ['photos', '']], + 'repeated slashes' => ['a//b.txt//', ['b', 'txt']], + 'a slash alone' => ['/', ['', '']], + + /* pathinfo() answers `.` and `..` for these, and the deny-list is handed nothing */ + 'this directory' => ['..', ['.', '']], + 'three dots' => ['...', ['..', '']], + ]; + } + /** * @dataProvider provideNamesToSplit * @@ -198,8 +246,12 @@ public function provideDeviceComponents(): array */ public function testSanitizeTextSurvivesWithoutMbstring(): void { + /* `chr()` rather than a quoted "a\nb": `escapeshellarg()` quotes with `"` on Windows + and cannot escape a `"` inside the argument, so the inner quotes were dropped and + PHP read `a\nb` as a constant. Nothing here needs a quote character of its own. */ $script = sprintf( - 'require %s; echo \GravityPdf\Upload\Filename::sanitizeForDisplay("a\nb");', + 'require %s; echo \GravityPdf\Upload\Filename::sanitizeForDisplay(' + . 'chr(97) . chr(10) . chr(98));', var_export(dirname(__DIR__, 2) . '/vendor/autoload.php', true) ); diff --git a/tests/Upload/Storage/FileSystemTest.php b/tests/Upload/Storage/FileSystemTest.php index 45634d2..e2858b3 100644 --- a/tests/Upload/Storage/FileSystemTest.php +++ b/tests/Upload/Storage/FileSystemTest.php @@ -717,18 +717,44 @@ public function testCollisionMessageSanitizesTheNameItQuotes(string $name, strin ); } + /** + * The two cases carrying control characters, which NTFS refuses in a filename outright — + * `touch()` cannot create the colliding file, so there is nothing for the reservation to + * find in the way. Split out and grouped rather than skipped, since a data set cannot + * carry a group of its own. + * + * @dataProvider providerNamesWithControlCharactersASeamMayReturn + * + * @group posix + */ + public function testCollisionMessageSanitizesControlCharactersInTheNameItQuotes( + string $name, + string $message + ): void { + $this->testCollisionMessageSanitizesTheNameItQuotes($name, $message); + } + /** * @return array> */ - public function providerNamesAnOverriddenSeamMayReturn(): array + public function providerNamesWithControlCharactersASeamMayReturn(): array { return [ 'controls collapsed' => ["report\x07\x08.txt", 'A file named "report .txt" already exists'], - 'bidi deleted' => ["resume\xE2\x80\xAEtxt.gpj", 'A file named "resumetxt.gpj" already exists'], 'nothing but controls' => ["\x01\x02", 'A file with that name already exists'], ]; } + /** + * @return array> + */ + public function providerNamesAnOverriddenSeamMayReturn(): array + { + return [ + 'bidi deleted' => ["resume\xE2\x80\xAEtxt.gpj", 'A file named "resumetxt.gpj" already exists'], + ]; + } + /** * The rewrite runs before the deny-list, so hiding a blocked extension behind one of these * does not carry it past the check. @@ -789,7 +815,7 @@ public function testTrailingDotsAndSpacesAreNotStored(): void $storage = $this->makeStorage($workingDirectory, true); $this->assertSame( - $workingDirectory . '/report.txt', + $this->destinationOf($workingDirectory, 'report.txt'), $storage->upload($this->makeHostileFileInfo('report.txt. ')) ); } @@ -1117,6 +1143,19 @@ protected function entriesIn(string $directory): array /** * A scratch directory that is removed again in tear_down() */ + /** + * The path `upload()` will return for a name stored in this directory + * + * It composes that as `$this->directory . $filename`, and the constructor ends the + * directory with `DIRECTORY_SEPARATOR`. These tests build their working directory with + * `/`, which PHP treats as the same path on Windows but is not the same *string* — so an + * assertion joining with `/` fails there against a file that was stored correctly. + */ + protected function destinationOf(string $directory, string $filename): string + { + return $directory . DIRECTORY_SEPARATOR . $filename; + } + protected function makeWorkingDirectory(): string { $workingDirectory = sys_get_temp_dir() . '/upload-test-' . uniqid('', true) . '/uploads'; @@ -1163,7 +1202,7 @@ protected function assertStoredAs(string $stored, FileInfoInterface $fileInfo): $workingDirectory = $this->makeWorkingDirectory(); $storage = $this->makeStorage($workingDirectory, true); - $this->assertSame($workingDirectory . '/' . $stored, $storage->upload($fileInfo)); + $this->assertSame($this->destinationOf($workingDirectory, $stored), $storage->upload($fileInfo)); $this->assertFileExists($workingDirectory . '/' . $stored); } @@ -1284,7 +1323,7 @@ public function testReturnsUploadedFileName(): void $storage = $this->makeStorage($workingDirectory, true); $this->assertSame( - $workingDirectory . '/foo.txt', + $this->destinationOf($workingDirectory, 'foo.txt'), $storage->upload(new FileInfo($this->assetsDirectory . '/foo.txt', 'foo.txt')) ); $this->assertSame($workingDirectory, $storage->getDirectory()); @@ -1460,7 +1499,7 @@ public function testAllowAnyExtensionClearsTheDenyList(): void $this->assertSame([], $storage->getBlockedExtensions()); $this->assertSame( - $workingDirectory . '/shell.php', + $this->destinationOf($workingDirectory, 'shell.php'), $storage->upload($this->makeHostileFileInfo('shell.php')) ); } From 5f0300e20248f20937519ba265dfd86a982ef1fe Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 24 Aug 2026 15:21:26 +1000 Subject: [PATCH 2/5] Route the last locator assertions through the separator helper `testAUnicodeSpaceIsKeptInAStoredName` was the one site the first pass missed, and the only Windows failure left: 9 of them, one per unicode space, all the same `/` against `DIRECTORY_SEPARATOR` comparison. The four remaining sites are inside `@group posix` tests, so Windows never reached them, but they carry the same assumption and would surface the day one of those tests stops being excluded. `destinationOf()` is now the only way this suite asserts a locator. Co-Authored-By: Claude Opus 5 (1M context) --- tests/Upload/FileListTest.php | 5 +++-- tests/Upload/Storage/FileSystemTest.php | 8 ++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/Upload/FileListTest.php b/tests/Upload/FileListTest.php index cd9bcd7..fade519 100644 --- a/tests/Upload/FileListTest.php +++ b/tests/Upload/FileListTest.php @@ -421,8 +421,9 @@ public function testAVouchedFileIsValidatedSanitizedAndStored(): void $this->assertTrue($list->upload()); - /* The interior dot is rewritten by `FileInfo::setName()`, as on the `$_FILES` path */ - $stored = $workingDirectory . '/holiday-photo.txt'; + /* The interior dot is rewritten by `FileInfo::setName()`, as on the `$_FILES` path. + Joined with `DIRECTORY_SEPARATOR` because that is how storage composes a locator. */ + $stored = $workingDirectory . DIRECTORY_SEPARATOR . 'holiday-photo.txt'; $this->assertSame([$stored], $list->getUploadedLocators()); $this->assertFileExists($stored); diff --git a/tests/Upload/Storage/FileSystemTest.php b/tests/Upload/Storage/FileSystemTest.php index e2858b3..e2c5b6f 100644 --- a/tests/Upload/Storage/FileSystemTest.php +++ b/tests/Upload/Storage/FileSystemTest.php @@ -617,7 +617,7 @@ public function testAUnicodeSpaceIsKeptInAStoredName(string $space): void foreach (['evil.php' . $space, 'evil.' . $space . 'php', $space . 'con.txt'] as $name) { $this->assertSame( - $workingDirectory . '/' . $name, + $this->destinationOf($workingDirectory, $name), $storage->upload($this->makeHostileFileInfo($name)), bin2hex($name) ); @@ -949,7 +949,7 @@ public function testStoresAFileNotUploadedByPhpOnceTheCallerAllowsIt(): void $stored = $storage->upload(new FileInfo($source, 'upload.txt')); - $this->assertSame($workingDirectory . '/upload.txt', $stored); + $this->assertSame($this->destinationOf($workingDirectory, 'upload.txt'), $stored); $this->assertStringEqualsFile($stored, 'tmp file bytes'); $this->assertSame('0640', $this->modeOf($stored)); @@ -1038,7 +1038,7 @@ public function testStoresAFileFromAnotherFileSystem(): void try { $stored = $storage->upload(new FileInfo($source, 'upload.txt')); - $this->assertSame($workingDirectory . '/upload.txt', $stored); + $this->assertSame($this->destinationOf($workingDirectory, 'upload.txt'), $stored); $this->assertStringEqualsFile($stored, 'bytes from another file system'); $this->assertSame('0640', $this->modeOf($stored)); $this->assertFileDoesNotExist($source); @@ -1092,7 +1092,7 @@ public function testCopiesTheFileWhenItCannotBeRenamedAcrossFileSystems(): void $stored = $this->makeAcceptingStorage($workingDirectory)->upload($fileInfo); - $this->assertSame($workingDirectory . '/upload.txt', $stored); + $this->assertSame($this->destinationOf($workingDirectory, 'upload.txt'), $stored); $this->assertStringEqualsFile($stored, 'tmp file bytes'); $this->assertSame('0640', $this->modeOf($stored)); From 5e66733b244207934c4696688b292b54af39f798 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 24 Aug 2026 15:26:40 +1000 Subject: [PATCH 3/5] Do not refuse a reservation where the inode is unavailable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows under PHP 7.3 reports `ino` as 0 for both `fstat()` and `lstat()`, so `reserveDestination()`'s identity comparison had nothing to compare — and read that as a mismatch. Every reservation was answered `'Destination is a symbolic link'`, which is every upload the default `$overwrite = false` makes: the platform could not store a single file. Nine of the ten remaining Windows 7.3 failures were that one line, the tenth its knock-on. Skipped where the inode is unavailable rather than failed. Nothing has been established either way, a real file has a real inode so POSIX gives up nothing, and the symlink protections in this class were already documented as not load-bearing on Windows, where a symlink needs a privilege an uploading process should not hold. The comment above the check claimed the comparison "degrades to same-drive and detects nothing" there. It detected everything. Corrected. `lstatEntry()` is the seam for asserting this on the platform the suite actually runs on, so the new test stubs the answer that platform gives — no inode, and a `dev` of its own, so it is the missing inode that decides rather than a lucky match on the drive. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + UPGRADE.md | 4 +++ src/Upload/Storage/FileSystem.php | 17 ++++++++++--- tests/Upload/Storage/FileSystemTest.php | 34 +++++++++++++++++++++++++ 4 files changed, 53 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ce44ad..3a38c5f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,7 @@ Both still work and neither raises a runtime notice. ## Bug Fixes +* **`Storage\FileSystem` can store a file on Windows under PHP 7.3 again.** `reserveDestination()` confirms its exclusive create by comparing `fstat()` and `lstat()`, which catches an `x` that followed a symlink. That platform reports `ino` as 0 for both, so the comparison had nothing to compare — and reading no information as a mismatch answered `'Destination is a symbolic link'` to every reservation, which is every upload the default `$overwrite = false` makes. The comparison is skipped where the inode is unavailable; a real file has a real inode, so nothing changes on POSIX, and the symlink protections here were already documented as not load-bearing on Windows. **7.3 only** — 7.4 onwards reports an inode there * **A backslash in a client filename is a character, not a path separator.** `FileInfo::setNameWithExtension()` and `Filename::sanitizeNameWithExtension()` split through `pathinfo()`, which treats `\` as a separator on Windows and as an ordinary character on POSIX — so `a\b.txt` was stored as `a-b.txt` on one and `b.txt` on the other, and `..\..\windows\win.ini` as `windows-win.ini` or `win.ini`. `Filename::rewriteCharacters()` rewrites `\` to `-`, so the rule both layers read from `Filename` is that it stays in the name; a rule cannot depend on which platform applies it. `Filename::splitNameAndExtension()` owns the split now and treats `/` alone as a separator, on every platform. **On Windows this keeps name content that was previously discarded** * **`$file[] = $fileInfo` appends instead of silently discarding a file.** PHP passes `offsetSet()` a null offset for the append syntax, and assigning it straight through wrote the string key `''` rather than the next integer: the second append overwrote the first, and a key the `ArrayAccess` contract does not admit reached `getUploadedLocators()`, which `store()` keys by collection offset. PHP 8.5 also deprecates the null offset, so every append raised a notice. On `FileList`, the offset the append lands at is read back before the source key is dropped, so `getSourceKeys()[$i]` keeps naming `$list[$i]` * **`Validation\Size` rejects a bound that is not a byte count, at construction.** A float — what a limit read out of JSON or arrived at by division actually is — reached the `int`-typed `scale()` and raised a `TypeError` from inside `validate()`, where `File::runValidations()` absorbed it as `Validation could not be completed`: the developer's misconfiguration shown to whoever submitted the file, with nothing anywhere naming the cause. Both bounds are now checked in the constructor, along with a negative bound and a minimum above the maximum, all as `InvalidArgumentException` diff --git a/UPGRADE.md b/UPGRADE.md index 55927ba..f470317 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -324,6 +324,10 @@ Each of these is listed in full in the [changelog](CHANGELOG.md). sequence no longer reports every error twice. * `$file[0] = $value` throws `InvalidArgumentException` unless the value is a `FileInfoInterface`. +* **Windows under PHP 7.3 could not store a file at all with the default `$overwrite = false`, + and now can.** The reservation's inode check read that platform's absent inode as a mismatch + and refused every upload as `'Destination is a symbolic link'`. Nothing changes on POSIX or + on PHP 7.4 and later. * **On Windows, a backslash in a client filename is no longer treated as a path separator.** `pathinfo()` splits on it there and not on POSIX, so `a\b.txt` was stored as `b.txt` under Windows and `a-b.txt` everywhere else. It is now `a-b.txt` on both, which is what diff --git a/src/Upload/Storage/FileSystem.php b/src/Upload/Storage/FileSystem.php index 70c8202..4feced5 100644 --- a/src/Upload/Storage/FileSystem.php +++ b/src/Upload/Storage/FileSystem.php @@ -453,8 +453,9 @@ protected function reserveDestination(string $destinationFile, FileInfoInterface is the destination only if the directory entry is that same file; a symlink has an inode of its own, so a mismatch means the name was a link. - POSIX only. Before PHP 7.4 `stat()` on Windows reports `ino` as 0 and `dev` as the - drive number, so this comparison degrades to same-drive and detects nothing. Windows + POSIX only. Before PHP 7.4 `stat()` on Windows reports `ino` as 0, so this + comparison has no information — the guard below skips it rather than reading no + information as a mismatch, which refused every upload on that platform. Windows symlinks need a privilege an uploading process should not hold, so the residual risk is small, but the symlink protections in this class are not load-bearing there. */ $entry = $this->lstatEntry($destinationFile); @@ -473,7 +474,17 @@ protected function reserveDestination(string $destinationFile, FileInfoInterface ); } - if ($opened['dev'] !== $entry['dev'] || $opened['ino'] !== $entry['ino']) { + /* An inode of 0 is what Windows reports before PHP 7.4, for both stats, so the + comparison below has nothing to compare. Treating that as a mismatch answered + 'Destination is a symbolic link' to every reservation, which is every upload the + default configuration makes — the platform could not store a file at all. Skipped + rather than failed: nothing has been established either way, and the symlink + protections here were never load-bearing on Windows, where a symlink needs a + privilege an uploading process should not hold. A real file has a real inode, so + this gives up nothing on POSIX. */ + $identified = $opened['ino'] !== 0 && $entry['ino'] !== 0; + + if ($identified && ($opened['dev'] !== $entry['dev'] || $opened['ino'] !== $entry['ino'])) { /* The write is already refused at this point and nothing of the victim's was overwritten, but `x` has created a file at the far end of the link, outside the upload directory. Take that back too. */ diff --git a/tests/Upload/Storage/FileSystemTest.php b/tests/Upload/Storage/FileSystemTest.php index e2c5b6f..2997525 100644 --- a/tests/Upload/Storage/FileSystemTest.php +++ b/tests/Upload/Storage/FileSystemTest.php @@ -1392,6 +1392,40 @@ public function testAReservationThatCannotBeConfirmedIsNotReportedAsASymlink(): $this->assertFileDoesNotExist($workingDirectory . '/foo.txt'); } + /** + * Windows before PHP 7.4 reports `ino` as 0 for both stats, so the inode comparison has + * nothing to compare. Reading that as a mismatch answered `'Destination is a symbolic + * link'` to every reservation — which is every upload the default configuration makes, so + * the platform could not store a single file. `dev` differs here as well, to show it is + * the missing inode that decides and not a lucky match on the drive. + * + * Stubbed rather than run on Windows: this has to hold on the platform the suite is + * actually asserted on, and `lstatEntry()` is the seam for exactly this. + */ + public function testAReservationIsNotRefusedWhereTheInodeIsUnavailable(): void + { + $workingDirectory = $this->makeWorkingDirectory(); + + $storage = $this->getMockBuilder(FileSystem::class) + ->setConstructorArgs([$workingDirectory, false]) + ->onlyMethods(['moveUploadedFile', 'lstatEntry']) + ->getMock(); + + $storage->method('moveUploadedFile')->willReturnCallback( + static function (string $source, string $destination): bool { + return copy($source, $destination); + } + ); + + /* What that platform answers: no inode, and a `dev` of its own */ + $storage->method('lstatEntry')->willReturn(['dev' => 2, 'ino' => 0]); + + $stored = $storage->upload(new FileInfo($this->assetsDirectory . '/foo.txt', 'foo.txt')); + + $this->assertSame($this->destinationOf($workingDirectory, 'foo.txt'), $stored); + $this->assertFileExists($stored); + } + /** * refuseBlockedExtensions() matches one dot-separated component at a time, so an entry that * is itself compound has to be split or it silently blocks nothing. From 7f2a23f0cf3bd62ec0eac506286f6451c619fc0d Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 24 Aug 2026 15:29:50 +1000 Subject: [PATCH 4/5] Ask is_link() whether the destination is a link MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `releaseReservation()` inferred it from whether `readlink()` failed. That is not the same question on every platform: PHP's Windows `readlink()` answers a *regular file* with its own canonical path instead of failing, so the 0-byte placeholder took the symlink branch, found nothing matching the inode it opened, and was never removed — holding the caller's name against every later upload of it. `is_link()` asks directly. The stat cache is cleared first because `reserveDestination()` has already lstat'd that path. The last of the Windows failures. Both branches are covered on POSIX already — `testAReservationThatCannotBeConfirmedIsNotReportedAsASymlink` for the plain file, `testReservationThroughASymlinkLeavesNothingAtItsTarget` for the link — and the first of those is what the Windows job caught this with, on the platform where `readlink()` behaves that way. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 1 + src/Upload/Storage/FileSystem.php | 20 +++++++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a38c5f..910316c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,7 @@ Both still work and neither raises a runtime notice. ## Bug Fixes +* **A reservation that has to be released is released on Windows too.** `releaseReservation()` decided whether the destination was a symlink by whether `readlink()` failed. PHP's Windows `readlink()` answers a regular file with its own canonical path rather than failing, so the 0-byte placeholder took the symlink branch, matched nothing there, and was left behind — holding the caller's name against every later upload of it. `is_link()` asks the question directly, on every platform * **`Storage\FileSystem` can store a file on Windows under PHP 7.3 again.** `reserveDestination()` confirms its exclusive create by comparing `fstat()` and `lstat()`, which catches an `x` that followed a symlink. That platform reports `ino` as 0 for both, so the comparison had nothing to compare — and reading no information as a mismatch answered `'Destination is a symbolic link'` to every reservation, which is every upload the default `$overwrite = false` makes. The comparison is skipped where the inode is unavailable; a real file has a real inode, so nothing changes on POSIX, and the symlink protections here were already documented as not load-bearing on Windows. **7.3 only** — 7.4 onwards reports an inode there * **A backslash in a client filename is a character, not a path separator.** `FileInfo::setNameWithExtension()` and `Filename::sanitizeNameWithExtension()` split through `pathinfo()`, which treats `\` as a separator on Windows and as an ordinary character on POSIX — so `a\b.txt` was stored as `a-b.txt` on one and `b.txt` on the other, and `..\..\windows\win.ini` as `windows-win.ini` or `win.ini`. `Filename::rewriteCharacters()` rewrites `\` to `-`, so the rule both layers read from `Filename` is that it stays in the name; a rule cannot depend on which platform applies it. `Filename::splitNameAndExtension()` owns the split now and treats `/` alone as a separator, on every platform. **On Windows this keeps name content that was previously discarded** * **`$file[] = $fileInfo` appends instead of silently discarding a file.** PHP passes `offsetSet()` a null offset for the append syntax, and assigning it straight through wrote the string key `''` rather than the next integer: the second append overwrote the first, and a key the `ArrayAccess` contract does not admit reached `getUploadedLocators()`, which `store()` keys by collection offset. PHP 8.5 also deprecates the null offset, so every append raised a notice. On `FileList`, the offset the append lands at is read back before the source key is dropped, so `getSourceKeys()[$i]` keeps naming `$list[$i]` diff --git a/src/Upload/Storage/FileSystem.php b/src/Upload/Storage/FileSystem.php index 4feced5..fc7fbac 100644 --- a/src/Upload/Storage/FileSystem.php +++ b/src/Upload/Storage/FileSystem.php @@ -523,15 +523,29 @@ protected function lstatEntry(string $path) */ private function releaseReservation(string $destinationFile, $opened): void { - $target = @readlink($destinationFile); - - if ($target === false) { + /* `is_link()` rather than a failed `readlink()`, which is not the same question on + every platform: PHP's Windows `readlink()` answers a *regular file* with its own + canonical path instead of failing, so the placeholder took the link branch below, + matched nothing there and was never removed — leaving the caller's name held + against every later upload. The stat cache is cleared because `reserveDestination()` + has already lstat'd this path. */ + clearstatcache(true, $destinationFile); + + if (!is_link($destinationFile)) { /* Not a link, so the name is the file. `unlink()` does not follow one in any case. */ @unlink($destinationFile); return; } + $target = @readlink($destinationFile); + + /* A link this cannot read the target of: neither it nor whatever it points at is this + upload's to remove. */ + if ($target === false) { + return; + } + /* A symlink was already here and `x` created its target. Remove that file and only that file: the inode has to be the one this call opened, so a link re-pointed between the create and this check cannot make us delete a bystander. Failing that test leaves the From 30424063283372beae72c2461221a80947937621 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 24 Aug 2026 15:32:09 +1000 Subject: [PATCH 5/5] Make the Windows job a gate Both versions pass: 654 tests and 1284 assertions on each, identical to `--exclude-group posix` locally, so the grouping selects the same work on both platforms and what is left is real difference rather than drift. It earned the promotion on its first outing, with two library bugs on the default `$overwrite = false` path that no Linux job could reach. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/phpunit.yml | 9 +++++---- CLAUDE.md | 2 +- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index 4ca8f7a..22f89fb 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -93,13 +93,14 @@ jobs: # nothing. Excluded by group rather than skipped, so `cross-file-system` keeps the single # `markTestSkipped()` its `--fail-on-skipped` guard depends on. # - # `continue-on-error` until this has been green once: nothing in the project has ever run on - # Windows, so the first runs are discovery rather than a gate. Remove it, and this comment, - # once the job passes — a red job nobody may merge past is the point of adding it. + # It found two library bugs on its first outing, both on the default `$overwrite = false` + # path and neither reachable from Linux: the reservation's inode comparison read Windows' + # absent inode as a mismatch and refused every upload, and `releaseReservation()` inferred + # "not a symlink" from a failed `readlink()`, which answers a regular file with its own path + # there. It is a gate now, so a third does not go unnoticed. windows: name: ${{ matrix.php }} on windows-latest runs-on: windows-latest - continue-on-error: true strategy: fail-fast: false matrix: diff --git a/CLAUDE.md b/CLAUDE.md index d90c4c4..50f7cc4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -49,7 +49,7 @@ The `phpunit` workflow carries a second job, `cross-file-system`, which mounts a `Filename::splitNameAndExtension()` is what `FileInfo::setNameWithExtension()` and `Filename::sanitizeNameWithExtension()` split with, not `pathinfo()`: that treats `\` as a path separator on Windows and as an ordinary character on POSIX, so one client name split two ways. `rewriteCharacters()` rewrites `\` to `-`, so the shared rule is that a backslash stays in the name. `/` is a separator on every platform. Do not put `pathinfo()` back. -The `windows` job is `continue-on-error: true` until it has been green once — nothing in this project had ever run on Windows, so the first runs are discovery rather than a gate. **Remove the flag once it passes**, and the comment above it with it: a job nobody may merge past is why it is there. `phpunit.xml` sets `convertDeprecationsToExceptions`, `failOnWarning` and `failOnRisky`, so a deprecation raised from `src/` fails the suite rather than passing through a green run — which is how `$file[] = $fileInfo` wrote a string key and warned about it under PHP 8.5 while the suite reported OK. +The `windows` job is a gate. It found two bugs on its first outing, both on the default `$overwrite = false` path and neither reachable from Linux: `reserveDestination()` read Windows' absent inode as a mismatch and refused every upload as a symlink, and `releaseReservation()` inferred "not a symlink" from a failed `readlink()`, which on Windows answers a regular file with its own canonical path. Ask `is_link()`, and gate an inode comparison on the inode being available. `phpunit.xml` sets `convertDeprecationsToExceptions`, `failOnWarning` and `failOnRisky`, so a deprecation raised from `src/` fails the suite rather than passing through a green run — which is how `$file[] = $fileInfo` wrote a string key and warned about it under PHP 8.5 while the suite reported OK. `composer phpstan` bootstraps PHPStan from `tools/phpstan/` rather than the root `require-dev`. PHPStan 2.x needs PHP 7.4 to run, and the root manifest has to stay resolvable on 7.3 or the 7.3 test and PHPCS jobs cannot install at all. `phpstan.neon` sets `phpVersion` to the 7.3-8.5 range, so the analysis still covers the whole supported range from whatever version runs it.