privatemsg_message_load should return FALSE if the user has no access to the specified thread ID, but it is returning all the user's messages.
This results in a thread view page showing a weird thread instead of a page not found or access denied error if you attempt to open a thread you do not have access to.
I traced the problem to this line:
privatemsg.module:512
$thread['messages'] = privatemsg_message_load_multiple($query->execute()->fetchCol(), $conditions);
$query->execute()->fetchCol() will return an empty array because there are no messages in the thread the user has access to.
The empty array is sent to privatemsg_message_load_multiple which uses entity_load().
According to entity_load() API doc, if the first argument ($ids) is FALSE, it returns ALL the entities matching the condition.
The problem is that in PHP, an empty array is false. Thus, privatemsg loads all the messages belonging to the user instead of zero messages like it should in this condition.
Comments
Comment #1
cedric CreditAttribution: cedric commentedAttached patch skips the call to privatemsg_message_load_multiple() if there are no private messages to load.
This fixes the issue for me, an access denied page is generated as expected when attempting to access a thread you have no access to.
Comment #3
ptmkenny CreditAttribution: ptmkenny commented#1: privatemsg-fix_thread_load_no_access-2033161-1.patch queued for re-testing.
Comment #5
ptmkenny CreditAttribution: ptmkenny commented#1: privatemsg-fix_thread_load_no_access-2033161-1.patch queued for re-testing.
Comment #7
cedric CreditAttribution: cedric commentedCan somebody help me with the failing tests?
My patch is simple and brings back the expected behavior of the privatemsg_thread_load function. I don't understand why it is causing failure in PrivatemsgTagsTestCase, which seems unrelated..?
Is there a bug in the PrivatemsgTagsTestCase which I am not aware of?
Comment #8
BerdirBecause you specified an old, broken version.
Comment #9
Berdir#1: privatemsg-fix_thread_load_no_access-2033161-1.patch queued for re-testing.
Comment #10
cedric CreditAttribution: cedric commentedGreat, tests passed.
If you want to quickly see what this patch attempts to solve, simply try to access a private message thread you do not have access on a test site. You will see a weird thread containing all your *other* messages.
At first I thought this was a security flaw, because I did not get the expected Access Denied page. Then, I realize it can't really be exploited to gain anything interesting, but I was compelled to find the source of this weird behavior.
Comment #11
ptmkenny CreditAttribution: ptmkenny commentedAwesome. It would be great if this patch had a test to confirm that an access denied is now being returned properly in this particular case. Most of the patches that have been committed to Privatemsg recently include tests that demonstrate their functionality.
Comment #12
cedric CreditAttribution: cedric commentedSure. I looked at the test suite and the test is already there (testPrivatemsgReadPrivatemsgPermission), but it is too simple and it is not triggering the bug.
If the user is not involved in *any* thread, the 403 Access Denied occurs. This is what testPrivatemsgReadPrivatemsgPermission() checks.
I added one line of code in the test that creates a second, unrelated thread, but to which the user has access. This triggers the bug, and now, the test fails.
The attached patch contains only the modified test. Test bot should FAIL.
Comment #13
cedric CreditAttribution: cedric commentedAnd finally, the combined patch (#1 + #12).
This one should pass tests again!
Comment #14
ptmkenny CreditAttribution: ptmkenny commentedChecking against the 2.x-dev series of tests as well, since those are more extensive.
Comment #15
ptmkenny CreditAttribution: ptmkenny commented#13: privatemsg-add_test_thread_load_no_access-2033161-13.patch queued for re-testing.
Comment #16
ptmkenny CreditAttribution: ptmkenny commented#12: privatemsg-add_test_thread_load_no_access-2033161-12.patch queued for re-testing.
Comment #17
ptmkenny CreditAttribution: ptmkenny commentedOk, I successfully applied this to my own installation and ran my set of Selenium tests for my site; everything worked fine, so I am marking RTBC.
Comment #18
BerdirThat comment is > 80 characters and also doesn't really explain the problem. That behavior is by design, not a quirk.
Maybe something like this?
// Make sure that $no_recipient is involved in another thread to assert that no unrelated messages are displayed, see http://...
Wrapped at 80 characters :)
Comment #19
BerdirComment #20
cedric CreditAttribution: cedric commentedentity_load documents the second parameter as:
Is it really by design that if I do the following, I will receive all entities as a result :
To me it is an undocumented corner-case.
Comment #20.0
cedric CreditAttribution: cedric commentedtypo
Comment #21
dgtlmoon CreditAttribution: dgtlmoon commentedThis is a really horrible bug if you have a large/busy website
Patch should be a no brainer, straight after it loads the id's (which are then empty) there is
Looks like it should have supported this but was simply missed
Comment #25
dgtlmoon CreditAttribution: dgtlmoon commentedComment #27
dgtlmoon CreditAttribution: dgtlmoon commentedComment #28
rfayIf this is a serious bug shouldn't it also go into 7.x-1.x?
Comment #30
dgtlmoon CreditAttribution: dgtlmoon commentedSure, backport waiting on patch
Comment #31
rfayDoesn't show well here, but this passed testbot on 7.x-1.x, https://qa.drupal.org/pifr/test/1074663
Comment #32
BerdirThe patch that was committed didn't contain the test coverage from the previous patch, can someone provide a patch that includes that. Just for 7.x-1.x is fine, I can then cherry-pick that parts to 7.x-2.x.
Coding standards of the committed patch also weren't correct, > 80 characters and missing . at the end.
Comment #33
dgtlmoon CreditAttribution: dgtlmoon commentedMost of the codebase seems to be > 80 chars...
fixed the .
attached is the test
Comment #35
dgtlmoon CreditAttribution: dgtlmoon commentedComment #38
oadaeh CreditAttribution: oadaeh at Hook 42 commentedI applied the patch to the 7.x-1.x branch, including the test and the updated comments, and I also added the test and updated comments to the 7.x-2.x branch.