Nodes with menu items bypass node permissions system

Here is one way to reproduce this bug.

1. Go to Administer > Access Control
2. Toggle OFF: node module - access content - anonymous users
3. Create a new content item - blog, pagem story, whatever. Say it's "node/6".
4. Add a new menu item, titled "menu item 1" with a ptah of "node/6".
5. Log out.
6. Note that "menu item 1" is visible
7. Click on menu item 1. Note that the content appears.

At the bay area bugfix meetup. I'll work on this...

Another way to reproduce this bug is to follow my previous comment, but instead of creating a new menu item, create a new primary link item.

Again the content will be displayed even though access for anonymous users for all nodes has been turned off.

Able to reproduce. Not sure about fix. I suspect node_access grants are what are allowing the view.

It's weird that without the menu entry, access is denied. But with it, access is granted. Because node_access table has an entry to grant all, I'm not sure which is the right behavior.

To summarize:

1) access to all nodes is granted to all users by the default row in the node_access table.

2) node_access() the method is not called when displaying nodes. That would be inefficient, and that's why node_access table is used.

3) if you want to restrict access to all nodes to some users, it can be done with an access control module. It takes some configuration, but if you're serious about privacy, this is the way to go.

4) The current behavior is extremely confusing to users who expect unchecking "access content" to really limit access.

A quick fix would be to add something like this to node_view():

if (!user_access('access content')) {
  drupal_access_denied();
  exit();
}

This would prevent any node that uses the normal channels from displaying to users without that permission. It would do nothing to limit access to blocks. It would hide all pages and make it difficult to present anything beyond a login prompt to anonymous users.

This fix would also not protect content presented by modules which display without going through node_view.

Neil Drumm suggests changing "access content" to something like "access posts" to make it clear that nodes would be hidden, but not blocks and such.

Moving to node.module. This issue does not belong to page.module, as story and all other node types are equally affected.

hmm... if I uncheck "access content" from anonymous users in access control, I get a 403 error when trying node/1 as guest.

Could the problem be related to the fact that menus are cached, and maybe not rebuilt when disabling the node.module, as per instructions above?

Reply to markus_petrus

If a node is referenced by a menu - even if access control has been toggled off for an anonymous user - then you can display that node by clicking on the menu *or* by typing in the URL or by linking in from another site. So I do not think cache is an issue.

Reply to Dave (yogadex):

Your item 4 is the crux of the issue:

4) The current behavior is extremely confusing to users who expect unchecking "access content" to really limit access.

I have shown you two ways - using primary links feature and menu items - to display nodes I "stated" using access control that I did not want anonymouse users ever to see. How many other ways are there to display this data? What modules could a naive user install that would display nodes they thought were hidden for sure?

The ability to hide data fron anonymous users should be easy and fail/safe. I do understand that Drupal really tries hard to make data easy to display - so that hiding data goes against the grain.

I don't yet understand the drupal structure/API. One naive thought: would it be possible to add access control for each node type? So that page, blog, story would each have it's own access control? In this way I could put, for example, stories in menus - but having turned off their access control for guests - I would be certain that they would never display accidently.

In comparison to Mamba/Joomla and other CMSs, it is not easy easy to make certain data absolutely hidden or private for anonymous users. In fact it was the difficulty in creating private data that made me leave drupal last year - only to return more recently because of its many other good features.

I don't think caching or menu items are directly related to this issue. If start with a default install, you simply need to create a page, then visit node/1 from another browser where you are not logged in. (Make sure your browsers caching is off).

Many people here (at the bay area bugfix) cannot reproduce. I believe this is because they have some module installed in some way that calls drupal_access_denied() in these cases. If it's called, drupal stops building the page then and there, and that's why the node is not displayed.

I hope that made sense.

hmmm... maybe what happens is something like:

a) node/1 is appended by the node module using user_access(), in hook_menu.
b) then, the menu system appends the menu item created by hand, which overrides the one created in previous step. bang!

If the menu system could evaluate the menu item appended by the hook_menu in node module after the one created by hand, if would have the user_access rule.

see what I mean?

The issue is not so much hook_menu, because in those functions the software can assign access to be true or false. The problem is adding node items through the admin interface, which has no way to make the item conditional.

I have an idea for a nice solution using db_rewrite_sql hook. Will post it shortly....

Here's about as simple a patch as I can think of. If user has no access content permission, use db_rewrite_sql to find nothing. I don't know why I didn't think of it sooner.

Using this, such a user visiting node/X get's access denied when they would otherwise see the node. I don't understand why its "access denied" and not "page not found". But it's the same behavior as when such a user seeks node/Y where there is no node with nid Y.

Another approach would have been to change node_access_grants to not return the default 'all' permission if !user_access('access content'). But that would be called only when querying nodes. This patch will also affect queries for taxonomies and such, so it denies access with extreme prejudice.

i can't reproduce the problem. lets use your default install example. inded an anon user is able to view your first node. thats because anon has 'access content' permission by default. and even when you take that away, you have to reset the menu cache to make the change effective (we should probably do that whenever saving permissions page).

It turns out that to reproduce, you must create a menu item linking to node/X. The node module creates its own menu item when arg(0) == 'node' and is_numeric(arg(1)). And when it creates that menu item it actually calls node_access().

But create another menu item specifically for node/X and it will have no access restrictions.

I still think my patch or something like it is needed.

To tangent, I'm not convince that my patch logic is unrelated. hook_db_rewrite_sql is exaclty the place for this sort of thing. node.module defines the 'access content' permission, and it is node.module's hook_db_rewrite_sql which makes it possible for users without that permission to view the content.

Here's another approach you might consider cleaner. In this case when 'access content' is not allowed, the user does not get the standard grant for the 'all' realm. The effect again is that nodes will not be found.

I consider this patch inferior to my previous one, as it does nothing with the primary field is not 'nid'. The earlier patch hid taxonomy and everything else db_rewrite_sql applies to. This only hides nodes.

Cleaned up previous patch.

My first patch used db_rewite_sql to append "WHERE 0" to all queries. This was a bit heavy handed. Perhaps it is best for node.module to only affect node privileges. The taxonomy module could define its own access rights for taxonomy, user module for users, etc...

This patch also allows other modules to grant users access to some content, if they choose to in their own hook_node_grants. Which means access control module developers would be wise to test user_access('access content') if it applies to their code.

this looks much better ... what is the point of $grants = array('all' => array(-1))? can whole else statement be removed?

one more thought ...

if menu items with undefined access control just inherited access control of their parent then this would be a non issue (we would inherit path=node) ... i assume there is a reason why this is not done.

not expensive, considering we already have the whole menu tree as an array.

what is the point of $grants = array('all' => array(-1))? can whole else statement be removed?

It causes a WHERE clause to be inserted which will evaluate to false. Without something here, no where clause will be inserted at all (I'm pretty sure) and that would lead to unfettered access. True, the -1 looks hacky.

if menu items with undefined access control just inherited access control of their parent then this would be a non issue (we would inherit path=node) ... i assume there is a reason why this is not done.

There are many paths which can be added to the menus, which are not paths in the main navigation menu. That is, paths the menu system does not know about. So we can't rely on anything like that.

Take the path node/1 as an example. When you are viewing node/1, this item is in the menu, but not cached. Let's say you add node/1 in a custom menu item. When attempting to view node/1, the system could be smart and see that node/1 is already a menu item with access restricted to "access content". It could be smart enough to hide the custom menu item if the user does not have that privilege. But now the user visits node/2. All the sudden there is no entry for node/1, except the custom one that has been added by hand and has no privilege associated. So the smart system shows it to the user anyway. Did that make sense?

I'm removing myself from the assignee field because I don't plan to modify this patch further, and I don't have permission to check it in if approved.

is still too awkward. there is a real issue here ... please folks - investigate and patch.

I don't think this has much of anything to do with nodes. It looks like this will happen with any dynamic menu item with a custom menu item defined in its place.

Any reason we can't remove the 'access' => TRUEs from menu.inc lines 1082 and 1094?

The recent security patch implemented JonBob's suggestion from #25. I now correctly get "access denied" so I'm marking this issue fixed. Feel free to re-open it if you can reproduce the bug.

Richard: I'm still seeing this on a fresh HEAD from this morning.

Steps to reproduce:

1. Create a page (or any kind of node), uncheck the "Published" box under "Publishing options." Submit the form.
2. Attempt to view the node as an anonymous user. You will get access denied.
3. Go back and edit the node and assign it to a menu item.
4. Anonymous user now sees both the menu item and the node.

I was able to reproduce this bug but so it's still there.

This bug isn't showing up in the issue queue. Testing.

Closing this issue for now - please continue this thread here.

-K

Issue fixed - reopening. Apologies for all the white noise :)

-K

I downloaded a fresh Drupal HEAD this afternoon, and can't duplicate the issue as described by webchick above. I created the menu items both in the node edit form and in the "admin/menu" page, and both were invisible to an anonymous user when "access content" was unchecked. Is it fixed? Should we close this bug?

The nature of this issue has changed.. now the issue (still in HEAD) is that when you create a menu item for an unpublished node, both the menu item and node itself are visible to anonymous users, despite the "unpublished" status.

This issue IMO is more related to menu system than node permissions system.
The problem is in _menu_item_is_accessible($mid), which follows the path up to find the first "access" attribute (see code below).
Therefore, menu items pointing to unpublished nodes (for example ?q=node/100) inherit "access" attribute from ?q=node (the first node of the site), which is true for anonymous unless "access content" is unchecked.

function _menu_item_is_accessible($mid) {
  $menu = menu_get_menu();

// Follow the path up to find the first "access" attribute.
  $path = isset($menu['items'][$mid]['path']) ? $menu['items'][$mid]['path'] : NULL;
  while ($path && (!isset($menu['path index'][$path]) || !isset($menu['items'][$menu['path index'][$path]]['access']))) {
    $path = substr($path, 0, strrpos($path, '/'));
  }

In our case the value of $path is 'node' at this point, so _menu_item_is_accessible() returns TRUE :

return $menu['items'][$menu['path index'][$path]]['access']; 

Seems like noone debugged this for five minutes. I did and the solution is blatantly simple.

What made attach the whole module??

It is not possible to remove the item from the menu because when the menu is build, only the $may_cache part can be taken into account. The only possibility would be to change $_GET['q'] to each menu path and append contextual items for each.

chx's patch works fine

About the visibility -when the node is not published-, I agree there's no easy solution for now, so the workaround in this case is that the administrator disables also the menu item when unpublishing that node.

When the "access content" permission is not granted the menu item doesn't show up so for this case the visibility is not an issue.

applied

I have not tested #41 on an actual install, but from chx's walking me through it and from looking at the code it seems like the right fix.

forgot callback arguments.

An now you forgot the '$' in '$_menu' :-)

The thing is the patch works, but I still see the menu item -though it returns access denied-. Should I see it?

yes you should. it's still not possible to cure, unless you do the aforementioned set $_GET[q'] to path and gather !$may_cache for each. Would be slow as hell.

applied