D8 followup DRUPAL-SA-CORE-2012-002 - Access bypass - forum listing

The node must be removed from forum_index once it is marked as unpublished.

The check for node->status is unnecessary because the index must be updated, and it is checked later for the other operations, please review the attached patch.

The bug is in D7 too.

Patch backported to d7.

This cleanly applies to both D7 and D8. Looks good to me, marking RTBC.

Ok, let's try to make some tests.

Patch attached.

The same patch applies too to 7.x

A nifty trick in such cases is to post two patches, one with tests only, and one with tests plus the fix.

In this way you can demonstrate (via the red/green color combination) that your tests fail before the fix, and pass afterwards.

Makes sense, here it is.

Something must be wrong if the second one passes...

Another patch with a test that looks better. Let's see what the testbot thinks.

Edited --DUPLICATED--

Patch that includes the fix for the issue.

For me is RTBC, unassigning me so it gets reviewed.

As suggested by chx, added an update path for 7.x sites in 7.x branch.

This needs testing, but depends on #1182296: Add tests for 7.0->7.x upgrade path (random test failures) AFAIU.

Unassign me, waiting for comments.

Needs refill post /core patch

#18 doesn't really aply to D8.x

#16 rerolled for /core

Rerolled patches for D8 to HEAD.

#23: forum-topic-unpublished-1302404-22.patch queued for re-testing.

We have tested the patch #23 and it keeps failing.

@heilop @edusempai

I have tested this and I can't reproduce this bug in D8-HEAD or D7-dev.

Unpublished forum topics don't appear in the topic list and don't appear in the block list.

More steps to reproduce the bug?

Untaging

@oriol_e9g I reproduced the bug with the given instructions. The problem happens when you unpublish an existing and published post. I'll try to edit the summary to be more explicit.

Ok, I was testing the wrong thing: I was testing about the topics count displayed, and that was correct.

I added now another asserts for testing if the topic is or not listed.

The code approach has changed too.

Let's see if the build bot gives some love back.

Per @aspilicious review on IRC:

* one comment exceeds 80 chars and the summary line should be less or equal than 80 chars

Fixed those issues, improved wording and Doxygen comments.
Merged two last lines of aux function.

Bot!

Code looks ok to me, will leave this to others to see if they can identify possible problems with the approach.

The code looks good, same patch but with t() removal in assertions for #500866: [META] remove t() from assert message

I have tested the code and for my is RTBC.

This has been considered a vulnerability in D7 and fixed in http://drupal.org/node/1557938, so I think that is not bad increasing priority.

Advisory ID: DRUPAL-SA-CORE-2012-002
Access bypass - forum listing
CVE: CVE-2012-1590

This was fixed in D7 as part of SA-CORE-2012-002. However, we still need the fix in D8.

Here's the patch from the security team issue tracker (which also needs the small typo fix in #1558402: D8 followup DRUPAL-SA-CORE-2012-002 - Access bypass - forum listing).

Commit credit should include mlhess, coltrane, xjm, and jhodgdon, as well as penyaskito.

I have not yet looked to see how different the committed D7 patch is from the one in this issue.

I'll roll this one. The patches were pretty much the same.

I have attached the output of git diff -b over the module to make the change super easy to see.

Thanks folks. Committed/pushed this one to 8.x.