Allow menu items which link to unpublished nodes to be selected in the parent item selector

This introduces a security issue, we're now not limiting access to menu items that point to unpublished nodes for anonymous users

And the test is adding a hook to check specifically for the actual item.

My comment/suggestion at #68 hasn't been addressed either.

Please don't reroll or use patches #73 through #75 otherwise you're introducing a security risk on your site.

Sorry, the issue is with patch #75 only, #73 and #74 have an extra check.

#68

I think we almost need a lesser permission here, something like 'Reference unpublished nodes' which could also be used in the Node selection handler

Introducing a new permission does not seem very sensible at this point, since this issue already requires checking for specific permissions for generic use cases, for example 🐛 User can't reference unpublished content even when they have access to it Needs work
As a result, the access check can easily result in incorrect access cache metadata: 🐛 Access cacheability is not correct when "view own unpublished content" is in use Needs work

As a more general solution, we can rely on either node access or the broad 'view any unpublished content' permission. Note that the 'view own unpublished nodes' permission was explicitly ignored to not require cache entries per user in #2250315: Regression: Bring back node access optimization to menu trees → . This requirement is untouched.

#79.1 Added explicit coverage to test unpublished content is not available in the 'Parent link' select list

#79.2 Removed the hook from the test that checked for an actual item.

The patch implements the following changes:

1. The 'Parent link' select list now relies on node access for projects with node_grants implementations.
2. The 'Parent link' select list now lists unpublished nodes for projects for projects without node_grants implementations where the user has 'view any unpublished content' permission.
3. DefaultMenuLinkTreeManipulators::__construct() now requires the ModuleHandler, added a change record at https://www.drupal.org/node/3336973 → .
4. Added a deprecation test for the new DefaultMenuLinkTreeManipulators::__construct() $module_handler argument

#81 rerolled for 9.5.x

Seem to have CI errors in #81.

Added a property for $moduleHandler, fixing the PHPstan error.

The last submitted patch, 84: 2807629-84-test-only.patch, failed testing. View results →

1. +++ b/core/lib/Drupal/Core/Menu/DefaultMenuLinkTreeManipulators.php
   @@ -149,7 +164,9 @@ public function checkNodeAccess(array $tree) {
   +        if (!$this->moduleHandler->hasImplementations('node_grants') && !$this->account->hasPermission('view any unpublished content')) {
   

   So we now have the check for node_grants being used, but we don't have test coverage for this. There should be some test modules around that we could install in the test to cover this I think

2. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiContentModerationTest.php
   @@ -188,4 +189,53 @@ public function testMenuUiWithPendingRevisions() {
   +    $this->assertSession()->optionNotExists('edit-menu-menu-parent', 'main:' . $link->getPluginId());
   

   First I was worried that we only end on negative assertions, but optionNotExists does check if the specified select element exists and fails otherwise, so that is great.

Also the current proposed fix no longer lines up with the proposed fix in the IS, so the IS could use an update

#86.1 Added a test with the node_grants implementation from node_access_test

#86.2 Updated the issue summary proposed solution in line with the patch

1. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiNodeTest.php
   @@ -377,4 +377,57 @@ public function testMultilingualMenuNodeFormWidget() {
   +    ]);
   +    $this->drupalLogin($admin_user_without_content_access);
   +    $this->drupalLogin($admin_user_without_content_access);
   +    $this->assertFalse($node->access('view', $admin_user_without_content_access));
   

   Nitpick: Double login

2. +++ b/core/modules/node/tests/modules/node_access_test/node_access_test.module
   @@ -15,8 +15,8 @@
   - * @see \Drupal\node\Tests\NodeQueryAlterTest
   - * @see \Drupal\node\Tests\NodeAccessBaseTableTest
   + * @see \Drupal\Tests\node\Functional\NodeQueryAlterTest
   + * @see \Drupal\Tests\node\Functional\NodeAccessBaseTableTest
   
   @@ -44,8 +44,8 @@
   - * @see \Drupal\node\Tests\NodeQueryAlterTest::testNodeQueryAlterOverride()
   - * @see \Drupal\node\Tests\NodeAccessPagerTest
   + * @see \Drupal\Tests\node\Functional\NodeQueryAlterTest::testNodeQueryAlterOverride()
   + * @see \Drupal\Tests\node\Functional\NodeAccessPagerTest
   

   Unrelated changes, if these were the only bad testing namespaces left in core I'd be ok with sneaking it in here, but they are not, so let's not fix them here, but do a new issue to fix this everywhere.
   If I search core with regex "\\Drupal\\(.*)\\Tests\\" I find a whole bunch of these all over core.
   Did a quick look but couldn't find an existing issue for it.

#88.1 Removed the duplicate login

#88.2 Removed the unrelated changes and filed a follow-up issue at 📌 Update outdated class references from Drupal\*\Tests to Drupal\Tests\* Needs work

The last submitted patch, 87: 2807629-87-test-only.patch, failed testing. View results →

The last submitted patch, 89: 2807629-89-test-only.patch, failed testing. View results →

Looks ready to me, thanks!

Only patch #82 🐛 Allow menu items which link to unpublished nodes to be selected in the parent item selector Fixed worked for me (D 9.5.3).
Patches #84, #87 and #89 did not apply.

+++ b/core/tests/Drupal/Tests/Core/Menu/DefaultMenuLinkTreeManipulatorsTest.php
@@ -81,12 +89,14 @@ protected function setUp(): void {
+    $container->set('module_handler', $this->moduleHandler);

Can we confirm that this test has existing coverage for the unpublished status filter.

It looks like node 3 in that instance covers the unpublished case.

But I can't see a functional test in core to ensure that an unpublished node link isn't output to the page.

If we confirm we have existing coverage for that I'll be happy we're not accidentally creating an access bypass here.

The fact tests didn't fail on #75 makes me think that we only have this unit test, and it is too tightly bound to the current implementation to fail when we make this behaviour change.

So to be clear, we need to confirm we have a test that renders a menu with a link in it that points to an unpublished node, that confirms the link isn't output to users without access.

Sorry for being pedantic here, but security is something we've always prided ourselves on, and I don't want to be the one to commit something that harms that.

Existing functional test coverage is available \Drupal\Tests\menu_ui\Functional\MenuUiNodeTest::testMenuNodeFormWidget(). This test implements a scenario where an editor is able to add/edit an unpublished node, but not view it:

    $admin_user = $this->drupalCreateUser([
      'access administration pages',
      'administer content types',
      'administer nodes',
      'administer menu',
      'create page content',
      'edit any page content',
    ]);
    $this->drupalLogin($admin_user);
    // Assert that the link does not exist if unpublished.
    $edit = [
      'menu[enabled]' => 1,
      'menu[title]' => $node_title,
      'status[value]' => FALSE,
    ];
    $this->drupalGet('node/' . $node->id() . '/edit');
    $this->submitForm($edit, 'Save');
    $this->drupalGet('test-page');
    $this->assertSession()->linkNotExists($node_title, 'Found no menu link with the node unpublished');
    // Assert that the link exists if published.
    $edit['status[value]'] = TRUE;
    $this->drupalGet('node/' . $node->id() . '/edit');
    $this->submitForm($edit, 'Save');
    $this->drupalGet('test-page');
    $this->assertSession()->linkExists($node_title, 0, 'Found a menu link with the node published');

Queued a 9.5 test

Just to be super cautious, I did some manual testing of this and it works well.

Steps I took:

* Install umami with patch applied
* Modify permissions for Editor role to allow editing menu
* As admin create some pages, mix of published and unpublished and place them in the main menu
* As editor edit a page (cannot create pages in Umami as Editor which feels odd, but is not related), verify you can see the unpublished items as menu parent
* View page as editor, unpublished items show up in the menu
* View page as anonymous, unpublished items do not show up
* Confirm that both pages are still cacheable (for editor with DPC, for anon with page cache and DPC)

In my screenshots, rubbish and recycling is the unpublished item

Screenshots are for anon, and for editor
My screenshot tool won't capture the open select field, but the unpublished item is in the list

Saving issue credits

Whilst this is a bug report, we're introducing a new deprecation and changing the parameters to the menu tree manipulators service - so that means it can only be in a new minor.

I've committed this to 10.1.x and published the change notice.

Thanks folks

Oh, and I meant to say thanks to @idebr for doing the work to find the test coverage I was asking about, much appreciated.

Nice to have this committed, thanks!

Recently updated one of my instances to 9.5.2 and PHP 8.1.14.

I removed patch #42 from my codebase and the problem I was having reappeared. The problem I have is that, on the edit screen, the
Menu Settings > Parent link" dropdown will omit any unpublished items and all of it's children.

I applied #82 and my problem is resolved.

@karenann the fix has not been backported to 9.5, it is only in 10.1.x. You will need the patch until upgrading to 10.1

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

Attached is the backport for 9.x.

Here is a patch for Drupal 9.5.10

Cannot apply the latest patch on Drupal 9.5.10!

I've looked into why the 2807629-105.patch wouldn't apply on D9.5.10 and it was just a small fix in de core.services.yml part.
Here it is again!