Fix request->is(xml) returning true for HTML requests (#16585)

Add html/text as an option for all content-type based detectors so that we can differentiate between requests that want HTML but also take xml or json from being confused as a non-html type. Assuming browser based traffic is a safe default in my opinion. Fixes #16583 Co-authored-by: ADmad <ADmad@users.noreply.github.com>
cakephp · Jun 28, 2022 · 9d3a923 · 9d3a923
1 parent 9d5add5
commit 9d3a923
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 3 deletions.
diff --git a/src/Http/ServerRequest.php b/src/Http/ServerRequest.php
@@ -129,7 +129,12 @@ class ServerRequest implements ServerRequestInterface
         'ssl' => ['env' => 'HTTPS', 'options' => [1, 'on']],
         'ajax' => ['env' => 'HTTP_X_REQUESTED_WITH', 'value' => 'XMLHttpRequest'],
         'json' => ['accept' => ['application/json'], 'param' => '_ext', 'value' => 'json'],
-        'xml' => ['accept' => ['application/xml', 'text/xml'], 'param' => '_ext', 'value' => 'xml'],
+        'xml' => [
+            'accept' => ['application/xml', 'text/xml'],
+            'exclude' => ['text/html'],
+            'param' => '_ext',
+            'value' => 'xml',
+        ],
     ];
 
     /**
@@ -553,9 +558,25 @@ protected function _is(string $type, array $args): bool
     protected function _acceptHeaderDetector(array $detect): bool
     {
         $content = new ContentTypeNegotiation();
-        $accepted = $content->preferredType($this, $detect['accept']);
+        $options = $detect['accept'];
 
-        return $accepted !== null;
+        // Some detectors overlap with the default browser Accept header
+        // For these types we use an exclude list to refine our content type
+        // detection.
+        $exclude = $detect['exclude'] ?? null;
+        if ($exclude) {
+            $options = array_merge($options, $exclude);
+        }
+
+        $accepted = $content->preferredType($this, $options);
+        if ($accepted === null) {
+            return false;
+        }
+        if ($exclude && in_array($accepted, $exclude, true)) {
+            return false;
+        }
+
+        return true;
     }
 
     /**

diff --git a/tests/TestCase/Http/ContentTypeNegotiationTest.php b/tests/TestCase/Http/ContentTypeNegotiationTest.php
@@ -26,6 +26,21 @@ public function testPreferredTypeNoAccept()
         $this->assertNull($content->preferredType($request));
     }
 
+    public function testPreferredTypeFirefoxHtml()
+    {
+        $content = new ContentTypeNegotiation();
+        $request = new ServerRequest([
+            'url' => '/dashboard',
+            'environment' => [
+                'HTTP_ACCEPT' => 'text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8',
+            ],
+        ]);
+        $this->assertEquals('text/html', $content->preferredType($request));
+        $this->assertEquals('text/html', $content->preferredType($request, ['text/html', 'application/xml']));
+        $this->assertEquals('application/xml', $content->preferredType($request, ['application/xml']));
+        $this->assertNull($content->preferredType($request, ['application/json']));
+    }
+
     public function testPreferredTypeFirstMatch()
     {
         $content = new ContentTypeNegotiation();

diff --git a/tests/TestCase/Http/ServerRequestTest.php b/tests/TestCase/Http/ServerRequestTest.php
@@ -92,6 +92,12 @@ public function testAcceptHeaderDetector(): void
         $request = new ServerRequest();
         $request = $request->withEnv('HTTP_ACCEPT', 'text/plain, */*');
         $this->assertFalse($request->is('json'));
+
+        $request = new ServerRequest();
+        $request = $request->withEnv('HTTP_ACCEPT', 'text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8');
+        $this->assertFalse($request->is('json'));
+        $this->assertFalse($request->is('xml'));
+        $this->assertFalse($request->is('xml'));
     }
 
     public function testConstructor(): void