From 97d398221a2787e4b8ee17b34063ca60b3b9e31d Mon Sep 17 00:00:00 2001 From: Mark Story Date: Fri, 24 Jun 2022 23:22:17 -0400 Subject: [PATCH 1/5] Fix request->is(xml) returning true for HTML requests 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 --- src/Http/ServerRequest.php | 7 +++++-- .../TestCase/Http/ContentTypeNegotiationTest.php | 15 +++++++++++++++ tests/TestCase/Http/ServerRequestTest.php | 6 ++++++ 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/Http/ServerRequest.php b/src/Http/ServerRequest.php index b5399f1ff71..56e3106feac 100644 --- a/src/Http/ServerRequest.php +++ b/src/Http/ServerRequest.php @@ -553,9 +553,12 @@ 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']; + // We add text/html as the browsers often combine xml + html types together. + $options[] = 'text/html'; + $accepted = $content->preferredType($this, $options); - return $accepted !== null; + return $accepted !== null && $accepted != 'text/html'; } /** diff --git a/tests/TestCase/Http/ContentTypeNegotiationTest.php b/tests/TestCase/Http/ContentTypeNegotiationTest.php index 11fc7b91bb4..7dce2b60fb9 100644 --- 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/json'])); + $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 index 2164ed0107f..688558070ce 100644 --- 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 From 688b27a34f32472d2f8b0ff5074b8bae7d1f3403 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Sat, 25 Jun 2022 10:22:38 -0400 Subject: [PATCH 2/5] Make content type detection more robust. Someone could have a detector that wants to find text/html and the previous change would break that. --- src/Http/ServerRequest.php | 27 ++++++++++++++++--- .../Http/ContentTypeNegotiationTest.php | 2 +- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/Http/ServerRequest.php b/src/Http/ServerRequest.php index 56e3106feac..44f8c469ae6 100644 --- 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' + ], ]; /** @@ -554,11 +559,25 @@ protected function _acceptHeaderDetector(array $detect): bool { $content = new ContentTypeNegotiation(); $options = $detect['accept']; - // We add text/html as the browsers often combine xml + html types together. - $options[] = 'text/html'; + + // Some detectors overlap with the default browser Accept header + // For these types we use an exclude list to refine our content type + // detection. + $exclude = null; + if (!empty($detect['exclude'])) { + $exclude = $detect['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 $accepted !== null && $accepted != 'text/html'; + return true; } /** diff --git a/tests/TestCase/Http/ContentTypeNegotiationTest.php b/tests/TestCase/Http/ContentTypeNegotiationTest.php index 7dce2b60fb9..f6afdb6d349 100644 --- a/tests/TestCase/Http/ContentTypeNegotiationTest.php +++ b/tests/TestCase/Http/ContentTypeNegotiationTest.php @@ -36,7 +36,7 @@ public function testPreferredTypeFirefoxHtml() ], ]); $this->assertEquals('text/html', $content->preferredType($request)); - $this->assertEquals('text/html', $content->preferredType($request, ['text/html', 'application/json'])); + $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'])); } From fa5d52148756d0ff1152fad581df1ec693137eac Mon Sep 17 00:00:00 2001 From: Mark Story Date: Mon, 27 Jun 2022 09:58:50 -0400 Subject: [PATCH 3/5] Fix phpcs --- src/Http/ServerRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/ServerRequest.php b/src/Http/ServerRequest.php index 44f8c469ae6..af9cf860b2f 100644 --- a/src/Http/ServerRequest.php +++ b/src/Http/ServerRequest.php @@ -133,7 +133,7 @@ class ServerRequest implements ServerRequestInterface 'accept' => ['application/xml', 'text/xml'], 'exclude' => ['text/html'], 'param' => '_ext', - 'value' => 'xml' + 'value' => 'xml', ], ]; From 3f1c0548fa1b2121786d630a4079afbc30a41bdf Mon Sep 17 00:00:00 2001 From: Mark Story Date: Tue, 28 Jun 2022 10:08:21 -0400 Subject: [PATCH 4/5] Apply suggestions from code review Co-authored-by: ADmad --- src/Http/ServerRequest.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Http/ServerRequest.php b/src/Http/ServerRequest.php index af9cf860b2f..46f64354ae2 100644 --- a/src/Http/ServerRequest.php +++ b/src/Http/ServerRequest.php @@ -563,9 +563,8 @@ protected function _acceptHeaderDetector(array $detect): bool // Some detectors overlap with the default browser Accept header // For these types we use an exclude list to refine our content type // detection. - $exclude = null; - if (!empty($detect['exclude'])) { - $exclude = $detect['exclude']; + $exclude = $detect['exclude'] ?: null; + if ($exclude) { $options = array_merge($options, $exclude); } From e7aaba33fb320ef9c0d3e97213846a0da3fe4760 Mon Sep 17 00:00:00 2001 From: Mark Story Date: Tue, 28 Jun 2022 10:31:22 -0400 Subject: [PATCH 5/5] Use ?? instead of ?: --- src/Http/ServerRequest.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/ServerRequest.php b/src/Http/ServerRequest.php index 46f64354ae2..e784cdf089b 100644 --- a/src/Http/ServerRequest.php +++ b/src/Http/ServerRequest.php @@ -563,7 +563,7 @@ protected function _acceptHeaderDetector(array $detect): bool // 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; + $exclude = $detect['exclude'] ?? null; if ($exclude) { $options = array_merge($options, $exclude); }