Resolve findings from the security hardening audit - #20
Open
jakejackson1 wants to merge 8 commits into
Open
Conversation
jakejackson1
force-pushed
the
hardening
branch
4 times, most recently
from
August 4, 2026 01:33
f5307d5 to
a3e2bc7
Compare
jakejackson1
added a commit
to GravityPDF/gravity-pdf
that referenced
this pull request
Aug 14, 2026
Five changes to how PDF templates get installed. Multiple zips can now be selected or dropped together. Previously only the first survived: the saga used takeLatest, which cancelled every in-flight upload but the last, and the reducer held a single success/error object that concurrent results overwrote. Uploads now use takeEvery, results carry the filename they belong to and are appended to a templateUploadResults array, and the component drains that array with a batch counter so results arriving in one React render can't be lost. Each file reports its own outcome. Zips with the templates nested inside a folder now install. Safari auto-extracts a template zip on download; users then re-zip the folder, which buries the PHP files one level deep where the non-recursive get_all_templates_in_folder() couldn't see them. Helper_Templates now descends through single-directory wrappers to find the templates. Multiple directories in the root stay invalid, as #1336 specified. The upload limit goes from 10MB to 32MB, clamped by wp_max_upload_size(). The clamp matters because a POST over post_max_size is discarded by PHP before the request reaches us, which surfaced as a nonce failure rather than a size error. One constant now feeds the server-side validator, the JS pre-flight check and the error message, which reports the real limit via size_format(). The drop target is the whole Template Manager window instead of the tile at the foot of the list. TemplateUploader wraps the manager with noClick/noKeyboard and shares the file picker with the "Add New Template" tile through context. Dragging anywhere shows a full-viewport overlay, and progress and results appear in a toast pinned to the modal so they're visible wherever the list is scrolled. The bundled upload library moves to 4.0, a security-hardening release. Storage stages each upload and moves it into place rather than writing to the destination directly, refuses traversal, dotfiles, control characters and symlinked destinations, applies a default extension deny-list, and stores files as 0640. Template zips move from Extension + Mimetype to the new Validation\FileType: the old pair checked two independent allow-lists, so the extension and the sniffed contents never had to describe the same format. The octet-stream allowance is kept, because plenty of servers report a zip that way. GFPDF\Helper\Fonts\LocalFile overrides isValid() wholesale to skip the is-uploaded-file check, so it did not inherit 4.0's reset of the error list -- without it, upload() calling isValid() again reported every font validation error twice. Note for review: composer.json points at dev-hardening while GravityPDF/Upload#20 is open. It needs repointing at ^4.0 once that is tagged, before this can merge. Closes #1336 Closes #1337 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
jakejackson1
marked this pull request as draft
August 14, 2026 07:13
Correct the deny-list documentation and pin it with a test
The upgrade guide, changelog and README all stated that a blocked word
anywhere between dots is refused, giving `release.config.zip` as an
example. That is wrong: `FileInfo::setName()` has always rewritten
interior dots to hyphens, so the name reaching storage carries a single
extension and is stored as `release-config.zip`.
The every-component check in `FileSystem::upload()` is real, but it is a
backstop for a caller-supplied `FileInfoInterface`, which is how the
existing tests reach it. Say that instead, and re-anchor the
narrow-the-list example to a name that genuinely refuses.
Adds the test that would have caught the claim, driving a real `FileInfo`
through `upload()` rather than the hostile stub. No behaviour change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Close two gaps in reserved-name and reserved-character handling
The Windows device-name list stopped at COM9/LPT9. Microsoft's list also
carries COM0, LPT0 and a superscript twin of each single digit, each of
which resolves to the same device, so `COM0.txt` and `COM<U+00B9>.txt`
were stored unchanged. The list moves to the public constant
`FileInfo::RESERVED_WINDOWS_NAMES`; the protected accessor stays, so an
override still works.
`FileSystem::resolveFilename()` screened control characters but not the
rest of the set Windows disallows, and did not screen device names at
all. Both now refuse, matching that layer's existing stance of rejecting
rather than rewriting. `:` is the one that fails unhelpfully rather than
loudly: on NTFS it names an alternate data stream instead of erroring.
Neither is reachable through the shipped `FileInfo`, which rewrites or
blanks all of it first. Both are about the `FileInfoInterface` extension
point, which is what that layer exists to distrust.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rewrite Windows-disallowed characters rather than refusing them
POSIX allows `< > : " | ? *` in a filename, so refusing them rejected
names that are perfectly ordinary on the system doing the storing. They
are replaced with `-` instead, matching how `FileInfo` treats the same
set one layer up.
The rewrite runs before the deny-list and the device-name check, so those
see the name that will actually be written and a blocked extension cannot
ride in behind one of these characters.
Reserved device names still refuse: `CON.txt` is a whole name rather than
a character, and there is nothing to rewrite it to that the caller would
recognise as their file.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Simplify the Windows reserved-name checks and their tests
`refuseReservedWindowsNames()` used `strstr($filename . '.', '.', true)`,
where the appended dot dodged a `false` return and the cast that followed
dodged PHPStan's complaint about the same `false`. Two workarounds for a
return type the code never wanted. `explode('.', $filename, 2)[0]` needs
neither, and matches how `refuseBlockedExtensions()` already splits the
name, so the two checks cannot disagree about where the first component
ends. `trim()` narrows to spaces, which is the rule Windows actually
applies rather than whatever bytes bare `trim()` happens to cover.
The device-name test was a byte-identical clone of
`testRejectsUnusableFileNames`, asserting the same exception and message
from a second provider; its cases move into `providerUnusableFileNames`
beside the other names storage refuses. Three tests that each rebuilt the
same working-directory-upload-assert body now share `assertStoredAs()`.
Docblocks lose a rationale that had been copied to four sites and now
lives once on the constant, and `getReservedWindowsNames()` says why it
survives as a one-line wrapper.
No behaviour change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Name the file in the collision message
Sanitizing is many-to-one, in `FileInfo` as well as at the storage layer:
`report?.txt` and `report*.txt` both resolve to `report-.txt`, and
`my+file.txt` and `my%20file.txt` have always both resolved to
`my-file.txt`. A caller told only `'File already exists'` cannot tell
which of their names collided, or that two of them collided with each
other rather than with something already on disk.
The message becomes `'A file named "report-.txt" already exists'`. The
basename only, never the path, and it has been through
`resolveFilename()` already, so it carries no separators or control
characters.
Callers matching on the old string need updating; noted in the changelog.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sanitize the name a custom FileInfoInterface puts into getErrors()
`File::isValid()` formatted `getNameWithExtension()` straight into three
error strings. Through the shipped `FileInfo` that name has already been
sanitized, but `FileInfoInterface` is a public extension point and
`FileInfo::setFactory()` lets any code in the process supply one, so the
value was whatever that implementation returned — a line break forging a
log line, a terminal escape, a bidi override reversing how the name
reads. `getErrors()` is the one thing the library documents as safe to
show an end user.
`uploadErrorMessage()` already solved this for the `$_FILES` path; the
new `safeName()` applies the same treatment, and for the same reason uses
`new FileInfo()` rather than the factory a caller can replace.
Storage already refused to trust a `FileInfoInterface`; this brings
`File` into line. Sanitizing is still not escaping, and output still
needs it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
jakejackson1
force-pushed
the
hardening
branch
from
August 19, 2026 00:59
40ec588 to
0a8b71d
Compare
jakejackson1
marked this pull request as ready for review
August 19, 2026 01:00
01 The README claimed renaming in `beforeUpload` could not dodge
validation. `upload()` validates and only then enters the loop that
fires the hook, so a name set there is never validated and only the
storage deny-list constrains it. The claim is replaced with what
actually happens, and points at `beforeValidate` for callers who need
the final name validated. A test now pins the ordering.
02 `FileInfoInterface`'s three setters declared `: FileInfo`, so an
implementation that did not extend `FileInfo` compiled and then threw
`TypeError` on the first setter call. It could not narrow its own
return type either — covariant returns arrived in 7.4 and this
library supports 7.3. The interface now declares no return type;
`FileInfo` keeps its own.
03 The reserved Windows device name check ran before the encoding
repair, so `con\xC3.txt` was not `con` when the check looked and was
`con.txt` afterwards. It now runs after.
04 Both control-character filters covered C0 only. The C1 range is valid
UTF-8, so `FileInfo` kept it and the storage layer accepted it —
U+0085 ends a line for anything matching on `\R`. Both sites now
match `\xC2[\x80-\x9F]`.
05 `humanReadableToBytes()` read an unrecognised unit as bytes, so
`Size('1T')` was a one byte bound that rejected every upload. The
pattern now admits only B/K/M/G and anything else takes the existing
throw. This changes a behaviour 3.x pinned deliberately.
06 `strtolower()` follows `LC_CTYPE` before PHP 8.2, so a Turkish locale
turned `photo.TIFF` into an extension `setExtension()` discarded. The
thirteen folds that expect ASCII go through a new `AsciiCase` trait.
07 `getUploadedFiles()` was reset after `upload()`'s guards, so a call
that failed validation handed back an earlier successful call's paths
and the documented rollback loop would delete them.
08 The 255 byte budget was divided against the extension set when
`setName()` ran, so a later `setExtension()` overshot by its length.
The fitting step is now `fitToLimit()` and `setExtension()` redoes it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Findings 09 through 15.
09 A reservation whose stat does not answer is reported as a failed create
rather than as a symbolic link, and takes its own placeholder back. Left
behind, it held the caller's name against every later upload.
10 A reservation that followed a symbolic link removes the file `x` mode
created at the far end. Inode-verified, so a link re-pointed in between
cannot make it delete a bystander; the link itself is left alone.
11 A developer error is no longer thrown as the type callers catch for a
failed upload. `File::upload()` throws LogicException with no validations
configured, `FileInfo::getHash()` throws InvalidArgumentException for an
unsupported algorithm.
12 `blockExtensions()` splits a compound entry into its components. The list
is matched one component at a time, so `tar.gz` was accepted and blocked
nothing. Empty and duplicate entries are dropped.
13 The single-file constructor branch guards a non-string `tmp_name`/`name`,
as the multi-file branch already did.
14 The class name of an absorbed throwable no longer reaches `getErrors()`,
which defeated the sanitizing already on that line.
15 `afterValidate` fires for a file that fails the uploaded-file check, so
the pair a caller opens and closes a resource with cannot leak.
Adds `statEntry()` as a seam for 09's branch, which is otherwise reachable
only by racing the file system, and `ExposedFileSystem` for 10's, which
`upload()`'s own `is_link()` check rejects before the reservation runs.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ntry Found while testing finding 13, which covered the single-file branch only. The multi-file branch checked `tmp_name` and `name` per file but assumed `name` and `error` were arrays of the same length as `tmp_name`. They are, from the SAPI. From a PSR-7 bridge or a test harness they need not be, and indexing them is worse than useless: an `error` that is an int warns with "Trying to access array offset on int", and a `name` that is a string yields a single character, which passes the per-file check, so the file is stored under a one-letter name with nothing reported. Both keys are now reduced to `[]` unless they are arrays, and the per-file check covers the error code as well, so a bad entry costs only itself while well-formed files in the same request are still collected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Findings 16 through 20.
16 The `upload()` docblock claimed the file appears under its final name
only once complete. True with `overwrite = true`; with the default it is
preceded by the placeholder that claims the name. Docblock corrected, and
the placeholder now carries the configured mode rather than sitting at
the umask for the whole transfer.
17 Documented that a storage exception message is for logs. It distinguishes
"already exists" from "could not be created", which is an existence check
on the upload directory. Kept the split, which is a real usability win.
18 `StorageInterface::upload()` specifies its return value: a locator the
implementation defines, an absolute path only for `FileSystem`. Its
`@throws` said "If validation fails", which storage does not do. The
README's `unlink()` rollback is qualified to match.
19 Added `allowAnyExtension()`, `getBlockedExtensions()` and `getMode()`.
`blockExtensions()`'s polarity locks at 4.0 — absent means the full list,
empty means none — so the additive counterparts land now.
20 `FileInfo::BIDI_CONTROLS` is the whole of Unicode's Bidi_Control property
plus the zero-width marks, separators and BOM, so U+061C and U+206A-206F
join the set. `FileSystem::resolveFilename()` refuses a name still
carrying one, which only a custom FileInfoInterface can produce.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
One of the four test gaps the audit lists. Every existing test passed an already-lowercase list, so nothing proved blockExtensions(['PHP']) still refuses shell.php. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Each was verified by removing the branch and watching the new test fail. Validation\Size's unmeasurable-file branch. Added this release so one file that no longer stats records an error against itself instead of aborting the batch through SplFileInfo::getSize(), and SizeTest.php was not touched at all. A missing path reaches it without mocking, since FileInfo::getSize() already returns false for one. The @rename() failure branch. The last step of the staged-write path the symlink story rests on. Driven with setMode(null) so the @chmod two lines above does not fail first, and asserts the reservation placeholder goes too — left behind it holds the name against every later upload of it. discard() under overwrite = true. Every failure-path test built storage with overwriting off, so the guard that stops a failed upload deleting the caller's existing file had no coverage; dropping the conditional passed CI. Covered from both sides, since the same conditional has to clear the placeholder when overwriting is off. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Six reviewers over the branch. One real defect, the rest documentation that overstated or misdescribed what the code does. File::__construct() still warned, and on PHP 8 fatally errored, on the very shapes the finding-13 guard was added for. The guard checked the values but dereferenced $_FILES[$key] and ['tmp_name'] before establishing they exist, so a string entry was an uncaught TypeError; and the single-file branch never type-checked the error code, so an array there reported the file as exceeding upload_max_filesize, because (int) [0] is 1. Seven shapes now covered. RESERVED_WINDOWS_NAMES lost its docblock when BIDI_CONTROLS was inserted between it and its documentation. Reflection returned false for it; the Microsoft link and the @var read as documenting a regex. Two comment rationales were fiction, both verified by execution: forceValidUtf8() is not repairing sequences this class broke. Every rewrite is single-byte ASCII except \xC2[\x80-\x9F], which matches both bytes as a unit -- 0 of the code points U+0080-U+2FFF produce invalid UTF-8 through the chain. mb_strcut() cuts on a boundary, and the substr() fallback runs only when mbstring is absent, which is when the repair is skipped. It exists for names that arrive invalid, which is its second sentence. The mb_detect_order() ValueError is unreachable: mb_strcut() accepts every encoding mb_detect_encoding() can return. Also corrected: humanReadableToBytes()'s docblock still promised the behaviour finding 05 removed; Size's "aborts the whole batch" predates the Throwable catch that absorbs it; Size's "must be less than" described an inclusive bound as exclusive; blockExtensions() documented leading-dot removal but not the splitting; the inode comparison is now marked POSIX-only, since Windows before 7.4 reports ino as 0 and the check degrades to same-drive; and "five of the eight supported PHP versions" is four. README: getHash() named the exception type it was changed away from, setExtension() was described as rejecting uppercase when it folds case first and does not mention discarding device names, Exception was said to extend \Exception rather than \RuntimeException, upload()'s empty-collection throw was missing, the 255-byte limit is shared with the extension, and a cross-reference pointed above at a table 120 lines below. CHANGELOG: three claims about 3.x were actually about this branch's own unreleased intermediate state -- neither upload() nor getHash() ever threw Upload\Exception in 3.x, and the photo.TIFF outcome is 4.0's stricter setExtension(), not a 3.x bug. Plus a duplicated getHash bullet, a self-contradiction about the placeholder, and "removes" where C1 and DEL are rewritten to a hyphen. UPGRADE: told interface implementers that returning $this "now works" while omitting the step that makes it work -- deleting : FileInfo from their own setters, without which the TypeError is unchanged. Invented an Exception subclass migration PHP does not require. Claimed report.txt. is stored as report.txt when the shipped FileInfo makes it report-txt. Missed the collision-message string change, the only bolded caller action in the changelog with no route into the guide. 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.
The 4.0.0 security hardening release, with the findings from a follow-up audit of the work folded in.
Storage writes each upload to a staged file and moves it into place rather than writing to the destination directly, applies a default extension deny-list, and refuses traversal, dotfiles, control and bidi characters, Windows device names and symlinked destinations.
Validation\FileTypepairs an extension with the media type sniffed from the contents. Filenames and extensions are sanitized, including in the stringsgetErrors()returns, stored files get mode0640, andupload()now requires something to validate against.Five things to know before merging
UPGRADE.mdis the step-by-step guide..htmland.svgincluded.UPGRADE.mdhas the snippets to narrow or replace the list. Dots inside the name are not extension separators —FileInforewrites them to hyphens, sorelease.config.zipis stored asrelease-config.zip. A deny-list entry that itself contains dots is split into components, soblockExtensions(['tar.gz'])blocks bothtarandgzwhere before it blocked nothing.'5MB'parsed as 5 bytes and now parses as 5 MiB, so anyMB/KB/GBsuffixed limit becomes much larger. A unit outsideB/K/M/Gnow throws instead of being read as bytes —'1T'was a one byte bound that rejected every upload while reading as a generous one. Check everyValidation\Sizebound you configure.File::upload()throws\LogicExceptionwith no validations configured, andFileInfo::getHash()throws\InvalidArgumentExceptionfor an unsupported algorithm. Both wereUpload\Exception— the typeisValid()catches and formats intogetErrors(), so a misspelled algorithm reached the end user as a rejected file. Code catchingUpload\Exceptionspecifically aroundupload()needs\LogicExceptiontoo; thecatch (\Exception $e)the README shows is unaffected.'File already exists'now names the file that is in the way, because sanitizing is many-to-one and the old wording could not say which name collided. Update anything matching on the old string, and log storage messages rather than showing them to whoever submitted the file — the wording distinguishes a name that exists from a destination that could not be created.If you implement
FileInfoInterfaceyourself, note that its three setters no longer declare a return type. They declared: FileInfo, the concrete class, so an implementation that did not extendFileInfosatisfied the compiler and then raised aTypeErroron the first setter call — a customFileInfoInterfaceis only now actually implementable.StorageInterface::upload()'s return value is specified for the first time as well: a locator your storage defines, which is why the README'sunlink()rollback is right forFileSystemand not necessarily for yours.The audit
Seven independent reviewers over
0a8b71dproduced 20 findings and 4 untested failure branches. All are resolved in the six commits that follow it. The two that mattered most:beforeUploadcould not dodge validation. It runs after validation, so a name set there is never validated — only the storage deny-list andFileSystem's filename rules apply. Corrected, with a test pinning the hook order.FileInfoInterface's setters, above. The interface had never been implementable by anything that was not aFileInfo, despite this library documenting it as an extension point.Each fix carries a test that was verified to fail against the code before it, including the four failure branches, which were checked by deleting the branch and watching the test go red.
Full detail in
CHANGELOG.md,UPGRADE.mdand the README.