From 41dfad388cbdb43664003f420b516b07a70e713e Mon Sep 17 00:00:00 2001
From: Lee Rowlands <lee.rowlands@previousnext.com.au>
Date: Sat, 12 May 2018 08:20:15 +1000
Subject: [PATCH] Issue #2854543 by Wim Leers, polynya, dawehner, mpdonadio,
gabesullice, larowlan, klausi: NegotiationMiddleware calls
$request->setRequestFormat('html') when there is no _format request
parameter, but shouldn't
---
.../Core/Routing/RequestFormatRouteFilter.php | 37 +++++++---
.../StackMiddleware/NegotiationMiddleware.php | 8 ++-
.../modules/rest_test/rest_test.services.yml | 4 ++
.../EntityResource/EntityResourceTestBase.php | 68 +++++++++++++------
.../default_format_test.info.yml | 6 ++
.../default_format_test.routing.yml | 28 ++++++++
.../src/DefaultFormatTestController.php | 15 ++++
.../Routing/DefaultFormatTest.php | 32 +++++++++
.../NegotiationMiddlewareTest.php | 10 +--
9 files changed, 170 insertions(+), 38 deletions(-)
create mode 100644 core/modules/system/tests/modules/default_format_test/default_format_test.info.yml
create mode 100644 core/modules/system/tests/modules/default_format_test/default_format_test.routing.yml
create mode 100644 core/modules/system/tests/modules/default_format_test/src/DefaultFormatTestController.php
create mode 100644 core/tests/Drupal/FunctionalTests/Routing/DefaultFormatTest.php
diff --git a/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
index dbfb801b7856..7448e6f1efc6 100644
--- a/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
+++ b/core/lib/Drupal/Core/Routing/RequestFormatRouteFilter.php
@@ -4,6 +4,7 @@
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException;
+use Symfony\Component\Routing\Route;
use Symfony\Component\Routing\RouteCollection;
/**
@@ -17,7 +18,14 @@ class RequestFormatRouteFilter implements FilterInterface {
public function filter(RouteCollection $collection, Request $request) {
// Determine the request format.
$default_format = static::getDefaultFormat($collection);
- $format = $request->getRequestFormat($default_format);
+ // If the request does not specify a format then use the default.
+ if (is_null($request->getRequestFormat(NULL))) {
+ $format = $default_format;
+ $request->setRequestFormat($default_format);
+ }
+ else {
+ $format = $request->getRequestFormat($default_format);
+ }
$routes_with_requirement = [];
$routes_without_requirement = [];
@@ -60,7 +68,9 @@ public function filter(RouteCollection $collection, Request $request) {
*
* By default, use 'html' as the default format. But when there's only a
* single route match, and that route specifies a '_format' requirement
- * listing a single format, then use that as the default format.
+ * listing a single format, then use that as the default format. Also, if
+ * there are multiple routes which all require the same single format then
+ * use it.
*
* @param \Symfony\Component\Routing\RouteCollection $collection
* The route collection to filter.
@@ -69,15 +79,20 @@ public function filter(RouteCollection $collection, Request $request) {
* The default format.
*/
protected static function getDefaultFormat(RouteCollection $collection) {
- $default_format = 'html';
- if ($collection->count() === 1) {
- $only_route = $collection->getIterator()->current();
- $required_format = $only_route->getRequirement('_format');
- if (strpos($required_format, '|') === FALSE) {
- $default_format = $required_format;
- }
- }
- return $default_format;
+ // Get the set of formats across all routes in the collection.
+ $all_formats = array_reduce($collection->all(), function (array $carry, Route $route) {
+ // Routes without a '_format' requirement are assumed to require HTML.
+ $route_formats = !$route->hasRequirement('_format')
+ ? ['html']
+ : explode('|', $route->getRequirement('_format'));
+ return array_merge($carry,$route_formats);
+ }, []);
+ $formats = array_unique(array_filter($all_formats));
+
+ // The default format is 'html' unless ALL routes require the same format.
+ return count($formats) === 1
+ ? reset($formats)
+ : 'html';
}
}
diff --git a/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php b/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
index e3864610f4f9..f246cce4624e 100644
--- a/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
+++ b/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php
@@ -46,7 +46,9 @@ public function handle(Request $request, $type = self::MASTER_REQUEST, $catch =
}
// Determine the request format using the negotiator.
- $request->setRequestFormat($this->getContentType($request));
+ if ($requested_format = $this->getContentType($request)) {
+ $request->setRequestFormat($requested_format);
+ }
return $this->app->handle($request, $type, $catch);
}
@@ -88,8 +90,8 @@ protected function getContentType(Request $request) {
return $request->query->get('_format');
}
- // Do HTML last so that it always wins.
- return 'html';
+ // No format was specified in the request.
+ return NULL;
}
}
diff --git a/core/modules/rest/tests/modules/rest_test/rest_test.services.yml b/core/modules/rest/tests/modules/rest_test/rest_test.services.yml
index d316cf6072b8..85b418a6c68a 100644
--- a/core/modules/rest/tests/modules/rest_test/rest_test.services.yml
+++ b/core/modules/rest/tests/modules/rest_test/rest_test.services.yml
@@ -12,3 +12,7 @@ services:
public: false
tags:
- { name: page_cache_request_policy }
+ rest_test.encoder.foobar:
+ class: Drupal\serialization\Encoder\JsonEncoder
+ tags:
+ - { name: encoder, format: foobar }
diff --git a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
index 2ab45273d505..b5d7fbb7d45d 100644
--- a/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -156,12 +156,22 @@ abstract class EntityResourceTestBase extends ResourceTestBase {
/**
* Provides an entity resource.
+ *
+ * @param bool $single_format
+ * Provisions a single-format entity REST resource. Defaults to FALSE.
*/
- protected function provisionEntityResource() {
+ protected function provisionEntityResource($single_format = FALSE) {
+ if ($existing = $this->resourceConfigStorage->load(static::$resourceConfigId)) {
+ $existing->delete();
+ }
+
+ $format = $single_format
+ ? [static::$format]
+ : [static::$format, 'foobar'];
// It's possible to not have any authentication providers enabled, when
// testing public (anonymous) usage of a REST resource.
$auth = isset(static::$auth) ? [static::$auth] : [];
- $this->provisionResource([static::$format], $auth);
+ $this->provisionResource($format, $auth);
}
/**
@@ -434,20 +444,6 @@ public function testGet() {
}
$this->provisionEntityResource();
- // Simulate the developer again forgetting the ?_format query string.
- $url->setOption('query', []);
-
- // DX: 406 when ?_format is missing, except when requesting a canonical HTML
- // route.
- $response = $this->request('GET', $url, $request_options);
- if ($has_canonical_url && (!static::$auth || static::$auth === 'cookie')) {
- $this->assertSame(403, $response->getStatusCode());
- }
- else {
- $this->assert406Response($response);
- }
-
- $url->setOption('query', ['_format' => static::$format]);
// DX: forgetting authentication: authentication provider-specific error
// response.
@@ -472,10 +468,44 @@ public function testGet() {
unset($request_options[RequestOptions::HEADERS]['REST-test-auth-global']);
$request_options = NestedArray::mergeDeep($request_options, $this->getAuthenticationRequestOptions('GET'));
- // DX: 403 when unauthorized.
- $response = $this->request('GET', $url, $request_options);
+ // First: single format. Drupal will automatically pick the only format.
+ $this->provisionEntityResource(TRUE);
$expected_403_cacheability = $this->getExpectedUnauthorizedAccessCacheability();
- $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', 'MISS');
+ // DX: 403 because unauthorized single-format route, ?_format is omittable.
+ $url->setOption('query', []);
+ $response = $this->request('GET', $url, $request_options);
+ if ($has_canonical_url) {
+ $this->assertSame(403, $response->getStatusCode());
+ $this->assertSame(['text/html; charset=UTF-8'], $response->getHeader('Content-Type'));
+ }
+ else {
+ $this->assertResourceErrorResponse(403, FALSE, $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', 'MISS');
+ }
+ $this->assertSame(static::$auth ? [] : ['MISS'], $response->getHeader('X-Drupal-Cache'));
+ // DX: 403 because unauthorized.
+ $url->setOption('query', ['_format' => static::$format]);
+ $response = $this->request('GET', $url, $request_options);
+ $this->assertResourceErrorResponse(403, FALSE, $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', $has_canonical_url ? 'MISS' : 'HIT');
+
+ // Then, what we'll use for the remainder of the test: multiple formats.
+ $this->provisionEntityResource();
+ // DX: 406 because despite unauthorized, ?_format is not omittable.
+ $url->setOption('query', []);
+ $response = $this->request('GET', $url, $request_options);
+ if ($has_canonical_url) {
+ $this->assertSame(403, $response->getStatusCode());
+ $this->assertSame(['HIT'], $response->getHeader('X-Drupal-Dynamic-Cache'));
+ }
+ else {
+ $this->assertSame(406, $response->getStatusCode());
+ $this->assertSame(['UNCACHEABLE'], $response->getHeader('X-Drupal-Dynamic-Cache'));
+ }
+ $this->assertSame(['text/html; charset=UTF-8'], $response->getHeader('Content-Type'));
+ $this->assertSame(static::$auth ? [] : ['MISS'], $response->getHeader('X-Drupal-Cache'));
+ // DX: 403 because unauthorized.
+ $url->setOption('query', ['_format' => static::$format]);
+ $response = $this->request('GET', $url, $request_options);
+ $this->assertResourceErrorResponse(403, $this->getExpectedUnauthorizedAccessMessage('GET'), $response, $expected_403_cacheability->getCacheTags(), $expected_403_cacheability->getCacheContexts(), static::$auth ? FALSE : 'MISS', 'HIT');
$this->assertArrayNotHasKey('Link', $response->getHeaders());
$this->setUpAuthorization('GET');
diff --git a/core/modules/system/tests/modules/default_format_test/default_format_test.info.yml b/core/modules/system/tests/modules/default_format_test/default_format_test.info.yml
new file mode 100644
index 000000000000..0a02a0688a13
--- /dev/null
+++ b/core/modules/system/tests/modules/default_format_test/default_format_test.info.yml
@@ -0,0 +1,6 @@
+name: 'Default format test'
+type: module
+description: 'Support module for testing default route format.'
+package: Testing
+version: VERSION
+core: 8.x
diff --git a/core/modules/system/tests/modules/default_format_test/default_format_test.routing.yml b/core/modules/system/tests/modules/default_format_test/default_format_test.routing.yml
new file mode 100644
index 000000000000..368dcb1946ce
--- /dev/null
+++ b/core/modules/system/tests/modules/default_format_test/default_format_test.routing.yml
@@ -0,0 +1,28 @@
+default_format_test.machine:
+ path: '/default_format_test/machine'
+ defaults:
+ # Same controller + method!
+ _controller: '\Drupal\default_format_test\DefaultFormatTestController::content'
+ requirements:
+ _access: 'TRUE'
+ _format: 'json'
+
+default_format_test.human:
+ path: '/default_format_test/human'
+ defaults:
+ # Same controller + method!
+ _controller: '\Drupal\default_format_test\DefaultFormatTestController::content'
+ requirements:
+ _access: 'TRUE'
+ _format: 'html'
+
+# Route definition identical to default_format_test.machine, only different name.
+# @see \Drupal\FunctionalTests\Routing\DefaultFormatTest::testMultiple
+default_format_test.machine.alias:
+ path: '/default_format_test/machine'
+ defaults:
+ # Same controller + method!
+ _controller: '\Drupal\default_format_test\DefaultFormatTestController::content'
+ requirements:
+ _access: 'TRUE'
+ _format: 'json'
diff --git a/core/modules/system/tests/modules/default_format_test/src/DefaultFormatTestController.php b/core/modules/system/tests/modules/default_format_test/src/DefaultFormatTestController.php
new file mode 100644
index 000000000000..6e52eea342bb
--- /dev/null
+++ b/core/modules/system/tests/modules/default_format_test/src/DefaultFormatTestController.php
@@ -0,0 +1,15 @@
+<?php
+
+namespace Drupal\default_format_test;
+
+use Drupal\Core\Cache\CacheableResponse;
+use Symfony\Component\HttpFoundation\Request;
+
+class DefaultFormatTestController {
+
+ public function content(Request $request) {
+ $format = $request->getRequestFormat();
+ return new CacheableResponse('format:' . $format, 200, ['Content-Type' => $request->getMimeType($format)]);
+ }
+
+}
diff --git a/core/tests/Drupal/FunctionalTests/Routing/DefaultFormatTest.php b/core/tests/Drupal/FunctionalTests/Routing/DefaultFormatTest.php
new file mode 100644
index 000000000000..bf695d658bae
--- /dev/null
+++ b/core/tests/Drupal/FunctionalTests/Routing/DefaultFormatTest.php
@@ -0,0 +1,32 @@
+<?php
+
+namespace Drupal\FunctionalTests\Routing;
+
+use Drupal\Tests\BrowserTestBase;
+
+/**
+ * @group routing
+ */
+class DefaultFormatTest extends BrowserTestBase {
+
+ /**
+ * {@inheritdoc}
+ */
+ public static $modules = ['system', 'default_format_test'];
+
+ public function testFoo() {
+ $this->drupalGet('/default_format_test/human');
+ $this->assertSame('format:html', $this->getSession()->getPage()->getContent());
+ $this->assertSame('MISS', $this->drupalGetHeader('X-Drupal-Cache'));
+
+ $this->drupalGet('/default_format_test/machine');
+ $this->assertSame('format:json', $this->getSession()->getPage()->getContent());
+ $this->assertSame('MISS', $this->drupalGetHeader('X-Drupal-Cache'));
+ }
+
+ public function testMultipleRoutesWithSameSingleFormat() {
+ $this->drupalGet('/default_format_test/machine');
+ $this->assertSame('format:json', $this->getSession()->getPage()->getContent());
+ }
+
+}
diff --git a/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php b/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
index 10d2df8b8ba2..7b5217d4c5ee 100644
--- a/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
+++ b/core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php
@@ -68,10 +68,10 @@ public function testFormatViaQueryParameter() {
*
* @covers ::getContentType
*/
- public function testUnknowContentTypeReturnsHtmlByDefault() {
+ public function testUnknowContentTypeReturnsNull() {
$request = new Request();
- $this->assertSame('html', $this->contentNegotiation->getContentType($request));
+ $this->assertNull($this->contentNegotiation->getContentType($request));
}
/**
@@ -83,7 +83,7 @@ public function testUnknowContentTypeButAjaxRequest() {
$request = new Request();
$request->headers->set('X-Requested-With', 'XMLHttpRequest');
- $this->assertSame('html', $this->contentNegotiation->getContentType($request));
+ $this->assertNull($this->contentNegotiation->getContentType($request));
}
/**
@@ -98,7 +98,7 @@ public function testHandle() {
$request->setFormat()->shouldNotBeCalled();
// Request format will be set with default format.
- $request->setRequestFormat('html')->shouldBeCalled();
+ $request->setRequestFormat()->shouldNotBeCalled();
// Some getContentType calls we don't really care about but have to mock.
$request_data = $this->prophesize(ParameterBag::class);
@@ -127,7 +127,7 @@ public function testSetFormat() {
$request->setFormat('david', 'geeky/david')->shouldBeCalled();
// Some calls we don't care about.
- $request->setRequestFormat('html')->shouldBeCalled();
+ $request->setRequestFormat()->shouldNotBeCalled();
$request_data = $this->prophesize(ParameterBag::class);
$request_data->get('ajax_iframe_upload', FALSE)->shouldBeCalled();
$request_mock = $request->reveal();
--
GitLab