ModerationInformation::isLatestRevision() unconditionally calls isLatestRevision() on ModerationInformation::getLatestRevision(), but ModerationInformation::getLatestRevision() can return NULL, which causes a fatal error. This is precisely what happens on the node/{nid}/latest path if you have node access in place. Here are steps to reproduce:

  1. Install Drupal using the Standard install profile.
  2. Install workbench_moderation.
  3. Create a user role with permission create Articles and permission to perform all the default state transitions.
  4. Create a user with that role.
  5. Log out and back into the site as the new user. (User 1 bypasses all access checks and thus won't see the bug.)
  6. Create an article and save it in the Draft state.
  7. You should be redirected to node/1/latest, and it should display just fine.
  8. Install any node access module. I used view_unpublished. (Don't rebuild the node access permissions yet.)
  9. Reload node/1/latest, and it should display just fine.
  10. Rebuild node access permissions and reload node/1/latest.
  11. Bug: You will get an error like this:

    Fatal error: Call to a member function getRevisionId() on null in /var/www/d8/modules/contrib/workbench_moderation/src/ModerationInformation.php on line 177

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TravisCarden created an issue. See original summary.

TravisCarden’s picture

Assigned: TravisCarden » Unassigned
Status: Active » Needs review
FileSize
740 bytes

Here's a patch the prevents the fatal error by making ModerationInformation::isLatestRevision() return FALSE if it doesn't get a valid revision--but that leads to weird semantics and the side effect that the node/{nid}/latest page returns a 404 rather than a 403. I'm mostly curious what the testbot will say.

Status: Needs review » Needs work

The last submitted patch, 2: 2665708-2.patch, failed testing.

  • Crell committed 5037f30 on authored by TravisCarden
    Issue #2665708 by TravisCarden: ModerationInformation::isLatestRevision...
Crell’s picture

Status: Needs work » Fixed

(The test fail was just a botfail, works on second try.)

I hate NULL. I really really do. But yeah, given what entity API itself does this is the best we can do.

I don't know what to do with the 403/404 question. Given Entity API, I don't suspect there's a way we could turn that into a 403. Or rather, not without a huge amount of work.

I've committed this for the time being, because fatals == bad. :-) If someone has a good idea for the error code question later, please open a new issue. Thanks, Travis!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.