Regression: Shortcut links access is not checked

I consider this at least as major. Note: access checking was done previously using menu_tree() so we kinda lost the functionality while converting shortcuts to its own entity type.
You could use this nice little shortcut:

\Drupal::service('access_manager')->checkNamed...

---

I will try it.

+++ b/core/modules/shortcut/shortcut.module
@@ -256,10 +256,13 @@ function shortcut_renderable_links($shortcut_set = NULL) {
+    $access = \Drupal::service('access_manager')->checkNamedRoute($shortcut->getRouteName(), $shortcut->getRouteParameters(), \Drupal::currentUser());
+    if ($access) {
+      $links[] = array(
+        'title' => $shortcut->label(),
+        'url' => Url::fromRoute($shortcut->getRouteName(), $shortcut->getRouteParameters()),
+      );
+    }

Note: #2235457: Use link field for shortcut entity probably also fixes the bug. For this case here you could store the $url object and then use $url->access() ...

I think testAccessShortcutsPermission needs to give the "Administer site configuration" permission to authenticated user in order to fix the 2 remaining fails.

That fix for itself is fine.

Added a beta evaluation.

Information disclosure -> critical.

Working on it in the sky

Did not a straight reroll but rather adapted the test coverage to be a little bit more sensible.

looking at fails

So the fix broke existing tests for using shortcuts, which is good - and what we want - so I fixed them and expanded them to ensure we have coverage for 'using' shortcuts.

The new tests in earlier patches for admin lists, didn't handle access there.
So changed that to not show the operation links if no access to the link (we don't let you add them, so why would we let you edit them) - and same story for titles - if you can't access the link - we make the title 'N/A' - the form still works - but you can't get the information disclosure.

This is inline with how we handle those with 'administer comments' who can don't have access to commented entity.

+++ b/core/modules/shortcut/src/Form/SetCustomize.php
@@ -51,11 +51,12 @@ public function form(array $form, FormStateInterface $form_state) {
+      $url = $shortcut->getUrl();
       $form['shortcuts']['links'][$id]['#attributes']['class'][] = 'draggable';
       $form['shortcuts']['links'][$id]['name'] = array(
         '#type' => 'link',
-        '#title' => $shortcut->getTitle(),
-      ) + $shortcut->getUrl()->toRenderArray();
+        '#title' => $url->access() ? $shortcut->getTitle() : t('N/A'),
+      ) + $url->toRenderArray();
       unset($form['shortcuts']['links'][$id]['name']['#access_callback']);

Not convinced here ... the URL the shortcut links to could also have information in there, as the path aliases might got resolved.

Ok, I can make #access => false on the row, but we then need to fix #theme table (or w/e it uses) to not output an empty row

well that, or just wrap it with an if() statement. I think this is fine for security purposes, honestly.

Isn't completely hiding a shortcut row going to mess up the weights which are set there?

Removing access to see shortcuts by a user with "administer shortcuts" doesn't need to be part of this issue.

Removing access to see shortcuts by a user with "administer shortcuts" doesn't need to be part of this issue.

I'm not so sure, we don't let people create shortcuts to paths they have no access to, so we shouldn't let them view/edit them either.
Let's get some committer feedback

'administer shortcuts' doesn't have restrict access: true so unless we add that, then you shouldn't be able to see anything with that permission that you wouldn't otherwise have access to.

Tagging.

So let's hide it entirely.

@Berdir
As far as I see, we don't recalculate the weights?

I think the JS does, if you move something around?

So access overview with a shortcut that you can't access, move something around, then the hidden one will not be updated and be in a wrong location.

@berdir
In this case we have the same problem for menu trees as well, or pretty much everything which uses the JS and access checking.

Just a reroll in the meantime.

Sure, I guess it is a very theoretical problem, because which site is going to give that permission to users that do not have full access anyway. But better safe than sorry :)

Let's fix the missing permissions.

So access overview with a shortcut that you can't access, move something around, then the hidden one will not be updated and be in a wrong location.

But it won't effect you because you'll never see those shortcuts anyway.

It will effect the admin, who can go in an fix it.

In other words I think we can live with that.

+++ b/core/modules/shortcut/src/Form/SetCustomize.php
@@ -77,6 +81,7 @@ public function form(array $form, FormStateInterface $form_state) {
+        '#access' => $url->access(),

This can go now (done in attached)

Manual testing

Super-admin adds shortcut to admin/config/development/performance

Non super admin user has shortcut admin role

shortcut admin role has permission to edit shortcuts

Non super admin user can edit shortcut links but doesn't see links they should not

Non super admin can't see shortcuts with no access

RTBC in my book

Missed last screenshot

+++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
@@ -251,9 +262,17 @@ public function testAccessShortcutsPermission() {
+    $this->assertNoLink('Shortcuts', 0, 'Shortcut link not found on page.');
+    $this->assertNoLink('Cron', 0, 'Cron shortcut link not found on page.');

Remove 2nd parameter.

+++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
@@ -251,9 +262,17 @@ public function testAccessShortcutsPermission() {
     $this->assertNoLink('Shortcuts', 0, 'Shortcut link not found on page.');

same.

note: I see 2 other places in Core with the same problem.

What?

Nice find.

AssertNoLink() has no index (the 0). so we're using 0 as the message, and the message as the group.

ooh, lokapujya++

Good catch!!

Back to RTBC then.

+++ b/core/modules/shortcut/src/Tests/ShortcutLinksTest.php
@@ -249,11 +260,19 @@ public function testAccessShortcutsPermission() {
+    // Verify that users without the 'administer site configuration' permission
+    // can't see the cron shortcuts.
+    $this->drupalLogin($this->drupalCreateUser(array('access toolbar', 'access shortcuts')));
+    $this->assertNoLink('Shortcuts', 'Shortcut link not found on page.');
+    $this->assertNoLink('Cron', 'Cron shortcut link not found on page.');

Shouldn't we be clicking shortcuts here and then testing that cron does not appear? Because I think what we are verfiying here is something else.

Shouldn't we be clicking shortcuts here and then testing that cron does not appear? Because I think what we are verfiying here is something else.

Well, ... those shortcuts does not appear on the page, as the user doesn't has access to it. How would you click on them?

Yes. This is all about visibility of the shortcut links. Whether the user can actually visit that page or not has nothing to do with shortcut.

Great. Committed and pushed to 8.0.x. Thanks!