Port Form API security fix SA-CORE-2014-002 to Drupal 8

The D7 fix is publicly accessible http://drupalcode.org/project/drupal.git/commitdiff/6642fbc7001c728e2181...

So here's the D7 patch for reference.

Working on this.

It still does not work, but it is a start.

Created UpdateBuildIdCommand. Renamed command key from updateBuildId to update_build_id for consistency. Proper cache config saving.
Still same failures.

Fixed FormBuilderTest by injecting services. Had to redefine drupal_page_is_cacheable(), ugly as hell, but I guess acceptable.

Argh, this introduces a circular dependency :(

Circular reference detected for service "form_builder", path: "route_content_form_controller_subscriber -> form_builder -> breadcrumb -> system.breadcrumb.default -> router -> router.dynamic -> route_enhancer.entity".

I was getting "A fatal error occurred:" with no other information, and it was because of AccessDeniedHttpException in AccessSubscriber::onKernelRequestAccessCheck. Maybe we should add a message there in a different issue.

The attached patch should fix at least some tests.

I was calling the wrong form. Locally this is down to 1 failure in FormCacheTest.

Clean-up of the patch, nothing functional.

I'm not sure if the failure is expected with this change. Unassigning for now.

The main conflict looks to be #2328777: Refactor FAPI getCache()/setCache() into a standalone class, so I would think the caching changes need to be moved to Drupal\Core\Form\FormCache.

Here's the patch using FormCache, with added unit tests.

The interdiff isn't exact, as I had to rebase first and move the code to the right class, but it should encapsulate my changes.

Reroll after #2332389: Finish adding methods to FormStateInterface

Tagging with D8 upgrade path, see issue summary of #2341575: [meta] Provide a beta to beta/rc upgrade path for why.

Rerolled (automerged).

Fixed the errors after drupal_page_is_cacheable() is gone (#2263981: Introduce a robust and extensible page cache-policy framework) and #2310255: [meta] Remove ArrayAccess from FormState is in.

1. +++ b/core/lib/Drupal/Core/Ajax/UpdateBuildIdCommand.php
   @@ -0,0 +1,65 @@
   + * user from accessing the form state of another anonymous users on Ajax enabled
   

   users » user ?

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -286,17 +286,25 @@ public function rebuildForm($form_id, FormStateInterface &$form_state, $old_form
   +    // RIA clients), or if the form already had a new build ID regenerated when it
   
   +++ b/core/lib/Drupal/Core/Form/FormCache.php
   @@ -75,6 +99,18 @@ public function getCache($form_build_id, FormStateInterface $form_state) {
   +        // Generate a new #build_id if the cached form was rendered on a cacheable
   
   @@ -124,15 +160,29 @@ public function setCache($form_build_id, $form, FormStateInterface $form_state)
   +    // Ensure that the form build_id embedded in the form structure is the same as
   
   +++ b/core/modules/system/tests/modules/form_test/src/Controller/FormTestController.php
   @@ -35,4 +37,25 @@ public function twoFormInstances() {
   +   * callback helps testing whether form_set_cache prevents resaving of immutable
   

   > 80

3. +++ b/core/lib/Drupal/Core/Form/FormCache.php
   @@ -143,4 +193,12 @@ public function setCache($form_build_id, $form, FormStateInterface $form_state)
   +   * Wraps drupal_page_is_cacheable().
   

   This doesn't appear to be the case anymore?

4. +++ b/core/lib/Drupal/Core/Form/FormCache.php
   @@ -143,4 +193,12 @@ public function setCache($form_build_id, $form, FormStateInterface $form_state)
   +    $request_policy = \Drupal::service('page_cache_request_policy');
   

   Can't we inject this service?

5. +++ b/core/lib/Drupal/Core/Form/FormCache.php
   @@ -143,4 +193,12 @@ public function setCache($form_build_id, $form, FormStateInterface $form_state)
   +    return ($request_policy->check(\Drupal::request()) === RequestPolicyInterface::ALLOW);
   

   We should be injecting the request stack service here

6. +++ b/core/modules/system/src/Controller/FormAjaxController.php
   @@ -94,13 +101,14 @@ public function content(Request $request) {
   -   *   An array containing the $form and $form_state. Use the list() function
   -   *   to break these apart:
   +   *   An array containing the $form, $form_state, $form_id, $form_build_id and an
   +   *   initial list of Ajax $commands. Use the list() function to break these
   +   *   apart:
       *   @code
   -   *     list($form, $form_state, $form_id, $form_build_id) = $this->getForm();
   +   *     list($form, $form_state, $form_id, $form_build_id, $commands) = $this->getForm();
   
   @@ -134,7 +153,7 @@ protected function getForm(Request $request) {
   +    return [$form, $form_state, $form_id, $form_build_id, $commands];
   

   This really sounds like we need a value object.
   Returning an array of objects where the order is important is bad form in my opinion.

7. +++ b/core/modules/system/src/Controller/FormAjaxController.php
   @@ -118,6 +126,17 @@ protected function getForm(Request $request) {
   +    // replaced from within form_get_cache. If this is the case, it is also
   

   form_get_cache is deprecated, can we reference it's replacement instead

8. +++ b/core/modules/system/src/Tests/Form/StorageTest.php
   @@ -159,4 +159,81 @@ function testFormStatePersist() {
   +  function testMutableForm() {
   ...
   +  function testImmutableForm() {
   

   Missing public scope modifier

9. +++ b/core/modules/system/tests/modules/form_test/src/Form/FormTestStorageForm.php
   @@ -88,6 +88,10 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +    if (\Drupal::request()->get('immutable')) {
   

   If this extends from FormBase you should have a method to get this without calling out to the static container (I think)

Covered the easy stuff from #26: 1, 2, 3, 8, 9.

For 7 I'm not sure, as we are still calling the deprecated method here, and having something else there could be confusing.
Changing the deprecated calls is out of scope here IMHO.

I will work on 4,5,6 next. Suggestions for naming the object at 6 are appreciated.

testbot anyway.

Done with service injection. Only #26.6 is pending.

Oops, typo.

Fixed unit tests. Misuse of requestStack as request object solved.

Created AjaxForm class (couldn't think of a better name) for wrapping the list object. Needs phpdocs for the new class and the function wrapping those, some cleanup and such, but let's look what the testbot has to say.

Cool, green! Needs work still.

Fixed phpdocs. Ready for reviews :-D

Sorry, right interdiff.

Calm down. New patch, new interdiff, since #34.

I messed up the last interdiff too. Look at the interdiff of #37 or the full patch at #38.

Looks great, just nitpicks that I can see

1. +++ b/core/lib/Drupal/Core/Ajax/AjaxForm.php
   @@ -0,0 +1,122 @@
   +} ¶
   

   nitpick: whitespace

2. +++ b/core/lib/Drupal/Core/Ajax/UpdateBuildIdCommand.php
   @@ -0,0 +1,65 @@
   + * This command is implemented by Drupal.AjaxCommands.prototype.update_build_id()
   
   +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -291,17 +291,25 @@ public function rebuildForm($form_id, FormStateInterface &$form_state, $old_form
   +    // RIA clients), or if the form already had a new build ID regenerated when it
   
   +++ b/core/lib/Drupal/Core/Form/FormCache.php
   @@ -124,15 +181,29 @@ public function setCache($form_build_id, $form, FormStateInterface $form_state)
   +    // Ensure that the form build_id embedded in the form structure is the same as
   
   +++ b/core/modules/system/src/Controller/FormAjaxController.php
   @@ -93,14 +106,11 @@ public function content(Request $request) {
   +   *   A wrapper object containing the $form, $form_state, $form_id, $form_build_id and an
   

   > 80

3. +++ b/core/modules/system/tests/modules/form_test/src/Controller/FormTestController.php
   @@ -35,4 +37,25 @@ public function twoFormInstances() {
   +   * form_get_cache and stored using form_set_cache after manipulation. This
   

   form_get_cache() is deprecated or gone?

+++ b/core/lib/Drupal/Core/Ajax/AjaxForm.php
@@ -0,0 +1,122 @@
+ * Wrapper for Ajax forms data and commands.

dawehner asked if we could directly document that this is used to replace a multi-return-value tuple

I like the idea to make things a little bit more clear, though I doubt that this class is really that reusable as the name suggests. Should we instead move it into system and prefix it with File or something else?
What do you think?

Wrapped comments to 80 chars and fixed nitpick at #40.

#40.3: Only changed one occurrence, as it is talking about D6. See context:

    * 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 form_set_cache prevents resaving of
-   * immutable forms.
+   * callback helps testing whether \Drupal::formBuilder()->setCache() prevents
+   * resaving of immutable forms.
    */

Renamed AjaxForm to FileAjaxForm (not happy with the name neither). Moved it to Drupal\system. Specified in phpdoc that it is avoiding a multi-return-value tuple.

+++ b/core/modules/system/src/FileAjaxForm.php
@@ -2,19 +2,19 @@
+ * Wrapper for Ajax forms data and commands, avoiding a multi-return-value tuple.

if re-rolling please reword to meet 80 char requirements - eg %s/avoiding/avoids

other than that, I think this is ready - thanks @penyaskito

I would like to figure out who worked on s.d.o for giving proper attribution. There are some in the IS, but not sure if all.

Oh, @larowlan already did that in the IS, thanks!

Issue #2242749 by znerol, torotil, rszrama, larowlan, dawehner, penyaskito, tim.plunkett, sun, Damien Tournoud, David_Rothstein,  effulgentsia: Fixed Port Form API security fix SA-CORE-2014-002 to Drupal 8.

Committed 3fb2ec3 and pushed to 8.0.x. Thanks!

diff --git a/core/lib/Drupal/Core/Form/FormCache.php b/core/lib/Drupal/Core/Form/FormCache.php
index b0476d1..c399f1a 100644
--- a/core/lib/Drupal/Core/Form/FormCache.php
+++ b/core/lib/Drupal/Core/Form/FormCache.php
@@ -216,8 +216,11 @@ 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($allow_caching = NULL) {
+  protected function isPageCacheable() {
     return ($this->requestPolicy->check($this->requestStack->getCurrentRequest()) === RequestPolicyInterface::ALLOW);
   }
 
diff --git a/core/modules/system/src/Controller/FormAjaxController.php b/core/modules/system/src/Controller/FormAjaxController.php
index b932498..c0a54e6 100644
--- a/core/modules/system/src/Controller/FormAjaxController.php
+++ b/core/modules/system/src/Controller/FormAjaxController.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\system\Controller;
 
-use Drupal\Core\Ajax\AjaxResponse;
 use Drupal\Core\Ajax\UpdateBuildIdCommand;
 use Drupal\Core\Form\FormState;
 use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
diff --git a/core/modules/system/src/Tests/Form/StorageTest.php b/core/modules/system/src/Tests/Form/StorageTest.php
index c8a4dc8..ca690a6 100644
--- a/core/modules/system/src/Tests/Form/StorageTest.php
+++ b/core/modules/system/src/Tests/Form/StorageTest.php
@@ -228,7 +228,7 @@ public function testImmutableFormLegacyProtection() {
     $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 preceeding call.
+    // 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');

Minor fixes on commit

#2421503: SA-CORE-2014-002 forward port only checks internal cache.