Skip to content

Resolve findings from the security hardening audit - #20

Open
jakejackson1 wants to merge 1 commit into
mainfrom
hardening
Open

Resolve findings from the security hardening audit#20
jakejackson1 wants to merge 1 commit into
mainfrom
hardening

Conversation

@jakejackson1

@jakejackson1 jakejackson1 commented Aug 4, 2026

Copy link
Copy Markdown
Member

The 4.0.0 security hardening release.

Storage stages each upload to a temporary 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\FileType pairs an extension with the media type sniffed from the contents. Filenames and extensions are sanitized, including in the strings getErrors() returns. Stored files get mode 0640, and upload() now requires something to validate against.

373 tests, green on all eight supported PHP versions. PHPStan level 9 over src and tests.

Breaking changes

UPGRADE.md is the step-by-step guide. The ones most likely to bite:

  • The deny-list refuses a file whose own extension is on it, markup such as .html and .svg included. Only the last dot-separated component counts as an extension, so release.config.zip is fine and release.config is not. A deny-list entry containing dots is now split into components, so blockExtensions(['tar.gz']) blocks tar and gz where it previously blocked nothing.

  • blockExtensions() takes a required, non-empty list. No argument no longer means the default set, and [] throws. allowAnyExtension() is the only way to turn the deny-list off, so a missing config value can't silently disable it.

  • Two size units changed meaning. '5MB' parsed as 5 bytes and now parses as 5 MiB, so any KB/MB/GB limit becomes much larger. A unit outside B/K/M/G now throws rather than being read as bytes — '1T' was a one-byte bound that rejected everything while reading as a generous one. Check every Validation\Size bound.

  • Four developer errors changed exception type, all previously Upload\Exception — the type isValid() catches and formats into getErrors(), so a typo in your own source reached the end user as a rejected upload. upload() with no validations and createFromFactory() with a bad factory now throw \LogicException; getHash() with an unsupported algorithm throws \InvalidArgumentException; __call() for an unknown method throws \BadMethodCallException. isValid() also re-throws \LogicException from a validator rather than absorbing it. Code catching Upload\Exception specifically around upload() needs \LogicException too. The catch (\Exception $e) in the README is unaffected.

  • The storage collision message changed. 'File already exists' now names the file in the way, since sanitizing is many-to-one and the old wording couldn't say which name collided. Update anything matching the old string.

  • FileInfoInterface's three setters no longer declare a return type. They declared : FileInfo, the concrete class, so an implementation that didn't extend FileInfo compiled and then raised a TypeError on the first setter call — the interface was only implementable by subclasses of the class it was meant to abstract. If yours declares : FileInfo to match, delete it or the TypeError is unchanged. StorageInterface::upload()'s return value is specified for the first time as well: a locator your storage defines, which is why the README's unlink() rollback suits FileSystem and not necessarily yours.

New: GravityPdf\Upload\Filename

Filename rules are applied at two altitudes with two different outcomes — FileInfo rewrites a client-supplied name, Storage\FileSystem refuses one that still breaks a rule, because a FileInfoInterface is a public extension point and inventing a filename isn't storage's job.

That split is deliberate; the two layers disagreeing about what the rules are was not. Both control-character filters covered C0 alone, and each had to be found and fixed separately. Filename declares MAX_LENGTH, MAX_EXTENSION_LENGTH, CONTROL_CHARACTERS, BIDI_CONTROLS and RESERVED_WINDOWS_NAMES once and owns the operations both layers share, including the dot-splitting that decides where a component ends. Neither caller splits a filename for itself any more.

Review passes

Five passes over the work. Every fix carries a test verified to fail against the code before it.

Audit — seven reviewers, 20 findings and 4 untested failure branches. The two that mattered: the README claimed beforeUpload couldn't dodge validation, when it runs after validation so a name set there is never validated; and the FileInfoInterface setters above.

Cleanup — reuse, simplification, layering. Found a docblock of mine claiming blockExtensions()'s polarity "cannot change now" when the method is new and unreleased, two exception types still contradicting the taxonomy the audit had just set, and setNameWithExtension() re-fitting a name the next line overwrote at 34% of FileInfo construction cost.

Method audit — a naming and suitability pass over the 40 methods this release adds, which turned up a deny-list bypass. The extension and reserved-name checks ran inside resolveFilename(), which is protected and documented as the seam for changing how names are chosen, so a subclass overriding it dropped both refusals while saying nothing about extensions:

FileSystem   refused Files with the extension "php" cannot be stored
MyStorage    STORED  shell.php

Both checks now run in upload() against whatever the seam returns, both are private, and a test pins it. The same pass renamed methods that misdescribed themselves — getUploadedFiles()getUploadedLocators(), normalizeExtension()acceptExtension(), Filename::sanitize()sanitizeName(), and seven more — and dropped ten methods to private. Nothing renamed had ever shipped.

Documentation — every claim in the README, changelog and upgrade guide executed against the 3.1.0 baseline rather than reasoned about. It caught a regression the cleanup pass introduced: the mbstring gate had been collapsed from function_exists() to extension_loaded('mbstring') on the grounds that five checks were "one fact". They aren't — symfony/polyfill-mbstring is userland, defines every mb_* function and registers no extension, so every polyfilled install would have silently skipped the UTF-8 repair while three documents promised the polyfill works. It also found three changelog claims that were false about 3.1.0, four describing fixes to code that never shipped, and a blank line that broke the FileInfo API table.

Prose. A final pass over UPGRADE.md, the changelog's 4.0.0 section and the README, which were burying each instruction under the reasoning for it and, in the changelog's case, running a paragraph per bullet where that file's own 3.0.0 and 2.0.0 sections use one terse line. Roughly a quarter shorter across the three, with every backtick-quoted identifier diffed old against new to catch anything lost in the compression. It also turned up a rendering bug: the unescaped | in the README's list of Windows-disallowed characters was splitting that table cell into a phantom column, since GFM does not exempt code spans from pipe parsing.

@jakejackson1
jakejackson1 force-pushed the hardening branch 4 times, most recently from f5307d5 to a3e2bc7 Compare August 4, 2026 01:33
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>
@jakejackson1

This comment was marked as outdated.

Storage writes each upload to an unguessable staged name inside the
destination directory and renames it into place, rather than writing to the
destination directly. move_uploaded_file() falls back to a stream copy across
file systems and that copy follows a symlink at the destination; rename()
replaces the directory entry instead of following it, so the bytes never
travel through a link someone planted and no partial content is readable
under the final name.

An extension deny-list is on by default -- 59 extensions a server executes
plus 15 a browser renders -- checked against every dot-separated component,
because Apache's AddHandler matches any component and serves evil.php.jpg as
PHP. Storage also refuses traversal, dotfiles, C0/C1 controls, DEL, the
bidi and zero-width characters that make a stored name render as something
it is not, Windows device names, and a symlinked destination. Stored files
get mode 0640. upload() now requires something to validate against.

Validation\FileType pairs an extension with the media type sniffed from the
contents and requires the two to agree. Validation\Extension and
Validation\Mimetype check independent allow-lists, so a GIF named avatar.png
satisfies both; they are deprecated but still work.

Filenames and extensions are sanitized, including in the strings getErrors()
returns. setExtension() discards an invalid extension whole rather than
stripping characters from it, because stripping turned avatar.p-h-p into a
stored, executable avatar.php.

GravityPdf\Upload\Filename holds the rules for what counts as a usable
filename. Two layers apply them to different ends -- FileInfo rewrites a
client-supplied name, storage refuses one that still breaks a rule, because
FileInfoInterface is a public extension point -- and that split is
deliberate. The two disagreeing about what the rules are was not: both
control-character filters covered C0 alone and each had to be fixed
separately. The constants and the splitters are declared once now, and
neither layer splits a filename for itself.

Breaking changes callers must act on, in full in UPGRADE.md:

  '5MB' parsed as 5 bytes and now parses as 5 MiB, so any MB/KB/GB suffixed
  Size bound becomes much larger. A unit outside B/K/M/G throws instead of
  being read as bytes -- '1T' was a one byte bound that rejected every upload
  while reading as a generous one.

  blockExtensions() takes a required, non-empty list. allowAnyExtension() is
  the only way to empty it, so a config value that turns out to be missing
  cannot silently disable the control.

  upload() throws LogicException with no validations configured, getHash()
  throws InvalidArgumentException for an unsupported algorithm, and __call()
  throws BadMethodCallException for an unknown method. All were
  Upload\Exception -- the type isValid() formats into getErrors() -- so a
  typo in the caller's own source was reported to the end user as a rejected
  upload. isValid() re-throws LogicException from a validator for the same
  reason.

  'File already exists' now names the file in the way, because sanitizing is
  many-to-one and the old wording could not say which name collided.

  FileInfoInterface's three setters no longer declare a return type. They
  declared : FileInfo, so an implementation that did not extend FileInfo
  satisfied the compiler and then raised a TypeError on the first setter
  call. A custom FileInfoInterface is only now actually implementable.

  getMd5() is gone and getHash() defaults to sha256.

Verified by 373 tests on PHP 7.3 through 8.5, PHPStan level 9 over src and
tests, and PSR-12. The branch was reviewed four times -- a seven-reviewer
audit, a cleanup pass, an audit of every method this release adds, and a
documentation pass -- each finding recorded in CHANGELOG.md. Fixes carry a
test verified to fail against the code before them.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant