From bc98ba9a1df767f48c5982da3d68544b7790736a Mon Sep 17 00:00:00 2001 From: Scott Robinson Date: Fri, 11 Sep 2026 12:05:43 +0100 Subject: [PATCH] fix: treat empty file inputs as no upload on add-to-cart MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The controller guard counted entries in the request's file collection and read a non-empty collection as proof that something had been uploaded. It is not. PHP puts every file input on a submitted multipart form into $_FILES, including ones the customer left empty, with error UPLOAD_ERR_NO_FILE and a zero size, and Laminas' mapPhpFiles keeps all of them when it converts the superglobal — nothing filters them out. The product view form carries enctype="multipart/form-data" whenever the product has any option at all, not only a file one (module-catalog form.phtml). So with cart_add_file off, any product offering an optional file option returned 403 on an ordinary purchase where the customer simply left the upload empty. The 403 was returned before the backstop could apply its own no-upload handling, so the earlier fix one layer down did not help here. The guard now walks the mapped entries and counts one only when it carries a real upload: an entry whose error is anything other than UPLOAD_ERR_NO_FILE. A file PHP rejected for exceeding the size limit still counts, because a file was still sent. The walk recurses, since Laminas nests entries when an input name carries brackets. Audited the other guards for the same assumption. The customer upload guard is unaffected: both the open-source and Commerce controllers check for an empty file collection before constructing the uploader, and neither sits on a purchase path. The REST guard reads option structure rather than files. The backstop uses core's own isUploaded(), which rejects an entry with an empty name, so it was already correct. 43 tests, 73 assertions, all green against the 2.4.8-p5 tree. New coverage: four shapes that must not be treated as uploads, including nested and multi-option submissions, and four that must, including a mixed submission and a file rejected for size. --- Plugin/DenyCartAddFile.php | 66 ++++++++++++--- Test/Unit/Plugin/DenyCartAddFileTest.php | 103 ++++++++++++++++++++++- 2 files changed, 155 insertions(+), 14 deletions(-) diff --git a/Plugin/DenyCartAddFile.php b/Plugin/DenyCartAddFile.php index 30c1f4e..a2ffcc9 100644 --- a/Plugin/DenyCartAddFile.php +++ b/Plugin/DenyCartAddFile.php @@ -47,7 +47,7 @@ public function aroundExecute(Add $subject, callable $proceed) return $proceed(); } - if (!$this->carriesFiles($subject)) { + if (!$this->carriesUpload($subject)) { return $proceed(); } @@ -61,29 +61,73 @@ public function aroundExecute(Add $subject, callable $proceed) } /** - * Whether the request brought any uploaded file at all. + * Whether the request actually carries an uploaded file. + * + * The presence of entries is not proof of an upload. PHP puts every file input on a + * submitted multipart form into $_FILES, including ones the customer left empty, with + * error UPLOAD_ERR_NO_FILE — and Laminas keeps them all when it maps the superglobal. + * The product view form is multipart whenever the product has any option at all, so + * counting entries would refuse ordinary purchases of any product that merely offers an + * optional file option. * * @param Add $subject * @return bool */ - private function carriesFiles(Add $subject): bool + private function carriesUpload(Add $subject): bool { $files = $subject->getRequest()->getFiles(); - if ($files === null) { - return false; + if ($files instanceof \ArrayObject) { + $files = $files->getArrayCopy(); + } elseif ($files instanceof \Traversable) { + $files = iterator_to_array($files); } - if (is_array($files)) { - return $files !== []; + if (!is_array($files)) { + // An unrecognised shape is treated as "no upload", in keeping with the module's + // fail-safe rule. The custom-option backstop still catches the upload itself. + return false; } - if ($files instanceof \Countable) { - return count($files) > 0; + return $this->containsUpload($files); + } + + /** + * Walk the mapped file parameters, which nest when an input name carries brackets. + * + * @param array $entries + * @return bool + */ + private function containsUpload(array $entries): bool + { + foreach ($entries as $entry) { + if (!is_array($entry)) { + continue; + } + + if ($this->isUpload($entry) || $this->containsUpload($entry)) { + return true; + } } - // An unrecognised shape is treated as "no files", in keeping with the module's - // fail-safe rule. The custom-option backstop is what catches the upload itself. return false; } + + /** + * Whether one mapped entry represents a file the customer actually submitted. + * + * Any error other than UPLOAD_ERR_NO_FILE still means a file was sent, even when PHP + * rejected it and left the size at zero, so those count as an upload and are denied. + * + * @param array $entry + * @return bool + */ + private function isUpload(array $entry): bool + { + if (!array_key_exists('error', $entry) || is_array($entry['error'])) { + return false; + } + + return (int)$entry['error'] !== UPLOAD_ERR_NO_FILE; + } } diff --git a/Test/Unit/Plugin/DenyCartAddFileTest.php b/Test/Unit/Plugin/DenyCartAddFileTest.php index 0a78317..8c39268 100644 --- a/Test/Unit/Plugin/DenyCartAddFileTest.php +++ b/Test/Unit/Plugin/DenyCartAddFileTest.php @@ -49,7 +49,54 @@ public function testPlainAddToCartProceedsWhileTheSwitchIsOff(): void $this->assertSame('core-result', $result); } - public function testUploadIsRefusedWithABare403(): void + /** + * The regression this guards: PHP puts every file input on a submitted multipart form + * into $_FILES, empty ones included, with UPLOAD_ERR_NO_FILE. The product view form is + * multipart whenever the product has any option, so counting entries refused ordinary + * purchases of any product offering an optional file option. + * + * @dataProvider noActualUploadProvider + */ + public function testEmptyFileInputsAreNotTreatedAsUploads(array $files): void + { + $plugin = $this->plugin([SwitchConfig::UPLOAD_CART_ADD_FILE => false]); + + $this->raw->expects($this->never())->method('setHttpResponseCode'); + + $result = $plugin->aroundExecute( + $this->controllerWithFiles(new \ArrayObject($files)), + static fn () => 'core-result' + ); + + $this->assertSame('core-result', $result); + } + + /** + * @return array + */ + public static function noActualUploadProvider(): array + { + return [ + 'optional file option left empty' => [[ + 'options_7_file' => self::emptyEntry(), + ]], + 'two optional options, both empty' => [[ + 'options_7_file' => self::emptyEntry(), + 'options_9_file' => self::emptyEntry(), + ]], + 'nested bracketed input, empty' => [[ + 'options' => [7 => self::emptyEntry()], + ]], + 'deeply nested, empty' => [[ + 'options' => ['custom' => [7 => self::emptyEntry()]], + ]], + ]; + } + + /** + * @dataProvider actualUploadProvider + */ + public function testRealUploadIsRefusedWithABare403(array $files): void { $plugin = $this->plugin([SwitchConfig::UPLOAD_CART_ADD_FILE => false]); @@ -57,7 +104,7 @@ public function testUploadIsRefusedWithABare403(): void $this->raw->expects($this->once())->method('setContents')->with(''); $result = $plugin->aroundExecute( - $this->controllerWithFiles(new \ArrayObject(['options' => ['tmp_name' => 'x']])), + $this->controllerWithFiles(new \ArrayObject($files)), static function () { self::fail('The controller must not run once the upload has been denied.'); } @@ -66,12 +113,62 @@ static function () { $this->assertSame($this->raw, $result); } + /** + * @return array + */ + public static function actualUploadProvider(): array + { + return [ + 'flat input carrying a file' => [[ + 'options_7_file' => self::uploadedEntry(), + ]], + 'nested bracketed input carrying a file' => [[ + 'options' => [7 => self::uploadedEntry()], + ]], + 'one empty option alongside one real upload' => [[ + 'options_7_file' => self::emptyEntry(), + 'options_9_file' => self::uploadedEntry(), + ]], + 'file rejected by PHP for exceeding the size limit' => [[ + 'options_7_file' => [ + 'name' => 'polyglot.gif', + 'type' => 'image/gif', + 'tmp_name' => '', + 'error' => UPLOAD_ERR_INI_SIZE, + 'size' => 0, + ], + ]], + ]; + } + + /** + * @return array + */ + private static function emptyEntry(): array + { + return ['name' => '', 'type' => '', 'tmp_name' => '', 'error' => UPLOAD_ERR_NO_FILE, 'size' => 0]; + } + + /** + * @return array + */ + private static function uploadedEntry(): array + { + return [ + 'name' => 'polyglot.gif', + 'type' => 'image/gif', + 'tmp_name' => '/tmp/phpAb12Cd', + 'error' => UPLOAD_ERR_OK, + 'size' => 2048, + ]; + } + public function testUploadProceedsWhenTheSwitchAllows(): void { $plugin = $this->plugin([SwitchConfig::UPLOAD_CART_ADD_FILE => true]); $result = $plugin->aroundExecute( - $this->controllerWithFiles(new \ArrayObject(['options' => ['tmp_name' => 'x']])), + $this->controllerWithFiles(new \ArrayObject(['options_7_file' => self::uploadedEntry()])), static fn () => 'core-result' );