Potential data loss: concurrent (i.e. by different users) node edits leak through preview

Yeah we can see the similar kind of issue here #1959806-56: Provide a generic 'entity_autocomplete' Form API element

Are you sure this is really a security issue? The node preview has an access check that does a check that the user is allowed to update that node object. If you have workflows with that, then you could prevent it there?

Would we validate the uid then if we add it? Or can you still share a preview with someone? Because that is a pretty useful feature, and we have another case on top of that, an external application that allows to create content and preview it in Drupal.

Tagging the issue.

The only problem in current implementation that current user is not able to preview revisions because no vid passed.
The stored form state in tempStore is set and retrieved using factory that adds current user, so each user will get only own stored form state.

CPS allows you to do it, but also warns that there's an inherent race condition when either of the revisions eventually gets published (i.e. last published wins). Apart from the warning I personally think it should at least have the option to lock an entity once there's a draft in any changeset. Note that race condition in CPS exists even if the revisions get edited on separate days. I do think you'd run into this bug if you had concurrent edits of the same entity at that same time, so it does exacerbate things.

Don't see how there's any security issue, it's a straight race condition.

I kind of that could at least prevent data loss

A test to show failure

Let's add unique key (based on node uuid and $form['#build_id']) for each preview cache record and store this key in user session.

oops, my bad =)

Next try (locally test passed)

No test in last patch!

+++ b/core/modules/node/src/NodeForm.php
@@ -338,9 +341,15 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+      \Drupal::service('session')->set('node_preview_key-' . $this->entity->uuid(), $node_preview_key);

Suppose to use session here is bad.

+++ b/core/modules/node/src/NodeForm.php
@@ -338,9 +341,15 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
+      $node_preview_key = $this->entity->uuid() . '--' . $form['#build_id'];
...
+    $store->set($node_preview_key, $form_state);

build ID addition allows editing node by one user in different tabs, no tests.

+++ b/core/modules/node/src/NodeForm.php
@@ -338,9 +341,15 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
     $form_state->setRedirect('entity.node.preview', array(
-      'node_preview' => $this->entity->uuid(),
+      'node_preview' => $node_preview_key,

nice idea... to abstract key that allows passing preview between sessions/users, great change for separate issue!!!

Are going to hardening the permission for entity-in-preview stored?
Current

+++ b/core/modules/node/src/ParamConverter/NodePreviewConverter.php
@@ -39,7 +39,7 @@ public function __construct(TempStoreFactory $temp_store_factory) {
-    if ($form_state = $store->get($value)) {
+    if ($form_state = $store->getIfOwner($value)) {
       return $form_state->getFormObject()->getEntity();

I think this change is needed or should be extended if we are not allowing access for entity/form state for different users

roger that, patch coming soon =)

Brand new version of patch. Includes test from 2421263-node-preview-10.patch with little fix in assertUrl (added option ['absolute' => TRUE])
I doesn't see a reason to use getIfOwner in this implementation, but i provide two version of patch: with getIfOwner and without.

looks good to me

Is hardcoding the session_id() there really a good idea?

We already use session_id() in temp storage

  function get($collection, $owner = NULL) {
    // Use the currently authenticated user ID or the active user ID unless
    // the owner is overridden.
    if (!isset($owner)) {
      $owner = \Drupal::currentUser()->id() ?: session_id();
    }

    // Store the data for this collection in the database.
    $storage = $this->storageFactory->get("user.tempstore.$collection");
    return new TempStore($storage, $this->lockBackend, $owner, $this->expire);
  }

Can't we just use the getIfOwner() and not do any of the session_id() appending in NodeForm. But I guess the problem is that multiple users should be able to preview the at the same time. Maybe we need a mutli-user tempstore here since putting this logic in NodeForm feels wrong.

I'm sure that hardcoding getIfOwner() is not good.

Suppose each submit should generate a hash based on values.
This hash defines a storage preview key and some kind of access logic to generate preview (to allow contrib to configure)

As comment module maintainer I'd like to unify preview logic for comments as well.

@andypost that's why I was suggesting a multi-user temp store which automatically appends the owner to the key.

@andypost when we come back from preview we have no values data and can't calculate hash. No hash - no key. But we can put hash value to form query url.

@alexpott we discussed this in irc with andypost, unfortunately in current implementation owner stored as part of storage value.
O:8:"stdClass":3:{s:5:"owner";s:1:"1";s:4:"data";O:26:"Drupal\Core\Form\FormState"

We can modify current TempStore class (for creating unique key names based on user), but i'm not sure this is good, because other modules can use this feature - one user can access to data added by other user with easy to calculate key name (like node uuid).
I think this is should be a different class for this (named something like MultiuserTempStore).

Also we can use per-user collections (PRIMARY KEY (`collection`,`name`)) instead of unique key names. Or move deeper and add column 'owner' to storage.

Here's a start - I've created FooTempStore for a temp store where the keys are guaranteed to be per user. I think we should rename the existing temp store to SharedTempStore but did not do that yet so we can sort out the names here first. MultiuserTempStore does not really work for the new class because both temp stores are designed to be used by multiple users it is just the use case is ever so slightly different.

I also discovered that quickedit probably has the same problem. The other places that are using tempstores are the multiple delete confirm forms for users and nodes - these don;t have this same problem because they are using the user ID as the key and will never be available to anonymous users. Yes it is rare but anonymous users (if the site is configured to allow it) should be able to create nodes.

Oh and I think there is a security issue here. What if your are changing something on the node that would prevent the other user getting access and they get your tempstore and can see the updates you would have made? In HEAD at the moment it is possible that the user gets the other users data from the tempstore when they preview the node.

Oops.

Doing some manual testing.

Validated #33 fixes this issue.

Steps to manual test:

• Create two users: user1 and user2 with administrator role.
• User 1 creates a node with sample content, and saves.
• User 1 then edits that node and previews
• User 2 edits that node at the same time and previews
• Preview for user 2 should not show any content from user 1 and visa versa

User 1 edit:

User 1 preview:

User 2 edit:

User 2 preview:

Renamed FooTempStore to UserTempStore which I think makes it clear that its a per-user tempstore?

Happy to call the bikeshed yellow, however. ;-)

I'd like my bikeshed red

I like PrivateTempStore as the name.

But yeah, fix looks good - names aside

The patch attached:

• Renames the new temp store to PrivateTempStore (good name @larowlan)
• Renames the old temp store to SharedTempStore (suggested by @xjm) to denote the fact the whilst it has the concept of owner the storage is (by design) accessible to other users. This makes the patch very large but i think the distinction is vital since it is extremely easy to use the shared temp store in a way that might cause data loss or over-sharing.
• Changes user cancellation and node deletion to use the PrivateTempStore since by using the current users uid as a key this is inline with the intention and it means that if someone was mad a gave permissions to anonymous to do this stuff wouldn't break.

Note that Quick Edit specifically added test coverage for concurrent editing: \Drupal\quickedit\Tests\QuickEditLoadingTest::testConcurrentEdit(). Done back in #1901100: Make Edit module work with TempStore, so revisions are not saved on all atomic field edits. EDIT: and discussed further in #2111427: All entity forms should implement (TempStore) locking to protect against concurrent edits, which was a follow-up to that one.

re #41/@Wim Leers - looking at the test - you're testing the save does not work because the timestamps have changed - that's not about the tempstore.

Yeah, it's intra-user concurrency, not inter-user concurrency.

Clarifying that in the title. I hope it also applies to the general issue here, and not just to Quick Edit.

Found some nits (below) and fixed them
Added draft change notice (https://www.drupal.org/node/2430511)
Updated issue summary
good to go

1. +++ b/core/modules/user/src/PrivateTempStore.php
   @@ -16,23 +16,23 @@
   + * The PrivateTempStore is different from a cache, because the data in it is not yet
   + * saved permanently and so it cannot be rebuilt. Typically, the PrivateTempStore
   

   nitpick > 80

2. +++ b/core/modules/user/src/SharedTempStoreFactory.php
   @@ -54,17 +54,17 @@ function __construct(KeyValueExpirableFactoryInterface $storage_factory, LockBac
   +   *   (optional) The owner of this SharedTempStore. By default, the SharedTempStore is
   

   same

3. +++ b/core/modules/views_ui/src/ViewUI.php
   @@ -70,7 +70,7 @@ class ViewUI implements ViewEntityInterface {
   +   * \Drupal\user\SharedTempStore::getMetadata(). Which can be a stdClass or NULL.
   

   same

RTBC + 1

Nice security improvement!

Needed a reroll...

First, rewinding head to replay your work on top of it...
Applying: Init commit
Using index info to reconstruct a base tree...
M	core/lib/Drupal/Core/Session/SessionManager.php
M	core/modules/views_ui/src/ViewUI.php
Falling back to patching base and 3-way merge...
Auto-merging core/modules/views_ui/src/ViewUI.php
Auto-merging core/modules/user/tests/src/Unit/SharedTempStoreTest.php
Auto-merging core/modules/user/src/SharedTempStoreFactory.php
Auto-merging core/modules/user/src/SharedTempStore.php
Auto-merging core/lib/Drupal/Core/Session/SessionManager.php

Git took care of it :)

Great. Committed and pushed to 8.0.x. Thanks! :D

hm, committed, anyway i say why i use session_id() instead of getOwner() =)

getOwner() return session id for anonymous and user id for logged user.

For example, potential data loss for form preview still exist, but now for single user.
It is easy to reproduce:
1. Open node twice in one browser (tabs), edit -> preview etc. -> data loss.
2. Open node in different browsers on different devices, edit -> preview etc. -> data loss.

I think p.1 impossible on production, but p.2 can be possible and session_id() prevent p.2

In other words i think key for private storage BwXCiG05F8jwsT3mysaazmB-KOfiC8Ck-oHHah-eK0E:22d2527b-c027-4eff-bb11-1726c3da72b8 better than 1:22d2527b-c027-4eff-bb11-1726c3da72b8 =)

@formatC'vt - The problem with using the session_id would mean that a logged user who opens two tabs in the same browser would experience a different behaviour than a user who logged in using two different browsers. Does that really make sense? Also is it data loss when it is being knowing done by the same user?