Content moderation can wrongly set old revisions as default when they're resaved

+1.
I am also facing the same issue. I have a Drupal 8.6.16 and workbench_moderation version 1.5

+1. I don't use Workbench. I use the core moderation system, so I suspect this is a core bug.

+1
I just had this happen to me in Drupal 9.

As per #4, this is apparently happening with people using core's Content Moderation module (which makes sense, since it was mostly a port of the contrib Workbench Moderation module). Moving this to the core issue queue. Also marking #3226771: Deleting user account reverted all content authored by user to old revisions as a duplicate of this, as I assume that user was also using Content Moderation.

See also #2977362: Revision user incorrectly appears as anonymous user when node author is canceled. The user in comment #29 experienced this as well, though it's not clear they were using Content Moderation or Workbench Moderation at the time.

In our case an admin deleted a user and suddenly web reverted back to few months ago, we had to use a backup. This is a major bug because the damage that can be done to the website is enormous.

I can't provide more data because we are still investigating to reproduce the bug.

In our case, we have identified the cause of the activation of an outdated revision as the default revision and the duplication of revisions.

It is caused by the Content Moderation module.

This module intervenes in the process through a "presave" hook, and under certain circumstances, it incorrectly designates that revision as new and sets it as the default revision.

namespace Drupal\content_moderation\Entity\Handler;

[.....]

 /**
   * {@inheritdoc}
   */
  public function onPresave(ContentEntityInterface $entity, $default_revision, $published_state) {
    // When entities are syncing, content moderation should not force a new
    // revision to be created and should not update the default status of a
    // revision. This is useful if changes are being made to entities or
    // revisions which are not part of editorial updates triggered by normal
    // content changes.
    if (!$entity->isSyncing()) {
     $entity->setNewRevision(TRUE);
     $entity->isDefaultRevision($default_revision);
    }

    // Update publishing status if it can be updated and if it needs updating.
    if (($entity instanceof EntityPublishedInterface) && $entity->isPublished() !== $published_state) {
      $published_state ? $entity->setPublished() : $entity->setUnpublished();
    }
  }

Conditions for this case to occur:

• - The Content Moderation module is enabled.
• - The content type of the revision being modified is a moderated content.
• - The Moderation State of the revision is not empty.

At this point, a new revision will be created.

For this revision to be set as the default, the current default revision has to belong to other user and meet one of these conditions:

• - That the revision's state is considered a default revision state.
• - Or that the default revision at this moment is not in a published state.

This cases can be generated for a unappropriated configuration or other modules. We believe that the consequences are too severe, and it would be advisable to devise a strategy to ensure that at the very least, old revisions are not activated by any module.

To resolve our case, the patch from the related issue https://www.drupal.org/project/drupal/issues/2977362 solves this problem: https://www.drupal.org/files/issues/2023-05-25/2977362.patch.

As I mentioned in my previous comment (https://www.drupal.org/project/drupal/issues/3089747#comment-15212151), there is a patch that addresses the issue of obsolete revisions being triggered as default revisions in our project, but we believe it's not the best solution.

The revision that is being updated should not pass through the Content Moderation presave hook, as this modification is unrelated to a publishing workflow.

Here is the process of replacement:

• First, obtain the identifiers of all revisions generated by the user.
• Load the node of each revision.
• Replace the uid and review_uid with 0 in all active translations for the content (I've found the same revision ID assigned to a user in both French and English).
• Proceed with saving the node.

This process cause that any presave or postsave hook can alter the entity or run inappropriate processes in this context.

Bumping to critical and changing the title to make it clearer what the bug is.

It's a very drastic solution that needs careful consideration, but since this type of modification doesn't belong to an editorial workflow, would it be unreasonable for the solution to involve preventing the Content Moderation module hooks from being triggered?

We conducted a test, and by disabling these hooks, the process went smoothly

I think that I agree with the approach in #8 (setting the syncing flag prior to re-saving).

I think an optional parameter added to node_mass_update would be a fairly elegant solution:

function node_mass_update(array $nodes, array $updates, $langcode = NULL, $load = FALSE, $revisions = FALSE, $syncing = FALSE) {

  // ...
  // ...

    foreach ($nodes as $node) {
      if ($load && $revisions) {
        $node = $storage->loadRevision($node);
      }
+    if ($syncing) {
+      $node->setSyncing(TRUE);
+    }
      _node_mass_update_helper($node, $updates, $langcode);
    }
    \Drupal::messenger()->addStatus(t('The update has been performed.'));
}

then invoked via node_user_cancel:

      node_mass_update($vids, [
        'uid' => 0,
        'revision_uid' => 0,
      ], NULL, TRUE, TRUE, TRUE);

I guess my only other question is -- do we have to restore the previous value of the syncing flag afterwards, so we aren't polluting the node state?

Yeah I also think ::setSyncing() is the right approach here, user module is going back in time to alter metadata for old revisions, it's not really updating the content or anything.

Perfect. So, I'm going to try to improve the patch with Luke Leber's proposal.

I will also attempt to address the issue of replacing all authors of revisions with an anonymous user since the current patch doesn't resolve that.

This case doesn't occur exactly in the code snippet mentioned in my previous comment 8, so I'll need to find the exact location where it occurs.

Thank you!

I have just committed the proposed solution in the previous comments and generated the Merge Request.

Ready for review.

Thanks.

Appears to be an open thread.

Also can we get a test case showing the issue?

Added PHPUnit tests, waiting to see their success on the build.
Added tests only patch to check that is failing without the fixes on the MR.

Upload the right patch. ONLY PHPUNIT TESTS.

Seems like on the last version of Drupal (11.x) this error is not happening.

Currently, we are not able to reproduce this problem, so the PHPUnit test is not accurate.
Someone could provide the step by step to reproduce this problem?

Hate putting a critical in PNMI but seems steps to reproduce are needed to continue.

So if anyone experiencing this could add those.

Also tagged for IS so steps could be added in addition to the solution was used.

Leaving tests tag as #22 shows current test passes as is.

Just following up if anyone is still experiencing this?

I found this issue after a few hours debugging what when wrong when deleting a user and setting the content to anonymous.
Concluding that something weird was happening with revisions I added some isDefaultRevision checks to our custom code, but that didn't fix our problem. A very old revision was marked as the default and skipped by this check. That is when I found this issue and yes the patch from the MR worked for me.

The issue for us presented in the group from the entity_reference field being reset after a user delete. We generate a PDF on a node update which crashed so we where lucky that the batch stopped there. The field wasn't empty, it was set to an old target id, one that no longer exists. If the ID did exist we probably wouldn't have found the problem so early in the batch.

I don't know how you would reproduce this, The user we where deleting had a lot of content and a lot of revisions. I expect that if you don't have something like we had with a broken entity reference that most of the time this goes completely unnoticed.

Re-roll against D11.4.4. Some big structural changes here because the hooks are now class-based:

• node_mass_update() → NodeBulkUpdate::process()
• _node_mass_update_helper() → NodeBulkUpdate::processNode()
• _node_mass_update_batch_process → NodeBulkUpdate::batchProcess()
• node_user_cancel() → NodeUserHooks::userCancel()

#24 and #25 asked for steps to reproduce, and #26 asked whether anyone is still
hitting this. Yes: we hit it on production, and here is the reproduction,
verified on both 10.6.13 and 11.4.5.

There is also a finding about the patch in #29 that we think matters. In short:

* MR !4712 on 10.6.13: the four failures below all resolve.
* The patch in #29 on 11.4.5: it throws before processing a single revision, so
nothing is fixed and the account is still not cancelled.
* The patch in #29 with one line removed, on 11.4.5: the four failures all
resolve.

The approach is right. On 11.2 and later it trips a guard that did not exist
when it was written. Details at the bottom.
Steps to reproduce

1. Install a site with `content_moderation`, `content_translation` and at least
two additional languages.
2. Use a workflow whose published state has `default_revision: TRUE`. The
standard editorial workflow qualifies, since both `published` and `archived`
are default revision states.
3. Enable moderation for a content type.
4. As editor A, create a node in the default language and publish it.
5. Still as editor A, add a translation in the second language, change the
default language content as well, and publish. That is revision 2.
6. Repeat for the third language. There are now three revisions, and the
earliest predates two of the translations.
7. Cancel editor A's account with "Delete the account and make its content
belong to the Anonymous user" (`user_cancel_reassign`).

Expected: ownership transfers to the anonymous user and nothing else changes.
What actually happens

Four distinct problems.

The default revision moves.** Revision 3 was the default before
cancelling, revision 6 is afterwards on 11.4.5. Worth being precise here, because a complete run on 11.x looks deceptively
harmless: the replays happen oldest first, so the last one copies the newest
revision and the live title still reads "Revision 3". The default revision has
still moved to a replay, and the damage becomes visible as soon as the run does
not complete, which on our production site is exactly what happened. See point 4.
The revision history is multiplied.** Every historical revision is replayed
and saved as a new one, so three revisions become six on 11.4.5.

On 10.6 the same fixture produces nine, because `NodeStorage::userRevisionIds()`
selects from the revision data table without `DISTINCT` and returns each
revision once per translation. On 11.x the hook builds its own entity query and
takes `array_keys()` of the result, which is keyed by revision ID, so those
duplicates collapse. That part is already better on 11.x. The replay itself is
not.
Ownership is not actually transferred.** The anonymous owner is written
into the newly created revisions. The original revisions are never touched.
Measured on 11.4.5, where the departing account is uid 1:

default vid before: 3, after: 6, live title: 'Revision 3'

vid 1 owners: en=1
vid 2 owners: en=1 de=1
vid 3 owners: en=1 fi=1 de=1
vid 4 owners: en=0
vid 5 owners: en=0 de=0
vid 6 [default] owners: en=0 fi=0 de=0

Once the account is deleted, revisions 1 to 3 still reference a user ID that no
longer exists, which is the dangling reference the operation exists to prevent.
An interrupted run leaves old content published.** This is the one that
does the visible harm. Revisions are processed oldest first, so a run that stops
part way leaves an early revision installed as the default revision. Processing
only the oldest revision reverts the published title from "Revision 3" to
"Revision 1". Since the batch does abort in practice, a partial run is the normal
outcome on a real site, not an edge case.

Two production shapes, one cause

Worth recording, because the visible symptom differs completely between them and
neither one looks like "old revisions became default" at first sight.

Multilingual site. Translation rows are not deleted. The published revision
reverts, and with Paragraphs the reverted `target_revision_id` values point at
paragraph revisions that have since been deleted. Pages then render empty while
looking perfectly healthy in the database. On that site it left 23 published
pages referencing deleted paragraphs, and reached us as "the translations are
gone".

Single language site, on 11.4.5. No translations to lose, so the damage
surfaces as publication state instead. Cancelling an account that owned 774
nodes aborted after 99 seconds, having rewritten 589 nodes, of which 163 went
from unpublished to published. Content that editors had deliberately taken
offline was suddenly live, with nothing to signal it. The account was not
deleted either, so from the outside the operation merely looked like it had
failed.

The exception that aborted it is worth noting, because it is the same guard
discussed below:

Drupal\Core\Entity\EntityStorageException: An existing default revision of the
'node' entity type can not be changed to a non-default revision.
ContentEntityStorageBase.php:889

So on 11.2 and later that guard cuts both ways. It aborts the unpatched run part
way through, which is what turns this from a cosmetic revision problem into
content being left in the wrong state, and it also blocks the proposed fix.

The second shape is the more dangerous of the two, because a site with a single
language may reasonably assume this issue cannot affect it.
Measured on 11.4.5

We ported the reproduction to a kernel test, since `node_user_cancel()` and
`_node_mass_update_helper()` no longer exist on 11.x and the cancellation has to
be driven through the `user_cancel` hook itself. Against stock 11.4.5:

Tests: 4, Assertions: 48, Failures: 4.

1) testReassignDoesNotChangeTheDefaultRevision
The published revision is unchanged.
Failed asserting that 6 is identical to 3.
2) testReassignDoesNotCreateRevisions
No revisions were created.
Failed asserting that two arrays are identical.
[1, 2, 3] became [1, 2, 3, 4, 5, 6]
3) testReassignTransfersOwnership
Failed asserting that 1 is identical to 0.
4) testInterruptedReassignDoesNotPublishOldContent
The published content was not replaced by an older revision.
-'Revision 3'
+'Revision 1'

The test is attached as a patch that adds it to
`core/modules/node/tests/src/Kernel/`, so it can be applied and run directly. It
references nothing outside core and is not tied to any particular language set
or workflow configuration. Happy to turn it into an MR if that is more useful.
MR !4712 fixes it on 10.6

We applied the approach from MR !4712, setting the syncing flag before saving so
that Content Moderation leaves the new revision and default revision flags
alone, and re-ran the same four tests on 10.6.13:

```
OK (4 tests, 55 assertions)
```

All four failures resolved.
The patch in #29 throws on 11.2 and later

Running the patch from #29 on 11.4.5, against the same three revision, three
language fixture, it throws on the first revision it touches:

revisions before: 5, after: 5
default vid: 5 -> 5
live title: 'Revision 3' -> 'Revision 3'
threw: EntityStorageException: An existing default revision of the 'node'
entity type can not be changed to a non-default revision.

Nothing is damaged, but nothing is processed either and the account is not
cancelled. The same happens against our production copy: 0 of the 5,762
revisions the hook selects.
What changed

`ContentEntityStorageBase::doPreSave()` gained this guard:

$previously_default_revision = $entity->wasDefaultRevision();
$no_longer_default = !$entity->isDefaultRevision();
$original_same_as_current = $entity->getOriginal()?->getRevisionId() == $entity->getLoadedRevisionId();
$not_new_revision = !$entity->isNewRevision();
if ($previously_default_revision && $no_longer_default && $original_same_as_current && $not_new_revision) {
throw new EntityStorageException("An existing default revision ...");
}

It is absent in 10.6.x, 11.0.x and 11.1.x, and present from 11.2.x.

The precondition is common rather than exotic. Any workflow with more than one
`default_revision` state stores `revision_default = 1` on almost every revision,
so `wasDefaultRevision()` is TRUE while `isDefaultRevision()` is FALSE. On our
data that holds for 1,477 of the first 1,500 revisions checked.

The fix itself is what trips the guard, because suppressing Content Moderation
is precisely what makes `$no_longer_default` true. Before 11.2 that was
harmless.
One further line closes it

`processNode()` carries a line that predates the patch:

// For efficiency manually save the original node before applying any changes.
$node->setOriginal(clone $node);

Core only loads an original by itself when `!$entity->wasDefaultRevision()`, so
without this line `getOriginal()` stays NULL, `$original_same_as_current` is
FALSE and the guard does not fire. Supplying the original is what makes it TRUE.
The line is not new, 10.6 has it as `$node->original = clone $node`, it simply
had no guard to trip.

Removing that call, with the rest of the patch as posted, on 11.4.5, against the
same fixture:

revisions before: 5, after: 5
default vid: 5 -> 5
live title: 'Revision 3' -> 'Revision 3'
threw: no
ownership: vid1/en uid=0 rev_uid=0 | vid2/en uid=0 rev_uid=0 | vid2/de uid=0 rev_uid=0
| vid3/en ... | vid4/fi ... | vid5/de uid=0 rev_uid=0 (all 11 combinations)

No exception, no revision added, the default revision does not move, and every
translation of every revision is reassigned.

We have also run the same technique against the production copy, though through
our own implementation rather than this patch, since we needed a wider query
than the hook uses. That transferred 7,035 revisions in about two and a half
minutes with no node lost, no default revision moved and no publication state
changed, which at least says the approach holds at that size.
What we could not reproduce

We could not reproduce the `LogicException` "The default translation flag cannot
be changed", which is what aborted the batch on the multilingual site. It needs
a divergence between the moderation state entity and the replayed revision that
we were unable to construct in a minimal fixture. The four failures above occur
without it.
Environment

Reproduced on Drupal 10.6.13 and 11.4.5, PHP 8.3, MySQL 8.0, against a minimal
fixture and against a production copy of roughly 5,000 nodes and 28,000
revisions.