From 0fcef5bd60f62ebdd4a10a2ad7035c22010c876c Mon Sep 17 00:00:00 2001 From: Nathaniel Catchpole Date: Thu, 28 Apr 2016 15:02:12 +0100 Subject: [PATCH] Issue #2702609 by Wim Leers, Berdir, Fabianx: Disable placeholdering (and BigPipe) on unsafe requests to help find forms as fast as possible (to allow EnforcedResponses to work) --- .../Core/Render/PlaceholderingRenderCache.php | 10 +++++ core/lib/Drupal/Core/Render/Renderer.php | 4 +- .../Render/Placeholder/BigPipeStrategy.php | 9 ++++- .../Placeholder/BigPipeStrategyTest.php | 23 +++++++----- .../Core/Render/RendererPlaceholdersTest.php | 37 +++++++++++++++++++ 5 files changed, 71 insertions(+), 12 deletions(-) diff --git a/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php b/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php index 98b58fc147f..42562834b83 100644 --- a/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php +++ b/core/lib/Drupal/Core/Render/PlaceholderingRenderCache.php @@ -92,6 +92,11 @@ class PlaceholderingRenderCache extends RenderCache { * {@inheritdoc} */ public function get(array $elements) { + // @todo remove this check when https://www.drupal.org/node/2367555 lands. + if (!$this->requestStack->getCurrentRequest()->isMethodSafe()) { + return FALSE; + } + // When rendering placeholders, special case auto-placeholdered elements: // avoid retrieving them from cache again, or rendering them again. if (isset($elements['#create_placeholder']) && $elements['#create_placeholder'] === FALSE) { @@ -121,6 +126,11 @@ class PlaceholderingRenderCache extends RenderCache { public function set(array &$elements, array $pre_bubbling_elements) { $result = parent::set($elements, $pre_bubbling_elements); + // @todo remove this check when https://www.drupal.org/node/2367555 lands. + if (!$this->requestStack->getCurrentRequest()->isMethodSafe()) { + return FALSE; + } + if ($this->placeholderGenerator->canCreatePlaceholder($pre_bubbling_elements) && $this->placeholderGenerator->shouldAutomaticallyPlaceholder($elements)) { // Overwrite $elements with a placeholder. The Renderer (which called this // method) will update the context with the bubbleable metadata of the diff --git a/core/lib/Drupal/Core/Render/Renderer.php b/core/lib/Drupal/Core/Render/Renderer.php index 3c13dfd3b28..09a1bf10322 100644 --- a/core/lib/Drupal/Core/Render/Renderer.php +++ b/core/lib/Drupal/Core/Render/Renderer.php @@ -338,7 +338,9 @@ class Renderer implements RendererInterface { // If instructed to create a placeholder, and a #lazy_builder callback is // present (without such a callback, it would be impossible to replace the // placeholder), replace the current element with a placeholder. - if (isset($elements['#create_placeholder']) && $elements['#create_placeholder'] === TRUE) { + // @todo remove the isMethodSafe() check when + // https://www.drupal.org/node/2367555 lands. + if (isset($elements['#create_placeholder']) && $elements['#create_placeholder'] === TRUE && $this->requestStack->getCurrentRequest()->isMethodSafe()) { if (!isset($elements['#lazy_builder'])) { throw new \LogicException('When #create_placeholder is set, a #lazy_builder callback must be present as well.'); } diff --git a/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php index 0edf3baa719..ae5cf1c4bb5 100644 --- a/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php +++ b/core/modules/big_pipe/src/Render/Placeholder/BigPipeStrategy.php @@ -105,12 +105,19 @@ class BigPipeStrategy implements PlaceholderStrategyInterface { * {@inheritdoc} */ public function processPlaceholders(array $placeholders) { + $request = $this->requestStack->getCurrentRequest(); + + // @todo remove this check when https://www.drupal.org/node/2367555 lands. + if (!$request->isMethodSafe()) { + return []; + } + // Routes can opt out from using the BigPipe HTML delivery technique. if ($this->routeMatch->getRouteObject()->getOption('_no_big_pipe')) { return []; } - if (!$this->sessionConfiguration->hasSession($this->requestStack->getCurrentRequest())) { + if (!$this->sessionConfiguration->hasSession($request)) { return []; } diff --git a/core/modules/big_pipe/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php b/core/modules/big_pipe/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php index 651f4343706..9806fbf6454 100644 --- a/core/modules/big_pipe/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php +++ b/core/modules/big_pipe/tests/src/Unit/Render/Placeholder/BigPipeStrategyTest.php @@ -22,8 +22,9 @@ class BigPipeStrategyTest extends UnitTestCase { * * @dataProvider placeholdersProvider */ - public function testProcessPlaceholders(array $placeholders, $route_match_has_no_big_pipe_option, $request_has_session, $request_has_big_pipe_nojs_cookie, array $expected_big_pipe_placeholders) { + public function testProcessPlaceholders(array $placeholders, $method, $route_match_has_no_big_pipe_option, $request_has_session, $request_has_big_pipe_nojs_cookie, array $expected_big_pipe_placeholders) { $request = new Request(); + $request->setMethod($method); if ($request_has_big_pipe_nojs_cookie) { $request->cookies->set(BigPipeStrategy::NOJS_COOKIE, 1); } @@ -45,7 +46,7 @@ class BigPipeStrategyTest extends UnitTestCase { $big_pipe_strategy = new BigPipeStrategy($session_configuration->reveal(), $request_stack->reveal(), $route_match->reveal()); $processed_placeholders = $big_pipe_strategy->processPlaceholders($placeholders); - if (!$route_match_has_no_big_pipe_option && $request_has_session) { + if ($request->isMethodSafe() && !$route_match_has_no_big_pipe_option && $request_has_session) { $this->assertSameSize($expected_big_pipe_placeholders, $processed_placeholders, 'BigPipe is able to deliver all placeholders.'); foreach (array_keys($placeholders) as $placeholder) { $this->assertSame($expected_big_pipe_placeholders[$placeholder], $processed_placeholders[$placeholder], "Verifying how BigPipeStrategy handles the placeholder '$placeholder'"); @@ -75,13 +76,13 @@ class BigPipeStrategyTest extends UnitTestCase { ]; return [ - '_no_big_pipe absent, no session, no-JS cookie absent' => [$placeholders, FALSE, FALSE, FALSE, []], - '_no_big_pipe absent, no session, no-JS cookie present' => [$placeholders, FALSE, FALSE, TRUE, []], - '_no_big_pipe present, no session, no-JS cookie absent' => [$placeholders, TRUE, FALSE, FALSE, []], - '_no_big_pipe present, no session, no-JS cookie present' => [$placeholders, TRUE, FALSE, TRUE, []], - '_no_big_pipe present, session, no-JS cookie absent' => [$placeholders, TRUE, TRUE, FALSE, []], - '_no_big_pipe present, session, no-JS cookie present' => [$placeholders, TRUE, TRUE, TRUE, []], - '_no_big_pipe absent, session, no-JS cookie absent: (JS-powered) BigPipe placeholder used for HTML placeholders' => [$placeholders, FALSE, TRUE, FALSE, [ + '_no_big_pipe absent, no session, no-JS cookie absent' => [$placeholders, 'GET', FALSE, FALSE, FALSE, []], + '_no_big_pipe absent, no session, no-JS cookie present' => [$placeholders, 'GET', FALSE, FALSE, TRUE, []], + '_no_big_pipe present, no session, no-JS cookie absent' => [$placeholders, 'GET', TRUE, FALSE, FALSE, []], + '_no_big_pipe present, no session, no-JS cookie present' => [$placeholders, 'GET', TRUE, FALSE, TRUE, []], + '_no_big_pipe present, session, no-JS cookie absent' => [$placeholders, 'GET', TRUE, TRUE, FALSE, []], + '_no_big_pipe present, session, no-JS cookie present' => [$placeholders, 'GET', TRUE, TRUE, TRUE, []], + '_no_big_pipe absent, session, no-JS cookie absent: (JS-powered) BigPipe placeholder used for HTML placeholders' => [$placeholders, 'GET', FALSE, TRUE, FALSE, [ $cases['html']->placeholder => $cases['html']->bigPipePlaceholderRenderArray, $cases['html_attribute_value']->placeholder => $cases['html_attribute_value']->bigPipeNoJsPlaceholderRenderArray, $cases['html_attribute_value_subset']->placeholder => $cases['html_attribute_value_subset']->bigPipeNoJsPlaceholderRenderArray, @@ -90,7 +91,8 @@ class BigPipeStrategyTest extends UnitTestCase { $cases['exception__lazy_builder']->placeholder => $cases['exception__lazy_builder']->bigPipePlaceholderRenderArray, $cases['exception__embedded_response']->placeholder => $cases['exception__embedded_response']->bigPipePlaceholderRenderArray, ]], - '_no_big_pipe absent, session, no-JS cookie present: no-JS BigPipe placeholder used for HTML placeholders' => [$placeholders, FALSE, TRUE, TRUE, [ + '_no_big_pipe absent, session, no-JS cookie absent: (JS-powered) BigPipe placeholder used for HTML placeholders — but unsafe method' => [$placeholders, 'POST', FALSE, TRUE, FALSE, []], + '_no_big_pipe absent, session, no-JS cookie present: no-JS BigPipe placeholder used for HTML placeholders' => [$placeholders, 'GET', FALSE, TRUE, TRUE, [ $cases['html']->placeholder => $cases['html']->bigPipeNoJsPlaceholderRenderArray, $cases['html_attribute_value']->placeholder => $cases['html_attribute_value']->bigPipeNoJsPlaceholderRenderArray, $cases['html_attribute_value_subset']->placeholder => $cases['html_attribute_value_subset']->bigPipeNoJsPlaceholderRenderArray, @@ -99,6 +101,7 @@ class BigPipeStrategyTest extends UnitTestCase { $cases['exception__lazy_builder']->placeholder => $cases['exception__lazy_builder']->bigPipeNoJsPlaceholderRenderArray, $cases['exception__embedded_response']->placeholder => $cases['exception__embedded_response']->bigPipeNoJsPlaceholderRenderArray, ]], + '_no_big_pipe absent, session, no-JS cookie present: no-JS BigPipe placeholder used for HTML placeholders — but unsafe method' => [$placeholders, 'POST', FALSE, TRUE, TRUE, []], ]; } diff --git a/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php index 5fd49a7ad24..4f5585c6b59 100644 --- a/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php +++ b/core/tests/Drupal/Tests/Core/Render/RendererPlaceholdersTest.php @@ -10,9 +10,12 @@ namespace Drupal\Tests\Core\Render; use Drupal\Component\Utility\Html; use Drupal\Core\Cache\Cache; use Drupal\Core\Render\Markup; +use Drupal\Core\Render\RenderContext; /** * @coversDefaultClass \Drupal\Core\Render\Renderer + * @covers \Drupal\Core\Render\RenderCache + * @covers \Drupal\Core\Render\PlaceholderingRenderCache * @group Render */ class RendererPlaceholdersTest extends RendererTestBase { @@ -781,6 +784,40 @@ class RendererPlaceholdersTest extends RendererTestBase { $this->assertPlaceholderRenderCache(FALSE, [], []); } + /** + * @covers ::render + * @covers ::doRender + * @covers \Drupal\Core\Render\RenderCache::get + * @covers \Drupal\Core\Render\PlaceholderingRenderCache::get + * @covers \Drupal\Core\Render\PlaceholderingRenderCache::set + * @covers ::replacePlaceholders + * + * @dataProvider providerPlaceholders + */ + public function testPlaceholderingDisabledForPostRequests($test_element, $args) { + $this->setUpUnusedCache(); + $this->setUpRequest('POST'); + + $element = $test_element; + + // Render without replacing placeholders, to allow this test to see which + // #attached[placeholders] there are, if any. + $this->renderer->executeInRenderContext(new RenderContext(), function () use (&$element) { + return $this->renderer->render($element); + }); + // Only test cases where the placeholders have been specified manually are + // allowed to have placeholders. This means that of the different situations + // listed in providerPlaceholders(), only type B can have attached + // placeholders. Everything else, whether: + // 1. manual placeholdering + // 2. automatic placeholdering via already-present cacheability metadata + // 3. automatic placeholdering via bubbled cacheability metadata + // All three of those should NOT result in placeholders. + if (!isset($test_element['#attached']['placeholders'])) { + $this->assertFalse(isset($element['#attached']['placeholders']), 'No placeholders created.'); + } + } + /** * Tests a placeholder that adds another placeholder. *