_node_revision_access() is a menu callback that checks whether the current user can do a particular operation to a node. Like a good citizen, it caches its results in a static variable.

However, it only caches the vid of the node it checked access for. It omits the operation. This means that if the callback is used to check access for two different operations in the same page load, only permissions for the first access check are reflected. For example, if the current user has the 'view revisions' permission but not 'update revisions':

_node_revision_access($node, 'view'); // returns TRUE
_node_revision_access($node, 'update'); // also returns TRUE, should return FALSE!

This function is an 'internal' function by its naming pattern, and folks can work around this bug by clearing the '_node_revision_access' static variable (drupal_static('_node_revision_access', array(), TRUE)), but this should still be fixed. Patch attached.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

Interesting, that patch should not have passed. Sounds like a bug in the testbot to me.

When creating patches, make sure that the new file name (file name on second line of the patch) matches the actual filename of the file. You should be able to fix it manually directly in the patch and re-upload.

I guess a test would be useful here as well and this will need to be backported to D6.

Oh and for anyone reading and wondering, I've check with the security team that this can not be exploited.

becw’s picture

Ah, that was a silly error on my part!

And it's true, this can not be exploited unless someone reuses it in a non-core module--which is not recommended, since the function name begins with an underscore (this convention indicates something is meant as an "internal" function). However, if a module is interacting with revisions, it may be tempting to re-use this function in order to re-use the core logic for determining revision access.

Corrected patch attached, no tests though.

Berdir’s picture

Status: Needs work » Needs review

Remember to set issues to needs review so that patches are tested.

greggles’s picture

Version: 7.0 » 7.x-dev
Issue tags: +Security improvements

Given that there are no known vulnerabilities in core or contrib associated with this bug it can be fixed publicly, but in general this kind of thing should have followed the process to report a security issue first and then could be made public after that.

greggles’s picture

Priority: Normal » Major

And the priority should probably be higher.

becw’s picture

This is an issue for D8 as well. The patch from #2 applies to D8 with some fuzz. Here are D7 and D8 versions that apply cleanly.

catch’s picture

Version: 7.x-dev » 8.x-dev
Issue tags: +Needs backport to D7
becw’s picture

catch’s picture

Priority: Major » Critical
Status: Needs review » Needs work

Moving this to critical since it's security (even if probably very minor), and CNW for tests.

becw’s picture

I'm sorry, what does "CNW" mean?

Also, what work does this patch need? I posted versions for both Drupal 7 and Drupal 8 in comment #6, and they both passed testing against their respective branches.

catch’s picture

CNW/code needs work.

Needs tests because the existing tests we have aren't catching the bug, so we should add additional test coverage that would fail without the patch being applied.

xjm’s picture

Assigned: Unassigned » xjm

I'll work on a test for this.

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
6.34 KB
4.39 KB

So, here's a test. Couple notes:

  1. I'm using a test module to check the return values from _node_revision_access() because otherwise it uses the uid of the parent site session.
  2. I created a new module to avoid any conflict with the various hook implementations in node_test.module.
  3. The test coverage for revision access is pretty thin. Access being allowed is tested implicitly, because existing tests grant all revision permissions to the test users. However, we might want to open a followup issue to add test coverage for access being denied.
  4. To expose access being denied for view revisions permission, we'd need to add node access into the mix, because otherwise the global all grant obscures access to the revision. (That, and/or the view revisions permission doesn't actually do anything and it shouldn't exist.)
  5. The weird crap in setUp() is from other node revision tests.
  6. I hate node revisions.
  7. Kittens, however, are awesome.

Status: Needs review » Needs work

The last submitted patch, 1064954-complete.patch, failed testing.

xjm’s picture

xjm’s picture

+++ b/core/modules/node/node.testundefined
@@ -262,6 +262,87 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
+  /**
+   * Tests multiple calls to _node_revision_access() within the same request.
+   */
+  function testNodeRevisionAccessCaching() {
+    foreach ($this->users as $user_op => $user) {
+      $this->drupalLogin($user);
+      $this->drupalGet('node_revision_access_test/' . $this->node_revisions[1]->nid . '/' .  $this->node_revisions[1]->vid);
+      foreach (array_keys($this->map) as $test_op) {
+        if ($test_op == $user_op) {
+          $this->assertText(format_string("@op granted", array('@op' => $test_op)));
+        }
+        else {
+          $this->assertNoText(format_string("@op granted", array('@op' => $test_op)));
+        }
+      }
+    }
+  }
+}

Wrong diff apparently; there are a couple of inline comments I'd added to this test method that are missing here. I'll fix that.

xjm’s picture

FileSize
1.39 KB
6.76 KB
4.81 KB

Added those comments back. I also adjusted the test method to use assertNoUniqueText() (as the text should appear twice). That was naggling overnight.

Tor Arne Thune’s picture

The code style looks great, except for these two:

+++ b/core/modules/node/node.testundefined
@@ -262,6 +262,87 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
+      'description' => 'Tests user permssions for node revision operations.',

permssions -> permissions.

+++ b/core/modules/node/node.testundefined
@@ -262,6 +262,87 @@ class NodeRevisionsTestCase extends DrupalWebTestCase {
+      $this->drupalGet('node_revision_access_test/' . $this->node_revisions[1]->nid . '/' .  $this->node_revisions[1]->vid);

Superfluous space.

I've tried to follow the logic and it looks ready to go for me, but I'm not that familiar with the revision system. The test fails without the fix and passes with the fix, so what's left to ask for? :)

xjm’s picture

@Tor Arne Thune, I don't see the superfluous space?

Tor Arne Thune’s picture

Two spaces in front of $this->node_revisions[1]->vid);

xjm’s picture

So there are. :) OK, cleaning that up too.

xjm’s picture

FileSize
1.15 KB
5.12 KB

Aforementioned style fixes; no other changes.

xjm’s picture

FileSize
6.76 KB

Oops, #22 is incomplete.

Dave Reid’s picture

Here's my proposal to fix this issue by adding an optional $account parameter for _node_revision_access() which appropriately gets passed along to all the user_access() and node_access() calls. This change is safe to backport to D7 as well since it is an API addition rather than a change. This patch also adds tests for a user with the 'administer nodes' permission, as well as a user who has no revision-related permissions at all.

Dave Reid’s picture

Without undefined $map in the test assertion message this time.

xjm’s picture

  • So, while the lack of the $account parameter drove me bonkers when I was working on this, I was really reluctant to add it here because it felt like scope creep. However, the patch is so much nicer that I think I'm changing my mind.
  • I like the new cache ID format better.
  • The test method now covers cache invalidation for both the account and op parameters.
  • generatePermutations() is an awesome method I didn't know about.
  • I'd like to open a followup to clean up the setUp() for the other test case in this manner as well.
+++ b/core/modules/node/node.moduleundefined
@@ -1908,21 +1908,41 @@ function theme_node_search_admin($variables) {
+ * @param obejct $account

Typo: "obejct"

+++ b/core/modules/node/node.moduleundefined
@@ -1908,21 +1908,41 @@ function theme_node_search_admin($variables) {
+  // Statically cache access by node revision ID, user account ID, and operation.

+++ b/core/modules/node/node.testundefined
@@ -2300,3 +2300,88 @@ class NodeTokenReplaceTestCase extends DrupalWebTestCase {
+    // Create an admin account (should return TRUE for all revision permissions).
...
+    // Create a normal account (should return FALSE for all revision permissions).

Looks like these comments are ever-so-slightly over 80 chars.

Dave Reid’s picture

Fixes issues addressed in #26 and also adds test coverage for invalid $node and $op parameters, as well as testing that $account defaults to the global user object.

Dave Reid’s picture

BLARGH GRAMMAR

xjm’s picture

Assigned: xjm » Unassigned
Status: Needs review » Reviewed & tested by the community

I approve of this patch.

webchick’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

I was a little bit concerned about:

-  return $access[$node->vid];
+
+  return $access[$cid];

However, ultimately this is just an internal refactor, that moves the access check from 2D to 3D. Seems sensible to me, and comes with good test coverage.

Committed and pushed to 8.x. However, looks like it needs a re-roll for 7.x.

xjm’s picture

Okay, so wow, this function doesn't even have a bedamned docblock in D7? How did that happen? I'll work on the reroll.

Edit: Ah, #1313980: Clean up API docs for node module never got backported. Going to backport the full docblock anyway since this is a critical (duh). That node docs backport is going to be damn hard to reroll already anyway.

webchick’s picture

Bedamned is officially my new favourite word.

Thanks, xjm!!

xjm’s picture

Status: Patch (to be ported) » Needs review
FileSize
7.21 KB

Needs review since I did some cut-and-paste action by hand. Also trying out for the first time using -d7 to mean simply "this is a patch for d7" rather than "testbot should categorically ignore this patch under all circumstances." Yay!

amateescu’s picture

Status: Needs review » Reviewed & tested by the community

The D7 reroll looks good to me. Down with criticals!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Awesome, thanks a lot for the fast turnaround on this, folks!

Committed and pushed to 7.x.

Status: Fixed » Closed (fixed)

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

anrikun’s picture

Version: 7.x-dev » 6.x-dev
Status: Closed (fixed) » Patch (to be ported)
Issue tags: +Needs backport to D6

What about D6?

Status: Patch (to be ported) » Closed (outdated)

Automatically closed because Drupal 6 is no longer supported. If the issue verifiably applies to later versions, please reopen with details and update the version.