See: https://www.drupal.org/SA-CORE-2015-003
http://cgit.drupalcode.org/drupal/commit/?h=7.x&id=731dfacab8bf39918c135...

Users without the "access content" permission can see the titles of nodes that they do not have access to, if the nodes are added to a menu on the site that the users have access to.

This issue needs verifying against Drupal 8, since so much of the menu system was changed.

Credit for the D6/D7 version of this patch (the security release):

David_Rothstein, matt2000, scor, greggles, meichr, larowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick created an issue. See original summary.

webchick’s picture

Assigned: Unassigned » webchick

Going to attempt to verify this one.

webchick’s picture

Nope, this isn't an issue in Drupal 8.

Steps to reproduce (/via David_Rothstein):

1. Create a node and add it to the main menu.
2. Remove the "access content" permission from anonymous users.
3. Log out and view the home page.

Works as expected.

webchick’s picture

Menu link access denied for anon.

larowlan’s picture

Issue summary: View changes
Status: Active » Closed (cannot reproduce)
FileSize
151.46 KB

Confirming cannot reproduce in D8

dawehner’s picture

Is it just me or should we maybe have some dedicated test coverage for it? I mean its like a regression we should absolute never have again.

webchick’s picture

Title: Port Information Disclosure in Menu Links - Access system fixes from SA-CORE-2015-003 to Drupal 8 » Tests for Information Disclosure in Menu Links - Access system fixes from SA-CORE-2015-003 to Drupal 8
Assigned: webchick » Unassigned
Priority: Critical » Major
Status: Closed (cannot reproduce) » Active
Issue tags: +Needs backport to D7, +Needs tests

That's true. We could totally do that.

webchick’s picture

Title: Tests for Information Disclosure in Menu Links - Access system fixes from SA-CORE-2015-003 to Drupal 8 » Tests for Information Disclosure in Menu Links - Access system fixes from SA-CORE-2015-003
StryKaizer’s picture

Status: Active » Needs review
FileSize
1.84 KB

Here's a test for D8.

As noted, this test does not fail, since this bug is not an issue at this moment in d8.

StryKaizer’s picture

Documentation fixes

stefan.r’s picture

Status: Needs review » Needs work
Issue tags: -Needs tests
  1. +++ b/core/modules/node/src/Tests/NodeAccessMenuLinkTest.php
    @@ -0,0 +1,69 @@
    +namespace Drupal\node\Tests;
    +use Drupal\user\RoleInterface;
    

    Nit: needs a newline between these.

  2. +++ b/core/modules/node/src/Tests/NodeAccessMenuLinkTest.php
    @@ -0,0 +1,69 @@
    + * Tests the interaction of the node access system with fields.
    

    s/fields/menu links/

  3. +++ b/core/modules/node/src/Tests/NodeAccessMenuLinkTest.php
    @@ -0,0 +1,69 @@
    +  public static $modules = array('menu_ui', 'block');
    +++ b/core/modules/node/src/Tests/NodeAccessMenuLinkTest.php
    @@ -0,0 +1,69 @@
    +    $edit = array(
    

    Nit: Bracket notation.

  4. +++ b/core/modules/node/src/Tests/NodeAccessMenuLinkTest.php
    @@ -0,0 +1,69 @@
    +    // this menu link
    

    Missing full stop.

  5. +++ b/core/modules/node/src/Tests/NodeAccessMenuLinkTest.php
    @@ -0,0 +1,69 @@
    +    $this->assertNoLink($menu_link_title);
    

    Just to isolate this test to the "access content" permission, can we give just that permission to the anonymous user and assert that the link is there?

  6. +++ b/core/modules/node/src/Tests/NodeAccessMenuLinkTest.php
    @@ -0,0 +1,69 @@
    +  }
    

    Nit: missing newline before class closing bracket.

StryKaizer’s picture

Status: Needs work » Needs review
FileSize
2.19 KB

@stefan.r: Thanks for the review!

Attached you'll find a revised patch

stefan.r’s picture

This looks great now as it tests for the same D7 vulnerability!

Just a nit: you missed converting the 4 other array()s and the first menu link assertion out of the 3 isn't necessary.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Looks fine. It'd be great if we can mention https://www.drupal.org/SA-CORE-2015-003 in someplace inside the test.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Grepped around a bit, and the format seems to be:

// DRUPAL-SA-XXXX-YYY: Blah de blah de blah.

...right above the test.

So added this:

+   * SA-CORE-2015-003: Tests menu links to nodes when node access is restricted.

(We seem to have dropped "DRUPAL" from the SAs sometime.)

...and committed/pushed to 8.0.x. Thanks!

  • webchick committed 4ebc668 on 8.0.x
    Issue #2554239 by StryKaizer, stefan.r, David_Rothstein, matt2000, scor...
jibran’s picture

Thank you @webchick for fixing that on commit. :)

webchick’s picture

Version: 8.0.x-dev » 7.x-dev
Status: Fixed » Patch (to be ported)

Oops, meant to do this.

  • webchick committed 4ebc668 on 8.1.x
    Issue #2554239 by StryKaizer, stefan.r, David_Rothstein, matt2000, scor...

  • webchick committed 4ebc668 on 8.3.x
    Issue #2554239 by StryKaizer, stefan.r, David_Rothstein, matt2000, scor...

  • webchick committed 4ebc668 on 8.3.x
    Issue #2554239 by StryKaizer, stefan.r, David_Rothstein, matt2000, scor...

  • webchick committed 4ebc668 on 8.4.x
    Issue #2554239 by StryKaizer, stefan.r, David_Rothstein, matt2000, scor...

  • webchick committed 4ebc668 on 8.4.x
    Issue #2554239 by StryKaizer, stefan.r, David_Rothstein, matt2000, scor...