diff --git a/CHANGELOG.md b/CHANGELOG.md index d90ac90..9159af4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,7 +39,7 @@ A security release. New protections are on by default and will refuse some uploa * **`FileSystem::blockExtensions()` takes a required, non-empty list.** `null` meant the full deny-list and `[]` meant none, so one expression turned a security control off depending on what a config key held. `[]` now throws `InvalidArgumentException`, and `allowAnyExtension()` is the only way to empty the list. Pass `getDefaultBlockedExtensions()` for the old no-argument meaning * **`Upload\Exception`'s constructor takes the error code and the message's values before `$code` and `$previous`.** The signature is now `__construct(string $message, ?FileInfoInterface $fileInfo = null, string $errorCode = ErrorCode::NONE, array $messageArgs = [], int $code = 0, ?Throwable $previous = null)`. **A validation of your own passing `$code` or `$previous` positionally is passing them to the wrong parameters.** `new Exception($message, $fileInfo)` is unaffected * **`File::recordError()` takes a message id, its values, an error code and a filename** rather than a finished line: `recordError(string $messageId, array $args = [], string $errorCode = ErrorCode::NONE, ?string $filename = null)`. A `File` subclass that records an error of its own passes the message and the filename separately, and the `"filename: message"` composition happens in one place. `recordError('Rejected by the scanner')` still does what it did -* **`File::$errors` and `File::$constructorErrors` are gone**, replaced by a `private $errorDetails` holding each error as its parts. A subclass appending to `$errors` directly was the one route around the sanitizing guarantee `getErrors()` carries, and it is what would have left `getErrorDetails()` with holes. **Use `recordError()`**, which is still `protected`, and `getErrors()` to read. Reading, writing or `isset()`-ing either old name throws `\LogicException` naming both methods, as does reaching for `$errorDetails`, `$constructorErrorDetails` or anything else `File` keeps `private`: an append to a property that no longer exists creates a dynamic one nothing reads, and before PHP 8.2 it raises nothing at all. A subclass that *declares* one of those names, or the removed `protected static $errorCodeMessages`, is refused when it is constructed — a declared property is in scope, so no magic method can see it, and `empty($this->errors)` would have answered `true` for a collection that rejected every file +* **`File::$errors` and `File::$constructorErrors` are gone**, replaced by a `private $errorDetails` holding each error as its parts. A subclass appending to `$errors` directly was the one route around the sanitizing guarantee `getErrors()` carries, and it is what would have left `getErrorDetails()` with holes. **Use `recordError()`**, which is still `protected`, and `getErrors()` to read. Reading, writing or `isset()`-ing either old name throws `\LogicException` naming both methods, as does reaching for `$errorDetails`, `$constructorErrorDetails` or anything else `File` keeps `private`: an append to a property that no longer exists creates a dynamic one nothing reads, and before PHP 8.2 it raises nothing at all. A subclass that *declares* one of those names, or the removed `protected static $errorCodeMessages`, is refused when it is constructed — a declared property is in scope, so no magic method can see it, and `empty($this->errors)` would have answered `true` for a collection that rejected every file. `__get()`, `__set()`, `__isset()` and `__unset()` are `final`: `guardPropertyAccess()` is `private`, so those four are the only route to it, and a subclass overriding one to give itself magic properties dropped the guard without a diagnostic * **`File::$errorCodeMessages` is now the method `File::getUploadErrorMessages()`.** A PHP 7.3 constant expression cannot call a function, so an array of literals is all a property could hold and no extractor can see one. Each value is marked with `Translation::__()` instead. A subclass overriding the wording overrides the method * **`'5MB'` now means 5 MiB, not 5 bytes.** `File::humanReadableToBytes()` reads a trailing `B`, where `substr($input, -1)` saw only the `B`. **Every `KB`/`MB`/`GB` bound you pass becomes much larger.** Bounds without the trailing `B` are unchanged * **An unrecognized unit throws** instead of being read as bytes. `new Size('1T')` was a one-byte bound that rejected every upload while reading as a generous one. **Check any bound whose unit isn't `B`, `K`, `M` or `G`** @@ -85,7 +85,7 @@ A security release. New protections are on by default and will refuse some uploa * **`FileList::getSourceKeys(): array`**: the key each file arrived under, by collection offset, so a form field name survives without becoming one. The collection stays keyed `0..n`; writing to an offset or unsetting one drops that key * **`File::formatUploadFailure(string $clientFilename, int $errorCode): string`**: static, the `getErrors()` string for an `UPLOAD_ERR_*` failure, sanitized the way the `$_FILES` path sanitizes it, so a `FileList` caller reports failed transfers in the same words. An unrecognized code, `UPLOAD_ERR_OK` included, reads as `Unknown error` * **`\GravityPdf\Upload\Filename`** holds the filename rules in one place, so the layers that apply them can't drift apart. Constants `MAX_LENGTH`, `MAX_EXTENSION_LENGTH`, `FALLBACK`, `CONTROL_CHARACTERS`, `BIDI_CONTROLS` and `RESERVED_WINDOWS_NAMES`; `sanitizeNameWithExtension()` for a whole filename, `sanitizeForDisplay()` for prose rather than a name (same character sets; controls collapse to a space, whitespace is trimmed, and none of the length, device-name or `%`/`/` handling applies), plus `acceptExtension()`, `deviceComponent()`, `extensionComponents()`, `normalizeComponents()`, `isReservedDeviceComponent()`, `hasControlCharacters()` and `hasBidiControls()` for reproducing the storage refusals in an implementation of your own. The two character-set constants are bare pattern fragments, so use the predicates rather than passing them to `preg_match()` -* **`File::init(StorageInterface $storage): void`**: `protected`, the tail both constructors share, so anything added to the `$_FILES` constructor still holds for `FileList`. Not an extension seam: `protected` only because a subclass constructor can't call a parent's `private` method +* **`File::init(StorageInterface $storage): void`**: `final protected`, the tail both constructors share, so anything added to the `$_FILES` constructor still holds for `FileList`. Not an extension seam: `protected` only because a subclass constructor can't call a parent's `private` method, and `final` because an override skipping it leaves a subclass free to declare `$errors` and costs `isValid()` its idempotence * **`FileInfo::resetFactory(): void`** clears a factory installed with `setFactory()`. There was no supported way to undo it in a long-lived process or between tests * **Three new `protected` members on `Storage\FileSystem`**: `resolveFilename()` decides the stored name and is the only one meant to be overridden; `reserveDestination()` and `lstatEntry()` exist so a test can reach branches that otherwise need a file-system race. `FileInfo::isReadableFile()` is the guard the metadata accessors share * New storage exception messages: `'Invalid destination file name'`, `'Destination is a symbolic link'`, `'Permissions could not be applied to the stored file'` and `'Could not generate a temporary file name'` diff --git a/UPGRADE.md b/UPGRADE.md index 6c46674..ee9382c 100644 --- a/UPGRADE.md +++ b/UPGRADE.md @@ -232,6 +232,12 @@ still `protected`, which now takes the message and the filename separately: `Validation\Size::scale()` is new, and the seam for the one question the wording leaves open: it names the unit and formats the number, decimal separator included. +`File::init()` and the four magic property methods — `__get()`, `__set()`, `__isset()`, +`__unset()` — are new in 4.0.0 and `final`. `init()` is the tail both constructors share, so +constructor work of your own goes in your own `__construct()`; the four are the only route to +the guard that refuses the replaced names above, so declare a property of your own rather than +making it magic. + **Other changes:** * `blockExtensions()` called with no argument no longer means the default list: pass diff --git a/src/Upload/File.php b/src/Upload/File.php index 38c22f1..16a11e9 100644 --- a/src/Upload/File.php +++ b/src/Upload/File.php @@ -254,11 +254,12 @@ public function __construct(string $key, StorageInterface $storage) * input produces and the two read entirely different inputs. * * `protected` only so `FileList::__construct()` can call it — a `private` method of this - * class is out of reach from a subclass's own constructor. It is not an extension seam: - * overriding it without snapshotting the errors costs `isValid()` its idempotence, since - * that is the list it resets to. + * class is out of reach from a subclass's own constructor. `final` because it is not an + * extension seam. An override that skips the snapshot costs `isValid()` its idempotence, + * since that is the list it resets to; one that skips `guardAgainstReplacedMembers()` + * leaves a subclass free to declare `$errors`. */ - protected function init(StorageInterface $storage): void + final protected function init(StorageInterface $storage): void { $this->guardAgainstReplacedMembers(); @@ -605,10 +606,15 @@ protected function recordUploadFailure(string $clientFilename, int $errorCode): * `$this->errors[] = $message` was how a 3.x subclass recorded a failure. An append is a * read, not a write, so it arrives here rather than at `__set()`. * + * `final`, as the other three are. `guardPropertyAccess()` is `private`, so these four are + * the only way the guard is reached: a subclass overriding one to give itself magic + * properties drops it without a diagnostic, and `$this->errors = [...]` goes back to + * landing where nothing reads it. + * * @return mixed * @throws LogicException If the property is one this class declares or has replaced */ - public function __get(string $name) + final public function __get(string $name) { $this->guardPropertyAccess($name); @@ -627,10 +633,12 @@ public function __get(string $name) * with `Indirect modification of overloaded property`, so a subclass with array state of * its own has to declare the property rather than let a first append create it. * + * `final` for the reason `__get()` gives. + * * @param mixed $value * @throws LogicException If the property is one this class declares or has replaced */ - public function __set(string $name, $value): void + final public function __set(string $name, $value): void { $this->guardPropertyAccess($name); @@ -643,9 +651,11 @@ public function __set(string $name, $value): void * `if (empty($this->errors))` was the 3.x way to ask whether anything had failed. Without * this it answers `true` on a collection that rejected every file. * + * `final` for the reason `__get()` gives. + * * @throws LogicException If the property is one this class declares or has replaced */ - public function __isset(string $name): bool + final public function __isset(string $name): bool { $this->guardPropertyAccess($name); @@ -655,9 +665,11 @@ public function __isset(string $name): bool /** * Refuse an `unset()` of a property this class does not expose * + * `final` for the reason `__get()` gives. + * * @throws LogicException If the property is one this class declares or has replaced */ - public function __unset(string $name): void + final public function __unset(string $name): void { $this->guardPropertyAccess($name); } diff --git a/tests/Upload/FileTest.php b/tests/Upload/FileTest.php index fff7974..61433ef 100644 --- a/tests/Upload/FileTest.php +++ b/tests/Upload/FileTest.php @@ -1965,6 +1965,43 @@ public function testTheEntryPointsCannotBeOverridden(): void } } + /** + * The other two `final` groups, both of which fail quietly rather than loudly when a + * subclass replaces one: the failure it should have reported never appears in + * `getErrors()`, and nothing says so. + * + * @dataProvider provideMethodsThatAreNotSeams + */ + public function testAMethodThatIsNotASeamIsFinal(string $method, string $reason): void + { + $this->assertTrue( + (new \ReflectionMethod(File::class, $method))->isFinal(), + $method . '() must stay final: ' . $reason + ); + } + + /** @return array */ + public function provideMethodsThatAreNotSeams(): array + { + /* `guardPropertyAccess()` is private, so these four are the only route to it. A + subclass overriding one to give itself magic properties drops the guard, and + `$this->errors = [...]` goes back to landing where nothing reads it. */ + $guard = 'an override of it removes the guard on the replaced property names'; + + /* `protected` so `FileList::__construct()` can call it, not so a subclass can replace + it: an override skipping the parent leaves `guardAgainstReplacedMembers()` unrun and + `isValid()` with no snapshot to reset to. */ + $tail = 'an override of it skips the invariant both constructors share'; + + return [ + '__get' => ['__get', $guard], + '__set' => ['__set', $guard], + '__isset' => ['__isset', $guard], + '__unset' => ['__unset', $guard], + 'init' => ['init', $tail], + ]; + } + /** * `$this->errors[] = $message` was the 3.x way to record a failure. The property is gone, * so the append writes somewhere nothing reads: `getErrors()` never shows the failure, and