Fix the thirteen findings from the repository audit - #40
Merged
Conversation
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<int, FileInfoInterface>` 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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
`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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
Make the suite pass on Windows
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) <noreply@anthropic.com>
jakejackson1
force-pushed
the
fix/audit-findings
branch
from
August 24, 2026 17:42
d892133 to
7f91254
Compare
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) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A full audit of the repository turned up thirteen issues. Every CI check was already green and the security model held under adversarial testing — path traversal, symlinked and dangling-symlinked destinations, and 43 hostile filenames all behaved as documented. What follows is what did not.
One defect lost data. Two validators reported a developer's misconfiguration to whoever submitted the upload. One layer applied every filename rule but the length. Three more were reachable only on Windows, which nothing in this project had ever run on. The check that would have caught the first is now a gate, and so is Windows.
The code fixes
$file[] = $fileInfoappends instead of silently discarding a file. PHP passesoffsetSet()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 theArrayAccess<int, FileInfoInterface>contract does not admit reachedgetUploadedLocators(), whichstore()keys by collection offset. PHP 8.5 deprecates the null offset besides, so every append raised a notice. OnFileList, only an offset the caller actually supplied is dropped from$sourceKeys— an append lands at a key that list cannot hold — sogetSourceKeys()[$i]keeps naming$list[$i].Validation\Sizerejects a bound it cannot use, at construction. A float — what a limit read out of JSON or arrived at by division actually is — reached theint-typedscale()and raised aTypeErrorfrom insidevalidate(), whereFile::runValidations()absorbed it asValidation could not be completed. The developer's mistake shown to the submitter, with nothing anywhere naming the cause. Both bounds now go through one privatetoBytes(), which also refuses a negative bound and a minimum above the maximum.InvalidArgumentExceptionis aLogicException, which that run deliberately re-throws.Validation\Mimetypefolds its allow-list.ExtensionandFileTypeboth put theirs throughAsciiCase::toLower(); this one compared what it was given. A media type is case-insensitive andFileInfo::getMimetype()always answers lowercase, sonew Mimetype(['IMAGE/PNG'])rejected every PNG. The sniffed type is folded too, which only a customFileInfoInterfacecan arrive with in another case. An entry the fold leaves empty is dropped rather than registered, asFileType::normalize()already dropped one:getMimetype()answers''for a file it cannot read, sonew Mimetype([' '])would have accepted exactly those.Storage\FileSystemrefuses a name longer thanFilename::MAX_LENGTH. It read the two character sets and the reserved device names fromFilenameand not the length, so an over-budget name from aFileInfoInterfaceof your own travelled to the exclusive create and failed on the file system's ownENAMETOOLONG— reported asDESTINATION_NOT_CREATED, the code that is supposed to mean the directory went away and sends a caller into a retry that cannot succeed. The rule isFilename::exceedsMaxLength(), besidehasControlCharacters()andhasBidiControls(), so a storage backend of your own can ask for it rather than restate it. Verified both ways: 256 bytes refused, 255 stored.Two smaller ones: the upload directory's trailing separator is trimmed on Windows as well as POSIX (
rtrim($directory, '/')leftC:\uploads\asC:\uploads\\internally, whichgetDirectory()then trimmed and the rest of the class did not), andFilename::maxNameLength()floors the name's budget at zero rather than handingmb_strcut()a negative length, which means "cut this many bytes off the end".The gate that was missing
phpunit.xmlsetbackupGlobalsand nothing else, so a deprecation raised fromsrc/passed straight through a green run. That is how the append warned under PHP 8.5 while the suite reported OK. It now setsconvertDeprecationsToExceptions,failOnWarningandfailOnRisky. Proven with a throwaway test doing$arr[null] = 'x', which now errors with the exact message the append raised. The 654 tests already onmainpass unchanged, so nothing else was quietly raising.Windows
Reserved device names,
:naming an NTFS alternate data stream, trailing dots and spaces resolving away,DIRECTORY_SEPARATOR, andreserveDestination()'s inode comparison wherestat()reportsinoas 0 — all of it is Windows behaviour this library reasons about and had never executed. Every job across all nine workflows wasubuntu-latest.There is now a
windowsjob on PHP 7.3 and 8.5, and it is a gate: it ran ascontinue-on-erroronly until it was green once.@group posixmarks the fifteen test methods that cannot run there —symlink()needs a privilege an uploading process should not hold,chmod()is close to a no-op,umask()means nothing, and two names carry control characters NTFS refuses outright — excluded by group rather than skipped, socross-file-systemkeeps the singlemarkTestSkipped()its--fail-on-skippedguard depends on.--exclude-group posixis 681 tests, 0 skips.It earned the promotion on its first outing, with three defects no Linux job could reach:
Storage\FileSystemcould not store a file at all on Windows under PHP 7.3.reserveDestination()confirms its exclusive create by comparingfstat()andlstat(), which catches anxthat followed a symlink. That platform reportsinoas 0 for both, so the comparison had nothing to compare — and reading no information as a mismatch answeredDestination is a symbolic linkto every reservation, which is every upload the default$overwrite = falsemakes. 7.3 only; 7.4 onwards reports an inode there.A reservation that had to be released was left behind.
releaseReservation()decided whether the destination was a symlink by whetherreadlink()failed. PHP's Windowsreadlink()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 holding the caller's name against every later upload of it.is_link()asks the question directly, on every platform.A backslash in a client filename is a character, not a path separator.
FileInfo::setNameWithExtension()andFilename::sanitizeNameWithExtension()split throughpathinfo(), which treats\as a separator on Windows and as an ordinary character on POSIX — soa\b.txtwas stored asa-b.txton one andb.txton the other, and..\..\windows\win.iniaswindows-win.iniorwin.ini.Filename::splitNameAndExtension()owns the split now, with/as the only separator on every platform. On Windows this keeps name content that was previously discarded.A deslop and simplify pass
One question, asked in one place.
reserveDestination()andreleaseReservation()were both asking whether a stat pair describes the same file, and only the first had the guard for a platform that reports no inode.isSameFile()owns it and answersnullwhere the platform cannot say, which both callers read as leave it alone — so on pre-7.4 Windows the far end of a symlink is no longer removed on a stat that identifies nothing.FileList::offsetSet()had read back the offset an append landed at in order to drop a key that cannot be there;$sourceKeysonly ever loses keys, so thearray_key_last()call and its unreachable null guard are gone. 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 offmakeWorkingDirectory()onto the method inserted above it.The comments got the same treatment:
reserveDestination()told the Windows inode story twice in adjacent blocks, and thewindowsjob carried fifteen lines of whatCLAUDE.mdnow records. Two claims a reader can check were wrong —splitNameAndExtension()creditedbasename()for artrim()it does itself, and@group posixwas documented as marking fourteen tests.Other CI
A
Composer Manifestworkflow runscomposer validate --strict --no-check-lockandcomposer audit. Both pass today, but nothing kept them passing:package.ymlguards what the dist archive contains, and this guards the manifest that describes it. Scoped to the root manifest — the threetools/manifests pin exact versions deliberately and would fail--strictfor reasons that are not bugs..gitattributespinsi18n/upload.potto LF. Thei18nworkflow regenerates that file and fails on a diff, andCatalogueTestparses it line by line; a CRLF checkout breaks both.Documentation
The README's API table credited
Storage\FileSystemwith "the five protections it applies", one of which (allowUnvalidatedUploads()) isFile's. The class docblock said "Three protections", omitting themove_uploaded_file()provenance refusal that is also on by default and also has its own opt-out. Both say four now and the docblock names the fourth;docs/turning-the-defaults-off.mdkeeps its five, which correctly span two classes.docs/api-reference.mdgains a subsection on the two artefacts the staged write leaves behind —upload-<32 hex>.partand the 0-byte reservation placeholder. Clearing a stale one is the operator's job, and it was written down only in the 3.x-to-4.0 upgrade guide, which nobody installing 4.0 fresh will open..github/CONTRIBUTING.mdrequired a bug report to include "an actual final HTML code" and warned that "pasting a template file is not enough" — inherited wording from a templating library, meaningless here — carried a typo, and named none of the checks a PR is actually measured against. Rewritten: what reproduces an upload bug, the commands to run before opening a PR, the doc-check commands that apply when an example changes, and the PHP 7.3 language constraints.Two headings were missing a preceding blank line. Both render fine; they were the only two of 114 in the repository.
Tests
44 added, 654 → 698. Each fix is pinned in both directions where that is possible — a rule that refuses everything passes a one-sided test, so the storage length rule is covered at 256 bytes and at exactly 255, and the
Mimetypefolding is covered by a type that should still be rejected.Verified locally
phpunit(full: 698 tests, 1 skip;--exclude-group posix: 681, 0 skips;--exclude-group mbstring: 674),lint,phpstan,check-syntax,base64-docs,psr7-readme,translator-readme,i18n:pot(byte-identical), the package archive diff,composer validate --strict,composer audit, and every relative doc link and anchor. The traversal, symlink and filename-sanitizer runs from the audit pass unchanged, with one visible improvement: an over-budget name is now refused by name rather than byENAMETOOLONG.Behaviour changes to check on upgrade
All are in
UPGRADE.md. Three can affect a caller already on 3.x:Sizebound from configuration that is not anintor a size string now throws.'5M'and5242880are fine;5.0is not.Mimetypelist written in any case other than lowercase previously rejected every upload. It will now start accepting files. Check any list that is not already lowercase.CHANGELOG.mdcarries nine new Bug Fixes entries.🤖 Generated with Claude Code
Correction: the test counts first published here (654 → 673, and 658 for the
posix-excluded run) were measured againsttest/cjk-and-4-byte-filenames, the branch this work was drafted on, rather than againstmain. That branch has since merged, somainis 654 tests and the counts above were re-measured against it.