Port SA-CONTRIB-2013-096 to D8

We wouldn't want to ship a D8 upgrade path beta without this, so tagging.

SA forward ports are critical, so tagging per discussion with @webchick, @alexpott, @catch, and @effulgentsia.

So this is the commit for sec issue http://cgit.drupalcode.org/entityreference/commit/?id=92747d9. I'll try to have look at it.

Well this took more then an hour. Talked to @larowlan to find the exact sec issue in D8. Added kill method to EFQ after chx advice.
We can move that to follow up but I just want to show the solution here.

+++ b/core/modules/node/src/Plugin/EntityReferenceSelection/NodeSelection.php
@@ -35,6 +35,10 @@ public function buildEntityQuery($match = NULL, $match_operator = 'CONTAINS') {
+    // If user can't access the content kill the query.
+    if (!$this->currentUser->hasPermission('access content')) {
+      $query->kill();
+    }

Since the newly added kill() method doesn't have a symmetric revive() method anyway, wouldn't it be simpler/better/cleaner to add a NullQuery class that always returns the empty result set, which this could then return?

That'd also allow us to move this logic to the top of \Drupal\node\Plugin\EntityReferenceSelection\NodeSelection::buildEntityQuery(), so that it becomes an early return, making it very cheap in case the necessary permissions are missing.

Also, IMO the fundamental problem here is that the node access/grants system does not match the behavior of calling Node::access(). That's not fixed in this patch.

Let's try test only patch one more time.

Added Null\Query class as per #10

Since entity query is sorta work for me (as it is a prerequisite for mongodb) I will chime in: the kill method allows for killing in alters and that feels like a very nice thing to have. The lack revive is a feature not a bug. It shouldn't be added. A comment on kill should perhaps mention that what is dead will remain dead (this is not Game of Thrones).

1. +++ b/core/lib/Drupal/Core/Entity/Query/Null/Condition.php
   @@ -0,0 +1,38 @@
   +  public function notExists($field, $langcode = NULL) {
   +
   +  }
   

   For empty methods I think we write {} on the same line ?

2. +++ b/core/lib/Drupal/Core/Entity/Query/Null/QueryFactory.php
   @@ -0,0 +1,48 @@
   +      throw new QueryException('Aggregation for NULL query is not supported');
   +  }
   

   Indents wrong

3. +++ b/core/modules/node/src/Plugin/EntityReferenceSelection/NodeSelection.php
   @@ -23,9 +28,57 @@
   +    // If user don't have access to content return null query.
   

   don't » doesn't and we need a comma after content

4. +++ b/core/modules/system/src/Tests/Entity/EntityReferenceSelection/EntityReferenceSelectionAccessTest.php
   @@ -109,6 +110,24 @@ public function testNodeHandler() {
   +    $role = Role::load(reset($rids));
   +    // Make sure user doesn't have access content permission.
   +    $role->revokePermission('access content')
   +      ->save();
   

   Is this still needed?

Also, IMO the fundamental problem here is that the node access/grants system does not match the behavior of calling Node::access(). That's not fixed in this patch.

Can you please elaborate @WimLeers? The fail patch from @amateescu doesn't reference any issues with node-access system. @jibran and I looked in depth at the entity query system and the node_access tag is added - so that implies the alter hook is firing. We were chasing that first, and then realised we'd gone down the wrong path - the issue was the 'access content' permission wasn't being considered.

#15: My comment was far from clear, sorry. First: it feels weird that there is no symmetry, but that is indeed a good thing in this case. Second: that doesn't mean somebody in contrib or in custom code won't add it.
That's why I proposed something like NullQuery: it's always going to return the empty result set, and it contains no query data whatsoever, therefore using it would completely and utterly prevent "reviving a killed query".

#16: indeed, the node_access tag is added. Which means the node grants system is ensuring that the query results only contain those nodes accessible by the current user. Unfortunately, the node grants system does *not* match what the entity access/node access system (i.e. the entity and node access control handlers and hooks) marks as accessible of forbidden. Hence:
- having a list of nodes A, B and C and calling ->access() on them may indicate that only A is accessible
- having a query tagged with node_access may indicate that A and B are accessible
Hence, a mismatch.

Berdir described this to me (IIRC) as "query access" versus "render access", but agreed that they *should* behave identically. He argued that it is up to the implementers of hook_node_grants() to ensure things match up. Yet even without any such implementation in core… things don't match up, because the "access content" permission is not taken into account by the node grants system (and hence node_access-tagged queries).

I am quite likely not expressing myself entirely correctly, but I believe that captures the gist of the problem: we have one access system that we use everywhere… except for in queries, which uses another. We can't use it in queries because the system is based on PHP logic, which obviously can't run inside a query. And we want Views to be able to return results that take access into account. So we've ended up with a system that is:
- highly confusing: actually 2 systems
- but the one for queries (node grants) only works for *one* entity type: nodes
- and hence is (IMHO) extremely prone to security problems in the form of information disclosure.

I hope this is at least a mostly accurate description. I hope Berdir can chime in, he can speak about this authoritatively.

so I think that means this is need work for #17 but to be honest I'm still not sure.

I guess next step is extend this test to enable node_access_test module and see what we get?

I had a very long comment where I tried to explain all of this. Will try to write that again, but some very short comments here first, will elaborate on them later if necessary.

• entity/node access vs grants is how it is, we can't change that we have two different systems. (Also, we actually have three)
• There are two different issues here. The 7.x SA that we are trying to forward-port here and the access content check.
  1. Create a node.
  2. Remove access content for anonymous users
  3. Edit the view, remove status = 1 and access content permission.
  4. Voila, anonymous users can now view content, but if they click on it, they get access denied. Same for unpublished content.

  => That is exactly the same as you get with ER (except it already checks for status).

  The actual issue that was fixed in the SA is something else, that was for cases where entities where displayed that did *not* come from a query, e.g. the default value. If anything, then it solved this problem only indirectly, because it would then display 10 suggestions but every single one would say "- Restricted access -". That's stupid :)

  The test that you need for that is editing a node that has a reference to an unpublished node, then you should not be able to see the title of that if you can't view unpublished content.

  Core simply assumes that you take care of those two things yourself and never allow the user to execute a query for nodes if he does not have the access content permission.

• I lost my long explanation, but in short, selection plugins were added in the contrib module for shortcomings in core's query access implementations (and entity field query shortcomings, but we did solve those), because there was no way to change how core worked. That's no longer the case.
• IMHO, making the code that embed an autocomplete element responsible for checking basic node access, especially if we hardcode a node reference, would be the quickfix that is consistent with core's (broken) node grant behavior
• In the long run, I think we should implement this logic by default in node_query_node_access_alter(), atleast when there are no grants (because if there are grants, then they actually might allow access to unpublished content, and the NodeSelection plugin correctly considers that). But that is a much bigger issue than this. Because we should do the same for comments, files and users.

To summarize: I think the current patches are solving not the wrong but a different problem than the SA was about, I'm not sure if we should mix those two things in the same issue. The problem that Wim Leers found is more or less working as designed (stupid design, but still..) and I'm pretty sure that it exists in 7.x as well.

Also, I do not think that any of that is an authoritative answer. That's just my opinion/how I understand it.

#19 Nice points and now I am official confused. Can someone please elaborate the next step for the patch? After reading #19 I think #17 and #18 are irrelevant to the patch so is it a NR again? And do we need more critical followups to fix #17, #18?
Bring the normal task #2418249: Consider improving the DX of #default_value for the entity_autocomplete form element in scope in order to fix critical bug seems wrong to me.

Nice work!

+++ b/core/lib/Drupal/Core/Entity/Element/EntityAutocomplete.php
@@ -160,6 +189,35 @@ public static function validateEntityAutocomplete(array &$element, FormStateInte
+      $key = ($entity->access('view')) ? $entity->label() : t('- Restricted access -');
+
+      // Take into account "autocreated" entities.
+      if (!$entity->isNew()) {
+        $key .= ' (' . $entity->id() . ')';
+      }
+
+      // Labels containing commas or quotes must be wrapped in quotes.
+      $entity_labels[] = Tags::encode($key);

$key that then becomes $entity_labels is a bit confusing, why not use $label instead?

:)

I think this is more or less ready to go, but I'm sure Wim can find a few nitpicks.

How do you think we should continue with the other issue?

Open a separate issue I guess, probably a critical? My recommendation would be to make the issue very specific as a start, with the option for a generic fix? Something like "access content permission is not considered for menu link path autocomplete?"? A generic fix might not be easy, we're missing a 'list' entity access operation/method, so we would have to route it through the selection plugins or introduce a new entity list access (related: #2409691: EntityAccessControlHandlers miss entity admin access).

Yet another issue that we should open I guess is whether the default node/file/user/comment query altering should consider the access content permission and/or publication status (this might also be another approach to fix the previous issue) but I don't think that needs to be critical, it has been like that for multiple version, so can't be suddenly release blocking for D8? (would not be the first time to decide that, though ;))

Works for me as well. My argument for a specific critical issue is that I'm not sure what exactly is critical. I don't think that moving the file status check to a file_access query alter is a critical thing that we need to fix, for example. I'm not even sure it is desired, unlike node status, files have no permission to see or not see temporary files, except some very specific logic in file_file_download(), so the fact that FileSelection does that is pretty custom.

So having a specific, targeted critical would allow for a quickfix, but we can always decide to go that way and start with a critical meta issue. Just worried a bit that we might discuss that for quite some time...

This can work as a test only patch.

why is it so wrong since that's exactly what we have to fix here?

ok, fine by me. :)

It's working for me.

Steps:

1. Created an Article
2. Created an Entity Reference field
3. Added default value to Article
4. Created a user with permissions to create an Article(Role: content editor)
5. Unpublished Article
6. Logged in as user with content editor role
7. Clicked Add content
8. The entity field doesn't display the default value as shown in the attached screenshot.

Oh I meant to RTBC in #34. Above patch doesn't contain a single loc written by me so I think I can RTBC. @Berdir is happy in #30, we have manual testing in #32 and we have a fail patch so RTBC.

Can we please add default value change in change notice New 'entity_autocomplete' form element added?

This issue addresses a critical bug and is allowed per https://www.drupal.org/core/beta-changes.

Committed ade57a9 and pushed to 8.0.x. Thanks!

Let's update the CR :)