I have installed nodeaccess on drupal 7.12

when I create a new content, save it (not published)
then any user with the url can view the node !

If I edit again the node, click on the "grant" tab and "save grants" and flush cache,
then it's ok, access becomes denied for anonymous users.

This seems a very serious security problem to me.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sanette’s picture

this is probably not the best thing to do but this seems to work: insert

if (!$node->status) {
// Don't create grants for unpublished nodes.
return NULL;
}

in function nodeaccess_node_access_records

(taken from https://drupal.org/node/1294566)

bkosborne’s picture

Priority: Major » Critical

This seems like a critical bug to me, and probably a pretty significant security issue unless I'm overlooking something here. When this module is enabled, it examines your Drupal role permissions for to see if "anonymous" and "authenticated" user roles have the "access content" permission. Then, for every node on the site, it adds a node_access record that allows those roles to view the content (assuming that role had "access content" perm).

It has no regard for the nodes current published status. Meaning, once you enable this module, any unpublished content you have becomes visible to everyone.

vlad.pavlovic’s picture

I will have a look at this immediately.

vlad.pavlovic’s picture

Who is the author of your nodes? Is it anonymous (ie: uid=0)?

vlad.pavlovic’s picture

Status: Active » Fixed
FileSize
1.42 KB

I have pushed a fix to dev version already and have attached a patch against 1.3 that will fix this issue.

Instead of just not setting the grants for anonymous or authenticated users, I have based it on the role's permission to 'bypass node access'.

You will have to rebuild your access in order to see the change. Let me know if you run into any new issue.

bkosborne’s picture

No the author was not anon, but I think you already figured that out. The patch seems to have fixed the issue.

sanette’s picture

The patch #6 doesn't work for me. The unpublished content is still visible

I'll try to look into it when I have some time ... :(

sanette’s picture

More precisely:

If I'm loggued as another user, then I cannot see the unpublished content

If I'm anonymous, then I can see the unpublished content :(

vlad.pavlovic’s picture

Please make sure to rebuild the permissions after the patch.

sanette’s picture

yes I did it, + cache clear

vlad.pavlovic’s picture

That must mean that anonymous users either have permission to 'bypass node access' or some other content access module (ex: Content Access, Domain access, some other one?) is giving access. Nodeaccess module no longer creates any entry into the node_access table for anonymous users (unless 'bypass node access' is granted).

sanette’s picture

no, anomymous users don't have this permission.
But I should mention that the node I'm testing contains some user defined grants
(sorry for this, I should have mentionned)
so it's logical that the patch doesn't affect this, since it applies only in the case the node has no own grants.

for a new content created without its own grant, the patch seems to work

So now the question is changed compared to #1. Here is the value of $grants for my node:
Array
(
[0] => Array
(
[gid] => 1
[realm] => nodeaccess_rid
[grant_view] => 1
[grant_update] => 0
[grant_delete] => 0
[priority] => 0
)

[1] => Array
(
[gid] => 9
[realm] => nodeaccess_rid
[grant_view] => 1
[grant_update] => 0
[grant_delete] => 0
[priority] => 0
)

[2] => Array
(
[grant_view] => 1
[grant_update] => 1
[grant_delete] => 1
[gid] => 1
[realm] => nodeaccess_author
[priority] => 0
)

)

vlad.pavlovic’s picture

Not sure what you mean by some user defined grants. But logically, if you are editing a node and want to grant access to a user and check the checkbox that this is what you want to do. That's what the module should do, regardless of what the permissions are for published/unpublished. This way you can granularly set access to a particular node. So if you don't want anon to have access to all unpublished nodes, but want to grant access to one unpublished node, you can do that.

This issue and the fix that followed repaired the condition where a node that is not published was granted access to a user upon install because the defaults were set to 'view' node for anonymous users.

If there is another issue, please create a separate issue in the queue so that it can be logged properly and will show up in other users' searches. Thanks.

sanette’s picture

I think my problem is now the same as
https://drupal.org/node/1444040
(I create a new node, give 'view' access to anonymous (using the grant tab), unpublish it --> it is visible by anonymous, which I don't want).
Thanks for the help.

vlad.pavlovic’s picture

Yeah that's exactly what I meant above. What if you didn't want anon to have access to all unpublished nodes, but wanted them to have access to one particular unpublished node. If I made the change to work as you request, it would be impossible to do so.

Sorry, I don't think that I will be making the change beyond what is already done.

sanette’s picture

I see what you mean. Let me just try and make my case.

* I use this great module because it lets me have a group of people (=role) who can modify a set of articles.

* of course these articles are visible by anonymous, and not editable by other users.

* sometimes they want to unpublish an article to work on it.

* but the unpublished article is still visible by anonymous users !

I believe my situation is very standard, and I continue to think that this is a serious issue, as when the node is not published, the authors think nobody can view it and some confidential text might be present in the article.

Did I miss something ? If not, of course I respect your decision, but I think I will hack my own copy (in my situation I don't see the point of letting anonymous users having access to any unpublished content.)

Status: Fixed » Closed (fixed)

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

manuel.adan’s picture

This major issue should be solved in stable version of the module. Current 7.x-1.3 still allow access to unpublished contents!.

MorganL’s picture

I second arcovia's comment. This is a HUGE security issue. It needs to be pushed into the stable release immediately.

willzyx’s picture

The patch solve the problem partially and introduced new issue;

Problem:
1) it isn't retroactive. All nodes with before-patch "user defined grants" may be visible by anonymous. This is a HUGE security issue.
2) Users can fall in error. They have not evidence that nodes are visible by anonymous even if unpublished.

New issue:
1) Modify grant on unpublished node with default settings break default configuration. see https://www.drupal.org/node/2140819

and big part of the patch is useless (as you can see here https://www.drupal.org/node/270000)

Users with permission to 'administer nodes' (Drupal 6) or 'bypass node access' (Drupal7) are never restricted by node access modules. Users who do not have permission to 'access content' (Drupal 6) or 'view published content' (Drupal 7) will never gain access from a node access module. Only users who have 'access content'/'view published content' and not 'administer nodes'/'bypass node access' are eligible for the wild world of node access module control.

so all the "else" part of the patch can be safely deleted.

Proposed solution:
1) Only write grants for published nodes or write grant_view = false if node is unpublished like a lot of content access modules does (content_access (per node), taxonomy_access, tac_lite, domain_access etc)
2) Add grant "view unpublished" to the allowed grants and manage it if node is unpublished (if "you didn't want anon to have access to all unpublished nodes, but wanted them to have access to one particular unpublished node" have sense)

willzyx’s picture

Status: Closed (fixed) » Needs work
vlad.pavlovic’s picture

Status: Needs work » Needs review
FileSize
1.19 KB

Agreed, adding patch that will remove the else, if it passes tests I will commit immediately to dev.

Status: Needs review » Needs work

The last submitted patch, 23: nodeaccess-unpublished_nodes_visible-23.patch, failed testing.

vlad.pavlovic’s picture

Version: 7.x-1.3 » 7.x-1.x-dev

Status: Needs work » Needs review

The last submitted patch, 6: nodeaccess-unpublished_nodes_visible-2228713-6.patch, failed testing.

  • vlad.pavlovic committed 502a499 on 7.x-1.x
    Issue #2228713 by vlad.pavlovic, willzyx: unpublished nodes are visible
    
vlad.pavlovic’s picture

Status: Needs review » Fixed
willzyx’s picture

ok code now is more clean.. but the patch don't solve the issue

It applies only for nodes without saved grants. The unpublished node with saved grants are still visible from anonymous user.

patch for solve this problem is attached

Note with this patch applied :
Only published nodes will get nodeaccess grants written
In order to maintain the consistency of the nodes grants the configuration is read from the table "nodeaccess" and not directly from "node_access" (which is subject to change) otherwise the grant tab would not show the correct grants configuration.

needs some serious test!

willzyx’s picture

Status: Fixed » Needs work
vlad.pavlovic’s picture

Please don't post patches that will not fix the issue that is being discussed in this thread, it creates confusion, especially when they are duplicate patches from other issues #2342327: node_access_write_grants should not be called directly .

willzyx’s picture

the patch will fix the issue
check it out!

the core is in nodeaccess_node_access_records

if (nodeaccess_disabling() || !$node->status) {
     return;
}

reroll of nodeaccess-2342327-read-config-from-nodeaccess-table.patch in this patch is needed for show the correct grants configuration on grant tab

vlad.pavlovic’s picture

Status: Needs work » Fixed

I see what you mean. The logic from your solution has been ported to and released to dev as part of #2037899: Expose operations as API. I have also added tests that confirm that things are working as they should. Let me know if there are issues.

Status: Fixed » Closed (fixed)

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

SunnyGambino’s picture

Dear Guys,
It is a really good feature and it is working. BUT.. :D
In my own custom Drupal 7 system I am using both published and unpublished nodes on an absolutely different way. Could this feature be a switchable option on the module configuration page?
Thanks a lot!

ElusiveMind’s picture

Patch lines 132-150 of nodeaccess.module.

Module was defaulting to NODE_ACCESS_IGNORE if user did not own content, did not have 'view own unpublished content' and 'bypass node access' set. Note that "bypass node access" causes hook_access_control to not be fired, and as such would have no impact on user's ability to control content for that permission

ElusiveMind’s picture

Assigned: Unassigned » ElusiveMind
ElusiveMind’s picture

This needs community review. I cannot update the status as I am not yet the maintainer.