We need to get the SA-CORE-2011-002 changes into D8.

Here's the patch from chx.

To summarize a great deal of discussion, basically taxonomy listings were not respecting node access because the queries did not specify a $query->addMetaData('base_table', 'taxonomy_index');. This new requirement has been posted at http://drupal.org/node/1204572. However, to care for existing contrib modules, a fallback mechanism was put in place which attempts to guess the base table if it's not specified.

I don't think the fallback mechanism makes any sense in D8. Not sure if we should just commit this and keep iterating on it, or if we want to come up with a new patch. Let's discuss!

Remaining Tasks

  • #44 Address: Sounds like we should throw an exception if a table is specified that doesn't exist? Right now it seems that we ignore it silently in that case?
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

The last submitted patch, sdo_43599_26.patch, failed testing.

catch’s picture

Issue tags: +Needs backport to D7

The patch is failing to apply due to #1136130: Regression: Reinstate WATCHDOG_* constants and document why they are necessary., which happens to be RTBC. Not sure if there are other conflicts after that but seems pointless to re-roll unless something specific is holding that other patch up.

Even with the addMetaData() the node access system still would not have correctly rewritten the queries if I understand correctly. Also those addMetaData calls are not added here, I would think we need those for at least taxonomy and forum, maybe tracker too. Those could be backported to Drupal 7 for clarity, for some reason I thought they were in the original patch. Adding Needs backport to D7 tag so that doesn't get lost - if it ends up happening in a followup issue we can untag this one.

My preference would be to commit the patch as it went into Drupal 7, then post a major followup for removing the fallback and ensuring everything works, doing it the other way around tended to leave SA followups lingering in the queue for months during D7.

I'm also wondering now if the fallback added here will have changed behaviour of EntityFieldQuery for queries involving node reference fields with an nid column.

webchick’s picture

Category: bug » task
Priority: Critical » Major

Ok great, #1136130: Regression: Reinstate WATCHDOG_* constants and document why they are necessary. was committed, so I committed and pushed this patch to 8.x in order to get the two in sync again.

Reducing from a critical bug to a major task to figure out the optimal solution to this for D8, now that we have some breathing room.

catch’s picture

@todos (may well be more than this):

Add the addMetaData() to at least taxonomy, forum and tracker queries.

Remove the fallback mechanism and throw a big error if the query is tagged node_access and does not add metadata.

Not sure what we should do if the base table is node - add metadata anyway or not?

pillarsdotnet’s picture

Note that the tests added in this issue wrongly assume that the "Read more" link will always be present for teaser nodes.

This is blocking: #823380: Read More link is always visible on teaser.

See also: #1300568: Fix tests that wrongly assume all teasers have a Read more link

xjm’s picture

This issue is only 5 comments long but I'm still confused. :) As far as I understand:

  • The SA forward-port has been resolved--so should we retitle this issue?
  • #4 seems to describe the current goal/scope? Kinda? Can we clarify?
  • #5 is not actually related to this issue.
Berdir’s picture

Status: Needs work » Needs review
FileSize
2.54 KB

Yes, I think there are two tasks here:

- Remove the magic fallback handling.
- Fix all instances which should provide a base_table.

The patch does the first thing, still keeps node as the default. Everything that does miss the base table should now fall on it's nose and throw an exception.

I guess the base_table definitions could then be backported to 7.x so that core doesn't have to rely on it's own fallback handling.

Let's see how many exception this gives us.

Status: Needs review » Needs work

The last submitted patch, remove_fallback-1204658.patch, failed testing.

xjm’s picture

Title: SA-CORE-2011-002 - Drupal core - Access bypass » Remove node access base table fallback
Berdir’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests
FileSize
5.89 KB

Ok, looks like we have test coverage for comments, taxonomy and forum (maybe not all, but at least some queries there).

Leaves tracker.module...

The attached patch fixes all missing base_tables I think.

webchick’s picture

Issue tags: -Needs backport to D7

Removing this tag since it doesn't make sense to backport the non-BC-compatible change to D7.

xjm’s picture

This looks correct to me. I might like to add the metadata immediately after the node access table and add a comment in each case indicating why it matters, since it's rather opaque otherwise.

Looks like we need to add test coverage for the forum, taxonomy, and tracker cases (because no failures for those are exposed when the fallback is ripped out without the metadata in #7).

Berdir’s picture

@webchick:

While the removal of the fallback is obviously not backportable, the actual changes in forum/comment/taxonomy/tracker can and imho should be backported. Core should implement it's own API's correctly and be an example for contrib and not rely on it's own security fallbacks. Same for the additional tests that are going to be added here.

webchick’s picture

Issue tags: +Needs backport to D7

Ah, well that's fair enough.

Berdir’s picture

There are already tests for taxonomy and forum in node.test (which should probably be moved, but that's another story). So that means we're left with tests for tracker.module. That one should be similar to e.g. NodeAccessBaseTableTestCase.

- Enable the tracker and node_access_test modules
- call variable_set('node_access_test_private') and node_access_rebuild(). Just like the test class mentioned above.
- Create two users, one with the permission "'node test view'" and one without.
- Create a few nodes
- Make sure the user with the node test view permission can see the nodes, the other can't.

kbasarab’s picture

Alright forgive me if this isn't quite right but I think I have this working at least basic functionality.

Three patches attached --
TrackerNodeAccessTest.patch - Simply the test cases for tracker module as documented in #15

TrackerNodeAccess-Pass.patch - This one rolls #10 in and adds the test case. Shows test case passing with patch from #10 untouched.

TrackerNodeAccess-Fail.patch - This one rolls #10 in but removes ->addMetaData in tracker/tracker.pages.inc though to show that the SimpleTest fails when that is removed from Tracker.

I know there are probably some more coding standards issues I probably missed and other ideas to make it better so have at it and let me know what should change. Trying to start helping out with core and this is the first core patch submission. Yay.

Status: Needs review » Needs work

The last submitted patch, TrackerNodeAccessTest.patch, failed testing.

kbasarab’s picture

Think I had to change to DrupalWebTestCase over to WebTestBase. Trying just the tracker test for now before rerolling the fail/pass patches.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
2.25 KB

Forgot to change status.

kbasarab’s picture

So patch in #19 is just the patch for the test. Attached two are applying patch from #10 and one is my test against #10 while the fail patch is my patch against #10 but with the addMetaData on Tracker removed.

Berdir’s picture

Status: Needs review » Needs work

Great, logic looks good to me, just some coding style stuff to clean up. See details below.

+++ b/core/modules/forum/forum.moduleundefined
+++ b/core/modules/forum/forum.moduleundefined
@@ -677,7 +677,8 @@ function forum_block_save($delta = '', $edit = array()) {

@@ -677,7 +677,8 @@ function forum_block_save($delta = '', $edit = array()) {
 function forum_block_view($delta = '') {
   $query = db_select('forum_index', 'f')
     ->fields('f')
-    ->addTag('node_access');
+    ->addTag('node_access')
+    ->addMetaData('base_table', 'forum_index');

I guess this one doesn't have test coverage yet...

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,63 @@
+/**
+* @file
+* Node Access tests
+*

No need to reference to the issue unless there really is something very important in there.

The standard for PSR-0 files is simply "Definition of Fully\Qualified\Classname.".

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,63 @@
+    $modules = array('node','comment','entity','text','field_sql_storage','tracker','node_access_test');

entity, text and field_sql_storage modules should be installed by default, you shouldn't need to specify them explicitly.

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,63 @@
+  function testTrackerNodeAccess() {
+    // Create user with node test view permission

setUp() and getInfo() don't need them, but each test*() mthod should have a simple one-line docblock that explains what is being tested here.

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,63 @@
+    // Create user without node test view permission
+    $no_access_user = $this->drupalCreateuser();
+
+    $this->drupalLogin($access_user);
+
+    // Create some nodesi
+    $private_node = $this->drupalCreateNode(array(

Comments should always end with a ".". Also the second one seems to have an "i" that doesn't belong there :)

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,63 @@
+    $this->drupalGet('tracker');
+    $this->assertText($private_node->title, t('Private node is visible to user with private access.'));
+    $this->assertText($public_node->title, t('Public node is visible to user with private access.'));
+
+    // User without access should not see private node
+    $this->drupalLogin($no_access_user);
+    $this->drupalGet('tracker');
+    $this->assertNoText($private_node->title, t('Private node is not visible to user without private access.'));
+    $this->assertText($public_node->title, t('Public node is visible to user without private access.'));

Assertion messages don't need t() anymore.

kbasarab’s picture

We'll try this one for the styling issues.

+++ b/core/modules/forum/forum.moduleundefined
+++ b/core/modules/forum/forum.moduleundefined
@@ -677,7 +677,8 @@ function forum_block_save($delta = '', $edit = array()) {

@@ -677,7 +677,8 @@ function forum_block_save($delta = '', $edit = array()) {
function forum_block_view($delta = '') {
   $query = db_select('forum_index', 'f')
     ->fields('f')
-    ->addTag('node_access');
+    ->addTag('node_access')
+    ->addMetaData('base_table', 'forum_index');

I guess this one doesn't have test coverage yet...

I was just testing in Tracker. To test for that just to see if I'm right here:

1. Add tests to forum.test since it hasn't been converted yet.
2. Create a private forum topic
3. Place the forum block on a page
4. See if private node access user can see and non private node access user cannot
5. Try same thing for a public forum topic?

Berdir’s picture

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,65 @@
+
+class TrackerNodeAccessTest extends WebTestBase {

The class should have a comment as well, see http://drupal.org/node/1354#classes for guidelines.

Also, according to our guidelines, it *might* not be necessary to keep using the Tracker prefix here. However, we *are* testing node access for the tracker pages, so I'm not sure that NodeAccess.php makes sense.

Thoughts, anyone?

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,65 @@
+  // Ensures private nodes are only visible to users ¶
+  // with permission on /tracker.

See the link above, this should be a docblock comment (/** ... */).

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,65 @@
+    $this->assertNoText($private_node->title, 'Private node is not visible to user without private access.');
+    $this->assertText($public_node->title, 'Public node is visible to user without private access.');
+  }
+}
+?>

There shouldn't be a closing php tag but make sure to keep the newline at the end, git diff doesn't like it if it's not there.

kbasarab’s picture

Status: Needs work » Needs review
FileSize
12.14 KB

Lets try this one. This adds in forum fallback testing as well. Full patch attached.

Made the styling modifications you mentioned berdir also. Just gotta get all of them stuck into my head.

tim.plunkett’s picture

Status: Needs review » Needs work

Coding style review:

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+ */
+
+class ForumNodeAccessTestCase extends WebTestBase {

Remove the blank line

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $modules = array('node','comment','forum','taxonomy','tracker','node_access_test','block');
+    parent::setUp($modules);

These commas need spaces after them, and this can just be combined with the setUp

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+   * Creates some users and creates a public node and a private node.
+   * Adds both Active forum topics and new forum topics blocks to the sidebar.

Add a space between these two lines, and decapitalize Active

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $this->drupalPost('node/add/forum/1',$edit, t('Save'));

Missing space after the comma

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $this->assertTrue(!empty($private_node). 'New private forum node found in database.');

That full stop should be a comma.

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $this->drupalPost('node/add/forum/1',$edit, t('Save'));

Missing a space after the comma

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $this->assertTrue(!empty($public_node). 'New public forum node found in database.');

That full stop should be a comma.

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $this->assertText(t('The block settings have been updated.'), t('Active forum topics forum block was enabled'));

Remove the t() from the assertion message (right side)

+++ b/core/modules/forum/forum.testundefined
@@ -653,3 +653,95 @@ class ForumIndexTestCase extends WebTestBase {
+    $this->assertText(t('The block settings have been updated.'), t('[New forum topics] Forum block was enabled'));

Remove the t() from the assertion message (right side)

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,70 @@
+<?php
+/**
+* @file
+* Definition of Drupal\tracker\Tests\TrackerNodeAccessTest.
+*
+*/

Missing a blank line between php and the comment, and the comment is indented incorrectly

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,70 @@
+Namespace Drupal\tracker\Tests;

Namespace should be namespace

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,70 @@
+    $modules = array('node','comment','tracker','node_access_test');
+    parent::setUp($modules);

Same as above (commas, combine)

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,70 @@
+   * Ensures private nodes are only visible to users ¶
+   * with permission on /tracker.

Reword so this fits under 80 character, and watch the trailing whitespace

+++ b/core/modules/tracker/lib/Drupal/tracker/Tests/TrackerNodeAccessTest.phpundefined
@@ -0,0 +1,70 @@
+}
+
diff --git a/core/modules/tracker/tracker.pages.inc b/core/modules/tracker/tracker.pages.inc

Remove this extra blank line

kbasarab’s picture

Status: Needs work » Needs review
FileSize
5.74 KB
13.96 KB

Thanks for review Tim. Silly mistakes. Rerolled and updated:

kbasarab’s picture

Found two additional comment style changes inside ForumTest.php.

tim.plunkett’s picture

FileSize
12.19 KB

Part of this was added in http://drupalcode.org/project/drupal.git/commit/352645e, and the rest was moved to PSR-0. Reroll.

tim.plunkett’s picture

Issue tags: -Needs tests

Still applies.

chx’s picture

Status: Needs review » Reviewed & tested by the community

I think this is good to go. As a reminder:

Also those addMetaData calls are not added here, I would think we need those for at least taxonomy and forum, maybe tracker too. Those could be backported to Drupal 7 for clarity, for some reason I thought they were in the original patch. Adding Needs backport to D7 tag so that doesn't get lost - if it ends up happening in a followup issue we can untag this one.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Glad that magic code is gone. Thanks all.

webchick’s picture

Status: Fixed » Patch (to be ported)

Not sure if we can backport this, but moving to 7.x for discussion.

Berdir’s picture

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

What we can backport are the explicit MedataData additions, so that core doesn't rely on it's own table fallback mechanism magic and the tests.

Here's a patch for that, haven't run the tests, no idea if that will work.

Status: Needs review » Needs work

The last submitted patch, node-access-metadata-tests-1204658-33.patch, failed testing.

Berdir’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs work » Needs review

Hm, nobody actually changed the version back to 7.x. Might work better then.

Also, does this need a change notice for 8.x?

Berdir’s picture

Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Security Advisory follow-up, +Needs backport to D7

The last submitted patch, node-access-metadata-tests-1204658-33.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
9.36 KB

Fixed the base class name and also made sure that the tests actually pass...

YesCT’s picture

xjm’s picture

Title: Remove node access base table fallback » Use query metadata to specify the node access base table
Priority: Major » Normal
Status: Needs review » Reviewed & tested by the community
Issue tags: +Novice

I think this is just a normal task for D7, since we are essentially just deprecating the base table fallback in #38 rather than removing it.

I also filed #1885420: Document the use of metadata to indicate a base table for node access queries.

xjm’s picture

Title: Use query metadata to specify the node access base table » Always use query metadata to specify the node access base table
David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs work
@@ -32,6 +32,7 @@ function tracker_page($account = NULL, $set_title = FALSE) {
   // while keeping the correct order.
   $nodes = $query
     ->addTag('node_access')
+    ->addMetaData('base_table', 'tracker_node')

This looks wrong - the query is actually sometimes against the 'tracker_user' table instead. I am not positive, but I think this would introduce a security issue?

---

Other issues (all very minor, but while I was reviewing):

  1. +  protected $access_user;
    +  protected $admin_user;
    +  protected $no_access_user;
    

    These variables in the forum test (but also similar ones in the tracker tests) don't look like they are ever used.

  2. There are some spacing/newline issues throughout the tests.
  3. +    $this->drupalGet('/');
    

    Hm, I guess this works (though with an extra slash in the test URLs) - seems like it should just be $this->drupalGet('') though.

  4. +    // Create some nodes.
    +    $private_node = $this->drupalCreateNode(array(
    +      'title' => t('Private node test'),
    +      'private'=> TRUE,
    +    ));
    +    $public_node = $this->drupalCreateNode(array(
    +      'title' => t('Public node test'),
    +      'private'=>FALSE,
    +    ));
    

    Spacing issues on both 'private' lines.

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Priority: Normal » Critical
Issue tags: +Needs change record

The issue with the tracker module seems to be present in Drupal 8 as well. Since it looks like a security issue, I'm bumping to critical (I don't see any tests that would have been related to node access on the per-user tracker page, so I'm still not positive it has security implications but I think it does). For what it's worth, most of the minor issues identified above look to be in Drupal 8 too.

Also, does this need a change notice for 8.x?

Yikes, I would definitely think so.

I don't see an existing one after a quick search, and the Drupal 8 patch in this issue changed the API in a way that would have security implications for contrib modules so it definitely needs one. Another reason to bump this to critical...

Berdir’s picture

Status: Needs work » Needs review
Issue tags: -Novice
FileSize
2.67 KB
4.74 KB

Sorry for not getting back to this earlier.

Yes, you were of course right, that is a security problem. Sounds like we should throw an exception if a table is specified that doesn't exist? Right now it seems that we ignore it silently in that case?

Fixed what I could find, not everything existed in 8.x

Status: Needs review » Needs work

The last submitted patch, node-access-table-1204658-44-tests-only.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review

Passed/Failed as expected, just in the wrong order :) Back to needs review.

Who wants to RTBC? :) Edit: Forgot my own question in #44, probably needs to be answered first.

xjm’s picture

I'd tentatively say that throwing an exception sounds like a much better idea than failing silently.

Berdir’s picture

Ok.

We still need the additional test coverage to actually trigger this bug but now it explodes with a meaningful error message (I think) and doesn't just silently not work.

benjifisher’s picture

#48: node-access-table-1204658-48.patch queued for re-testing.

druderman’s picture

Looking at issue via #drupalcon sprint no longer looking at this.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Sweet!

catch’s picture

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

Committed/pushed to 8.x, moving to 7.x for backport.

Berdir’s picture

Priority: Critical » Normal

Great, back to normal, as this is just a non-critical cleanup there

Berdir’s picture

Issue summary: View changes

added remaining tasks

  • webchick committed aa0aa46 on 8.3.x
    Issue #1204658 by chx: Fixed SA-CORE-2011-002 - Drupal core - Access...
  • Dries committed b4f471e on 8.3.x
    - Patch #1204658 by kbasarab, Berdir, tim.plunkett, webchick: remove...
  • Dries committed d1cb125 on 8.3.x
    - Patch #1204658 by kbasarab, Berdir, tim.plunkett, webchick: remove...
  • catch committed 0d3c967 on 8.3.x
    Issue #1204658 by kbasarab, Berdir, tim.plunkett, webchick: Always use...

  • webchick committed aa0aa46 on 8.3.x
    Issue #1204658 by chx: Fixed SA-CORE-2011-002 - Drupal core - Access...
  • Dries committed b4f471e on 8.3.x
    - Patch #1204658 by kbasarab, Berdir, tim.plunkett, webchick: remove...
  • Dries committed d1cb125 on 8.3.x
    - Patch #1204658 by kbasarab, Berdir, tim.plunkett, webchick: remove...
  • catch committed 0d3c967 on 8.3.x
    Issue #1204658 by kbasarab, Berdir, tim.plunkett, webchick: Always use...

  • webchick committed aa0aa46 on 8.4.x
    Issue #1204658 by chx: Fixed SA-CORE-2011-002 - Drupal core - Access...
  • Dries committed b4f471e on 8.4.x
    - Patch #1204658 by kbasarab, Berdir, tim.plunkett, webchick: remove...
  • Dries committed d1cb125 on 8.4.x
    - Patch #1204658 by kbasarab, Berdir, tim.plunkett, webchick: remove...
  • catch committed 0d3c967 on 8.4.x
    Issue #1204658 by kbasarab, Berdir, tim.plunkett, webchick: Always use...

  • webchick committed aa0aa46 on 8.4.x
    Issue #1204658 by chx: Fixed SA-CORE-2011-002 - Drupal core - Access...
  • Dries committed b4f471e on 8.4.x
    - Patch #1204658 by kbasarab, Berdir, tim.plunkett, webchick: remove...
  • Dries committed d1cb125 on 8.4.x
    - Patch #1204658 by kbasarab, Berdir, tim.plunkett, webchick: remove...
  • catch committed 0d3c967 on 8.4.x
    Issue #1204658 by kbasarab, Berdir, tim.plunkett, webchick: Always use...

  • webchick committed aa0aa46 on 9.1.x
    Issue #1204658 by chx: Fixed SA-CORE-2011-002 - Drupal core - Access...
  • Dries committed d1cb125 on 9.1.x
    - Patch #1204658 by kbasarab, Berdir, tim.plunkett, webchick: remove...
  • Dries committed b4f471e on 9.1.x
    - Patch #1204658 by kbasarab, Berdir, tim.plunkett, webchick: remove...
  • catch committed 0d3c967 on 9.1.x
    Issue #1204658 by kbasarab, Berdir, tim.plunkett, webchick: Always use...