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' );