Users with 'administer comments' should not be able to see the titles of unpublished nodes nor view/edit their comments

The question is whether it's good to load every node on this page. This is not a problem for performant sites, because they use entity-cache.

One advantage here is that there is no need to join the node table anymore.

Display 'Access denied' for the node title and for the comment subject "FooBar ('Access denied')" without the link to node or comment.

And I added a test.

A screenshot would be fine here.

Er, thats really weird behavior. IMO, this node nor any of its comments should be listed to unauthorized users. The comments belong to the node and are similarly inaccessible. So the fix is to add the node_access tag to the query.

This 1 line is all it takes IMO

removing needs tests tag because node access system has tests. we just weren't using node access.

the last patch don#t work for me.

You are going to have to provide more detail than that. Please elaborate. The intent of my patch is that these inaccessible comments are not shown at all. It is not Drupalish to show them but not be able to view/edit/delete. Whats the point of showing 'access denied'?

How are you restricting access? I am using the hidden node_access_test module.

my testuser with the permission "administer comments ... " see the comment/node title from a unpublished node and thats not what we wanted. Or in other words, I couldn#t see a different w/o the patch. And I believe addTag('node_access'); works only for selects like db_select('node', 'n').

@tobias - that unpublished use case is different than the one i was trying to fix. i was focused on nodes that were protected by the node access API. your case is not easy to solve in a performant way. the published/unpublished access control is part of hook_node_access() which is only applied to single nodes, and does not apply to a listing. A listing uses node access API. Sorry about the confusing language here. Its unfortunate.

We still need to do the patch I proposed.

Node access _alter queries should account for node visibility (published/unpublished). It is possible that the issue in #14 was solved by #920614: Unpublished content visible when a Node Access module is enabled.

Yes, hiding inaccessible nodes from admin/content/comment is a must.

The patch works fine with Forum Access (plus the core patch in #553944-154: Define hook_menu_get_item_alter() as a reliable hook that runs before the page is doomed).

comment/CID[/view] is already blocked, however comment/CID/edit isn't!

Is this a security hole in core?

Edit:

function comment_access($op, $comment) {
  global $user;

  if ($op == 'edit') {
    $node = node_load($comment->nid);
    return node_access('view', $node) && (($user->uid && $user->uid == $comment->uid && $comment->status == COMMENT_PUBLISHED && user_access('edit own comments')) || user_access('administer comments'));
  }
}

#18 updated with node_access() check for edit. Patch works great.

The snippet in #18 looks good.

However, you're eliminating rows after the pager query, which means you end up with less than 50 (possibly even zero!) rows per page.

We got side-tracked here. We should at least commit moshe's patch in #11.

The rest can be dealt with later.

Ok, #11 committed to HEAD.

Back to.. needs work?

Thanks! Yes.

My grasp of the unpublished situation is still not good enough to deal with this. The first priority should be to get this to work correctly without any node access module. What is correct behavior?

Probably to block view/edit of all comments of unpublished nodes that are not visible to the user. Taking into account the 'view' accessibility of the nodes (which includes published state) in listings should be necessary and sufficient. If the NA modules implement this correctly, then it should work with them, too.

Could this be as easy as adding $query->addTag('node_access'); to the comment query in comment_admin_overview()? How does core restrict the listing of unpublished nodes?

More questions than answers and no time right now...

A normal bug?

You could call it a sechole, too...

Did #11 make it into D7?

Yes. It's in 7.7.

+++ modules/comment/comment.admin.inc	14 Nov 2010 20:53:27 -0000
@@ -71,33 +71,33 @@ function comment_admin_overview($form, &
+    if (node_access('view', $nodes[$comment->nid]) === FALSE) {

I'm not sure about d7 but shouldn't we load the full user as well? Wait shouldn't this be the current user looking at the list of nodes?

+++ modules/comment/comment.module	14 Nov 2010 20:53:27 -0000
@@ -1397,7 +1397,9 @@ function comment_access($op, $comment) {
+    return node_access('view', $node) && (($user->uid && $user->uid == $comment->uid && $comment->status == COMMENT_PUBLISHED && user_access('edit own comments')) || user_access('administer comments'));

I'm wondering whether it would make sense to lazy-load the node only when the other access checks are TRUE

+++ modules/comment/comment.test	14 Nov 2010 20:53:27 -0000
@@ -1021,6 +1021,21 @@ class CommentApprovalTest extends Commen
+    // Check node access

It would be probably easier to understand if there would be a comment what exactly is the issue here.

-3 days to next Drupal core point release.

Updating the issue summary to reflect the current state of the issue; patch from Comment #11 is already committed

formatting change

Based on IRC discussion, the current security stance is that users do not need to be able to see a node to be able view/edit/delete a comment on it. This very much surprised me.

Opened the broader issue #1781766: Enforce that a user must be able to view a node to view/edit/delete the comments of that node to try and change that stance so that view/edit/etc. a comment is depending on ability to view the node.

Updated Issue Summary using the Issue Summary Template

Patch in #11 is committed, current security policy is to allow editing of comments on a node without permission for the node, discussion of this has moved to #1781766: Enforce that a user must be able to view a node to view/edit/delete the comments of that node, therefore close this issue as won't fix.

Almost sounds like it's a duplicate...

#1781766: Enforce that a user must be able to view a node to view/edit/delete the comments of that node is a follow-up to this one; it's IS explains the relationship.