global $user->roles get changed by argument_user_validate

This sounds like a real critical bug for me. Can you export one of these views?

Thanks

I've encountered a similar issue that sounds related. here are the symptoms i've found:

1) most pages are fine/i'm not seeing any issues with disappearing blocks though my site doesn't use blocks heavily so that might not be meaningful
2) only 2 pages that list articles [standard default content-type] or events [cck content type] have problems
2b) all other content type views work as expected
3) on those problem content-type pages, the $user->roles array doesn't indicate the Role label; rather, the array is [autoincrementing key]=>[role id #] ( vs. the expected [role id #] => [role label]
4) on those problem pages, only admin users have permission to edit articles / are rendered the edit link
5) on the actual node edit page, a given role has no problems editing

for now i've had to revert to 2.10 which fixed the problem temporarily. if i find anything else related, i'll post.

Wow wow wow ! This bug is currently breaking my site : when I do a global $user; I find the exact description of tdonia's problem

An array of autoincremented keys with $rid as values, problem is : it change the global $user object, and when I'm doing tests later in code with isset($user->roles[$rid]), it doesn't work.

This breaks my site since Sunday. So yes, this is critical. I'm digging into the code right now to find the problem...

I've found this issue searching for $user->roles on Views project because I notices a line in last Changelog :
#815392 by killes and dereine: Incorrect construction of $account caused accidental non-permanent change to $user->roles.

Here is a patch

Thanks! the patch seems to solve it for me. I'll post back here if i encounter any related issues but so far all testing is positive.

Much appreciated.

Actually - sorry to be a bother, it's possible this is my own operator error, but the patch seems to be for the version of views_plugin_argument_validate_user.inc that's part of 3.x-dev - is that OK / are those files interchangeable?

And now i'm noticing a new issue in the patched version - views are only showing content that's been created by the user that's viewing. Sorry for not catching that sooner. Seems to happen whether i (manually) apply the patch to 2.11 views_plugin_argument_validate_user or to the 3.x dev version. Will poke around and see if i can come up with any leads.

+++ modules/user/views_plugin_argument_validate_user.inc
@@ -102,13 +110,7 @@ class views_plugin_argument_validate_user extends views_plugin_argument_validate
+      if (!(bool) array_intersect_key($account->roles, $roles)) {

I really wonders how this worked. Roles has the same structure as $user->roles on a normal drupal.

@tdonia

What do you think this argument validation does? It gives/denies you access to views, if the argument is

a) the current user
b) one of role X

Good tip. If this returns false, per my view definitions, it'll display all results. If this returns true, it should only display the results that are allowed for the given user. That's sort of backwards from a design perspective, but the intended use here is 'if we know this person is a reporter, they should only see their own work; if we know they're an editor or admin, they can see everything'. What's still throwing me is that admin/editors are now being limited to only what they've created. Maybe it's possible that we've been relying on a bug and should just redesign our views? It's hard for me to judge what the 'correct' behavior here is supposed to be.

This is really something rather totally different

"if we know this person is a reporter, they should only see their own work;"
Then you would have to add a filter user: current

It seams that you want to have different views for different users

@dereine - thanks for the patch. I can confirm that it is working and has solved the problem I had after upgrading to 2.11 where a first use of a view would bring results but subsequent uses would return nothing (because the global user account roles were modified improperly).

That said, I believe the code path in my case is using the global user account, so I haven't tested your code to add the roles to a new local $account. From looking at the code it seems it should work (at least it's much better than the original :).

array_intersect_key() is PHP 5.1+ only -- we can't use that function as we need to retain PHP4 compatibility.

Here's another approach.

1. We already have the roles in the global $user object, so don't requery in that case.
2. Use array_intersect() as before on two arrays with role ids as their values.
3. More explicit variable naming.

Looks fine.

I didn't tested the patch yet.

So

Rename the title to be able to find it

This patch resolved the incorrect formatting of $user->roles for me. Thanks, @Mark Theunissen.

Yeeeah this was kind of a headache, but fixed by the patch. Thanks!

Oh, neat: this patch solves a problem I had with Better Formats and FCKeditor not respecting role settings, which I had initially traced only as far back as Project issue tracking (#859048: function project_issue_views_user_access breaks FCKeditor and default input formats.).

I hate to run a patched module on my production site though. I guess there's no way of knowing how soon a bugfix like this will make it into an official release?

Patch from comment #17 solved my problem. Thanks Mark Theunissen.

Quite critical issue, subscribing.

I have simular problem as in post #25
However i have no idea how i have to apply this patch? Can i do this manualy?

http://drupal.org/patch/apply is the tutorial how to apply patches. I would never suggest to do this manually, because this can cause many problems.

Is there anything specifically preventing this patch from being committed?

Yes, it appears that it applies to 2.x but not 3.x. Can I get a reroll for 3.x (and preferably 7.x-3.x as well?)

here are some reroles

I think these patches are the wrong approach. This argument validator shouldn't be touching the global $user object at all. And, if it weren't for PHP being evil, we wouldn't be. The root of the problem is here:

$account = $GLOBALS['user'];

If $GLOBALS['user'] were an int, this would do what we think it does -- you just get a copy (not a reference) to the int. For example:

$GLOBALS['foo'] = 1;
echo $GLOBALS['foo'] . "\n";
$foo = $GLOBALS['foo'];
$foo = 2;
echo $foo . "\n";
echo $GLOBALS['foo'] . "\n"; 

This gives you the following output, as you'd expect:

1
2
1

However, if you try it with an object:

$user = new stdClass();
$user->uid = 1;
echo $user->uid . "\n";
$GLOBALS['foo'] = $user;
$foo = $GLOBALS['foo'];
$foo->uid = 2;
echo $foo->uid . "\n";
echo $GLOBALS['foo']->uid . "\n"; 

You get this:

1
2
2

Argh. Thanks, PHP.

We don't want to be touching $GLOBALS['user'] here and modifying anything. I think a well-placed drupal_clone() will solve this much cleaner and safer. Stay tuned...

I'd normally consider a bug like this something to solve via the security team process. However, it doesn't appear that this bug can lead to elevated privs, so I guess we can continue to solve it here in public...

Untested. Rolled for D6--2 branch, although it applies (with minor fuzz) to both D6--3 and D7--3.

Note: I tested this. It does continue to work, and it solves the bug. I'm planning to just commit to all 3 branches later today unless there's a serious objection.

Let's get it fixed. This patch fix the issue and looks fine and explains the problem very well.

We talked a bit in IRC so we should try to clean up the plugin in another issue.

Thanks for the review! Committed to:

DRUPAL-7--3
DRUPAL-6--3
DRUPAL-6--2

IN d7 drupal_clone does not exist anymore. I commited a fix in the current CVS.

Thanks dereine: I should have tested more carefully before committing!

Hey, i set the patch as RTBC. This happens.

@dww,

A question - it's a common thing to see in drupal:

function foo($account = NULL) {
  if (empty($account)) {
    global $user;
    $account = $user;
  }
}

Does it mean, according to your findings, that the correct line should be $account = clone($user); ?

@Amitaibu: Yeah, a little further investigation says "yes". You need to clone($user) when you assign it to $account, or changes to $account will change the global $user. *sigh* We (the security team) should blog about this. I know we've been suggesting to use $account like this in the past, we should clarify.

Phew, this is a nasty one. Caused all sorts of problems with Content Access and nodereference on a site I'm building. The nodereference field was testing for permissions to view the referenced node using the $user->roles array that had been mangled by Views. So rather than using the actual role IDs it was using role IDs counting up from zero: i.e. anonymous and authenticated rather than the actual roles of the current user. So permission was denied when it shouldn't have been, resulting in "this post can't be referenced" even when the autocomplete field had found the node in question!

It could possibly result in elevated privileges, as the current user's role IDs are reduced from high numbers to a series of numbers starting from zero. So in this, probably fairly unusual situation, we could have problems:

rid 0 = anonymous
rid 1 = authenticated
rid 2 = site administrator (a high-privilege role with a low role ID value)
rid 3 = normal user (a low-privilege role)
rid 4 = other user (a second low-privilege role)

Demo user before roles get messed up:
$user->roles = array(1 => 'authenticated', 3 => 'normal user', 4 => 'other user');

And after:
$user->roles = array(0 => 1, 1 => 3, 2 => 4); // effective roles 'anonymous', 'authenticated', 'site administrator'.

i.e. a user could gain low-numbered roles that they aren't supposed to have, if they have more than two roles assigned to them.

In any case this causes problems as registered users will end up with both anonymous and authenticated roles. Not nice.

I have also just waisted lot of time with this issue which also caused elevated privileges on one of my sites. The most frustrating for my users was menu items appearing in the primary links they don't have access to. At least when they tried to access the link they got an access denied page.

When you combine this issue with panels things can get even more scary.

I would like to suggest a new release of views because of this issue as it is a security risk.

This bug is fixed. Please open a new issue (and link to this one) if you want to advocate for a new official views release. Simply re-opening this issue isn't going to help. Thanks.

This fix doesn't seem to have made it into today's 6.x-2.12. Can anyone clarify for me what is meant by "fixed", if it's not at least "will be included in the next release"?

@tinkerbelle: 6.x-2.12 was a security-only bug fix, created from the 6.x-2.11 codebase with only a single patch applied (to fix a security vulnerability). All of this you would know if you read the 6.x-2.12 release notes. So yeah, normally, "fixed" means "will be available in the next release. However, security releases are a special case, so this bug fix will be included in the 6.x-2.13 release. Such is life.

The bug gave problems using IMCE (with wysiwyg en imce_bridge) after applying the security update. I just used the patch again and that did work.

Thanks for the patch.. helped but wasted whole one day digging and blaming Panels module.

Must be in the 6.x-2.12 as update, might have saved good time.

Thanks again :)

i think this could create a security issue if the original user has a number of roles assigned >= 4, the modified object will have the number of those roles assigned as keys (0=>, 1=>, 2=>, 3=>..)<-user with 4 roles.., which grants the user access to your first custom created role, this could be a super admin account.. user_access only checks keys so the values are not important

if the view runs on a page where that user could benefit access, then he will

i didnt test this, but if im right, it should go through the security team.. ill make a note of this there before opening the issue again