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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

cedric’s picture

Status: Active » Needs review
FileSize
712 bytes

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

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

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
FileSize
788 bytes

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

dgtlmoon’s picture

Status: Needs review » Reviewed & tested by the community

  • dgtlmoon committed 573dabc on 7.x-2.x
    Issue #2033161: privatemsg_thread_load returns messages from all other...
dgtlmoon’s picture

Status: Reviewed & tested by the community » Fixed
rfay’s picture

Version: 7.x-2.x-dev » 7.x-1.x-dev
Status: Fixed » Needs review

If this is a serious bug shouldn't it also go into 7.x-1.x?

dgtlmoon’s picture

Sure, backport waiting on patch

rfay’s picture

Doesn't show well here, but this passed testbot on 7.x-1.x, https://qa.drupal.org/pifr/test/1074663

Berdir’s picture

Status: Needs review » Needs work

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

dgtlmoon’s picture

Status: Needs work » Needs review
FileSize
1.53 KB

Most of the codebase seems to be > 80 chars...

fixed the .

attached is the test

Status: Needs review » Needs work

The last submitted patch, 33: 2033161-include-test-and-tidy.patch, failed testing.

dgtlmoon’s picture

  • oadaeh committed ccfeeb8 on 7.x-1.x authored by cedric
    Issue #2033161 by cedric, dgtlmoon, ptmkenny, Berdir, rfay:...

  • oadaeh committed 6812cd8 on 7.x-2.x authored by cedric
    Issue #2033161 by cedric, dgtlmoon, ptmkenny, Berdir, rfay:...
oadaeh’s picture

Status: Needs review » Fixed

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

Status: Fixed » Closed (fixed)

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