In Drupal Core 6.x, the query that built the admin/content/node page was passed through the node access system, and any node access module that implemented hook_db_rewrite_sql() could remove the API nodes from the page.

In 7.x, the query for that page isn't run through the node access system (there is no "node_access" tag added to the query). Instead, anyone with "view content overview" permission gets to see all published nodes.

This needs tests which need to be backported to D7. There's a node_test.module that implements some node access stuff afaik.

Commit credit should go to jhodgdon. Here's the patch we committed to D7.

Private tracker #: 72648

CommentFileSizeAuthor
#12 1558478_12.patch1.77 KBchx
#8 node-access-admin-content-1558478-7-tests-only.patch1.92 KBBerdir
#8 node-access-admin-content-1558478-7.patch2.33 KBBerdir
#6 drupal-1558478-6.patch422 bytestim.plunkett
fix-node-access-admin-nodes-7x-do-not-test.patch402 byteswebchick
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Issue tags: +Security improvements

Oops. And also...

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Assigned: Unassigned » xjm

I'll work on this. I'm friends with the node access test module. :)

Bojhan’s picture

Bojhan CreditAttribution: Bojhan commented

Are you still working on this? Critical bugs are currently above thresholds.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Should it even be marked critical? The security patch was already committed. This is just open for tests that need to be written.

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented

This isn't committed to D8.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
drupal-1558478-6.patch422 bytes
aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Needs work

"This needs tests which need to be backported to D7. There's a node_test.module that implements some node access stuff afaik."

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs work » Needs review
FileSize
node-access-admin-content-1558478-7.patch2.33 KB
node-access-admin-content-1558478-7-tests-only.patch1.92 KB

Ok, NodeQueryAlterTest might not be the perfect place, but it is very easy to extend them to check admin/content as well.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

#8: node-access-admin-content-1558478-7.patch queued for re-testing.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs review » Reviewed & tested by the community

Couldn't be simpler.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Awesome, THANKS.

Committed and pushed to 8.x. Needs a small re-roll for 7.x.

chx’s picture

chx CreditAttribution: chx commented
Status: Patch (to be ported) » Reviewed & tested by the community
FileSize
1558478_12.patch1.77 KB

This only needs tests for D7 and the patch applied cleanly against node.test. If the bot comes back green it's good to go.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Yay! Committed and pushed to 7.x. Thanks!

Status: Fixed » Closed (fixed)

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

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Assigned: xjm » Unassigned