From 4997192be0c16d2d597718bdb384951580083bb0 Mon Sep 17 00:00:00 2001 From: Lee Rowlands Date: Wed, 11 Oct 2017 15:52:34 +1000 Subject: [PATCH] Issue #2449143 by damiankloip, tedbow, Wim Leers, dawehner, Pavan B S, Tybor, effulgentsia, larowlan: REST views specify HTML as a possible request format, so if there is a "regular" HTML view on the same path, it will serve JSON --- .../src/Plugin/views/display/RestExport.php | 94 ++++++++++++------- ...views.view.test_serializer_shared_path.yml | 70 ++++++++++++++ .../Functional/Views/StyleSerializerTest.php | 77 +++++++++------ .../rest/tests/src/Unit/CollectRoutesTest.php | 2 + core/modules/simpletest/src/WebTestBase.php | 2 +- .../views/src/Plugin/views/display/Page.php | 12 +++ .../tests/src/Functional/Wizard/BasicTest.php | 5 +- core/modules/views_ui/src/ViewEditForm.php | 23 +++-- core/modules/views_ui/src/ViewListBuilder.php | 15 ++- 9 files changed, 225 insertions(+), 75 deletions(-) create mode 100644 core/modules/rest/tests/modules/rest_test_views/test_views/views.view.test_serializer_shared_path.yml diff --git a/core/modules/rest/src/Plugin/views/display/RestExport.php b/core/modules/rest/src/Plugin/views/display/RestExport.php index 7aeb3fc6192..81e0b7dc538 100644 --- a/core/modules/rest/src/Plugin/views/display/RestExport.php +++ b/core/modules/rest/src/Plugin/views/display/RestExport.php @@ -70,7 +70,7 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac * * @var string */ - protected $mimeType; + protected $mimeType = 'application/json'; /** * The renderer. @@ -93,6 +93,13 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac */ protected $authenticationProviders; + /** + * The serialization format providers, keyed by format. + * + * @var string[] + */ + protected $formatProviders; + /** * Constructs a RestExport object. * @@ -110,12 +117,15 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac * The renderer. * @param string[] $authentication_providers * The authentication providers, keyed by ID. + * @param string[] $serializer_format_providers + * The serialization format providers, keyed by format. */ - public function __construct(array $configuration, $plugin_id, $plugin_definition, RouteProviderInterface $route_provider, StateInterface $state, RendererInterface $renderer, array $authentication_providers) { + public function __construct(array $configuration, $plugin_id, $plugin_definition, RouteProviderInterface $route_provider, StateInterface $state, RendererInterface $renderer, array $authentication_providers, array $serializer_format_providers) { parent::__construct($configuration, $plugin_id, $plugin_definition, $route_provider, $state); $this->renderer = $renderer; $this->authenticationProviders = $authentication_providers; + $this->formatProviders = $serializer_format_providers; } /** @@ -129,8 +139,8 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac $container->get('router.route_provider'), $container->get('state'), $container->get('renderer'), - $container->getParameter('authentication_providers') - + $container->getParameter('authentication_providers'), + $container->getParameter('serializer.format_providers') ); } /** @@ -139,21 +149,22 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac public function initDisplay(ViewExecutable $view, array &$display, array &$options = NULL) { parent::initDisplay($view, $display, $options); - $request_content_type = $this->view->getRequest()->getRequestFormat(); - // Only use the requested content type if it's not 'html'. If it is then - // default to 'json' to aid debugging. - // @todo Remove the need for this when we have better content negotiation. - if ($request_content_type != 'html') { - $this->setContentType($request_content_type); - } - // If the requested content type is 'html' and the default 'json' is not - // selected as a format option in the view display, fallback to the first - // format in the array. - elseif (!empty($options['style']['options']['formats']) && !isset($options['style']['options']['formats'][$this->getContentType()])) { - $this->setContentType(reset($options['style']['options']['formats'])); + // If the default 'json' format is not selected as a format option in the + // view display, fallback to the first format available for the default. + if (!empty($options['style']['options']['formats']) && !isset($options['style']['options']['formats'][$this->getContentType()])) { + $default_format = reset($options['style']['options']['formats']); + $this->setContentType($default_format); } - $this->setMimeType($this->view->getRequest()->getMimeType($this->contentType)); + // Only use the requested content type if it's not 'html'. This allows + // still falling back to the default for things like views preview. + $request_content_type = $this->view->getRequest()->getRequestFormat(); + + if ($request_content_type !== 'html') { + $this->setContentType($request_content_type); + } + + $this->setMimeType($this->view->getRequest()->getMimeType($this->getContentType())); } /** @@ -327,17 +338,21 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac if ($route = $collection->get("view.$view_id.$display_id")) { $style_plugin = $this->getPlugin('style'); - // REST exports should only respond to get methods. + + // REST exports should only respond to GET methods. $route->setMethods(['GET']); - // Format as a string using pipes as a delimiter. - if ($formats = $style_plugin->getFormats()) { - // Allow a REST Export View to be returned with an HTML-only accept - // format. That allows browsers or other non-compliant systems to access - // the view, as it is unlikely to have a conflicting HTML representation - // anyway. - $route->setRequirement('_format', implode('|', $formats + ['html'])); + $formats = $style_plugin->getFormats(); + + // If there are no configured formats, add all formats that serialization + // is known to support. + if (!$formats) { + $formats = $this->getFormatOptions(); } + + // Format as a string using pipes as a delimiter. + $route->setRequirement('_format', implode('|', $formats)); + // Add authentication to the route if it was set. If no authentication was // set, the default authentication will be used, which is cookie based by // default. @@ -421,15 +436,15 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac $build['#suffix'] = ''; unset($build['#markup']); } - elseif ($this->view->getRequest()->getFormat($this->view->element['#content_type']) !== 'html') { - // This display plugin is primarily for returning non-HTML formats. - // However, we still invoke the renderer to collect cacheability metadata. - // Because the renderer is designed for HTML rendering, it filters - // #markup for XSS unless it is already known to be safe, but that filter - // only works for HTML. Therefore, we mark the contents as safe to bypass - // the filter. So long as we are returning this in a non-HTML response - // (checked above), this is safe, because an XSS attack only works when - // executed by an HTML agent. + else { + // This display plugin is for returning non-HTML formats. However, we + // still invoke the renderer to collect cacheability metadata. Because the + // renderer is designed for HTML rendering, it filters #markup for XSS + // unless it is already known to be safe, but that filter only works for + // HTML. Therefore, we mark the contents as safe to bypass the filter. So + // long as we are returning this in a non-HTML response, + // this is safe, because an XSS attack only works when executed by an HTML + // agent. // @todo Decide how to support non-HTML in the render API in // https://www.drupal.org/node/2501313. $build['#markup'] = ViewsRenderPipelineMarkup::create($build['#markup']); @@ -465,4 +480,15 @@ class RestExport extends PathPluginBase implements ResponseDisplayPluginInterfac return $dependencies; } + /** + * Returns an array of format options. + * + * @return string[] + * An array of format options. Both key and value are the same. + */ + protected function getFormatOptions() { + $formats = array_keys($this->formatProviders); + return array_combine($formats, $formats); + } + } diff --git a/core/modules/rest/tests/modules/rest_test_views/test_views/views.view.test_serializer_shared_path.yml b/core/modules/rest/tests/modules/rest_test_views/test_views/views.view.test_serializer_shared_path.yml new file mode 100644 index 00000000000..bc4f6ef98b1 --- /dev/null +++ b/core/modules/rest/tests/modules/rest_test_views/test_views/views.view.test_serializer_shared_path.yml @@ -0,0 +1,70 @@ +langcode: en +status: true +dependencies: + module: + - rest + - user +id: test_serializer_shared_path +label: 'Test serializer shared path' +module: rest +description: '' +tag: '' +base_table: entity_test +base_field: id +core: 8.x +display: + default: + display_plugin: default + id: default + display_title: Master + position: null + display_options: + access: + type: perm + options: + perm: 'access content' + cache: + type: tag + query: + type: views_query + exposed_form: + type: basic + style: + type: serializer + row: + type: data_entity + sorts: + id: + id: standard + table: entity_test + field: id + order: DESC + plugin_id: date + entity_type: entity_test + entity_field: id + title: 'Test serialize' + arguments: { } + rest_export_1: + display_plugin: rest_export + id: rest_export_1 + display_title: serializer + position: null + display_options: + defaults: + access: false + path: test/serialize/shared + page_1: + display_plugin: page + id: page_1 + display_title: page + position: null + display_options: + defaults: + access: false + style: false + row: false + style: + type: default + row: + type: entity:entity_test + path: test/serialize/shared diff --git a/core/modules/rest/tests/src/Functional/Views/StyleSerializerTest.php b/core/modules/rest/tests/src/Functional/Views/StyleSerializerTest.php index a1545870577..94af5427e60 100644 --- a/core/modules/rest/tests/src/Functional/Views/StyleSerializerTest.php +++ b/core/modules/rest/tests/src/Functional/Views/StyleSerializerTest.php @@ -43,7 +43,7 @@ class StyleSerializerTest extends ViewTestBase { * * @var array */ - public static $testViews = ['test_serializer_display_field', 'test_serializer_display_entity', 'test_serializer_display_entity_translated', 'test_serializer_node_display_field', 'test_serializer_node_exposed_filter']; + public static $testViews = ['test_serializer_display_field', 'test_serializer_display_entity', 'test_serializer_display_entity_translated', 'test_serializer_node_display_field', 'test_serializer_node_exposed_filter', 'test_serializer_shared_path']; /** * A user with administrative privileges to look at test entity and configure views. @@ -85,6 +85,9 @@ class StyleSerializerTest extends ViewTestBase { $url = $this->buildUrl('test/serialize/auth_with_perm'); $response = \Drupal::httpClient()->get($url, [ 'auth' => [$this->adminUser->getUsername(), $this->adminUser->pass_raw], + 'query' => [ + '_format' => 'json', + ], ]); // Ensure that any changes to variables in the other thread are picked up. @@ -134,7 +137,7 @@ class StyleSerializerTest extends ViewTestBase { $this->assertIdentical($actual_json, (string) drupal_render_root($output), 'The expected JSON preview output was found.'); // Test a 403 callback. - $this->drupalGet('test/serialize/denied'); + $this->drupalGet('test/serialize/denied', ['query' => ['_format' => 'json']]); $this->assertResponse(403); // Test the entity rows. @@ -169,7 +172,7 @@ class StyleSerializerTest extends ViewTestBase { $this->assertIdentical($actual_json, $expected, 'The expected HAL output was found.'); $this->assertCacheTags($expected_cache_tags); - // Change the default format to xml. + // Change the format to xml. $view->setDisplay('rest_export_1'); $view->getDisplay()->setOption('style', [ 'type' => 'serializer', @@ -207,6 +210,28 @@ class StyleSerializerTest extends ViewTestBase { $this->assertSame(trim($expected), $actual_xml); } + /** + * Verifies REST export views work on the same path as a page display. + */ + public function testSharedPagePath() { + // Test with no format as well as html explicitly. + $this->drupalGet('test/serialize/shared'); + $this->assertResponse(200); + $this->assertHeader('content-type', 'text/html; charset=UTF-8'); + + $this->drupalGet('test/serialize/shared', ['query' => ['_format' => 'html']]); + $this->assertResponse(200); + $this->assertHeader('content-type', 'text/html; charset=UTF-8'); + + $this->drupalGet('test/serialize/shared', ['query' => ['_format' => 'json']]); + $this->assertResponse(200); + $this->assertHeader('content-type', 'application/json'); + + $this->drupalGet('test/serialize/shared', ['query' => ['_format' => 'xml']]); + $this->assertResponse(200); + $this->assertHeader('content-type', 'text/xml; charset=UTF-8'); + } + /** * Verifies site maintenance mode functionality. */ @@ -336,6 +361,11 @@ class StyleSerializerTest extends ViewTestBase { $style_options = 'admin/structure/views/nojs/display/test_serializer_display_field/rest_export_1/style_options'; + // Test with no format. + $this->drupalGet('test/serialize/field'); + $this->assertHeader('content-type', 'text/html; charset=UTF-8'); + $this->assertResponse(406, 'A 406 response was returned when no format was requested.'); + // Select only 'xml' as an accepted format. $this->drupalPostForm($style_options, ['style_options[formats][xml]' => 'xml'], t('Apply')); $this->drupalPostForm(NULL, [], t('Save')); @@ -353,37 +383,27 @@ class StyleSerializerTest extends ViewTestBase { $this->drupalPostForm($style_options, ['style_options[formats][json]' => 'json'], t('Apply')); $this->drupalPostForm(NULL, [], t('Save')); - // Should return a 200. - // @todo This should be fixed when we have better content negotiation. - $this->drupalGet('test/serialize/field'); - $this->assertHeader('content-type', 'application/json'); - $this->assertResponse(200, 'A 200 response was returned when any format was requested.'); - - // Should return a 200. Emulates a sample Firefox header. + // Should return a 406. Emulates a sample Firefox header. $this->drupalGet('test/serialize/field', [], ['Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8']); - $this->assertHeader('content-type', 'application/json'); - $this->assertResponse(200, 'A 200 response was returned when a browser accept header was requested.'); + $this->assertHeader('content-type', 'text/html; charset=UTF-8'); + $this->assertResponse(406, 'A 406 response was returned when a browser accept header was requested.'); + + // Should return a 406. + $this->drupalGet('test/serialize/field', ['query' => ['_format' => 'html']]); + $this->assertHeader('content-type', 'text/html; charset=UTF-8'); + $this->assertResponse(406, 'A 406 response was returned when HTML was requested.'); // Should return a 200. $this->drupalGet('test/serialize/field', ['query' => ['_format' => 'json']]); $this->assertHeader('content-type', 'application/json'); $this->assertResponse(200, 'A 200 response was returned when JSON was requested.'); - $headers = $this->drupalGetHeaders(); - $this->assertEqual($headers['Content-Type'], ['application/json'], 'The header Content-type is correct.'); + // Should return a 200. $this->drupalGet('test/serialize/field', ['query' => ['_format' => 'xml']]); $this->assertHeader('content-type', 'text/xml; charset=UTF-8'); $this->assertResponse(200, 'A 200 response was returned when XML was requested'); - $headers = $this->drupalGetHeaders(); - $this->assertSame(['text/xml; charset=UTF-8'], $headers['Content-Type']); - // Should return a 406. - $this->drupalGet('test/serialize/field', ['query' => ['_format' => 'html']]); - // We want to show the first format by default, see - // \Drupal\rest\Plugin\views\style\Serializer::render. - $this->assertHeader('content-type', 'application/json'); - $this->assertResponse(200, 'A 200 response was returned when HTML was requested.'); - // Now configure now format, so all of them should be allowed. + // Now configure no format, so both serialization formats should be allowed. $this->drupalPostForm($style_options, ['style_options[formats][json]' => '0', 'style_options[formats][xml]' => '0'], t('Apply')); // Should return a 200. @@ -394,12 +414,11 @@ class StyleSerializerTest extends ViewTestBase { $this->drupalGet('test/serialize/field', ['query' => ['_format' => 'xml']]); $this->assertHeader('content-type', 'text/xml; charset=UTF-8'); $this->assertResponse(200, 'A 200 response was returned when XML was requested'); - // Should return a 200. + + // Should return a 406 for HTML still. $this->drupalGet('test/serialize/field', ['query' => ['_format' => 'html']]); - // We want to show the first format by default, see - // \Drupal\rest\Plugin\views\style\Serializer::render. - $this->assertHeader('content-type', 'application/json'); - $this->assertResponse(200, 'A 200 response was returned when HTML was requested.'); + $this->assertHeader('content-type', 'text/html; charset=UTF-8'); + $this->assertResponse(406, 'A 406 response was returned when HTML was requested.'); } /** @@ -598,7 +617,7 @@ class StyleSerializerTest extends ViewTestBase { // Check if we receive the expected result. $result = $this->xpath('//div[@id="views-live-preview"]/pre'); $json_preview = $result[0]->getText(); - $this->assertSame($json_preview, $this->drupalGet('test/serialize/field'), 'The expected JSON preview output was found.'); + $this->assertSame($json_preview, $this->drupalGet('test/serialize/field', ['query' => ['_format' => 'json']]), 'The expected JSON preview output was found.'); } /** diff --git a/core/modules/rest/tests/src/Unit/CollectRoutesTest.php b/core/modules/rest/tests/src/Unit/CollectRoutesTest.php index 3b1c1f7f398..e171e047c8e 100644 --- a/core/modules/rest/tests/src/Unit/CollectRoutesTest.php +++ b/core/modules/rest/tests/src/Unit/CollectRoutesTest.php @@ -84,6 +84,8 @@ class CollectRoutesTest extends UnitTestCase { ->method('getSortedProviders') ->will($this->returnValue(['basic_auth' => 'data', 'cookie' => 'data'])); + $container->setParameter('serializer.format_providers', ['json']); + \Drupal::setContainer($container); $this->restExport = RestExport::create($container, [], "test_routes", []); diff --git a/core/modules/simpletest/src/WebTestBase.php b/core/modules/simpletest/src/WebTestBase.php index 569103224e6..b5874b5ae65 100644 --- a/core/modules/simpletest/src/WebTestBase.php +++ b/core/modules/simpletest/src/WebTestBase.php @@ -817,7 +817,7 @@ abstract class WebTestBase extends TestBase { * The result of the request. */ protected function drupalGetWithFormat($path, $format, array $options = [], array $headers = []) { - $options += ['query' => ['_format' => $format]]; + $options = array_merge_recursive(['query' => ['_format' => $format]], $options); return $this->drupalGet($path, $options, $headers); } diff --git a/core/modules/views/src/Plugin/views/display/Page.php b/core/modules/views/src/Plugin/views/display/Page.php index aca0683f3c5..baf1434449e 100644 --- a/core/modules/views/src/Plugin/views/display/Page.php +++ b/core/modules/views/src/Plugin/views/display/Page.php @@ -84,6 +84,18 @@ class Page extends PathPluginBase { ); } + /** + * {@inheritdoc} + */ + protected function getRoute($view_id, $display_id) { + $route = parent::getRoute($view_id, $display_id); + + // Explicitly set HTML as the format for Page displays. + $route->setRequirement('_format', 'html'); + + return $route; + } + /** * Sets the current page views render array. * diff --git a/core/modules/views/tests/src/Functional/Wizard/BasicTest.php b/core/modules/views/tests/src/Functional/Wizard/BasicTest.php index eb2ddbbb677..d26968e492a 100644 --- a/core/modules/views/tests/src/Functional/Wizard/BasicTest.php +++ b/core/modules/views/tests/src/Functional/Wizard/BasicTest.php @@ -163,8 +163,9 @@ class BasicTest extends WizardTestBase { $this->drupalPostForm('admin/structure/views/add', $view4, t('Save and edit')); $this->assertRaw(t('The view %view has been saved.', ['%view' => $view4['label']])); - // Check that the REST export path works. - $this->drupalGet($view4['rest_export[path]']); + // Check that the REST export path works. JSON will work, as all core + // formats will be allowed. JSON and XML by default. + $this->drupalGet($view4['rest_export[path]'], ['query' => ['_format' => 'json']]); $this->assertResponse(200); $data = Json::decode($this->getSession()->getPage()->getContent()); $this->assertEqual(count($data), 1, 'Only the node of type page is exported.'); diff --git a/core/modules/views_ui/src/ViewEditForm.php b/core/modules/views_ui/src/ViewEditForm.php index 7066476c548..1781e3312e4 100644 --- a/core/modules/views_ui/src/ViewEditForm.php +++ b/core/modules/views_ui/src/ViewEditForm.php @@ -15,6 +15,7 @@ use Drupal\user\SharedTempStoreFactory; use Drupal\views\Views; use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\HttpFoundation\RequestStack; +use Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException; /** * Form controller for the Views edit form. @@ -417,15 +418,25 @@ class ViewEditForm extends ViewFormBase { // path. elseif ($view->status() && $view->getExecutable()->displayHandlers->get($display['id'])->hasPath()) { $path = $view->getExecutable()->displayHandlers->get($display['id'])->getPath(); + if ($path && (strpos($path, '%') === FALSE)) { - if (!parse_url($path, PHP_URL_SCHEME)) { - // @todo Views should expect and store a leading /. See: - // https://www.drupal.org/node/2423913 - $url = Url::fromUserInput('/' . ltrim($path, '/')); + // Wrap this in a try/catch as trying to generate links to some + // routes may throw a NotAcceptableHttpException if they do not + // respond to HTML, such as RESTExports. + try { + if (!parse_url($path, PHP_URL_SCHEME)) { + // @todo Views should expect and store a leading /. See: + // https://www.drupal.org/node/2423913 + $url = Url::fromUserInput('/' . ltrim($path, '/')); + } + else { + $url = Url::fromUri("base:$path"); + } } - else { - $url = Url::fromUri("base:$path"); + catch (NotAcceptableHttpException $e) { + $url = '/' . $path; } + $build['top']['actions']['path'] = [ '#type' => 'link', '#title' => $this->t('View @display_title', ['@display_title' => $display_title]), diff --git a/core/modules/views_ui/src/ViewListBuilder.php b/core/modules/views_ui/src/ViewListBuilder.php index 42166be1a5a..628408e997f 100644 --- a/core/modules/views_ui/src/ViewListBuilder.php +++ b/core/modules/views_ui/src/ViewListBuilder.php @@ -9,6 +9,7 @@ use Drupal\Core\Entity\EntityStorageInterface; use Drupal\Core\Entity\EntityTypeInterface; use Drupal\Core\Url; use Symfony\Component\DependencyInjection\ContainerInterface; +use Symfony\Component\HttpKernel\Exception\NotAcceptableHttpException; /** * Defines a class to build a listing of view entities. @@ -255,9 +256,17 @@ class ViewListBuilder extends ConfigEntityListBuilder { if ($display->hasPath()) { $path = $display->getPath(); if ($view->status() && strpos($path, '%') === FALSE) { - // @todo Views should expect and store a leading /. See: - // https://www.drupal.org/node/2423913 - $rendered_path = \Drupal::l('/' . $path, Url::fromUserInput('/' . $path)); + // Wrap this in a try/catch as trying to generate links to some + // routes may throw a NotAcceptableHttpException if they do not + // respond to HTML, such as RESTExports. + try { + // @todo Views should expect and store a leading /. See: + // https://www.drupal.org/node/2423913 + $rendered_path = \Drupal::l('/' . $path, Url::fromUserInput('/' . $path)); + } + catch (NotAcceptableHttpException $e) { + $rendered_path = '/' . $path; + } } else { $rendered_path = '/' . $path;