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/.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/CHANGELOG.md b/CHANGELOG.md index a5c8f16..910316c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,9 @@ 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]` * **`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..50f7cc4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -47,7 +47,9 @@ 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 `. -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. +`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 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. diff --git a/UPGRADE.md b/UPGRADE.md index c8216e9..f470317 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -324,6 +324,15 @@ 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 + `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/src/Upload/Storage/FileSystem.php b/src/Upload/Storage/FileSystem.php index 70c8202..fc7fbac 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. */ @@ -512,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 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..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); @@ -503,7 +504,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..2997525 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) ); @@ -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. ')) ); } @@ -923,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)); @@ -1012,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); @@ -1066,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)); @@ -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()); @@ -1353,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. @@ -1460,7 +1533,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')) ); }