fix: treat empty file inputs as no upload on add-to-cart

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;
+    }
 }

Test/Unit/Plugin/DenyCartAddFileTest.php

@@ -49,15 +49,62 @@ 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<string, array{0: 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]);
 
         $this->raw->expects($this->once())->method('setHttpResponseCode')->with(403);
         $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<string, array{0: 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<string, mixed>
+     */
+    private static function emptyEntry(): array
+    {
+        return ['name' => '', 'type' => '', 'tmp_name' => '', 'error' => UPLOAD_ERR_NO_FILE, 'size' => 0];
+    }
+
+    /**
+     * @return array<string, mixed>
+     */
+    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'
         );