diff --git a/.gitattributes b/.gitattributes index e95e208..d897a8e 100644 --- a/.gitattributes +++ b/.gitattributes @@ -7,3 +7,7 @@ /phpunit.xml export-ignore /phpstan.neon export-ignore /CLAUDE.md export-ignore + +# The catalogue's exact bytes are a guarantee: the `i18n` workflow regenerates it and fails on +# a diff, and `CatalogueTest` parses it line by line. A CRLF checkout on Windows breaks both. +/i18n/upload.pot text eol=lf diff --git a/.github/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..4512ed5 --- /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 not published and pin exact versions, so they are not + # checked. + - 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..39ecf37 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`, `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 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. + windows: + name: ${{ matrix.php }} on windows-latest + runs-on: windows-latest + 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 }} + # 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 + + - 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..662a5b8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -98,6 +98,13 @@ 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. 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 * `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..89336b3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -45,7 +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. 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 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. @@ -185,17 +189,17 @@ 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`. `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..444a0fd 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -324,6 +324,28 @@ 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 + 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. 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 + 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..edf25a0 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. @@ -120,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/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..bad8ddc 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -1,4 +1,10 @@ + diff --git a/src/Upload/File.php b/src/Upload/File.php index 54d7a0a..103bca8 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,16 @@ 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 a key the `ArrayAccess` annotation + does not admit reached `getUploadedLocators()`. PHP 8.5 deprecates it as well. */ + if ($offset === null) { + $this->objects[] = $value; + + return; + } + $this->objects[$offset] = $value; } diff --git a/src/Upload/FileInfo.php b/src/Upload/FileInfo.php index 2cec2ec..2762623 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 { + /* 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 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/FileList.php b/src/Upload/FileList.php index b09b962..1ad2f92 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,14 @@ 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, 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]); + } } /** @param int $offset */ diff --git a/src/Upload/Filename.php b/src/Upload/Filename.php index 067243f..832008f 100644 --- a/src/Upload/Filename.php +++ b/src/Upload/Filename.php @@ -143,12 +143,47 @@ 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 + * `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, 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, as + `pathinfo()` answers. */ + $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 * @@ -270,10 +305,14 @@ 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. `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 self::MAX_LENGTH - ($extension !== '' ? strlen($extension) + 1 : 0); + return max(0, self::MAX_LENGTH - ($extension !== '' ? strlen($extension) + 1 : 0)); } /** @@ -381,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 884d4be..4006725 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,10 @@ public function __construct(string $directory, bool $overwrite = false) throw new InvalidArgumentException('Directory is not writable'); } - $this->directory = rtrim($directory, '/') . DIRECTORY_SEPARATOR; + /* 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; $this->blockExtensions(self::getDefaultBlockedExtensions()); @@ -447,10 +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 and `dev` as the - drive number, so this comparison degrades to same-drive and detects nothing. 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) { @@ -467,7 +472,7 @@ protected function reserveDestination(string $destinationFile, FileInfoInterface ); } - if ($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. */ @@ -506,9 +511,14 @@ 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()`: 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)) { /* Not a link, so the name is the file. `unlink()` does not follow one in any case. */ @unlink($destinationFile); @@ -517,24 +527,56 @@ private function releaseReservation(string $destinationFile, $opened): void /* 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 * @@ -603,12 +645,18 @@ 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. * + * 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 */ private function refuseUnsafeName(string $filename, FileInfoInterface $fileInfo): void { if ( $filename === '' + || 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 f2321fb..cff79b0 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,21 @@ 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()` 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; + } + } } /** @@ -73,7 +88,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..6818c49 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,64 @@ 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 + )); + } + } + + /** + * 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` 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. + * + * @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 + */ + private function toBytes($size, string $parameter): int + { + if (is_string($size)) { + $size = File::humanReadableToBytes($size); } - $this->maxSize = $maxSize; + 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 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', + $parameter, + $size + )); } - $this->minSize = $minSize; + return $size; } /** 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 90db812..fade519 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 { @@ -400,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); @@ -482,7 +504,7 @@ public function testTheLifecycleCallbacksFirePerFile(): void foreach (['beforeValidate', 'afterValidate', 'beforeUpload', 'afterUpload'] as $hook) { $list->$hook(static function (FileInfoInterface $fileInfo) use ($hook): void { - echo ucfirst($hook) . ': ' . $fileInfo->getName(), PHP_EOL; + echo ucfirst($hook) . ': ' . $fileInfo->getName(), "\n"; }); } diff --git a/tests/Upload/FileTest.php b/tests/Upload/FileTest.php index 3b56720..399d934 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); @@ -238,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 970583b..f3d80cd 100644 --- a/tests/Upload/FilenameTest.php +++ b/tests/Upload/FilenameTest.php @@ -112,7 +112,68 @@ public function testSanitizeTextRepairsASequenceTheBoundSplits(): void } /** - * @dataProvider provideNamesToSplit + * `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)); + } + + /** + * `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 provideNamesToSplitIntoExtensionComponents * * @param string[] $expected */ @@ -124,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']], @@ -185,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 61fe14f..f611d78 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,32 @@ 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`, so only an implementation of + your own arrives over it. See `refuseUnsafeName()`. */ + '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 +230,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 +275,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 +321,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 { @@ -584,7 +614,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) ); @@ -665,9 +695,33 @@ 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 + { + $this->assertCollisionMessageQuotes( + "resume\xE2\x80\xAEtxt.gpj", + 'A file named "resumetxt.gpj" already exists' + ); + } + + /** + * 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->assertCollisionMessageQuotes($name, $message); + } + + private function assertCollisionMessageQuotes(string $name, string $message): void { $workingDirectory = $this->makeWorkingDirectory(); $destinationFile = $workingDirectory . '/' . $name; @@ -687,11 +741,10 @@ public function testCollisionMessageSanitizesTheNameItQuotes(string $name, strin /** * @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'], ]; } @@ -756,7 +809,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. ')) ); } @@ -796,6 +849,7 @@ public function testReadmeDocumentsTheDefaultDenyList(): void ); } + /** @group posix */ public function testSetModeAppliesPermissions(): void { $workingDirectory = $this->makeWorkingDirectory(); @@ -806,6 +860,7 @@ public function testSetModeAppliesPermissions(): void $this->assertSame('0600', $this->modeOf($workingDirectory . '/private.txt')); } + /** @group posix */ public function testDefaultModeIsAppliedWithoutBeingAskedFor(): void { $workingDirectory = $this->makeWorkingDirectory(); @@ -819,6 +874,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 { @@ -873,7 +930,11 @@ public function testRefusesAFileThatPhpDidNotReceiveAsAnUploadByDefault(): void $this->assertFileExists($source); } - /** The opt-out, and the whole of it: everything else about the write is unchanged */ + /** + * The opt-out, and the whole of it: everything else about the write is unchanged + * + * @group posix + */ public function testStoresAFileNotUploadedByPhpOnceTheCallerAllowsIt(): void { $workingDirectory = $this->makeWorkingDirectory(); @@ -885,7 +946,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)); @@ -897,6 +958,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 +1008,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 { @@ -970,7 +1035,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); @@ -1010,6 +1075,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 { @@ -1022,7 +1089,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)); @@ -1070,6 +1137,19 @@ protected function entriesIn(string $directory): array return $entries === false ? [] : array_values(array_diff($entries, ['.', '..'])); } + /** + * 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; + } + /** * A scratch directory that is removed again in tear_down() */ @@ -1090,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) { @@ -1119,7 +1200,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); } @@ -1240,7 +1321,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()); @@ -1254,6 +1335,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 { @@ -1290,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); @@ -1307,6 +1387,31 @@ 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->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]); + + $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. @@ -1370,6 +1475,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 { @@ -1412,7 +1519,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')) ); } diff --git a/tests/Upload/Validation/MimetypeTest.php b/tests/Upload/Validation/MimetypeTest.php index aec2386..14ca4d4 100644 --- a/tests/Upload/Validation/MimetypeTest.php +++ b/tests/Upload/Validation/MimetypeTest.php @@ -43,4 +43,59 @@ 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 '], + ]; + } + + /** + * `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. + */ + 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); + } }