From 39bd81567149a37f62a290d420d1a730d147fb41 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 24 Aug 2026 14:57:18 +1000 Subject: [PATCH 1/9] Fix the thirteen findings from the repository audit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An append lost a file, two validators reported a developer's misconfiguration to whoever submitted the upload, and storage applied every filename rule but the length. The check that would have caught the first is now a gate. `File::offsetSet()` appends on a null offset. PHP passes one for `$file[] = $fileInfo`, and assigning it straight through wrote the string key `''`: the second append overwrote the first, `getUploadedLocators()` came back with a key the `ArrayAccess` contract does not admit, and PHP 8.5 deprecated the offset besides. `FileList::offsetSet()` reads back the offset the append landed at before dropping the source key, so `getSourceKeys()[$i]` still names `$list[$i]`. `Validation\Size` reads both bounds through one private `toBytes()` in the constructor, refusing a non-int, a negative, and a minimum above the maximum. A float — what a bound out of JSON or a division actually is — reached the `int`-typed `scale()` and raised a `TypeError` from inside `validate()`, where `runValidations()` absorbed it as `Validation could not be completed`. `InvalidArgumentException` is a `LogicException`, which that run re-throws. `Validation\Mimetype` folds its allow-list and the sniffed type, as `Extension` and `FileType` already did. A media type is case-insensitive and `getMimetype()` always answers lowercase, so `['IMAGE/PNG']` matched nothing and rejected every PNG. `Storage\FileSystem::refuseUnsafeName()` refuses a name over `Filename::MAX_LENGTH`. It read the two character sets and the device names from `Filename` and not the length, so an over-budget name from a `FileInfoInterface` of your own failed at the exclusive create on ENAMETOOLONG — reported as `DESTINATION_NOT_CREATED`, the code that means the directory went away. The upload directory's trailing separator is now trimmed on Windows too, and `Filename::maxNameLength()` floors the name's budget at zero rather than handing `mb_strcut()` a negative length. phpunit.xml sets `convertDeprecationsToExceptions`, `failOnWarning` and `failOnRisky`. Without them a deprecation raised from `src/` passed through a green run, which is how the append warned under 8.5 for a release. A `windows` job runs the suite on 7.3 and 8.5. Reserved device names, `:` naming an NTFS stream, trailing dots resolving away and the note about `ino` reporting 0 are all Windows behaviour this library reasons about and had never executed. `@group posix` marks the fourteen tests needing `symlink()`, `chmod()` or `umask()`, excluded by group rather than skipped so `cross-file-system` keeps the single `markTestSkipped()` its `--fail-on-skipped` depends on. It is `continue-on-error` until it has been green once; nothing here has run on Windows before, so the first runs are discovery rather than a gate. A `Composer Manifest` workflow runs `composer validate --strict` and `composer audit`. `package.yml` guards what the archive contains; this guards the manifest describing it. The README credited `Storage\FileSystem` with five protections, one of which is `File`'s; the class docblock said three, omitting the `move_uploaded_file()` refusal that is also on by default and also has an opt-out. Both say four now, and `turning-the-defaults-off.md` keeps the five that span two classes. `docs/api-reference.md` documents the staging file and the reservation placeholder, since sweeping a stale one is the operator's job and it was written down only in the upgrade guide. CONTRIBUTING.md asked for "an actual final HTML code" and named none of the checks CI runs. Co-Authored-By: Claude Opus 5 (1M context) --- .github/CONTRIBUTING.md | 87 +++++++++++++++++++----- .github/workflows/composer-manifest.yml | 49 +++++++++++++ .github/workflows/phpunit.yml | 39 +++++++++++ CHANGELOG.md | 6 ++ CLAUDE.md | 6 +- README.md | 3 +- UPGRADE.md | 15 ++++ docs/api-reference.md | 19 ++++++ docs/translation/README.md | 1 + phpunit.xml | 7 ++ src/Upload/File.php | 13 +++- src/Upload/FileList.php | 12 +++- src/Upload/Filename.php | 7 +- src/Upload/Storage/FileSystem.php | 21 ++++-- src/Upload/Validation/Mimetype.php | 19 +++++- src/Upload/Validation/Size.php | 62 +++++++++++++++-- tests/Upload/FileListTest.php | 21 ++++++ tests/Upload/FileTest.php | 39 +++++++++++ tests/Upload/FilenameTest.php | 13 ++++ tests/Upload/Storage/FileSystemTest.php | 48 +++++++++++++ tests/Upload/Validation/MimetypeTest.php | 41 +++++++++++ tests/Upload/Validation/SizeTest.php | 67 ++++++++++++++++++ 22 files changed, 556 insertions(+), 39 deletions(-) create mode 100644 .github/workflows/composer-manifest.yml diff --git a/.github/CONTRIBUTING.md b/.github/CONTRIBUTING.md index 5ba94a6..ade3e4e 100644 --- a/.github/CONTRIBUTING.md +++ b/.github/CONTRIBUTING.md @@ -4,37 +4,88 @@ Contributing Issue tracker ------------- -The Issue tracker serves mainly as a place to report bugs and request new features. -Please do not abuse it as a general questions or troubleshooting location. +The issue tracker is for bug reports and feature requests. Please do not use it for general +questions or troubleshooting, and please report one bug or one feature per issue. Bug reports ------------- -* Bug reports **MUST** contain a small example in php/html that reproduces the bug. -* The code example **MUST** be reproducible by copy&paste assuming composer dependencies are installed. That means: - * No calling unrelated funcions, - * an actual final HTML code has to be present, pasting a template file is not enough, -* Failing to provide necessary information or not using the issue template will cause the issue to be closed until required information is provided. -* Please report one feature or one bug per issue. +A report has to be reproducible by copy and paste, assuming `composer require gravitypdf/upload` +and nothing else. That means it includes: + +* **The input.** The `$_FILES` entry as a literal array, or the `FileInfoInterface` objects + handed to `FileList`. A filename that triggers the bug matters down to the byte — say so if + it carries a control character, a bidi mark or invalid UTF-8, since those do not survive a + paste into a browser. +* **The configuration.** The validations added, and any of the opt-outs in + [Turning the defaults off](../docs/turning-the-defaults-off.md) that are in play. +* **What happened and what you expected**, as the return value, the `getErrors()` entries or + the exception — not a description of them. +* **The PHP version**, and whether `ext-mbstring` is loaded. Several behaviours differ across + the supported 7.3 to 8.5 range, and filename truncation differs without `mbstring`. + +Security issues do not belong in the tracker. Report those privately through +[GitHub's security advisories](https://github.com/GravityPDF/Upload/security/advisories/new). Feature requests ------------- -Feature requests have to be labeled as such and have to include reasoning for the change in question. +Label the issue as a feature request and say what problem it solves. This library is a +security boundary before it is a convenience, so a request that relaxes a default needs to say +what it stops applying and why that is acceptable. Pull requests ------------- -Pull requests should be always based on the default [main](https://github.com/GravityPDF/upload/tree/main) -branch except for backports to older versions. **Note: When you first open a PR GitHub automatically references the upstream package and you'll need to change it.** +Pull requests target the default [main](https://github.com/GravityPDF/upload/tree/main) branch, +except for backports to older versions. **When you first open a PR, GitHub sets the base to the +upstream `codeguy/upload` repository; you have to change it.** + +Before opening one, run the checks CI will run: + +```bash +composer phpunit # the test suite +composer lint # PHPCS, PSR-12 +composer phpstan # PHPStan level 9, over src and tests +composer check-syntax # parallel-lint, all PHP files +``` + +Change an example in `README.md` or `docs/` and two more apply, since those snippets are read +out of the Markdown and executed: + +```bash +composer psr7-readme # docs/psr7.md, against nyholm/psr7 and guzzlehttp/psr7 +composer base64-docs # docs/base64-uploads.md +composer translator-readme # docs/translation/, against the real translation libraries +``` + +Change or add an error message and regenerate the catalogue, or the `i18n` workflow fails on +the diff: + +```bash +composer i18n:pot +``` Guidelines: -* Use an aptly named feature branch for the Pull request. -* Only files and lines affecting the scope of the Pull request must be affected. -* Make small, *atomic* commits that keep the smallest possible related code changes together. -* Code must be accompanied by a unit test testing expected behaviour whenever possible. -* To be incorporated, the PR should contain a change in the CHANGELOG.md file describing itself +* Use an aptly named feature branch. +* Only the lines within the scope of the pull request should change. +* Make small, atomic commits that keep related changes together. +* Code must be accompanied by a test whenever one can be written. New behaviour on the storage + or filename path needs a test of the refusal as well as of the acceptance — a rule that + refuses everything passes a one-sided test. +* User-facing changes need an entry in `CHANGELOG.md` in the same pull request. It is the + documented record of what breaks between major versions. + +Supported PHP versions +------------- + +7.3 through 8.5, tested on all eight. Anything that will not parse on 7.3 cannot go in: no +typed properties, no union types, no `mixed`, no arrow functions, no constructor promotion. +Property and parameter types beyond what 7.3 allows live in docblocks. + +`ext-fileinfo` is the only required extension. `ext-mbstring` is a suggestion, so every `mb_*` +call has to be guarded by the function that makes it — CI runs the suite with the extension, +with `symfony/polyfill-mbstring`, and with neither. -When updating a PR, do not create a new one, just `git push --force` to your former feature branch, the PR will -update itself. \ No newline at end of file +To update a pull request, push to the same branch rather than opening a new one. diff --git a/.github/workflows/composer-manifest.yml b/.github/workflows/composer-manifest.yml new file mode 100644 index 0000000..febb900 --- /dev/null +++ b/.github/workflows/composer-manifest.yml @@ -0,0 +1,49 @@ +name: Composer Manifest + +on: + push: + branches: + - main + pull_request: + +# The workflows only read the repository; nothing needs a write-capable GITHUB_TOKEN. +permissions: + contents: read + +# Cancels all previous workflow runs for pull requests that have not completed. +concurrency: + # The concurrency group contains the workflow name and the branch name for pull requests + # or the commit hash for any other events. + group: ${{ github.workflow }}-${{ github.event_name == 'pull_request' && github.head_ref || github.sha }} + cancel-in-progress: true + +# `package.yml` guards what the dist archive contains. This guards the manifest that describes +# it: `composer.json` is what Packagist reads and what resolves the supported PHP range, and a +# schema error there is not something any other job would notice. +jobs: + run: + name: Valid and free of advisories + runs-on: ubuntu-latest + steps: + - name: Checkout + uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.0.0 + + - name: Install PHP + uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # v2.35.5 + with: + php-version: '8.5' + + # --strict, because this manifest is published: a missing `name` or an unbound version + # constraint is an error here where it is only a warning for an application. The three + # manifests under tools/ are deliberately not published and pin exact versions, so they + # are not checked — that pinning is what makes the doc checks reproducible. + - name: Validate composer.json + run: composer validate --strict --no-check-lock + + - name: Install Composer dependencies + run: composer install --no-scripts --no-interaction + + # The library itself requires nothing but ext-fileinfo, so this is about require-dev: + # PHPCS, PHPUnit and parallel-lint run over this repository on every push. + - name: Check for security advisories + run: composer audit diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index c690a9f..241fa8f 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -83,6 +83,45 @@ jobs: files: tmp/coverage/report-xml/php-coverage1.xml token: ${{ secrets.CODECOV_TOKEN }} + # Reserved device names, `:` naming an NTFS alternate data stream, trailing dots and spaces + # resolving away, `DIRECTORY_SEPARATOR`, and the note that `reserveDestination()`'s inode + # comparison degrades where `stat()` reports `ino` as 0 — all of it is Windows behaviour this + # library reasons about carefully and had never executed on Windows. + # + # `@group posix` marks what genuinely cannot run there: `symlink()` needs a privilege an + # uploading process should not hold, `chmod()` is close to a no-op, and `umask()` means + # 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. + windows: + name: ${{ matrix.php }} on windows-latest + runs-on: windows-latest + continue-on-error: true + strategy: + fail-fast: false + matrix: + # The ends of the supported range. The middle versions differ from these in nothing + # this job exercises, and a Windows runner costs several times a Linux one. + php: [ '7.3', '8.5' ] + + steps: + - name: Checkout + uses: actions/checkout@d23441a48e516b6c34aea4fa41551a30e30af803 # v6.0.0 + + - name: Install PHP + uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # v2.35.5 + with: + php-version: ${{ matrix.php }} + + - name: Install Composer dependencies + run: composer install --no-scripts + + - name: Run the test suite + run: vendor/bin/phpunit --exclude-group posix + # `ext-mbstring` is `suggest`, not `require`, and every job above runs with it loaded, so an # unguarded `mb_*` call fatals on a supported install without any of them noticing. # diff --git a/CHANGELOG.md b/CHANGELOG.md index 864bffe..a5c8f16 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,12 @@ Both still work and neither raises a runtime notice. ## Bug Fixes +* **`$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 +* **`Storage\FileSystem` refuses a name longer than `Filename::MAX_LENGTH`.** It applied every other rule `Filename` declares and not the length, so an over-long name from a `FileInfoInterface` of your own travelled to the exclusive create and failed on the file system's own `ENAMETOOLONG` — reported as `'Destination file could not be created'`, which is supposed to mean the directory went away and sends a caller into a retry that cannot succeed. The shipped `FileInfo` truncates, so nothing on that path changes +* `Filename::maxNameLength()` floors the name's budget at zero. `acceptExtension()` caps an extension at 32 bytes, so nothing in this library reaches it, but `finalize()` takes the extension from its caller and a longer one made the budget negative — which `mb_strcut()` reads as "cut this many bytes off the end" +* The upload directory's trailing separator is trimmed on Windows as well as POSIX. `rtrim($directory, '/')` left `C:\uploads\` as `C:\uploads\\` internally, which `getDirectory()` then trimmed and the rest of the class did not * **Case folding no longer follows the host locale.** `strtolower()` follows `LC_CTYPE` before PHP 8.2, so on four of the eight supported versions a `setlocale()` call elsewhere in the application changed what counted as the same extension, media type or device name. Under a Turkish locale `strtolower('TIFF')` is `tıff`, which 4.0's stricter `setExtension()` discards, storing `photo.TIFF` with no extension at all * `ext-mbstring` is detected with `function_exists()` rather than `extension_loaded()`, so `symfony/polyfill-mbstring` satisfies it as the README has always said it does. The polyfill is userland and registers no extension, so every polyfilled install was silently on the byte-wise fallback. The check is per function rather than one flag covering five: the polyfill does not ship `mb_strcut()`, so a single flag denied polyfilled installs the UTF-8 repair as well as character-boundary truncation. Its `mb_convert_encoding()` is `iconv()`-backed and warns on the input the repair exists for, so that call is silenced. CI now covers the extension, the polyfill and neither * `mb_detect_encoding()` is called with an explicit detect order and falls back to UTF-8, so an application's own `mb_detect_order()` can't change what this library makes of the same bytes. The fallback also covers the orders where detection returns `false` and `mb_strcut()` would raise a `ValueError` diff --git a/CLAUDE.md b/CLAUDE.md index d30a5b0..10e4d97 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,7 +45,9 @@ The `i18n` workflow regenerates the catalogue and fails on a diff, which is what `docs/base64-uploads.md` is the same arrangement for a file posted as base64 in a JSON body, run by `tools/base64-docs/verify.php`. That directory has no manifest: a base64 payload is a string in the request body, so there is no third-party implementation to run the bridge against, and the check loads the library through the root `vendor/autoload.php` — the autoloader a consumer gets, `src/Upload/i18n.php` included. The page declares one snippet each for the bridge and for validating and storing what it returns. -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. Set `UPLOAD_TEST_OTHER_FS` to run it locally: on macOS, `hdiutil attach -nomount ram://8192` then `diskutil erasevolume HFS+ UPLOADTMP `. +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. `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. @@ -195,7 +197,7 @@ With `overwrite = false`, `reserveDestination()` claims the name first with an e `tests/Upload/Storage/FileSystemTest.php` covers both routes against an **unmocked** `FileSystem`. Every other test there stubs `moveUploadedFile()`, which is what let the `FileList` path report green while it could not store a byte — a test that mocks the move cannot see that a caller's file is not a POST upload either. -**Three protections are on by default, each with its own opt-out**, applied in the constructor: `$overwrite = false`, `blockExtensions()` (turn off with `allowAnyExtension()`, the only route to the empty list — `blockExtensions()` takes a required, non-empty argument and throws otherwise, so a missing config key cannot quietly disable it) and `setMode(self::DEFAULT_MODE)` (turn off with `setMode(null)`). The deny-list default is `getDefaultBlockedExtensions()`, a **method** rather than a constant because PHP 7.3 constant expressions cannot call `array_merge()` on the two constants it joins. `EXECUTABLE_EXTENSIONS` is what a server runs; `MARKUP_EXTENSIONS` is what a browser renders, which is stored XSS rather than RCE. A caller who needs SVG `array_diff()`s `svg` and `svgz` out of `getDefaultBlockedExtensions()` — passing `EXECUTABLE_EXTENSIONS` alone unblocks all fifteen markup extensions to let one through, so do not document that as the way to accept a markup format. Entries passed to `blockExtensions()` are lowercased, trimmed, stripped of a leading dot and **split on any remaining dots**, because the list is matched one component at a time — `'tar.gz'` has to block `tar` and `gz` separately or it blocks nothing. The README table is pinned to `getDefaultBlockedExtensions()` by `FileSystemTest::testReadmeDocumentsTheDefaultDenyList()`. +**Four protections are on by default, each with its own opt-out.** Three are applied in the constructor: `$overwrite = false`, `blockExtensions()` (turn off with `allowAnyExtension()`, the only route to the empty list — `blockExtensions()` takes a required, non-empty argument and throws otherwise, so a missing config key cannot quietly disable it) and `setMode(self::DEFAULT_MODE)` (turn off with `setMode(null)`). The fourth is `$acceptsUnuploadedFiles = false`, a property default rather than a constructor call because it turns nothing on; `acceptFilesNotUploadedByPhp()` is its opt-out. `docs/turning-the-defaults-off.md` counts five, because `File::allowUnvalidatedUploads()` is on that page too — the README's `Storage\FileSystem` row must say four, not five. The deny-list default is `getDefaultBlockedExtensions()`, a **method** rather than a constant because PHP 7.3 constant expressions cannot call `array_merge()` on the two constants it joins. `EXECUTABLE_EXTENSIONS` is what a server runs; `MARKUP_EXTENSIONS` is what a browser renders, which is stored XSS rather than RCE. A caller who needs SVG `array_diff()`s `svg` and `svgz` out of `getDefaultBlockedExtensions()` — passing `EXECUTABLE_EXTENSIONS` alone unblocks all fifteen markup extensions to let one through, so do not document that as the way to accept a markup format. Entries passed to `blockExtensions()` are lowercased, trimmed, stripped of a leading dot and **split on any remaining dots**, because the list is matched one component at a time — `'tar.gz'` has to block `tar` and `gz` separately or it blocks nothing. The README table is pinned to `getDefaultBlockedExtensions()` by `FileSystemTest::testReadmeDocumentsTheDefaultDenyList()`. ## Conventions diff --git a/README.md b/README.md index 2f92e20..d9e670b 100644 --- a/README.md +++ b/README.md @@ -431,6 +431,7 @@ placeholders do not match throws `ArgumentCountError` — on the failure path, o `Translation::render()` handles this and falls back to English. `Exception::getErrorCode()` returns the same codes, including for storage failures. + ## Security notes **Prefer `FileType` over `Mimetype` and `Extension` separately.** Those two check independent @@ -526,7 +527,7 @@ it throws. The entry points: | [`File`](docs/api-reference.md#file) | Reads `$_FILES[$key]` into a collection: validations, callbacks, `upload()` and `uploadValid()`. | | [`FileList`](docs/api-reference.md#filelist) | The same collection, built from files you supply. | | [`FileInfo`](docs/api-reference.md#fileinfo) | The per-file value object — name, extension, sniffed media type, size, hash, dimensions. | -| [`Storage\FileSystem`](docs/api-reference.md#storagefilesystem) | The shipped backend: where a file lands, and the five protections it applies on the way. | +| [`Storage\FileSystem`](docs/api-reference.md#storagefilesystem) | The shipped backend: where a file lands, and the four protections it applies on the way. | | [Validations](docs/api-reference.md#validations) | `FileType` and `Size`, plus the two classes deprecated in 4.0. | | [`Filename`](docs/api-reference.md#filename) | The filename rules both layers read, and the predicates for reproducing them. | | [`Exception`](docs/api-reference.md#exception), [`ErrorCode`](docs/api-reference.md#errorcode) | What a failure carries, and the stable code to branch on. | diff --git a/UPGRADE.md b/UPGRADE.md index f137f90..c8216e9 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -324,6 +324,21 @@ 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`. +* `$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 + that is not an `int` or a size string, a negative one, or a minimum above the maximum. A + float bound previously became a `TypeError` inside `validate()`, which the collection + absorbed and reported to the submitter as `Validation could not be completed`. **Check any + bound that comes from configuration** — `'5M'` and `5242880` are both fine, `5.0` is not. +* **`Validation\Mimetype` lowercases its allow-list**, as `Extension` and `FileType` already + did. A list written in any other case previously matched nothing and rejected every upload, + so **a `new Mimetype(['IMAGE/PNG'])` that appeared to be rejecting files correctly will now + start accepting them.** Check any list that is not already lowercase. +* `Storage\FileSystem` refuses a destination name longer than `Filename::MAX_LENGTH` + (255 bytes) with `'Invalid destination file name'`, where it used to fail at the write with + `'Destination file could not be created'`. Only reachable from a `FileInfoInterface` of your + own; the shipped `FileInfo` truncates to the budget. * Sanitized filenames are valid UTF-8 where `ext-mbstring` is available, so they survive `json_encode()` and `utf8mb4` columns. Without it the guarantee doesn't hold, as in 3.x. * Windows reserved names are matched against the whole extension, so `doc.conf` keeps diff --git a/docs/api-reference.md b/docs/api-reference.md index 5231772..a6270b8 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -94,6 +94,25 @@ and what each one stops applying is in [Turning the defaults off](turning-the-de | `getDirectory(): string` | The destination directory, without trailing slash. | | `FileSystem::getDefaultBlockedExtensions(): string[]` | Static: `EXECUTABLE_EXTENSIONS` merged with `MARKUP_EXTENSIONS`, the table under "Extensions blocked by default". | +### Two things it leaves in your upload directory + +Both are artefacts of the staged write, and clearing a stale one is the operator's job. + +`upload-<32 hex>.part` is the staging file, in the destination directory because `rename()` +is only atomic within one file system. It exists for the length of one transfer and is +removed on any failure. A process killed mid-transfer leaves one behind: the name is +unguessable, so nothing will ever collide with it, and nothing will remove it either. + +The other is the 0-byte placeholder, and only with `$overwrite = false`. The destination +name is claimed before the bytes move, so two concurrent requests cannot both win it, and +`rename()` replaces the placeholder with the finished upload. A process killed between the +two leaves a 0-byte file under the caller's name, and every later upload of that name +reports `A file named "…" already exists` until it is cleared. There is no such window with +`$overwrite = true`, which does not reserve the name at all. + +A sweep for `upload-*.part` and 0-byte files older than your longest plausible request is +enough. Both are ordinary files; nothing in this library reads them back. + ## Validations Each implements `ValidationInterface` and throws `Exception` on failure. diff --git a/docs/translation/README.md b/docs/translation/README.md index b6f922b..63ae74b 100644 --- a/docs/translation/README.md +++ b/docs/translation/README.md @@ -92,6 +92,7 @@ otherwise blank the message. Each page carries a working adapter, the commands to build the catalogue, and what to watch for. They are checked against the real libraries by `composer translator-readme`. + ## Stating a size in a locale `Validation\Size` writes `4.7 MB` with a `.`, because picking another separator needs a locale diff --git a/phpunit.xml b/phpunit.xml index 2b1f059..c58a7d7 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -1,4 +1,11 @@ + diff --git a/src/Upload/File.php b/src/Upload/File.php index 54d7a0a..ff18e72 100644 --- a/src/Upload/File.php +++ b/src/Upload/File.php @@ -968,7 +968,7 @@ public function offsetGet($offset) } /** - * @param int $offset + * @param int|null $offset Null for `$file[] = $fileInfo`, which appends * @param mixed $value A FileInfoInterface; the type is checked at runtime, not declared * @throws InvalidArgumentException If the value is not a FileInfoInterface */ @@ -981,6 +981,17 @@ public function offsetSet($offset, $value): void ); } + /* PHP hands this method a null offset for `$file[] = $fileInfo`. Assigned straight + through, that writes the string key `''` rather than appending: the second append + overwrote the first, and `getUploadedLocators()` — which `store()` keys by + collection offset — came back with a key the `ArrayAccess` + annotation does not admit. PHP 8.5 deprecates the null offset as well. */ + if ($offset === null) { + $this->objects[] = $value; + + return; + } + $this->objects[$offset] = $value; } diff --git a/src/Upload/FileList.php b/src/Upload/FileList.php index b09b962..1b49dea 100644 --- a/src/Upload/FileList.php +++ b/src/Upload/FileList.php @@ -166,7 +166,7 @@ private function describeKey($key): string * than reassigned: a file the caller put here directly has no key of their own for this * class to report. * - * @param int $offset + * @param int|null $offset Null for `$list[] = $fileInfo`, which appends * @param mixed $value A FileInfoInterface; the type is checked at runtime, not declared * @throws InvalidArgumentException If the value is not a `FileInfoInterface` */ @@ -174,7 +174,15 @@ public function offsetSet($offset, $value): void { parent::offsetSet($offset, $value); - unset($this->sourceKeys[$offset]); + /* An append has no offset of the caller's to drop, so read back the one it landed at + rather than assuming which that is: `unset($this->sourceKeys[null])` reaches the key + `''` and leaves the real entry in place. The parent has just written, so the array + cannot be empty — the null check is for the analyser. */ + $written = $offset ?? array_key_last($this->objects); + + if ($written !== null) { + unset($this->sourceKeys[$written]); + } } /** @param int $offset */ diff --git a/src/Upload/Filename.php b/src/Upload/Filename.php index 067243f..26d1460 100644 --- a/src/Upload/Filename.php +++ b/src/Upload/Filename.php @@ -270,10 +270,15 @@ public static function finalize(string $name, string $extension = '', ?array $re /** * How many bytes a name may use once its extension has taken its share + * + * Floored at zero. `acceptExtension()` caps an extension at `MAX_EXTENSION_LENGTH`, so + * nothing in this library spends the whole budget, but `finalize()` takes the extension + * from its caller: a longer one made this negative, and a negative length means "cut this + * many bytes off the end" to `mb_strcut()`. */ private static function maxNameLength(string $extension): int { - return self::MAX_LENGTH - ($extension !== '' ? strlen($extension) + 1 : 0); + return (int) max(0, self::MAX_LENGTH - ($extension !== '' ? strlen($extension) + 1 : 0)); } /** diff --git a/src/Upload/Storage/FileSystem.php b/src/Upload/Storage/FileSystem.php index 884d4be..70c8202 100644 --- a/src/Upload/Storage/FileSystem.php +++ b/src/Upload/Storage/FileSystem.php @@ -43,10 +43,12 @@ /** * FileSystem Storage * - * Three protections are on by default and each has to be turned off explicitly: existing files + * Four protections are on by default and each has to be turned off explicitly: existing files * are never overwritten (`$overwrite`), the extensions in `getDefaultBlockedExtensions()` are - * never written (`allowAnyExtension()`), and stored files get `DEFAULT_MODE` rather than - * whatever the process umask allows (`setMode(null)`). + * never written (`allowAnyExtension()`), stored files get `DEFAULT_MODE` rather than whatever + * the process umask allows (`setMode(null)`), and only a file PHP received as an upload is + * stored at all (`acceptFilesNotUploadedByPhp()`). The first three are applied in the + * constructor; the fourth is the default state of a property, since it turns nothing on. * * @author Josh Lockhart * @since 1.0.0 @@ -153,7 +155,11 @@ public function __construct(string $directory, bool $overwrite = false) throw new InvalidArgumentException('Directory is not writable'); } - $this->directory = rtrim($directory, '/') . DIRECTORY_SEPARATOR; + /* Both separators, or a Windows path already ending in one gains a second: `/` alone + left `C:\uploads\` as `C:\uploads\\`, where `getDirectory()` trims what this line + does not. On POSIX the charlist is `/` either way, so a directory legitimately + named with a trailing backslash is untouched. */ + $this->directory = rtrim($directory, DIRECTORY_SEPARATOR . '/') . DIRECTORY_SEPARATOR; $this->overwrite = $overwrite; $this->blockExtensions(self::getDefaultBlockedExtensions()); @@ -603,12 +609,19 @@ protected function resolveFilename(FileInfoInterface $fileInfo): string * A leading dot is refused here rather than left to the deny-list, which is a list of * extensions and so does not cover `.env` at all. * + * `Filename::MAX_LENGTH` is refused here for the reason the two character sets are: it is + * a rule `FileInfo` applies by truncating, so only a `FileInfoInterface` of your own + * arrives over it. Without this the name travelled to the exclusive create and failed on + * the file system's own `ENAMETOOLONG`, reported as `DESTINATION_NOT_CREATED` — the code + * that is supposed to mean the directory went away. + * * @throws Exception If the name is not one that may be written */ private function refuseUnsafeName(string $filename, FileInfoInterface $fileInfo): void { if ( $filename === '' + || strlen($filename) > Filename::MAX_LENGTH || strpos($filename, '.') === 0 || Filename::hasControlCharacters($filename) || Filename::hasBidiControls($filename) diff --git a/src/Upload/Validation/Mimetype.php b/src/Upload/Validation/Mimetype.php index f2321fb..f8efd37 100644 --- a/src/Upload/Validation/Mimetype.php +++ b/src/Upload/Validation/Mimetype.php @@ -33,6 +33,7 @@ namespace GravityPdf\Upload\Validation; +use GravityPdf\Upload\AsciiCase; use GravityPdf\Upload\ErrorCode; use GravityPdf\Upload\Exception; use GravityPdf\Upload\FileInfoInterface; @@ -54,7 +55,7 @@ */ class Mimetype implements ValidationInterface { - /** @var string[] Valid media types */ + /** @var string[] Valid media types, lowercased */ protected $mimetypes; /** @@ -65,7 +66,16 @@ public function __construct($mimetypes) if (is_string($mimetypes)) { $mimetypes = [$mimetypes]; } - $this->mimetypes = $mimetypes; + + /* Folded, as `Extension` and `FileType` fold theirs. A media type is case-insensitive + and `FileInfo::getMimetype()` always answers lowercase, so `'IMAGE/PNG'` matched + nothing and rejected every file the list was written to accept. */ + $this->mimetypes = array_map( + static function (string $mimetype): string { + return AsciiCase::toLower(trim($mimetype)); + }, + $mimetypes + ); } /** @@ -73,7 +83,10 @@ public function __construct($mimetypes) */ public function validate(FileInfoInterface $fileInfo): void { - if (!in_array($fileInfo->getMimetype(), $this->mimetypes, true)) { + /* The shipped FileInfo lowercases in getMimetype(); a custom FileInfoInterface need not */ + $mimetype = AsciiCase::toLower(trim($fileInfo->getMimetype())); + + if (!in_array($mimetype, $this->mimetypes, true)) { throw new Exception( /* translators: %1$s: comma-separated list of the accepted media types */ __('Invalid mimetype. Must be one of: %1$s'), diff --git a/src/Upload/Validation/Size.php b/src/Upload/Validation/Size.php index 37fb326..ca8708d 100644 --- a/src/Upload/Validation/Size.php +++ b/src/Upload/Validation/Size.php @@ -38,6 +38,7 @@ use GravityPdf\Upload\File; use GravityPdf\Upload\FileInfoInterface; use GravityPdf\Upload\ValidationInterface; +use InvalidArgumentException; use function GravityPdf\Upload\__; @@ -69,21 +70,68 @@ class Size implements ValidationInterface /** * @param int|string $maxSize Maximum acceptable file size in bytes (inclusive) * @param int|string $minSize Minimum acceptable file size in bytes (inclusive) - * @throws \InvalidArgumentException If a string bound cannot be parsed as a file size + * @throws InvalidArgumentException If a bound is not an int of bytes or a size string, + * if a string bound cannot be parsed, or if the two + * bounds accept nothing between them */ public function __construct($maxSize, $minSize = 0) { - if (is_string($maxSize)) { - $maxSize = File::humanReadableToBytes($maxSize); + $this->maxSize = $this->toBytes($maxSize, 'maxSize'); + $this->minSize = $this->toBytes($minSize, 'minSize'); + + if ($this->minSize > $this->maxSize) { + throw new InvalidArgumentException(sprintf( + 'Size was given a minimum of %d bytes and a maximum of %d bytes, which no ' + . 'file can satisfy. The maximum is the first argument.', + $this->minSize, + $this->maxSize + )); } + } - $this->maxSize = $maxSize; + /** + * Read one bound as a byte count + * + * The types are checked here rather than left to `scale()`, which is declared `int` and + * raises a `TypeError` when a bound is a float — from inside `validate()`, where + * `File::runValidations()` absorbs it as `Validation could not be completed` and reports + * the developer's misconfiguration to whoever submitted the file. A float is what a bound + * read out of JSON or arrived at by division actually is. + * + * `InvalidArgumentException` is a `LogicException`, which that run re-throws, so a bound + * rejected after construction still reaches the developer. + * + * @param mixed $size Whatever the caller passed. Declared wider than the constructor's + * `int|string`, because a docblock is not enforced at runtime and this + * method exists to answer for the values that ignore it + * @param string $parameter The parameter being read, named in the message + * @throws InvalidArgumentException If the bound cannot be a byte count + */ + private function toBytes($size, string $parameter): int + { + if (is_string($size)) { + $size = File::humanReadableToBytes($size); + } + + if (!is_int($size)) { + throw new InvalidArgumentException(sprintf( + 'Size::$%s must be an int of bytes or a string such as "5MB", %s given', + $parameter, + gettype($size) + )); + } - if (is_string($minSize)) { - $minSize = File::humanReadableToBytes($minSize); + /* A negative bound reads as generous and rejects every upload, which is the failure + `File::humanReadableToBytes()` already refuses a string for. */ + if ($size < 0) { + throw new InvalidArgumentException(sprintf( + 'Size::$%s cannot be negative, %d given', + $parameter, + $size + )); } - $this->minSize = $minSize; + return $size; } /** diff --git a/tests/Upload/FileListTest.php b/tests/Upload/FileListTest.php index 90db812..d569d7e 100644 --- a/tests/Upload/FileListTest.php +++ b/tests/Upload/FileListTest.php @@ -288,6 +288,25 @@ public function testTheSourceKeysDoNotOutliveTheFilesTheyName(): void $this->assertSame([], $list->getSourceKeys()); } + /** + * `$list[] = $file` reaches `offsetSet()` with a null offset, which is not an offset the + * caller gave a key for. The append has to land at a fresh integer, and the key drop has + * to find the offset it landed at rather than reaching `$sourceKeys['']`. + */ + public function testAppendingKeepsTheCollectionAndTheKeysInStep(): void + { + $list = new FileList(['avatar' => $this->vouchedFile('foo.txt')], $this->storage); + + $list[] = $this->vouchedFile('bar.txt'); + + $this->assertCount(2, $list); + $this->assertSame([0, 1], array_keys(iterator_to_array($list))); + $this->assertSame('bar.txt', $list[1] === null ? null : $list[1]->getNameWithExtension()); + + /* The appended file has no key of the caller's, and the one that does keeps it */ + $this->assertSame([0 => 'avatar'], $list->getSourceKeys()); + } + /******************************************************************************** * Failed-transfer tests *******************************************************************************/ @@ -386,6 +405,8 @@ public function testAPlainFileInfoIsRejectedAsNotAnUploadedFile(): void * write this, since a test's own file is not a POST upload — hides the fact that a * caller's file is not one either, and the whole path reported green while it could not * store a byte. + * + * @group posix */ public function testAVouchedFileIsValidatedSanitizedAndStored(): void { diff --git a/tests/Upload/FileTest.php b/tests/Upload/FileTest.php index 3b56720..2537fd5 100644 --- a/tests/Upload/FileTest.php +++ b/tests/Upload/FileTest.php @@ -185,6 +185,45 @@ public function testConstructionWithMultipleFiles(): void $this->assertSame('file-2.doc', $file[1]->getNameWithExtension()); /* @phpstan-ignore-line */ } + /** + * PHP passes `offsetSet()` a null offset for `$file[] = $fileInfo`. Assigned straight + * through it becomes the string key `''`, so the second append overwrote the first and a + * key the `ArrayAccess` annotation does not admit reached + * `getUploadedLocators()`, which `store()` keys by collection offset. + * + * Two appends, because one is indistinguishable from a working append: the file is there + * either way and only the key is wrong. + */ + public function testAppendingTwiceKeepsBothFiles(): void + { + $file = new File('single', $this->storage); + + $file[] = new FileInfo($this->assetsDirectory . '/foo.txt', 'foo.txt'); + $file[] = new FileInfo($this->assetsDirectory . '/bar.txt', 'bar.txt'); + + $this->assertCount(3, $file); + $this->assertSame([0, 1, 2], array_keys(iterator_to_array($file))); + $this->assertSame( + ['single.txt', 'foo.txt', 'bar.txt'], + $file->getNameWithExtension() + ); + } + + /** + * The offsets an append produces are what `getUploadedLocators()` is keyed by, and the + * README tells a caller to read `$file[$offset]` against them. + */ + public function testAnAppendedFileGetsAnIntegerLocatorOffset(): void + { + $file = new File('single', $this->storage); + $file[] = new VouchedFileInfo($this->assetsDirectory . '/foo.txt', 'foo.txt'); + $file->allowUnvalidatedUploads(); + + $file->upload(); + + $this->assertSame([0, 1], array_keys($file->getUploadedLocators())); + } + public function testConstructionWithSingleFile(): void { $file = new File('single', $this->storage); diff --git a/tests/Upload/FilenameTest.php b/tests/Upload/FilenameTest.php index 970583b..29620e4 100644 --- a/tests/Upload/FilenameTest.php +++ b/tests/Upload/FilenameTest.php @@ -111,6 +111,19 @@ public function testSanitizeTextRepairsASequenceTheBoundSplits(): void ); } + /** + * `acceptExtension()` caps an extension at `MAX_EXTENSION_LENGTH`, so nothing in this + * library spends the whole budget — but `finalize()` takes the extension from its caller. + * A longer one made the name's budget negative, and a negative length means "cut this many + * bytes off the end" to `mb_strcut()` rather than "keep nothing". + */ + public function testFinalizeSurvivesAnExtensionLongerThanTheWholeBudget(): void + { + $extension = str_repeat('x', Filename::MAX_LENGTH + 45); + + $this->assertSame(Filename::FALLBACK, Filename::finalize('report', $extension)); + } + /** * @dataProvider provideNamesToSplit * diff --git a/tests/Upload/Storage/FileSystemTest.php b/tests/Upload/Storage/FileSystemTest.php index 61fe14f..45634d2 100644 --- a/tests/Upload/Storage/FileSystemTest.php +++ b/tests/Upload/Storage/FileSystemTest.php @@ -3,6 +3,7 @@ namespace GravityPdf\Upload\Storage; use InvalidArgumentException; +use GravityPdf\Upload\Filename; use GravityPdf\Upload\Exception; use GravityPdf\Upload\FileInfo; use GravityPdf\Upload\FileInfoInterface; @@ -179,9 +180,35 @@ public function providerUnusableFileNames(): array 'device before further dots' => ['CON.tar.gz'], 'numbered device' => ['COM0.log'], 'superscript device' => ["LPT\u{00B9}.log"], + + /* `FileInfo` truncates to `Filename::MAX_LENGTH`; only an implementation of your + own arrives over it. Without this refusal the name reached the exclusive create + and failed on the file system's own ENAMETOOLONG, reported as + 'Destination file could not be created' — the code that means the directory + went away, which sends a caller down a retry that cannot succeed. */ + 'longer than MAX_LENGTH' => [str_repeat('a', 252) . '.txt'], ]; } + /** + * The budget is name and extension together, so the boundary is the whole filename rather + * than either half. Pinned in both directions: a rule that refused everything would pass + * the test above just as well. + */ + public function testAcceptsANameOfExactlyMaxLength(): void + { + $workingDirectory = $this->makeWorkingDirectory(); + $filename = str_repeat('a', Filename::MAX_LENGTH - 4) . '.txt'; + + $this->assertSame(Filename::MAX_LENGTH, strlen($filename)); + + $storage = $this->makeStorage($workingDirectory, true); + $storage->upload($this->makeHostileFileInfo($filename)); + + $this->assertFileExists($workingDirectory . '/' . $filename); + } + + /** @group posix */ public function testRefusesToWriteThroughASymlink(): void { $workingDirectory = $this->makeWorkingDirectory(); @@ -206,6 +233,8 @@ public function testRefusesToWriteThroughASymlink(): void * different routes to the write and only one of them reserves the name first. * * @dataProvider providerOverwriteSettings + * + * @group posix */ public function testWillNotWriteThroughASymlinkOntoAnExistingFile(bool $overwrite): void { @@ -249,6 +278,8 @@ public function providerOverwriteSettings(): array /** * `x` mode resolves the path through PHP's stream layer, so it follows a dangling symlink * and creates the target. The inode comparison is what catches that. + * + * @group posix */ public function testExclusiveCreateDetectsDanglingSymlink(): void { @@ -293,6 +324,8 @@ public function testFailedMoveDoesNotLeaveAPlaceholderBehind(): void * can change in between. What keeps the upload out of a symlinked target is that the bytes * go to a staged name and are `rename()`d on, which replaces the entry rather than * resolving it. The stub stands in for an attacker winning that race. + * + * @group posix */ public function testSymlinkPlantedDuringTheWriteIsReplacedNotFollowed(): void { @@ -796,6 +829,7 @@ public function testReadmeDocumentsTheDefaultDenyList(): void ); } + /** @group posix */ public function testSetModeAppliesPermissions(): void { $workingDirectory = $this->makeWorkingDirectory(); @@ -806,6 +840,7 @@ public function testSetModeAppliesPermissions(): void $this->assertSame('0600', $this->modeOf($workingDirectory . '/private.txt')); } + /** @group posix */ public function testDefaultModeIsAppliedWithoutBeingAskedFor(): void { $workingDirectory = $this->makeWorkingDirectory(); @@ -819,6 +854,8 @@ public function testDefaultModeIsAppliedWithoutBeingAskedFor(): void /** * `setMode(null)` hands the mode back to the process umask, which is what * `move_uploaded_file()` does on its own. + * + * @group posix */ public function testModeCanBeHandedBackToTheUmask(): void { @@ -874,6 +911,7 @@ public function testRefusesAFileThatPhpDidNotReceiveAsAnUploadByDefault(): void } /** The opt-out, and the whole of it: everything else about the write is unchanged */ + /** @group posix */ public function testStoresAFileNotUploadedByPhpOnceTheCallerAllowsIt(): void { $workingDirectory = $this->makeWorkingDirectory(); @@ -897,6 +935,8 @@ public function testStoresAFileNotUploadedByPhpOnceTheCallerAllowsIt(): void /** * The opt-in authorises a source the SAPI did not write. It does not authorise anything * about the destination, so the refusals that make the write safe still hold. + * + * @group posix */ public function testAFileNotUploadedByPhpStillCannotBeWrittenThroughASymlink(): void { @@ -945,6 +985,8 @@ public function providerBlockedForAnyOrigin(): array * Linux, which covers CI; set `UPLOAD_TEST_OTHER_FS` to a writable directory on another * mount to run it anywhere else (macOS: `hdiutil attach -nomount ram://8192` then * `diskutil erasevolume HFS+ UPLOADTMP `). + * + * @group posix */ public function testStoresAFileFromAnotherFileSystem(): void { @@ -1010,6 +1052,8 @@ protected function otherFileSystemDirectory(): ?string * stream wrapper, which is what is left once its plain-files EXDEV handling is accounted * for (see the test above). Also the other half of that branch — a source the move cannot * unlink afterwards must not fail an upload whose bytes are already at the destination. + * + * @group posix */ public function testCopiesTheFileWhenItCannotBeRenamedAcrossFileSystems(): void { @@ -1254,6 +1298,8 @@ public function testReturnsUploadedFileName(): void * Driven through `reserveDestination()` rather than `upload()`, because `upload()`'s own * `is_link()` check rejects a link that is already in place. Reaching here means the link * was planted after it, which is a race a test cannot stage. + * + * @group posix */ public function testReservationThroughASymlinkLeavesNothingAtItsTarget(): void { @@ -1370,6 +1416,8 @@ public function testAnOrdinaryNonAsciiNameIsStillStored(): void /** * The placeholder holds the final name for the whole transfer, so it must not sit there at * whatever the umask allowed. + * + * @group posix */ public function testTheReservationPlaceholderCarriesTheConfiguredMode(): void { diff --git a/tests/Upload/Validation/MimetypeTest.php b/tests/Upload/Validation/MimetypeTest.php index aec2386..f308bce 100644 --- a/tests/Upload/Validation/MimetypeTest.php +++ b/tests/Upload/Validation/MimetypeTest.php @@ -43,4 +43,45 @@ public function testInvalidMimetype(): void $validation = new Mimetype(['image/png']); $validation->validate($file); } + + /** + * A media type is case-insensitive, and `FileInfo::getMimetype()` always answers + * lowercase, so an allow-list written in any other case matched nothing and rejected + * every file it was written to accept. `Extension` and `FileType` both fold theirs. + * + * @dataProvider provideTheSameMediaTypeInDifferentCases + */ + public function testTheAllowListIsCaseInsensitive(string $configured): void + { + $file = new FileInfo($this->assetsDirectory . '/foo.txt', 'foo.txt'); + + (new Mimetype([$configured]))->validate($file); + + $this->addToAssertionCount(1); + } + + /** @return array> */ + public function provideTheSameMediaTypeInDifferentCases(): array + { + return [ + 'lowercase' => ['text/plain'], + 'uppercase' => ['TEXT/PLAIN'], + 'mixed case' => ['Text/Plain'], + 'padded' => [' text/plain '], + ]; + } + + /** + * A custom `FileInfoInterface` is a public extension point and need not lowercase what it + * sniffs, which is the half `FileType` already folds. + */ + public function testTheSniffedTypeIsFoldedToo(): void + { + $fileInfo = $this->createMock(\GravityPdf\Upload\FileInfoInterface::class); + $fileInfo->method('getMimetype')->willReturn('IMAGE/PNG'); + + (new Mimetype(['image/png']))->validate($fileInfo); + + $this->addToAssertionCount(1); + } } diff --git a/tests/Upload/Validation/SizeTest.php b/tests/Upload/Validation/SizeTest.php index d84046e..aea6010 100644 --- a/tests/Upload/Validation/SizeTest.php +++ b/tests/Upload/Validation/SizeTest.php @@ -160,4 +160,71 @@ public function testASubclassCanFormatTheAmountItsOwnWay(): void (new GermanSize(5000000))->validate($this->fileOfSize(PHP_INT_MAX)); } + + /** + * A bound that is not a byte count used to reach `scale()`, which is declared `int` and + * raised a `TypeError` from inside `validate()` — where `File::runValidations()` absorbs + * it as `Validation could not be completed` and shows the developer's misconfiguration to + * whoever submitted the file. `InvalidArgumentException` is a `LogicException`, which + * that run re-throws. + * + * @dataProvider provideBoundsThatAreNotByteCounts + * + * @param mixed $maxSize + */ + public function testRejectsABoundThatIsNotAByteCount($maxSize, string $expected): void + { + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage($expected); + + new Size($maxSize); /* @phpstan-ignore-line */ + } + + /** @return array> */ + public function provideBoundsThatAreNotByteCounts(): array + { + return [ + /* What a bound read out of JSON, or arrived at by dividing, actually is */ + 'a float' => [1.5, 'must be an int of bytes or a string such as "5MB", double given'], + 'null' => [null, 'must be an int of bytes or a string such as "5MB", NULL given'], + 'an array' => [[5], 'must be an int of bytes or a string such as "5MB", array given'], + 'a bool' => [true, 'must be an int of bytes or a string such as "5MB", boolean given'], + /* Reads as a generous limit and rejects every upload, which is why + `File::humanReadableToBytes()` already refuses the string form */ + 'a negative int' => [-1, 'Size::$maxSize cannot be negative, -1 given'], + ]; + } + + public function testRejectsAMinimumThatIsNotAByteCount(): void + { + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Size::$minSize must be an int of bytes'); + + new Size(500, 1.5); /* @phpstan-ignore-line */ + } + + /** + * The maximum is the first argument, so the two are easy to pass the wrong way round — + * and a pair in that order accepts nothing at all. + */ + public function testRejectsBoundsNoFileCanSatisfy(): void + { + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage( + 'Size was given a minimum of 1000 bytes and a maximum of 10 bytes, which no file ' + . 'can satisfy. The maximum is the first argument.' + ); + + new Size(10, 1000); + } + + /** Equal bounds accept exactly one size, which is a limit somebody may well mean */ + public function testAcceptsBoundsThatMeet(): void + { + $validation = new Size(500, 500); + + $validation->validate($this->fileOfSize(500)); + + $this->addToAssertionCount(1); + } } From 1220900bd7df43547990788ddce97dd40fd5b5f2 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 24 Aug 2026 15:00:12 +1000 Subject: [PATCH 2/9] Enable fileinfo on the Windows runners `composer install` could not resolve: `setup-php` enables neither `ext-fileinfo` nor `ext-mbstring` on its Windows builds, where the Linux ones ship both. The first is this library's only requirement and the second is PHPUnit's own, so the job failed before it ran a test. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/phpunit.yml | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index 241fa8f..4ca8f7a 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -115,6 +115,10 @@ jobs: uses: shivammathur/setup-php@f3e473d116dcccaddc5834248c87452386958240 # v2.35.5 with: php-version: ${{ matrix.php }} + # Named because the Windows builds enable neither by default, where the Linux ones + # ship both: `ext-fileinfo` is this library's only requirement and PHPUnit declares + # `ext-mbstring`, so `composer install` cannot resolve without them. + extensions: fileinfo, mbstring - name: Install Composer dependencies run: composer install --no-scripts From 05c7ae4af88d22ae6b03b52ac6c8e901e15782da Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 24 Aug 2026 15:17:46 +1000 Subject: [PATCH 3/9] 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 274c457..b0ba872 100644 --- a/tests/Upload/FileInfoTest.php +++ b/tests/Upload/FileInfoTest.php @@ -321,6 +321,15 @@ public function providerSetNameSanitizing(): array 'jpeg', str_repeat('a', 246) . "\u{20B9F}" . str_repeat('b', 40) . '.jpeg', ], + + /* 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. */ + 90 => ['a-b', 'txt', 'a\b.txt'], + 91 => ['windows-win', 'ini', '..\..\windows\win.ini'], + 92 => ['b', 'txt', 'a/b.txt'], + 93 => ['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 7a117c21d3c80d67573d255a991f21d6215f5286 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 24 Aug 2026 15:21:26 +1000 Subject: [PATCH 4/9] 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 e2a823cd3361554c1cb24ba1c9f664c72ac61401 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 24 Aug 2026 15:26:40 +1000 Subject: [PATCH 5/9] 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 bab9f05b972dee22a24d5b2a7745ac5915721b20 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 24 Aug 2026 15:29:50 +1000 Subject: [PATCH 6/9] 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 08079991517989fc460cbd99afeed56c2086fae9 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Mon, 24 Aug 2026 15:32:09 +1000 Subject: [PATCH 7/9] 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. From 7f91254eecf9778f3d1a6289b884865ebd8f5a43 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Tue, 25 Aug 2026 03:27:11 +1000 Subject: [PATCH 8/9] Ask one question in one place, and cut the comments back MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two passes over the audit-findings branch. The comments first. `reserveDestination()` told the Windows `ino == 0` story twice in adjacent blocks; the `windows` job carried fifteen lines of what CLAUDE.md now records, "reasons about carefully" among them. Those and a dozen more are cut to the fact and the bug they name. Two claims a reader can check were wrong: `splitNameAndExtension()` credited `basename()` for a `rtrim()` it does itself, and `@group posix` marks fifteen tests rather than fourteen, one of them for a name NTFS refuses rather than for `symlink()`. Then the code. `reserveDestination()` and `releaseReservation()` were both asking whether a stat pair describes one file, and only the first had the guard for a platform that reports no inode; `isSameFile()` owns the question and answers `null` there, which both callers read as leave it alone. On pre-7.4 Windows the far end of a symlink is no longer removed on a stat that identifies nothing. `FileList::offsetSet()` read back the offset an append landed at so it could drop a key that cannot be there — `$sourceKeys` only ever loses keys, so the `array_key_last()` call and its unreachable null guard go. The length rule was the one refusal in `refuseUnsafeName()` written inline while its neighbours are `Filename` predicates: `Filename::exceedsMaxLength()` now sits with them, and the two doc lists a custom backend is told to follow name it. One hole, opened by the branch's own `trim()`: `new Mimetype([' '])` folded to `''`, which is what `getMimetype()` answers for a file it cannot read, so an unreadable file passed a list written to accept PNGs. Empty entries are dropped, as `FileType::normalize()` already dropped them. In the tests, a test called another test as its body, two mocks were hand-rolled beside the helper that builds them, and a docblock had drifted off `makeWorkingDirectory()` onto the method inserted above it. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/composer-manifest.yml | 4 +- .github/workflows/phpunit.yml | 19 ++--- CHANGELOG.md | 4 +- CLAUDE.md | 8 +- UPGRADE.md | 4 +- docs/api-reference.md | 3 +- docs/extending.md | 6 +- phpunit.xml | 3 +- src/Upload/File.php | 5 +- src/Upload/FileInfo.php | 4 +- src/Upload/FileList.php | 15 ++-- src/Upload/Filename.php | 35 +++++---- src/Upload/Storage/FileSystem.php | 98 +++++++++++++----------- src/Upload/Validation/Mimetype.php | 21 +++-- src/Upload/Validation/Size.php | 20 ++--- tests/Upload/FilenameTest.php | 4 +- tests/Upload/Storage/FileSystemTest.php | 88 +++++++++------------ tests/Upload/Validation/MimetypeTest.php | 14 ++++ 18 files changed, 185 insertions(+), 170 deletions(-) diff --git a/.github/workflows/composer-manifest.yml b/.github/workflows/composer-manifest.yml index febb900..4512ed5 100644 --- a/.github/workflows/composer-manifest.yml +++ b/.github/workflows/composer-manifest.yml @@ -35,8 +35,8 @@ jobs: # --strict, because this manifest is published: a missing `name` or an unbound version # constraint is an error here where it is only a warning for an application. The three - # manifests under tools/ are deliberately not published and pin exact versions, so they - # are not checked — that pinning is what makes the doc checks reproducible. + # manifests under tools/ are not published and pin exact versions, so they are not + # checked. - name: Validate composer.json run: composer validate --strict --no-check-lock diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index 22f89fb..39ecf37 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -84,20 +84,15 @@ jobs: token: ${{ secrets.CODECOV_TOKEN }} # Reserved device names, `:` naming an NTFS alternate data stream, trailing dots and spaces - # resolving away, `DIRECTORY_SEPARATOR`, and the note that `reserveDestination()`'s inode - # comparison degrades where `stat()` reports `ino` as 0 — all of it is Windows behaviour this - # library reasons about carefully and had never executed on Windows. + # resolving away, `DIRECTORY_SEPARATOR`, `stat()` reporting `ino` as 0 — Windows behaviour + # this library reasons about and had never executed against. It found two bugs on its first + # outing, both on the default `$overwrite = false` path and neither reachable from Linux, so + # it is a gate. # - # `@group posix` marks what genuinely cannot run there: `symlink()` needs a privilege an - # uploading process should not hold, `chmod()` is close to a no-op, and `umask()` means - # nothing. Excluded by group rather than skipped, so `cross-file-system` keeps the single + # `@group posix` marks what cannot run there: `symlink()` needs a privilege an uploading + # process should not hold, `chmod()` is close to a no-op, and `umask()` means nothing. + # Excluded by group rather than skipped, so `cross-file-system` keeps the single # `markTestSkipped()` its `--fail-on-skipped` guard depends on. - # - # 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 diff --git a/CHANGELOG.md b/CHANGELOG.md index 910316c..1aa919a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -103,8 +103,8 @@ Both still work and neither raises a runtime notice. * **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 -* **`Storage\FileSystem` refuses a name longer than `Filename::MAX_LENGTH`.** It applied every other rule `Filename` declares and not the length, so an over-long name from a `FileInfoInterface` of your own travelled to the exclusive create and failed on the file system's own `ENAMETOOLONG` — reported as `'Destination file could not be created'`, which is supposed to mean the directory went away and sends a caller into a retry that cannot succeed. The shipped `FileInfo` truncates, so nothing on that path changes +* **`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. An entry the fold leaves empty is dropped rather than registered, as `FileType` already dropped one: `getMimetype()` answers `''` for a file it cannot read, so `new Mimetype([' '])` would have accepted exactly those +* **`Storage\FileSystem` refuses a name longer than `Filename::MAX_LENGTH`.** It applied every other rule `Filename` declares and not the length, so an over-long name from a `FileInfoInterface` of your own travelled to the exclusive create and failed on the file system's own `ENAMETOOLONG` — reported as `'Destination file could not be created'`, which is supposed to mean the directory went away and sends a caller into a retry that cannot succeed. The shipped `FileInfo` truncates, so nothing on that path changes. The rule is `Filename::exceedsMaxLength()`, beside `hasControlCharacters()` and `hasBidiControls()`, so a storage backend of your own can ask for it rather than restate it * `Filename::maxNameLength()` floors the name's budget at zero. `acceptExtension()` caps an extension at 32 bytes, so nothing in this library reaches it, but `finalize()` takes the extension from its caller and a longer one made the budget negative — which `mb_strcut()` reads as "cut this many bytes off the end" * The upload directory's trailing separator is trimmed on Windows as well as POSIX. `rtrim($directory, '/')` left `C:\uploads\` as `C:\uploads\\` internally, which `getDirectory()` then trimmed and the rest of the class did not * **Case folding no longer follows the host locale.** `strtolower()` follows `LC_CTYPE` before PHP 8.2, so on four of the eight supported versions a `setlocale()` call elsewhere in the application changed what counted as the same extension, media type or device name. Under a Turkish locale `strtolower('TIFF')` is `tıff`, which 4.0's stricter `setExtension()` discards, storing `photo.TIFF` with no extension at all diff --git a/CLAUDE.md b/CLAUDE.md index 50f7cc4..89336b3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,11 +45,11 @@ The `i18n` workflow regenerates the catalogue and fails on a diff, which is what `docs/base64-uploads.md` is the same arrangement for a file posted as base64 in a JSON body, run by `tools/base64-docs/verify.php`. That directory has no manifest: a base64 payload is a string in the request body, so there is no third-party implementation to run the bridge against, and the check loads the library through the root `vendor/autoload.php` — the autoloader a consumer gets, `src/Upload/i18n.php` included. The page declares one snippet each for the bridge and for validating and storing what it returns. -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 `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 ones that need `symlink()`, `chmod()` or `umask()` to mean something, or a name NTFS refuses outright, 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 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. +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 ask `isSameFile()` whether two stats describe one file — it answers `null` where the platform reports no inode, and both callers read that as "leave it alone". `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. @@ -189,11 +189,11 @@ Sanitizing is **not** escaping — output still needs HTML escaping. The exact t **The write is staged, never direct.** The bytes go to `upload-<32 hex>.part` in the destination directory and are then `rename()`d onto the destination. `move_uploaded_file()` falls back to a stream copy across file systems, and that copy follows a symlink at the destination; sending it to an unguessable name is what removes the race. `rename()` replaces the directory entry rather than following it. **Don't "simplify" this back into a direct write.** -With `overwrite = false`, `reserveDestination()` claims the name first with an exclusive `fopen(…, 'xb')` — not `is_file()` then a move, which two concurrent requests both pass. PHP resolves the path through its own stream layer before the create, so `x` **follows a dangling symlink and creates the target**; the `fstat`/`lstat` inode comparison that follows is what catches it, and `releaseReservation()` removes the file created at the far end. That comparison is POSIX-only: before PHP 7.4 Windows reports `ino` as 0. The placeholder carries the configured mode, so the name is briefly held by a 0-byte file — a process killed mid-transfer leaves it behind. +With `overwrite = false`, `reserveDestination()` claims the name first with an exclusive `fopen(…, 'xb')` — not `is_file()` then a move, which two concurrent requests both pass. PHP resolves the path through its own stream layer before the create, so `x` **follows a dangling symlink and creates the target**; the `fstat`/`lstat` comparison in `isSameFile()` is what catches it, and `releaseReservation()` removes the file created at the far end. That comparison is POSIX-only: before PHP 7.4 Windows reports `ino` as 0, which `isSameFile()` answers `null` for rather than reading as a mismatch. The placeholder carries the configured mode, so the name is briefly held by a 0-byte file — a process killed mid-transfer leaves it behind. `resolveFilename()` reduces the name to a `basename()`, strips trailing dots and spaces (Windows resolves `evil.php.` to `evil.php`), and rewrites `<>:"|?*` to `-` — rewritten, not refused, because POSIX allows all of them, and `:` would otherwise name an NTFS alternate data stream. That is all it does: it decides what the name **is**, and `upload()` decides whether it may be written. -**No refusal is in there.** `resolveFilename()` is protected — the seam for changing how names are chosen — and while a refusal sat inside it, an override that said nothing about the subject dropped it. `upload()` runs all three against whatever the seam returns, and all three are `private`: `refuseUnsafeName()` (`''`, a leading `.`, `Filename::CONTROL_CHARACTERS`, `Filename::BIDI_CONTROLS`), `refuseReservedWindowsName()` and `refuseBlockedExtensions()`. The leading `.` is in the first of those rather than left to the deny-list, which is a list of extensions and does not cover `.env` at all. A refusal rather than a rewrite throughout — inventing a filename is the value object's job, not storage's. **Do not move any of them back into the seam**, and do not answer "a subclass can reproduce it": `return basename($fileInfo->getNameWithExtension())` is the override people actually write. The collision message in `reserveDestination()` treats the seam's output the same way, running the basename through `Filename::sanitizeForDisplay()` before quoting it — the base `resolveFilename()` refuses control and bidi characters, an override need not, and a storage message is written to a log. A name left with nothing by that says `'A file with that name already exists'` rather than quoting an empty string. A blocked extension is refused in **any** dot-separated component, because a web server does not necessarily treat the last component as the extension. Do not narrow that to one component, and do not move the two refusals back. +**No refusal is in there.** `resolveFilename()` is protected — the seam for changing how names are chosen — and while a refusal sat inside it, an override that said nothing about the subject dropped it. `upload()` runs all three against whatever the seam returns, and all three are `private`: `refuseUnsafeName()` (`''`, a leading `.`, `Filename::exceedsMaxLength()`, `Filename::CONTROL_CHARACTERS`, `Filename::BIDI_CONTROLS`), `refuseReservedWindowsName()` and `refuseBlockedExtensions()`. The leading `.` is in the first of those rather than left to the deny-list, which is a list of extensions and does not cover `.env` at all. A refusal rather than a rewrite throughout — inventing a filename is the value object's job, not storage's. **Do not move any of them back into the seam**, and do not answer "a subclass can reproduce it": `return basename($fileInfo->getNameWithExtension())` is the override people actually write. The collision message in `reserveDestination()` treats the seam's output the same way, running the basename through `Filename::sanitizeForDisplay()` before quoting it — the base `resolveFilename()` refuses control and bidi characters, an override need not, and a storage message is written to a log. A name left with nothing by that says `'A file with that name already exists'` rather than quoting an empty string. A blocked extension is refused in **any** dot-separated component, because a web server does not necessarily treat the last component as the extension. Do not narrow that to one component, and do not move the two refusals back. **`move_uploaded_file()` is the storage half of the provenance decision.** It refuses any source PHP did not receive as an upload, so `Storage\FileSystem` cannot store a `FileList` file — the class validates and then fails at the write — until `acceptFilesNotUploadedByPhp()` says otherwise. That is deliberate, and the pairing is the point: `FileInfo::isUploadedFile()` says where the file came from, this says the caller is willing to store it, and neither is a default. Do not make the fallback automatic on a failed `move_uploaded_file()` — a path an attacker steered would take exactly that branch, which is the whole attack the SAPI check exists to stop. With the opt-in on, `moveFile()` renames, with a copy behind it. The copy is **not** the cross-file-system path, whatever the PSR-7 adapter plan says: `rename(2)` does fail with `EXDEV` between a tmpfs tmp directory and a disk upload directory, but PHP's plain-files wrapper catches that itself and copies, so `rename()` returns `true` — verified against two real mounts, and pinned by a test that skips when it cannot find a second file system. The fallback covers what PHP does not absorb, a stream-wrapper source among it. Both send the bytes at the **staging** path rather than the destination, so the copy's symlink-following is closed off by the unguessable name the same way `move_uploaded_file()`'s own copy fallback is. `moveFile()` is `protected` for the same testability reason `moveUploadedFile()` is; `moveIntoStaging()` picks between them and is `private`. diff --git a/UPGRADE.md b/UPGRADE.md index f470317..2e7fcf2 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -343,7 +343,9 @@ Each of these is listed in full in the [changelog](CHANGELOG.md). * **`Validation\Mimetype` lowercases its allow-list**, as `Extension` and `FileType` already did. A list written in any other case previously matched nothing and rejected every upload, so **a `new Mimetype(['IMAGE/PNG'])` that appeared to be rejecting files correctly will now - start accepting them.** Check any list that is not already lowercase. + start accepting them.** Check any list that is not already lowercase. An entry that is empty + once trimmed is dropped from the list rather than matching the `''` that `getMimetype()` + answers for a file it cannot read. * `Storage\FileSystem` refuses a destination name longer than `Filename::MAX_LENGTH` (255 bytes) with `'Invalid destination file name'`, where it used to fail at the write with `'Destination file could not be created'`. Only reachable from a `FileInfoInterface` of your diff --git a/docs/api-reference.md b/docs/api-reference.md index a6270b8..edf25a0 100644 --- a/docs/api-reference.md +++ b/docs/api-reference.md @@ -139,7 +139,8 @@ a public extension point and inventing a filename is not storage's job. | `Filename::sanitizeNameWithExtension(string $filename, ?array $reserved = null): string` | The whole treatment for one string: splits name from extension, rewrites the first, validates the second, fits both to `MAX_LENGTH`. This is what `getErrors()` runs client-supplied names through. `$reserved` replaces `RESERVED_WINDOWS_NAMES`, for a `FileInfo` subclass that overrides which names it blanks. | | `Filename::sanitizeForDisplay(string $value, int $maxLength = Filename::MAX_DISPLAY_LENGTH): string` | The same character sets applied to prose rather than to a name: bidi controls deleted, runs of control characters collapsed to a single space, the result cut to `$maxLength` bytes, forced to valid UTF-8 where `ext-mbstring` is loaded, then trimmed of surrounding whitespace. No device-name blanking, and `%`, `/` and dots are left alone. **It does not escape** `<`, `>`, `&` or `"`, so escape on output as well. This is what `getErrors()` runs a validation failure's message through; use it for error strings of your own. | | `Filename::acceptExtension(string $extension, ?array $reserved = null): string` | The extension this library will keep, or `''` for one it will not. What `setExtension()` validates with. | -| `Filename::hasControlCharacters(string $value): bool` / `hasBidiControls(string $value): bool` | The two refusals `Storage\FileSystem` applies to a name. Use these rather than the constants below. | +| `Filename::hasControlCharacters(string $value): bool` / `hasBidiControls(string $value): bool` | Two of the refusals `Storage\FileSystem` applies to a name. Use these rather than the constants below. | +| `Filename::exceedsMaxLength(string $filename): bool` | Whether a name and its extension together spend more than `MAX_LENGTH` bytes. The third refusal, and what `FileInfo` truncates to. | | `Filename::deviceComponent(string $filename): string` / `isReservedDeviceComponent(string $value, ?array $reserved = null): bool` | The component that decides whether a name resolves to a Windows device, and whether it does. Windows ignores spaces around the name and everything from the first dot on, so `" con .txt"` is `con`. | | `Filename::extensionComponents(string $filename): string[]` | Every dot-separated component after the first, lowercased and trimmed — what a deny-list is matched against. The first is dropped: a file called `php` is not a file that runs as PHP. | | `Filename::MAX_LENGTH` / `MAX_EXTENSION_LENGTH` | `255` and `32` bytes. The name's budget is `MAX_LENGTH` minus the extension and its dot. | diff --git a/docs/extending.md b/docs/extending.md index a95a844..224b068 100644 --- a/docs/extending.md +++ b/docs/extending.md @@ -120,6 +120,6 @@ class ObjectStorage implements StorageInterface The protections under "Security notes" (the deny-list, the `basename()` reduction, the symlink refusal, the staged write) live in `Storage\FileSystem`. A custom backend needs its own equivalents, and does not have to restate the rules to get them: [`Filename`](api-reference.md#filename) is -public for this, with `hasControlCharacters()`, `hasBidiControls()`, `deviceComponent()`, -`isReservedDeviceComponent()` and `extensionComponents()` answering exactly what the shipped -storage asks before it writes. \ No newline at end of file +public for this, with `hasControlCharacters()`, `hasBidiControls()`, `exceedsMaxLength()`, +`deviceComponent()`, `isReservedDeviceComponent()` and `extensionComponents()` answering +exactly what the shipped storage asks before it writes. \ No newline at end of file diff --git a/phpunit.xml b/phpunit.xml index c58a7d7..bad8ddc 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -1,7 +1,6 @@ + on PHP 8.5 for a release, with the suite reporting OK. --> ` - annotation does not admit. PHP 8.5 deprecates the null offset as well. */ + overwrote the first, and a key the `ArrayAccess` annotation + does not admit reached `getUploadedLocators()`. PHP 8.5 deprecates it as well. */ if ($offset === null) { $this->objects[] = $value; diff --git a/src/Upload/FileInfo.php b/src/Upload/FileInfo.php index 2bf160b..2762623 100644 --- a/src/Upload/FileInfo.php +++ b/src/Upload/FileInfo.php @@ -189,8 +189,8 @@ 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(). */ + /* Not `pathinfo()`, which treats `\` as a separator on Windows alone: see + `Filename::splitNameAndExtension()`. */ list($base, $extension) = Filename::splitNameAndExtension($name); /* Not setExtension(): that re-fits the name to the new budget, and the setName() below diff --git a/src/Upload/FileList.php b/src/Upload/FileList.php index 1b49dea..1ad2f92 100644 --- a/src/Upload/FileList.php +++ b/src/Upload/FileList.php @@ -174,14 +174,13 @@ public function offsetSet($offset, $value): void { parent::offsetSet($offset, $value); - /* An append has no offset of the caller's to drop, so read back the one it landed at - rather than assuming which that is: `unset($this->sourceKeys[null])` reaches the key - `''` and leaves the real entry in place. The parent has just written, so the array - cannot be empty — the null check is for the analyser. */ - $written = $offset ?? array_key_last($this->objects); - - if ($written !== null) { - unset($this->sourceKeys[$written]); + /* An append has no offset of the caller's to drop, and lands at a key `$sourceKeys` + cannot hold: the constructor puts the caller's offsets there and entries only ever + leave. Guarded rather than dropped unconditionally, because + `unset($this->sourceKeys[null])` reaches the key `''` and leaves the real entry in + place. */ + if ($offset !== null) { + unset($this->sourceKeys[$offset]); } } diff --git a/src/Upload/Filename.php b/src/Upload/Filename.php index cb9151e..832008f 100644 --- a/src/Upload/Filename.php +++ b/src/Upload/Filename.php @@ -155,13 +155,11 @@ public static function sanitizeNameWithExtension(string $filename, ?array $reser * 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. + * is the only separator. `pathinfo()` treats `\` as one on Windows and not on POSIX, so + * `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. * - * Trailing slashes go first, as `basename()` drops them, so `photos/` still names `photos`. + * Trailing slashes go first, so `photos/` still names `photos`. * * @return array The name and the extension, either of which may be `''` * @phpstan-return array{0: string, 1: string} @@ -175,8 +173,8 @@ public static function splitNameAndExtension(string $filename): array $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. */ + /* The last dot, wherever it is: `.htaccess` is all extension and no name, as + `pathinfo()` answers. */ $dot = strrpos($basename, '.'); if ($dot === false) { @@ -308,14 +306,13 @@ public static function finalize(string $name, string $extension = '', ?array $re /** * How many bytes a name may use once its extension has taken its share * - * Floored at zero. `acceptExtension()` caps an extension at `MAX_EXTENSION_LENGTH`, so - * nothing in this library spends the whole budget, but `finalize()` takes the extension - * from its caller: a longer one made this negative, and a negative length means "cut this - * many bytes off the end" to `mb_strcut()`. + * Floored at zero. `finalize()` takes the extension from its caller, and one longer than + * `acceptExtension()` would keep made this negative — which `mb_strcut()` reads as "cut + * this many bytes off the end". */ private static function maxNameLength(string $extension): int { - return (int) max(0, self::MAX_LENGTH - ($extension !== '' ? strlen($extension) + 1 : 0)); + return max(0, self::MAX_LENGTH - ($extension !== '' ? strlen($extension) + 1 : 0)); } /** @@ -423,6 +420,18 @@ public static function isReservedDeviceComponent(string $value, ?array $reserved ); } + /** + * Whether a name and its extension together spend more than the byte budget + * + * Bytes rather than characters, and the whole name rather than either half: it is the same + * `MAX_LENGTH` `sanitizeName()` truncates to, asked as a question. `FileInfo` fits a name + * to it, so only a `FileInfoInterface` of your own hands storage one that does not. + */ + public static function exceedsMaxLength(string $filename): bool + { + return strlen($filename) > self::MAX_LENGTH; + } + public static function hasControlCharacters(string $value): bool { return preg_match('/' . self::CONTROL_CHARACTERS . '/', $value) === 1; diff --git a/src/Upload/Storage/FileSystem.php b/src/Upload/Storage/FileSystem.php index fc7fbac..4006725 100644 --- a/src/Upload/Storage/FileSystem.php +++ b/src/Upload/Storage/FileSystem.php @@ -155,10 +155,9 @@ public function __construct(string $directory, bool $overwrite = false) throw new InvalidArgumentException('Directory is not writable'); } - /* Both separators, or a Windows path already ending in one gains a second: `/` alone - left `C:\uploads\` as `C:\uploads\\`, where `getDirectory()` trims what this line - does not. On POSIX the charlist is `/` either way, so a directory legitimately - named with a trailing backslash is untouched. */ + /* Both separators, since a Windows path may end in either: `/` alone left + `C:\uploads\` as `C:\uploads\\`. On POSIX the charlist is `/` either way, so a + directory named with a trailing backslash keeps it. */ $this->directory = rtrim($directory, DIRECTORY_SEPARATOR . '/') . DIRECTORY_SEPARATOR; $this->overwrite = $overwrite; @@ -453,11 +452,10 @@ 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, 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. */ + POSIX only. `isSameFile()` answers `null` where the platform reports no inode, + which is Windows before PHP 7.4. A symlink there needs 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 on it. */ $entry = $this->lstatEntry($destinationFile); if ($opened === false || $entry === false) { @@ -474,17 +472,7 @@ protected function reserveDestination(string $destinationFile, FileInfoInterface ); } - /* 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'])) { + if (self::isSameFile($opened, $entry) === false) { /* 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. */ @@ -523,12 +511,11 @@ protected function lstatEntry(string $path) */ private function releaseReservation(string $destinationFile, $opened): void { - /* `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. */ + /* `is_link()` rather than a failed `readlink()`: 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 left holding the caller's name. + The stat cache is cleared because `reserveDestination()` has already lstat'd this + path. */ clearstatcache(true, $destinationFile); if (!is_link($destinationFile)) { @@ -538,34 +525,58 @@ private function releaseReservation(string $destinationFile, $opened): void 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 - file behind, which is the safe way to be wrong. The link itself stays — it was not - this upload's to create, so it is not this upload's to remove. */ + create and this check cannot make us delete a bystander. Anything short of that + leaves the file behind, which is the safe way to be wrong — an unreadable target, an + inode the platform does not report. The link itself stays: it was not this upload's + to create, so it is not this upload's to remove. */ if ($opened === false) { return; } + $target = @readlink($destinationFile); + + if ($target === false) { + return; + } + if (preg_match('~^(?:/|[A-Za-z]:[\\\\/]|\\\\\\\\)~', $target) !== 1) { $target = dirname($destinationFile) . DIRECTORY_SEPARATOR . $target; } $stat = @lstat($target); - if ($stat !== false && $stat['dev'] === $opened['dev'] && $stat['ino'] === $opened['ino']) { + if ($stat !== false && self::isSameFile($stat, $opened) === true) { @unlink($target); } } + /** + * Whether two stats describe the same file, or `null` where the platform cannot say + * + * Windows reports `ino` as 0 for every file before PHP 7.4, so a comparison there is + * between two absences. Read as a mismatch, that answered 'Destination is a symbolic link' + * to every reservation — which is every upload the default `$overwrite = false` makes, so + * the platform could not store a file at all. A real file has a real inode, so nothing is + * given up where the platform reports one. + * + * Both callers ask the same question of a `fstat()` and an `lstat()`, and both treat `null` + * as "leave it alone": the reservation is not refused as a symlink, and the file at the far + * end of one is not removed. + * + * @param array $one + * @param array $other + */ + private static function isSameFile(array $one, array $other): ?bool + { + if ($one['ino'] === 0 || $other['ino'] === 0) { + return null; + } + + return $one['dev'] === $other['dev'] && $one['ino'] === $other['ino']; + } + /** * Remove what a failed upload left behind * @@ -634,11 +645,10 @@ protected function resolveFilename(FileInfoInterface $fileInfo): string * A leading dot is refused here rather than left to the deny-list, which is a list of * extensions and so does not cover `.env` at all. * - * `Filename::MAX_LENGTH` is refused here for the reason the two character sets are: it is - * a rule `FileInfo` applies by truncating, so only a `FileInfoInterface` of your own - * arrives over it. Without this the name travelled to the exclusive create and failed on - * the file system's own `ENAMETOOLONG`, reported as `DESTINATION_NOT_CREATED` — the code - * that is supposed to mean the directory went away. + * The length is refused for the reason the two character sets are: `FileInfo` applies it + * by truncating, so only a `FileInfoInterface` of your own arrives over it. Without this + * the name failed at the exclusive create on `ENAMETOOLONG`, reported as + * `DESTINATION_NOT_CREATED` — the code that means the directory went away. * * @throws Exception If the name is not one that may be written */ @@ -646,7 +656,7 @@ private function refuseUnsafeName(string $filename, FileInfoInterface $fileInfo) { if ( $filename === '' - || strlen($filename) > Filename::MAX_LENGTH + || Filename::exceedsMaxLength($filename) || strpos($filename, '.') === 0 || Filename::hasControlCharacters($filename) || Filename::hasBidiControls($filename) diff --git a/src/Upload/Validation/Mimetype.php b/src/Upload/Validation/Mimetype.php index f8efd37..cff79b0 100644 --- a/src/Upload/Validation/Mimetype.php +++ b/src/Upload/Validation/Mimetype.php @@ -68,14 +68,19 @@ public function __construct($mimetypes) } /* Folded, as `Extension` and `FileType` fold theirs. A media type is case-insensitive - and `FileInfo::getMimetype()` always answers lowercase, so `'IMAGE/PNG'` matched - nothing and rejected every file the list was written to accept. */ - $this->mimetypes = array_map( - static function (string $mimetype): string { - return AsciiCase::toLower(trim($mimetype)); - }, - $mimetypes - ); + and `FileInfo::getMimetype()` answers lowercase, so `'IMAGE/PNG'` matched nothing. + An entry left empty by the trim is dropped rather than registered, for the reason + `FileType::normalize()` drops one: `getMimetype()` answers `''` for a file it cannot + read, so `' '` in the list would accept exactly those. */ + $this->mimetypes = []; + + foreach ($mimetypes as $mimetype) { + $mimetype = AsciiCase::toLower(trim($mimetype)); + + if ($mimetype !== '') { + $this->mimetypes[] = $mimetype; + } + } } /** diff --git a/src/Upload/Validation/Size.php b/src/Upload/Validation/Size.php index ca8708d..6818c49 100644 --- a/src/Upload/Validation/Size.php +++ b/src/Upload/Validation/Size.php @@ -93,17 +93,13 @@ public function __construct($maxSize, $minSize = 0) * Read one bound as a byte count * * The types are checked here rather than left to `scale()`, which is declared `int` and - * raises a `TypeError` when a bound is a float — from inside `validate()`, where - * `File::runValidations()` absorbs it as `Validation could not be completed` and reports - * the developer's misconfiguration to whoever submitted the file. A float is what a bound - * read out of JSON or arrived at by division actually is. + * raises a `TypeError` on a float — from inside `validate()`, where `File::runValidations()` + * absorbs it as `Validation could not be completed` and shows the developer's + * misconfiguration to whoever submitted the file. A float is what a bound read out of JSON + * or arrived at by division is. * - * `InvalidArgumentException` is a `LogicException`, which that run re-throws, so a bound - * rejected after construction still reaches the developer. - * - * @param mixed $size Whatever the caller passed. Declared wider than the constructor's - * `int|string`, because a docblock is not enforced at runtime and this - * method exists to answer for the values that ignore it + * @param mixed $size Whatever the caller passed, wider than the constructor's `int|string` + * because a docblock is not enforced at runtime * @param string $parameter The parameter being read, named in the message * @throws InvalidArgumentException If the bound cannot be a byte count */ @@ -121,8 +117,8 @@ private function toBytes($size, string $parameter): int )); } - /* A negative bound reads as generous and rejects every upload, which is the failure - `File::humanReadableToBytes()` already refuses a string for. */ + /* A negative maximum rejects every file, which `humanReadableToBytes()` already + refuses a string for. */ if ($size < 0) { throw new InvalidArgumentException(sprintf( 'Size::$%s cannot be negative, %d given', diff --git a/tests/Upload/FilenameTest.php b/tests/Upload/FilenameTest.php index 6abf9bf..f3d80cd 100644 --- a/tests/Upload/FilenameTest.php +++ b/tests/Upload/FilenameTest.php @@ -173,7 +173,7 @@ public function provideFilenamesToSplit(): array } /** - * @dataProvider provideNamesToSplit + * @dataProvider provideNamesToSplitIntoExtensionComponents * * @param string[] $expected */ @@ -185,7 +185,7 @@ public function testExtensionComponents(string $filename, array $expected): void /** * @return array> */ - public function provideNamesToSplit(): array + public function provideNamesToSplitIntoExtensionComponents(): array { return [ 'an ordinary name' => ['evil.php', ['php']], diff --git a/tests/Upload/Storage/FileSystemTest.php b/tests/Upload/Storage/FileSystemTest.php index 2997525..f611d78 100644 --- a/tests/Upload/Storage/FileSystemTest.php +++ b/tests/Upload/Storage/FileSystemTest.php @@ -181,11 +181,8 @@ public function providerUnusableFileNames(): array 'numbered device' => ['COM0.log'], 'superscript device' => ["LPT\u{00B9}.log"], - /* `FileInfo` truncates to `Filename::MAX_LENGTH`; only an implementation of your - own arrives over it. Without this refusal the name reached the exclusive create - and failed on the file system's own ENAMETOOLONG, reported as - 'Destination file could not be created' — the code that means the directory - went away, which sends a caller down a retry that cannot succeed. */ + /* `FileInfo` truncates to `Filename::MAX_LENGTH`, so only an implementation of + your own arrives over it. See `refuseUnsafeName()`. */ 'longer than MAX_LENGTH' => [str_repeat('a', 252) . '.txt'], ]; } @@ -698,22 +695,12 @@ public function testCollisionNamesTheFileThatIsInTheWay(): void * exception is written to a log, which is the thing a control character forges a line in * and a bidi control reorders. * - * @dataProvider providerNamesAnOverriddenSeamMayReturn */ - public function testCollisionMessageSanitizesTheNameItQuotes(string $name, string $message): void + public function testCollisionMessageSanitizesTheNameItQuotes(): void { - $workingDirectory = $this->makeWorkingDirectory(); - $destinationFile = $workingDirectory . '/' . $name; - - /* Something already under the name, so the exclusive create finds it in the way */ - touch($destinationFile); - - $this->expectException(Exception::class); - $this->expectExceptionMessage($message); - - (new ExposedFileSystem($workingDirectory))->reserve( - $destinationFile, - $this->makeHostileFileInfo($name) + $this->assertCollisionMessageQuotes( + "resume\xE2\x80\xAEtxt.gpj", + 'A file named "resumetxt.gpj" already exists' ); } @@ -731,27 +718,34 @@ public function testCollisionMessageSanitizesControlCharactersInTheNameItQuotes( string $name, string $message ): void { - $this->testCollisionMessageSanitizesTheNameItQuotes($name, $message); + $this->assertCollisionMessageQuotes($name, $message); } - /** - * @return array> - */ - public function providerNamesWithControlCharactersASeamMayReturn(): array + private function assertCollisionMessageQuotes(string $name, string $message): void { - return [ - 'controls collapsed' => ["report\x07\x08.txt", 'A file named "report .txt" already exists'], - 'nothing but controls' => ["\x01\x02", 'A file with that name already exists'], - ]; + $workingDirectory = $this->makeWorkingDirectory(); + $destinationFile = $workingDirectory . '/' . $name; + + /* Something already under the name, so the exclusive create finds it in the way */ + touch($destinationFile); + + $this->expectException(Exception::class); + $this->expectExceptionMessage($message); + + (new ExposedFileSystem($workingDirectory))->reserve( + $destinationFile, + $this->makeHostileFileInfo($name) + ); } /** * @return array> */ - public function providerNamesAnOverriddenSeamMayReturn(): array + public function providerNamesWithControlCharactersASeamMayReturn(): array { return [ - 'bidi deleted' => ["resume\xE2\x80\xAEtxt.gpj", 'A file named "resumetxt.gpj" already exists'], + 'controls collapsed' => ["report\x07\x08.txt", 'A file named "report .txt" already exists'], + 'nothing but controls' => ["\x01\x02", 'A file with that name already exists'], ]; } @@ -936,8 +930,11 @@ public function testRefusesAFileThatPhpDidNotReceiveAsAnUploadByDefault(): void $this->assertFileExists($source); } - /** The opt-out, and the whole of it: everything else about the write is unchanged */ - /** @group posix */ + /** + * The opt-out, and the whole of it: everything else about the write is unchanged + * + * @group posix + */ public function testStoresAFileNotUploadedByPhpOnceTheCallerAllowsIt(): void { $workingDirectory = $this->makeWorkingDirectory(); @@ -1140,9 +1137,6 @@ protected function entriesIn(string $directory): array return $entries === false ? [] : array_values(array_diff($entries, ['.', '..'])); } - /** - * A scratch directory that is removed again in tear_down() - */ /** * The path `upload()` will return for a name stored in this directory * @@ -1156,6 +1150,9 @@ protected function destinationOf(string $directory, string $filename): string return $directory . DIRECTORY_SEPARATOR . $filename; } + /** + * A scratch directory that is removed again in tear_down() + */ protected function makeWorkingDirectory(): string { $workingDirectory = sys_get_temp_dir() . '/upload-test-' . uniqid('', true) . '/uploads'; @@ -1173,13 +1170,14 @@ protected function makeWorkingDirectory(): string * @param string $directory * @param bool $overwrite * @param callable|null $move + * @param string[] $extraMethods Further seams to stub, for a test that also drives one * @return FileSystem&\PHPUnit\Framework\MockObject\MockObject */ - protected function makeStorage(string $directory, bool $overwrite, $move = null) + protected function makeStorage(string $directory, bool $overwrite, $move = null, array $extraMethods = []) { $storage = $this->getMockBuilder(FileSystem::class) ->setConstructorArgs([$directory, $overwrite]) - ->onlyMethods(['moveUploadedFile']) + ->onlyMethods(array_merge(['moveUploadedFile'], $extraMethods)) ->getMock(); if ($move === null) { @@ -1375,10 +1373,7 @@ public function testAReservationThatCannotBeConfirmedIsNotReportedAsASymlink(): { $workingDirectory = $this->makeWorkingDirectory(); - $storage = $this->getMockBuilder(FileSystem::class) - ->setConstructorArgs([$workingDirectory, false]) - ->onlyMethods(['moveUploadedFile', 'lstatEntry']) - ->getMock(); + $storage = $this->makeStorage($workingDirectory, false, null, ['lstatEntry']); $storage->method('lstatEntry')->willReturn(false); @@ -1406,16 +1401,7 @@ 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); - } - ); + $storage = $this->makeStorage($workingDirectory, false, null, ['lstatEntry']); /* What that platform answers: no inode, and a `dev` of its own */ $storage->method('lstatEntry')->willReturn(['dev' => 2, 'ino' => 0]); diff --git a/tests/Upload/Validation/MimetypeTest.php b/tests/Upload/Validation/MimetypeTest.php index f308bce..14ca4d4 100644 --- a/tests/Upload/Validation/MimetypeTest.php +++ b/tests/Upload/Validation/MimetypeTest.php @@ -71,6 +71,20 @@ public function provideTheSameMediaTypeInDifferentCases(): array ]; } + /** + * `getMimetype()` answers `''` for a file it cannot read, so an entry the trim empties + * would accept exactly those — the drop `FileType::normalize()` makes for the same reason. + */ + public function testAnEmptyEntryDoesNotAcceptAnUnreadableFile(): void + { + $this->expectException(Exception::class); + + $fileInfo = $this->createMock(\GravityPdf\Upload\FileInfoInterface::class); + $fileInfo->method('getMimetype')->willReturn(''); + + (new Mimetype([' ']))->validate($fileInfo); + } + /** * A custom `FileInfoInterface` is a public extension point and need not lowercase what it * sniffs, which is the half `FileType` already folds. From b2988f3d2977ff93f6989d7f585b4f5293c29ad0 Mon Sep 17 00:00:00 2001 From: Jake Jackson Date: Tue, 25 Aug 2026 04:29:10 +1000 Subject: [PATCH 9/9] Do not report unreleased defects as v3 regressions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The two Windows entries described `reserveDestination()` reading an absent inode as a mismatch, and `releaseReservation()` inferring "not a symlink" from a failed `readlink()`. Both methods are new in 4.0.0 — 3.1.0's `upload()` is an `is_file()` check and a `move_uploaded_file()`, with no reservation to get wrong — so neither is something an upgrading caller ever saw. The behaviour that does ship is already in the exclusive-create feature entry, which says the inode verification holds on POSIX and not on Windows before PHP 7.4. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 2 -- UPGRADE.md | 4 ---- 2 files changed, 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1aa919a..662a5b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,8 +98,6 @@ 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` diff --git a/UPGRADE.md b/UPGRADE.md index 2e7fcf2..444a0fd 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -324,10 +324,6 @@ 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