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.

Files: 
CommentFileSizeAuthor
#13 privatemsg-add_test_thread_load_no_access-2033161-13.patch1.38 KBcedric
PASSED: [[SimpleTest]]: [MySQL] 4,913 pass(es).
[ View ]
#12 privatemsg-add_test_thread_load_no_access-2033161-12.patch698 bytescedric
FAILED: [[SimpleTest]]: [MySQL] 4,912 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#1 privatemsg-fix_thread_load_no_access-2033161-1.patch712 bytescedric
PASSED: [[SimpleTest]]: [MySQL] 3,157 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new712 bytes
PASSED: [[SimpleTest]]: [MySQL] 3,157 pass(es).
[ View ]

Attached 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.

Status:Needs review» Needs work

The last submitted patch, privatemsg-fix_thread_load_no_access-2033161-1.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, privatemsg-fix_thread_load_no_access-2033161-1.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, privatemsg-fix_thread_load_no_access-2033161-1.patch, failed testing.

Can 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?

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

Because you specified an old, broken version.

Status:Needs work» Needs review

Great, 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.

Awesome. 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.

StatusFileSize
new698 bytes
FAILED: [[SimpleTest]]: [MySQL] 4,912 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Sure. 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.

StatusFileSize
new1.38 KB
PASSED: [[SimpleTest]]: [MySQL] 4,913 pass(es).
[ View ]

And finally, the combined patch (#1 + #12).

This one should pass tests again!

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

Checking against the 2.x-dev series of tests as well, since those are more extensive.

Version:7.x-2.x-dev» 7.x-1.x-dev
Status:Needs review» Reviewed & tested by the community

Ok, 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.

Status:Reviewed & tested by the community» Needs work

+++ b/privatemsg.testundefined
@@ -85,6 +85,10 @@ class PrivatemsgTestCase extends PrivatemsgBaseTestCase {
+    // Because of a quirk in the entity_load function, $no_recipient should be involved in another, unrelated thread.
+    // see https://drupal.org/node/2033161
+    $unrelated = privatemsg_new_thread(array($no_recipient), $subject, $body, array('author' => $author));

That 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 :)

Priority:Normal» Major

entity_load documents the second parameter as:

$ids: An array of entity IDs, or FALSE to load all entities.

Is it really by design that if I do the following, I will receive all entities as a result :

entity_load("foo", array());

To me it is an undocumented corner-case.

Issue summary:View changes

typo