diff --git a/core/core.services.yml b/core/core.services.yml index 6316b1f25c2a4243d080e1703f14026901315be2..655bbe94e6e7a03fa325242ab3236c0ba6c913ef 100644 --- a/core/core.services.yml +++ b/core/core.services.yml @@ -182,7 +182,7 @@ services: arguments: ['@request_stack', '@url_generator'] form_cache: class: Drupal\Core\Form\FormCache - arguments: ['@keyvalue.expirable', '@module_handler', '@current_user', '@csrf_token'] + arguments: ['@keyvalue.expirable', '@module_handler', '@current_user', '@csrf_token', '@logger.channel.form', '@config.factory', '@request_stack', '@page_cache_request_policy'] public: false # Private to form_builder keyvalue: class: Drupal\Core\KeyValueStore\KeyValueFactory diff --git a/core/lib/Drupal/Core/Ajax/UpdateBuildIdCommand.php b/core/lib/Drupal/Core/Ajax/UpdateBuildIdCommand.php new file mode 100644 index 0000000000000000000000000000000000000000..2ce74db33c06b8cdc9e078119ac86b1d418c7197 --- /dev/null +++ b/core/lib/Drupal/Core/Ajax/UpdateBuildIdCommand.php @@ -0,0 +1,65 @@ +old = $old; + $this->new = $new; + } + + /** + * {@inheritdoc} + */ + public function render() { + return [ + 'command' => 'update_build_id', + 'old' => $this->old, + 'new' => $this->new, + ]; + } + +} diff --git a/core/lib/Drupal/Core/Form/FormBuilder.php b/core/lib/Drupal/Core/Form/FormBuilder.php index ebab63f5c9000dc118133a23f44df63aaf5741b4..dd2bcd82374a603502485a0f4aa1da91c2475a14 100644 --- a/core/lib/Drupal/Core/Form/FormBuilder.php +++ b/core/lib/Drupal/Core/Form/FormBuilder.php @@ -271,8 +271,8 @@ public function buildForm($form_id, FormStateInterface &$form_state) { exit; } - // If this was a successful submission of a single-step form or the last step - // of a multi-step form, then self::processForm() issued a redirect to + // If this was a successful submission of a single-step form or the last + // step of a multi-step form, then self::processForm() issued a redirect to // another page, or back to this page, but as a new request. Therefore, if // we're here, it means that this is either a form being viewed initially // before any user input, or there was a validation error requiring the form @@ -291,17 +291,25 @@ public function rebuildForm($form_id, FormStateInterface &$form_state, $old_form $form_state->setCached(); // If only parts of the form will be returned to the browser (e.g., Ajax or - // RIA clients), re-use the old #build_id to not require client-side code to - // manually update the hidden 'build_id' input element. + // RIA clients), or if the form already had a new build ID regenerated when + // it was retrieved from the form cache, reuse the existing #build_id. // Otherwise, a new #build_id is generated, to not clobber the previous // build's data in the form cache; also allowing the user to go back to an // earlier build, make changes, and re-submit. // @see self::prepareForm() $rebuild_info = $form_state->getRebuildInfo(); - if (isset($old_form['#build_id']) && !empty($rebuild_info['copy']['#build_id'])) { + $enforce_old_build_id = isset($old_form['#build_id']) && !empty($rebuild_info['copy']['#build_id']); + $old_form_is_mutable_copy = isset($old_form['#build_id_old']); + if ($enforce_old_build_id || $old_form_is_mutable_copy) { $form['#build_id'] = $old_form['#build_id']; + if ($old_form_is_mutable_copy) { + $form['#build_id_old'] = $old_form['#build_id_old']; + } } else { + if (isset($old_form['#build_id'])) { + $form['#build_id_old'] = $old_form['#build_id']; + } $form['#build_id'] = 'form-' . Crypt::randomBytesBase64(); } @@ -486,17 +494,17 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) { return; } - // If $form_state->isRebuilding() has been set and input has been processed - // without validation errors, we are in a multi-step workflow that is not - // yet complete. A new $form needs to be constructed based on the changes - // made to $form_state during this request. Normally, a submit handler - // sets $form_state->isRebuilding() if a fully executed form requires - // another step. However, for forms that have not been fully executed - // (e.g., Ajax submissions triggered by non-buttons), there is no submit - // handler to set $form_state->isRebuilding(). It would not make sense to - // redisplay the identical form without an error for the user to correct, - // so we also rebuild error-free non-executed forms, regardless of - // $form_state->isRebuilding(). + // If $form_state->isRebuilding() has been set and input has been + // processed without validation errors, we are in a multi-step workflow + // that is not yet complete. A new $form needs to be constructed based on + // the changes made to $form_state during this request. Normally, a submit + // handler sets $form_state->isRebuilding() if a fully executed form + // requires another step. However, for forms that have not been fully + // executed (e.g., Ajax submissions triggered by non-buttons), there is no + // submit handler to set $form_state->isRebuilding(). It would not make + // sense to redisplay the identical form without an error for the user to + // correct, so we also rebuild error-free non-executed forms, regardless + // of $form_state->isRebuilding(). // @todo Simplify this logic; considering Ajax and non-HTML front-ends, // along with element-level #submit properties, it makes no sense to // have divergent form execution based on whether the triggering element diff --git a/core/lib/Drupal/Core/Form/FormCache.php b/core/lib/Drupal/Core/Form/FormCache.php index 93da48c457028da6faa6f51ba2fa692e5814dcac..c399f1ad07ddffed7c70d8883878e88220734a5c 100644 --- a/core/lib/Drupal/Core/Form/FormCache.php +++ b/core/lib/Drupal/Core/Form/FormCache.php @@ -7,11 +7,16 @@ namespace Drupal\Core\Form; +use Drupal\Component\Utility\Crypt; use Drupal\Component\Utility\SafeMarkup; use Drupal\Core\Access\CsrfTokenGenerator; +use Drupal\Core\Config\ConfigFactoryInterface; use Drupal\Core\Extension\ModuleHandlerInterface; use Drupal\Core\KeyValueStore\KeyValueExpirableFactoryInterface; +use Drupal\Core\PageCache\RequestPolicyInterface; use Drupal\Core\Session\AccountInterface; +use Psr\Log\LoggerInterface; +use Symfony\Component\HttpFoundation\RequestStack; /** * Encapsulates the caching of a form and its form state. @@ -48,6 +53,34 @@ class FormCache implements FormCacheInterface { */ protected $moduleHandler; + /** + * Logger channel. + * + * @var \Drupal\Core\Logger\LoggerChannelInterface + */ + protected $logger; + + /** + * The config factory. + * + * @var \Drupal\Core\Config\ConfigFactoryInterface + */ + protected $configFactory; + + /** + * The request stack. + * + * @var \Symfony\Component\HttpFoundation\RequestStack + */ + protected $requestStack; + + /** + * A policy rule determining the cacheability of a request. + * + * @var \Drupal\Core\PageCache\RequestPolicyInterface + */ + protected $requestPolicy; + /** * Constructs a new FormCache. * @@ -60,12 +93,24 @@ class FormCache implements FormCacheInterface { * The current user. * @param \Drupal\Core\Access\CsrfTokenGenerator $csrf_token * The CSRF token generator. + * @param \Psr\Log\LoggerInterface $logger + * A logger instance. + * @param \Drupal\Core\Config\ConfigFactoryInterface $config_factory + * The configuration factory. + * @param \Symfony\Component\HttpFoundation\RequestStack $request_stack + * The request stack. + * @param \Drupal\Core\PageCache\RequestPolicyInterface $request_policy + * A policy rule determining the cacheability of a request. */ - public function __construct(KeyValueExpirableFactoryInterface $key_value_expirable_factory, ModuleHandlerInterface $module_handler, AccountInterface $current_user, CsrfTokenGenerator $csrf_token = NULL) { + public function __construct(KeyValueExpirableFactoryInterface $key_value_expirable_factory, ModuleHandlerInterface $module_handler, AccountInterface $current_user, CsrfTokenGenerator $csrf_token, LoggerInterface $logger, ConfigFactoryInterface $config_factory, RequestStack $request_stack, RequestPolicyInterface $request_policy) { $this->keyValueExpirableFactory = $key_value_expirable_factory; $this->moduleHandler = $module_handler; $this->currentUser = $current_user; + $this->logger = $logger; + $this->configFactory = $config_factory; $this->csrfToken = $csrf_token; + $this->requestStack = $request_stack; + $this->requestPolicy = $request_policy; } /** @@ -75,6 +120,18 @@ public function getCache($form_build_id, FormStateInterface $form_state) { if ($form = $this->keyValueExpirableFactory->get('form')->get($form_build_id)) { if ((isset($form['#cache_token']) && $this->csrfToken->validate($form['#cache_token'])) || (!isset($form['#cache_token']) && $this->currentUser->isAnonymous())) { $this->loadCachedFormState($form_build_id, $form_state); + + // Generate a new #build_id if the cached form was rendered on a + // cacheable page. + $build_info = $form_state->getBuildInfo(); + if (!empty($build_info['immutable'])) { + $form['#build_id_old'] = $form['#build_id']; + $form['#build_id'] = 'form-' . Crypt::randomBytesBase64(); + $form['form_build_id']['#value'] = $form['#build_id']; + $form['form_build_id']['#id'] = $form['#build_id']; + unset($build_info['immutable']); + $form_state->setBuildInfo($build_info); + } return $form; } } @@ -106,8 +163,8 @@ protected function loadCachedFormState($form_build_id, FormStateInterface $form_ require_once DRUPAL_ROOT . '/' . $file; } } - // Retrieve the list of previously known safe strings and store it - // for this request. + // Retrieve the list of previously known safe strings and store it for + // this request. // @todo Ensure we are not storing an excessively large string list // in: https://www.drupal.org/node/2295823 $build_info += ['safe_strings' => []]; @@ -124,15 +181,29 @@ public function setCache($form_build_id, $form, FormStateInterface $form_state) // 6 hours cache life time for forms should be plenty. $expire = 21600; + // Ensure that the form build_id embedded in the form structure is the same + // as the one passed in as a parameter. This is an additional safety measure + // to prevent legacy code operating directly with form_get_cache and + // form_set_cache from accidentally overwriting immutable form state. + if (isset($form['#build_id']) && $form['#build_id'] != $form_build_id) { + $this->logger->error('Form build-id mismatch detected while attempting to store a form in the cache.'); + return; + } + // Cache form structure. if (isset($form)) { if ($this->currentUser->isAuthenticated()) { $form['#cache_token'] = $this->csrfToken->get(); } + unset($form['#build_id_old']); $this->keyValueExpirableFactory->get('form')->setWithExpire($form_build_id, $form, $expire); } // Cache form state. + if ($this->configFactory->get('system.performance')->get('cache.page.use_internal') && $this->isPageCacheable()) { + $form_state->addBuildInfo('immutable', TRUE); + } + // Store the known list of safe strings for form re-use. // @todo Ensure we are not storing an excessively large string list in: // https://www.drupal.org/node/2295823 @@ -143,4 +214,14 @@ public function setCache($form_build_id, $form, FormStateInterface $form_state) } } + /** + * Checks if the page is cacheable. + * + * @return bool + * TRUE is the page is cacheable, FALSE if not. + */ + protected function isPageCacheable() { + return ($this->requestPolicy->check($this->requestStack->getCurrentRequest()) === RequestPolicyInterface::ALLOW); + } + } diff --git a/core/lib/Drupal/Core/Form/FormState.php b/core/lib/Drupal/Core/Form/FormState.php index 1c469d03cb3d0ce7cf1cf0a928b359ba3ad8924a..09db977d9b9ccc757cbdacaaec709bdd4f22ee7f 100644 --- a/core/lib/Drupal/Core/Form/FormState.php +++ b/core/lib/Drupal/Core/Form/FormState.php @@ -57,6 +57,12 @@ class FormState implements FormStateInterface { * processed. * - base_form_id: Identification for a base form, as declared in the form * class's \Drupal\Core\Form\BaseFormIdInterface::getBaseFormId() method. + * - immutable: If this flag is set to TRUE, a new form build id is + * generated when the form is loaded from the cache. If it is subsequently + * saved to the cache again, it will have another cache id and therefore + * the original form and form-state will remain unaltered. This is + * important when page caching is enabled in order to prevent form state + * from leaking between anonymous users. * * @var array */ diff --git a/core/misc/ajax.js b/core/misc/ajax.js index 705d592e2d99b225499d96c707969a9ba9d7273c..c432706c83afe745f0eeec9f23b8b3e922926f47 100644 --- a/core/misc/ajax.js +++ b/core/misc/ajax.js @@ -718,6 +718,13 @@ .filter(':odd').addClass('even'); }, + /** + * Command to update a form's build ID. + */ + update_build_id: function(ajax, response, status) { + $('input[name="form_build_id"][value="' + response.old + '"]').val(response.new); + }, + /** * Command to add css. * diff --git a/core/modules/file/src/Controller/FileWidgetAjaxController.php b/core/modules/file/src/Controller/FileWidgetAjaxController.php index 119867249d9a0b79bd2282a71338b28b8dd06442..8c136f137c7d0fe3aa1268c45dd677de2dc1d231 100644 --- a/core/modules/file/src/Controller/FileWidgetAjaxController.php +++ b/core/modules/file/src/Controller/FileWidgetAjaxController.php @@ -43,7 +43,11 @@ public function upload(Request $request) { } try { - list($form, $form_state) = $this->getForm($request); + /** @var $ajaxForm \Drupal\system\FileAjaxForm */ + $ajaxForm = $this->getForm($request); + $form = $ajaxForm->getForm(); + $form_state = $ajaxForm->getFormState(); + $commands = $ajaxForm->getCommands(); } catch (HttpExceptionInterface $e) { // Invalid form_build_id. @@ -80,6 +84,9 @@ public function upload(Request $request) { $settings = drupal_merge_js_settings($js['settings']['data']); $response = new AjaxResponse(); + foreach ($commands as $command) { + $response->addCommand($command, TRUE); + } return $response->addCommand(new ReplaceCommand(NULL, $output, $settings)); } diff --git a/core/modules/simpletest/src/WebTestBase.php b/core/modules/simpletest/src/WebTestBase.php index 98f68fcc84a0381bfe783b58fc87e5d12bd70fb8..686b9bcdb16ab3d465672e6670b0932d508fb745 100644 --- a/core/modules/simpletest/src/WebTestBase.php +++ b/core/modules/simpletest/src/WebTestBase.php @@ -1897,6 +1897,12 @@ protected function drupalProcessAjaxResponse($content, array $ajax_response, arr break; case 'add_css': break; + case 'update_build_id': + $buildId = $xpath->query('//input[@name="form_build_id" and @value="' . $command['old'] . '"]')->item(0); + if ($buildId) { + $buildId->setAttribute('value', $command['new']); + } + break; } } $content = $dom->saveHTML(); diff --git a/core/modules/system/src/Controller/FormAjaxController.php b/core/modules/system/src/Controller/FormAjaxController.php index c3c678c38c1c6202ab00d3b2a04adb64fbb75944..c0a54e642a9df6d78487383d36b9d49e6e3ab93d 100644 --- a/core/modules/system/src/Controller/FormAjaxController.php +++ b/core/modules/system/src/Controller/FormAjaxController.php @@ -7,8 +7,10 @@ namespace Drupal\system\Controller; +use Drupal\Core\Ajax\UpdateBuildIdCommand; use Drupal\Core\Form\FormState; use Drupal\Core\DependencyInjection\ContainerInjectionInterface; +use Drupal\system\FileAjaxForm; use Psr\Log\LoggerInterface; use Symfony\Component\DependencyInjection\ContainerInterface; use Symfony\Component\HttpFoundation\Request; @@ -63,7 +65,12 @@ public static function create(ContainerInterface $container) { * @throws \Symfony\Component\HttpKernel\Exception\HttpExceptionInterface */ public function content(Request $request) { - list($form, $form_state) = $this->getForm($request); + /** @var $ajaxForm \Drupal\system\FileAjaxForm */ + $ajaxForm = $this->getForm($request); + $form = $ajaxForm->getForm(); + $form_state = $ajaxForm->getFormState(); + $commands = $ajaxForm->getCommands(); + drupal_process_form($form['#form_id'], $form, $form_state); // We need to return the part of the form (or some other content) that needs @@ -81,7 +88,12 @@ public function content(Request $request) { if (empty($callback) || !is_callable($callback)) { throw new HttpException(500, t('Internal Server Error')); } - return call_user_func_array($callback, array(&$form, &$form_state)); + /** @var \Drupal\Core\Ajax\AjaxResponse $response */ + $response = call_user_func_array($callback, [&$form, &$form_state]); + foreach ($commands as $command) { + $response->addCommand($command, TRUE); + } + return $response; } /** @@ -93,14 +105,11 @@ public function content(Request $request) { * @param \Symfony\Component\HttpFoundation\Request $request * The current request object. * - * @return array - * An array containing the $form and $form_state. Use the list() function - * to break these apart: - * @code - * list($form, $form_state, $form_id, $form_build_id) = $this->getForm(); - * @endcode + * @return \Drupal\system\FileAjaxForm + * A wrapper object containing the $form, $form_state, $form_id, + * $form_build_id and an initial list of Ajax $commands. * - * @throws Symfony\Component\HttpKernel\Exception\HttpExceptionInterface + * @throws \Symfony\Component\HttpKernel\Exception\HttpExceptionInterface */ protected function getForm(Request $request) { $form_state = new FormState(); @@ -118,6 +127,17 @@ protected function getForm(Request $request) { throw new BadRequestHttpException(); } + // When a page level cache is enabled, the form-build id might have been + // replaced from within form_get_cache. If this is the case, it is also + // necessary to update it in the browser by issuing an appropriate Ajax + // command. + $commands = []; + if (isset($form['#build_id_old']) && $form['#build_id_old'] != $form['#build_id']) { + // If the form build ID has changed, issue an Ajax command to update it. + $commands[] = new UpdateBuildIdCommand($form['#build_id_old'], $form['#build_id']); + $form_build_id = $form['#build_id']; + } + // Since some of the submit handlers are run, redirects need to be disabled. $form_state->disableRedirect(); @@ -129,12 +149,12 @@ protected function getForm(Request $request) { '#action' => TRUE, ]); - // The form needs to be processed; prepare for that by setting a few internal - // variables. + // The form needs to be processed; prepare for that by setting a few + // internal variables. $form_state->setUserInput($request->request->all()); $form_id = $form['#form_id']; - return array($form, $form_state, $form_id, $form_build_id); + return new FileAjaxForm($form, $form_state, $form_id, $form_build_id, $commands); } } diff --git a/core/modules/system/src/FileAjaxForm.php b/core/modules/system/src/FileAjaxForm.php new file mode 100644 index 0000000000000000000000000000000000000000..1be14b4e5188eafe46c9ba9c0b0c305a958c786c --- /dev/null +++ b/core/modules/system/src/FileAjaxForm.php @@ -0,0 +1,122 @@ +form = $form; + $this->formState = $form_state; + $this->formId = $form_id; + $this->formBuildId = $form_build_id; + $this->commands = $commands; + } + + /** + * Gets all AJAX commands. + * + * @return \Drupal\Core\Ajax\CommandInterface[] + * Returns all previously added AJAX commands. + */ + public function getCommands() { + return $this->commands; + } + + /** + * Gets the form definition. + * + * @return array + */ + public function getForm() { + return $this->form; + } + + /** + * Gets the unique form build ID. + * + * @return string + */ + public function getFormBuildId() { + return $this->formBuildId; + } + + /** + * Gets the unique form ID. + * + * @return string + */ + public function getFormId() { + return $this->formId; + } + + /** + * Gets the form state. + * + * @return \Drupal\Core\Form\FormStateInterface + */ + public function getFormState() { + return $this->formState; + } + +} diff --git a/core/modules/system/src/Tests/Ajax/AjaxFormPageCacheTest.php b/core/modules/system/src/Tests/Ajax/AjaxFormPageCacheTest.php new file mode 100644 index 0000000000000000000000000000000000000000..3d9ffa252047667cd27ab3df307c251563dcce60 --- /dev/null +++ b/core/modules/system/src/Tests/Ajax/AjaxFormPageCacheTest.php @@ -0,0 +1,86 @@ +set('cache.page.use_internal', 1); + $config->set('cache.page.max_age', 300); + $config->save(); + } + + /** + * Return the build id of the current form. + */ + protected function getFormBuildId() { + $build_id_fields = $this->xpath('//input[@name="form_build_id"]'); + $this->assertEqual(count($build_id_fields), 1, 'One form build id field on the page'); + return (string) $build_id_fields[0]['value']; + } + + /** + * Create a simple form, then POST to system/ajax to change to it. + */ + public function testSimpleAJAXFormValue() { + $this->drupalGet('ajax_forms_test_get_form'); + $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS', 'Page was not cached.'); + $build_id_initial = $this->getFormBuildId(); + + $edit = ['select' => 'green']; + $commands = $this->drupalPostAjaxForm(NULL, $edit, 'select'); + $build_id_first_ajax = $this->getFormBuildId(); + $this->assertNotEqual($build_id_initial, $build_id_first_ajax, 'Build id is changed in the simpletest-DOM on first AJAX submission'); + $expected = [ + 'command' => 'update_build_id', + 'old' => $build_id_initial, + 'new' => $build_id_first_ajax, + ]; + $this->assertCommand($commands, $expected, 'Build id change command issued on first AJAX submission'); + + $edit = ['select' => 'red']; + $this->drupalPostAjaxForm(NULL, $edit, 'select'); + $build_id_second_ajax = $this->getFormBuildId(); + $this->assertEqual($build_id_first_ajax, $build_id_second_ajax, 'Build id remains the same on subsequent AJAX submissions'); + + // Repeat the test sequence but this time with a page loaded from the cache. + $this->drupalGet('ajax_forms_test_get_form'); + $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'HIT', 'Page was cached.'); + $build_id_from_cache_initial = $this->getFormBuildId(); + $this->assertEqual($build_id_initial, $build_id_from_cache_initial, 'Build id is the same as on the first request'); + + $edit = ['select' => 'green']; + $commands = $this->drupalPostAjaxForm(NULL, $edit, 'select'); + $build_id_from_cache_first_ajax = $this->getFormBuildId(); + $this->assertNotEqual($build_id_from_cache_initial, $build_id_from_cache_first_ajax, 'Build id is changed in the simpletest-DOM on first AJAX submission'); + $this->assertNotEqual($build_id_first_ajax, $build_id_from_cache_first_ajax, 'Build id from first user is not reused'); + $expected = [ + 'command' => 'update_build_id', + 'old' => $build_id_from_cache_initial, + 'new' => $build_id_from_cache_first_ajax, + ]; + $this->assertCommand($commands, $expected, 'Build id change command issued on first AJAX submission'); + + $edit = ['select' => 'red']; + $this->drupalPostAjaxForm(NULL, $edit, 'select'); + $build_id_from_cache_second_ajax = $this->getFormBuildId(); + $this->assertEqual($build_id_from_cache_first_ajax, $build_id_from_cache_second_ajax, 'Build id remains the same on subsequent AJAX submissions'); + } + +} diff --git a/core/modules/system/src/Tests/Form/FormStoragePageCacheTest.php b/core/modules/system/src/Tests/Form/FormStoragePageCacheTest.php new file mode 100644 index 0000000000000000000000000000000000000000..18389569ae18cdefc44ed26d72c7cdd0957f90d9 --- /dev/null +++ b/core/modules/system/src/Tests/Form/FormStoragePageCacheTest.php @@ -0,0 +1,115 @@ +set('cache.page.use_internal', 1); + $config->set('cache.page.max_age', 300); + $config->save(); + } + + /** + * Return the build id of the current form. + */ + protected function getFormBuildId() { + $build_id_fields = $this->xpath('//input[@name="form_build_id"]'); + $this->assertEqual(count($build_id_fields), 1, 'One form build id field on the page'); + return (string) $build_id_fields[0]['value']; + } + + /** + * Build-id is regenerated when validating cached form. + */ + public function testValidateFormStorageOnCachedPage() { + $this->drupalGet('form-test/form-storage-page-cache'); + $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS', 'Page was not cached.'); + $this->assertText('No old build id', 'No old build id on the page'); + $build_id_initial = $this->getFormBuildId(); + + // Trigger validation error by submitting an empty title. + $edit = ['title' => '']; + $this->drupalPostForm(NULL, $edit, 'Save'); + $this->assertText($build_id_initial, 'Old build id on the page'); + $build_id_first_validation = $this->getFormBuildId(); + $this->assertNotEqual($build_id_initial, $build_id_first_validation, 'Build id changes when form validation fails'); + + // Trigger validation error by again submitting an empty title. + $edit = ['title' => '']; + $this->drupalPostForm(NULL, $edit, 'Save'); + $this->assertText('No old build id', 'No old build id on the page'); + $build_id_second_validation = $this->getFormBuildId(); + $this->assertEqual($build_id_first_validation, $build_id_second_validation, 'Build id remains the same when form validation fails subsequently'); + + // Repeat the test sequence but this time with a page loaded from the cache. + $this->drupalGet('form-test/form-storage-page-cache'); + $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'HIT', 'Page was cached.'); + $this->assertText('No old build id', 'No old build id on the page'); + $build_id_from_cache_initial = $this->getFormBuildId(); + $this->assertEqual($build_id_initial, $build_id_from_cache_initial, 'Build id is the same as on the first request'); + + // Trigger validation error by submitting an empty title. + $edit = ['title' => '']; + $this->drupalPostForm(NULL, $edit, 'Save'); + $this->assertText($build_id_initial, 'Old build id is initial build id'); + $build_id_from_cache_first_validation = $this->getFormBuildId(); + $this->assertNotEqual($build_id_initial, $build_id_from_cache_first_validation, 'Build id changes when form validation fails'); + $this->assertNotEqual($build_id_first_validation, $build_id_from_cache_first_validation, 'Build id from first user is not reused'); + + // Trigger validation error by again submitting an empty title. + $edit = ['title' => '']; + $this->drupalPostForm(NULL, $edit, 'Save'); + $this->assertText('No old build id', 'No old build id on the page'); + $build_id_from_cache_second_validation = $this->getFormBuildId(); + $this->assertEqual($build_id_from_cache_first_validation, $build_id_from_cache_second_validation, 'Build id remains the same when form validation fails subsequently'); + } + + /** + * Build-id is regenerated when rebuilding cached form. + */ + public function testRebuildFormStorageOnCachedPage() { + $this->drupalGet('form-test/form-storage-page-cache'); + $this->assertEqual($this->drupalGetHeader('X-Drupal-Cache'), 'MISS', 'Page was not cached.'); + $this->assertText('No old build id', 'No old build id on the page'); + $build_id_initial = $this->getFormBuildId(); + + // Trigger rebuild, should regenerate build id. + $edit = ['title' => 'something']; + $this->drupalPostForm(NULL, $edit, 'Rebuild'); + $this->assertText($build_id_initial, 'Initial build id as old build id on the page'); + $build_id_first_rebuild = $this->getFormBuildId(); + $this->assertNotEqual($build_id_initial, $build_id_first_rebuild, 'Build id changes on first rebuild.'); + + // Trigger subsequent rebuild, should regenerate the build id again. + $edit = ['title' => 'something']; + $this->drupalPostForm(NULL, $edit, 'Rebuild'); + $this->assertText($build_id_first_rebuild, 'First build id as old build id on the page'); + $build_id_second_rebuild = $this->getFormBuildId(); + $this->assertNotEqual($build_id_first_rebuild, $build_id_second_rebuild, 'Build id changes on second rebuild.'); + } + +} diff --git a/core/modules/system/src/Tests/Form/StorageTest.php b/core/modules/system/src/Tests/Form/StorageTest.php index 01189d1803d49e64b3111a5b6d9b6c39e3c6dd13..ca690a6f6787c3f5913274baec4329776952716d 100644 --- a/core/modules/system/src/Tests/Form/StorageTest.php +++ b/core/modules/system/src/Tests/Form/StorageTest.php @@ -15,9 +15,9 @@ * * The tested form puts data into the storage during the initial form * construction. These tests verify that there are no duplicate form - * constructions, with and without manual form caching activiated. Furthermore + * constructions, with and without manual form caching activated. Furthermore * when a validation error occurs, it makes sure that changed form element - * values aren't lost due to a wrong form rebuild. + * values are not lost due to a wrong form rebuild. * * @group Form */ @@ -28,7 +28,7 @@ class StorageTest extends WebTestBase { * * @var array */ - public static $modules = array('form_test'); + public static $modules = array('form_test', 'dblog'); protected function setUp() { parent::setUp(); @@ -159,4 +159,81 @@ function testFormStatePersist() { $this->assertText('State persisted.'); } } + + /** + * Verify that the form build-id remains the same when validation errors + * occur on a mutable form. + */ + public function testMutableForm() { + // Request the form with 'cache' query parameter to enable form caching. + $this->drupalGet('form_test/form-storage', ['query' => ['cache' => 1]]); + $buildIdFields = $this->xpath('//input[@name="form_build_id"]'); + $this->assertEqual(count($buildIdFields), 1, 'One form build id field on the page'); + $buildId = (string) $buildIdFields[0]['value']; + + // Trigger validation error by submitting an empty title. + $edit = ['title' => '']; + $this->drupalPostForm(NULL, $edit, 'Continue submit'); + + // Verify that the build-id did not change. + $this->assertFieldByName('form_build_id', $buildId, 'Build id remains the same when form validation fails'); + } + + /** + * Verifies that form build-id is regenerated when loading an immutable form + * from the cache. + */ + public function testImmutableForm() { + // Request the form with 'cache' query parameter to enable form caching. + $this->drupalGet('form_test/form-storage', ['query' => ['cache' => 1, 'immutable' => 1]]); + $buildIdFields = $this->xpath('//input[@name="form_build_id"]'); + $this->assertEqual(count($buildIdFields), 1, 'One form build id field on the page'); + $buildId = (string) $buildIdFields[0]['value']; + + // Trigger validation error by submitting an empty title. + $edit = ['title' => '']; + $this->drupalPostForm(NULL, $edit, 'Continue submit'); + + // Verify that the build-id did change. + $this->assertNoFieldByName('form_build_id', $buildId, 'Build id changes when form validation fails'); + + // Retrieve the new build-id. + $buildIdFields = $this->xpath('//input[@name="form_build_id"]'); + $this->assertEqual(count($buildIdFields), 1, 'One form build id field on the page'); + $buildId = (string) $buildIdFields[0]['value']; + + // Trigger validation error by again submitting an empty title. + $edit = ['title' => '']; + $this->drupalPostForm(NULL, $edit, 'Continue submit'); + + // Verify that the build-id does not change the second time. + $this->assertFieldByName('form_build_id', $buildId, 'Build id remains the same when form validation fails subsequently'); + } + + /** + * Verify that existing contrib code cannot overwrite immutable form state. + */ + public function testImmutableFormLegacyProtection() { + $this->drupalGet('form_test/form-storage', ['query' => ['cache' => 1, 'immutable' => 1]]); + $build_id_fields = $this->xpath('//input[@name="form_build_id"]'); + $this->assertEqual(count($build_id_fields), 1, 'One form build id field on the page'); + $build_id = (string) $build_id_fields[0]['value']; + + // Try to poison the form cache. + $original = $this->drupalGetAJAX('form-test/form-storage-legacy/' . $build_id); + $this->assertEqual($original['form']['#build_id_old'], $build_id, 'Original build_id was recorded'); + $this->assertNotEqual($original['form']['#build_id'], $build_id, 'New build_id was generated'); + + // Assert that a watchdog message was logged by form_set_cache. + $status = (bool) db_query_range('SELECT 1 FROM {watchdog} WHERE message = :message', 0, 1, [':message' => 'Form build-id mismatch detected while attempting to store a form in the cache.']); + $this->assert($status, 'A watchdog message was logged by form_set_cache'); + + // Ensure that the form state was not poisoned by the preceding call. + $original = $this->drupalGetAJAX('form-test/form-storage-legacy/' . $build_id); + $this->assertEqual($original['form']['#build_id_old'], $build_id, 'Original build_id was recorded'); + $this->assertNotEqual($original['form']['#build_id'], $build_id, 'New build_id was generated'); + $this->assert(empty($original['form']['#poisoned']), 'Original form structure was preserved'); + $this->assert(empty($original['form_state']['poisoned']), 'Original form state was preserved'); + } + } diff --git a/core/modules/system/tests/modules/form_test/form_test.routing.yml b/core/modules/system/tests/modules/form_test/form_test.routing.yml index 109d8f37c3ea654b988340fc44f5830db112fe9e..bebb71c034d0d5965ac6438d546cdb5ad6f74516 100644 --- a/core/modules/system/tests/modules/form_test/form_test.routing.yml +++ b/core/modules/system/tests/modules/form_test/form_test.routing.yml @@ -433,3 +433,19 @@ form_test.two_instances: requirements: _module_dependencies: 'node' _permission: 'create page content' + +form_test.storage_legacy_handler: + path: '/form-test/form-storage-legacy/{form_build_id}' + defaults: + _controller: '\Drupal\form_test\Controller\FormTestController::storageLegacyHandler' + form_build_id: NULL + requirements: + _access: 'TRUE' + +form_test.form_storage_page_cache: + path: '/form-test/form-storage-page-cache' + defaults: + _form: '\Drupal\form_test\Form\FormTestStoragePageCacheForm' + _title: 'Form storage with page cache test' + requirements: + _access: 'TRUE' diff --git a/core/modules/system/tests/modules/form_test/src/Controller/FormTestController.php b/core/modules/system/tests/modules/form_test/src/Controller/FormTestController.php index 600b1d9572694e4b2b95112bb9a4a45933788f66..1950076b751fecfbe5d02e2e5a5d277f7e71709a 100644 --- a/core/modules/system/tests/modules/form_test/src/Controller/FormTestController.php +++ b/core/modules/system/tests/modules/form_test/src/Controller/FormTestController.php @@ -7,7 +7,9 @@ namespace Drupal\form_test\Controller; use Drupal\Core\Controller\ControllerBase; +use Drupal\Core\Form\FormState; use Drupal\Core\Language\LanguageInterface; +use Symfony\Component\HttpFoundation\JsonResponse; /** * Controller routines for form_test routes. @@ -35,4 +37,25 @@ public function twoFormInstances() { return $return; } + /** + * Emulate legacy AHAH-style ajax callback. + * + * Drupal 6 AHAH callbacks used to operate directly on forms retrieved using + * form_get_cache and stored using form_set_cache after manipulation. This + * callback helps testing whether \Drupal::formBuilder()->setCache() prevents + * resaving of immutable forms. + */ + public function storageLegacyHandler($form_build_id) { + $form_state = new FormState(); + $form = $this->formBuilder()->getCache($form_build_id, $form_state); + $result = [ + 'form' => $form, + 'form_state' => $form_state, + ]; + $form['#poisoned'] = TRUE; + $form_state->set('poisoned', TRUE); + $this->formBuilder()->setCache($form_build_id, $form, $form_state); + return new JsonResponse($result); + } + } diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestStorageForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestStorageForm.php index c5f166b997733aaf21c9a761791a9057c96a1058..60e0cdb082e707a3715fff39e9cc7bd6d55c0835 100644 --- a/core/modules/system/tests/modules/form_test/src/Form/FormTestStorageForm.php +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestStorageForm.php @@ -88,6 +88,10 @@ public function buildForm(array $form, FormStateInterface $form_state) { $form_state->setCached(); } + if ($this->getRequest()->get('immutable')) { + $form_state->addBuildInfo('immutable', TRUE); + } + return $form; } diff --git a/core/modules/system/tests/modules/form_test/src/Form/FormTestStoragePageCacheForm.php b/core/modules/system/tests/modules/form_test/src/Form/FormTestStoragePageCacheForm.php new file mode 100644 index 0000000000000000000000000000000000000000..223ed808dbe005995e76334a3a0da98c8b48b53d --- /dev/null +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestStoragePageCacheForm.php @@ -0,0 +1,80 @@ + 'textfield', + '#title' => 'Title', + '#required' => TRUE, + ); + + $form['test_build_id_old'] = array( + '#type' => 'item', + '#title' => 'Old build id', + '#markup' => 'No old build id', + ); + + $form['submit'] = array( + '#type' => 'submit', + '#value' => 'Save', + ); + + $form['rebuild'] = array( + '#type' => 'submit', + '#value' => 'Rebuild', + '#submit' => array(array($this, 'form_test_storage_page_cache_rebuild')), + ); + + $form['#after_build'] = array(array($this, 'form_test_storage_page_cache_old_build_id')); + $form_state->setCached(); + + return $form; + } + + /** + * Form element #after_build callback: output the old form build-id. + */ + function form_test_storage_page_cache_old_build_id($form) { + if (isset($form['#build_id_old'])) { + $form['test_build_id_old']['#markup'] = String::checkPlain($form['#build_id_old']); + } + return $form; + } + + /** + * Form submit callback: Rebuild the form and continue. + */ + function form_test_storage_page_cache_rebuild($form, FormStateInterface $form_state) { + $form_state->setRebuild(); + } + + /** + * {@inheritdoc} + */ + public function submitForm(array &$form, FormStateInterface $form_state) { + // Nothing must happen. + } + +} diff --git a/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php b/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php index d08557ac8d7f1896956572c73077701d47006a3e..41d76dfa981487858d97d47d5678f7d9d6dccced 100644 --- a/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php +++ b/core/tests/Drupal/Tests/Core/Form/FormCacheTest.php @@ -10,7 +10,6 @@ use Drupal\Component\Utility\SafeMarkup; use Drupal\Core\Form\FormCache; use Drupal\Core\Form\FormState; -use Drupal\Core\Session\AccountInterface; use Drupal\Tests\UnitTestCase; /** @@ -68,6 +67,34 @@ class FormCacheTest extends UnitTestCase { */ protected $formStateCacheStore; + /** + * The logger channel. + * + * @var \Psr\Log\LoggerInterface|\PHPUnit_Framework_MockObject_MockObject + */ + protected $logger; + + /** + * The config factory. + * + * @var \Drupal\Core\Config\ConfigFactoryInterface|\PHPUnit_Framework_MockObject_MockObject + */ + protected $configFactory; + + /** + * The request stack. + * + * @var \Symfony\Component\HttpFoundation\RequestStack|\PHPUnit_Framework_MockObject_MockObject + */ + protected $requestStack; + + /** + * A policy rule determining the cacheability of a request. + * + * @var \Drupal\Core\PageCache\RequestPolicyInterface|\PHPUnit_Framework_MockObject_MockObject + */ + protected $requestPolicy; + /** * {@inheritdoc} */ @@ -92,7 +119,14 @@ protected function setUp() { ->disableOriginalConstructor() ->getMock(); $this->account = $this->getMock('Drupal\Core\Session\AccountInterface'); - $this->formCache = new FormCache($this->keyValueExpirableFactory, $this->moduleHandler, $this->account, $this->csrfToken); + + $this->logger = $this->getMock('Psr\Log\LoggerInterface'); + $this->configFactory = $this->getConfigFactoryStub(['system.performance' => ['cache.page.use_internal' => FALSE]]); + $this->requestStack = $this->getMock('\Symfony\Component\HttpFoundation\RequestStack'); + $this->requestPolicy = $this->getMock('\Drupal\Core\PageCache\RequestPolicyInterface'); + + + $this->formCache = new FormCache($this->keyValueExpirableFactory, $this->moduleHandler, $this->account, $this->csrfToken, $this->logger, $this->configFactory, $this->requestStack, $this->requestPolicy); } /** @@ -212,6 +246,32 @@ public function testGetCacheNoForm() { $this->assertNull($form); } + /** + * @covers ::getCache + */ + public function testGetCacheImmutableForm() { + $form_build_id = 'the_form_build_id'; + $form_state = (new FormState()) + ->addBuildInfo('immutable', TRUE); + $cached_form = [ + '#build_id' => 'the_old_build_form_id', + ]; + + $this->account->expects($this->once()) + ->method('isAnonymous') + ->willReturn(TRUE); + $this->formCacheStore->expects($this->once()) + ->method('get') + ->with($form_build_id) + ->willReturn($cached_form); + + $form = $this->formCache->getCache($form_build_id, $form_state); + $this->assertSame($cached_form['#build_id'], $form['#build_id_old']); + $this->assertNotSame($cached_form['#build_id'], $form['#build_id']); + $this->assertSame($form['#build_id'], $form['form_build_id']['#value']); + $this->assertSame($form['#build_id'], $form['form_build_id']['#id']); + } + /** * @covers ::loadCachedFormState */ @@ -406,6 +466,63 @@ public function testSetCacheWithSafeStrings() { $this->formCache->setCache($form_build_id, $form, $form_state); } + /** + * @covers ::setCache + */ + public function testSetCacheBuildIdMismatch() { + $form_build_id = 'the_form_build_id'; + $form = [ + '#form_id' => 'the_form_id', + '#build_id' => 'stale_form_build_id', + ]; + $form_state = new FormState(); + + $this->formCacheStore->expects($this->never()) + ->method('setWithExpire'); + $this->formStateCacheStore->expects($this->never()) + ->method('setWithExpire'); + $this->logger->expects($this->once()) + ->method('error') + ->with('Form build-id mismatch detected while attempting to store a form in the cache.'); + $this->formCache->setCache($form_build_id, $form, $form_state); + } + + /** + * @covers ::setCache + */ + public function testSetCacheImmutableForm() { + $form_build_id = 'the_form_build_id'; + $form = [ + '#form_id' => 'the_form_id', + ]; + $form_state = new FormState(); + + $this->formCacheStore->expects($this->once()) + ->method('setWithExpire') + ->with($form_build_id, $form, $this->isType('int')); + $form_state_data = $form_state->getCacheableArray(); + $form_state_data['build_info']['safe_strings'] = []; + // Ensure that the form is marked immutable. + $form_state_data['build_info']['immutable'] = TRUE; + $this->formStateCacheStore->expects($this->once()) + ->method('setWithExpire') + ->with($form_build_id, $form_state_data, $this->isType('int')); + + // Rebuild the FormCache with a config factory that will return a config + // object with the internal page cache enabled. + $this->configFactory = $this->getConfigFactoryStub(['system.performance' => ['cache.page.use_internal' => TRUE]]); + $this->formCache = $this->getMockBuilder('Drupal\Core\Form\FormCache') + ->setConstructorArgs([$this->keyValueExpirableFactory, $this->moduleHandler, $this->account, $this->csrfToken, $this->logger, $this->configFactory, $this->requestStack, $this->requestPolicy]) + ->setMethods(['isPageCacheable']) + ->getMock(); + + $this->formCache->expects($this->once()) + ->method('isPageCacheable') + ->willReturn(TRUE); + + $this->formCache->setCache($form_build_id, $form, $form_state); + } + /** * Ensures SafeMarkup does not bleed from one test to another. */