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
#21 2033161-dont-load-all-threads-on-empty.patch788 bytesdgtlmoon
Test request sent.
Previous result: FAILED: [[SimpleTest]]: [MySQL] 4,913 pass(es), 0 fail(s), and 26 exception(s).
[ View ]
#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

cedric’s picture

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.

ptmkenny’s picture

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.

ptmkenny’s picture

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.

cedric’s picture

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?

Berdir’s picture

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

Because you specified an old, broken version.

Berdir’s picture

Status:Needs work» Needs review
cedric’s picture

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.

ptmkenny’s picture

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.

cedric’s picture

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.

cedric’s picture

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!

ptmkenny’s picture

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.

ptmkenny’s picture

ptmkenny’s picture

ptmkenny’s picture

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.

Berdir’s picture

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

Berdir’s picture

Priority:Normal» Major
cedric’s picture

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.

cedric’s picture

Issue summary:View changes

typo

dgtlmoon’s picture

Version:7.x-1.x-dev» 7.x-2.x-dev
Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new788 bytes
Test request sent.
Previous result: FAILED: [[SimpleTest]]: [MySQL] 4,913 pass(es), 0 fail(s), and 26 exception(s).
[ View ]

This 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

      // If there are no messages, don't allow access to the thread.
      if (empty($thread['messages'])) {

Looks like it should have supported this but was simply missed

Status:Needs review» Needs work

The last submitted patch, 21: 2033161-dont-load-all-threads-on-empty.patch, failed testing.

Status:Needs work» Needs review