Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.user.module via the admin/access menu produces a form to edit, for each permission defined for each module, which role is enabled. If one permission name (perm1) is a substring of another permission name (perm2), then if a role is enabled for perm2 it appears to be enabled also for perm1 in the form, and if the form is saved, also in the DB.
The patch is simple (also included as attachment):
--- user.module~ 2006-08-12 07:04:19.000000000 +0000
+++ user.module 2006-09-01 15:48:58.000000000 +0000
@@ -1791,7 +1791,8 @@
$form['permission'][$perm] = array('#type' => 'markup', '#value' => t($perm));
foreach ($role_names as $rid => $name) {
// Builds arrays for checked boxes for each role
- if (strstr($role_permissions[$rid], $perm)) {
+ // if (preg_match($role_permissions[$rid], $perm)) {
+ if (preg_match("/(^|,\s*)$perm(,|$)/", $role_permissions[$rid])) {
$status[$rid][] = $perm;
}
}
$perm is a permission string, $role_permissions[$rid] is a comma+space separated list of permissions enabled for role $rid. The strstr check also matches perm1 if perm2 belongs to the list, preg_match with the proper pattern doesn't.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | user_perm_substring_4_7.patch_1.txt | 1.23 KB | dww |
| #8 | user_perm_substring_5.patch_1.txt | 1.2 KB | dww |
| #7 | user_perm_substring_4_7.patch.txt | 1.22 KB | dww |
| #6 | user_perm_substring_5.patch.txt | 1.19 KB | dww |
| #1 | a_0.patch | 587 bytes | alusiani |
Comments
Comment #1
alusiani CreditAttribution: alusiani commentedI made an error on including the patch as attachment, I correct it here
Comment #2
bjaspan CreditAttribution: bjaspan commentedI reviewed this patch and agree with your approach. This problem still exists in 5.0, too. However,
(1) You should remove the commented-out call to preg_match for cleanliness and
(2) I think it would be better to use the word-boundary assertion (e.g. "/\\b$perm\\b/") instead of the conditional expression you used.
Comment #3
Zen CreditAttribution: Zen commentedAs per bjaspan, this occurs in 5. Therefore, please reroll for 5 and then backport to 4.7 once fixed.
Thanks,
-K
Comment #4
moshe weitzman CreditAttribution: moshe weitzman commentedthis is a rare event, so downgrading. i'm even tempted to downgrade to minor ...
Comment #5
Steven CreditAttribution: Steven commentedRegular expression word boundaries are unreliable. They match e.g. in the middle of "xml-rpc".
Comment #6
dwwIMHO, this is is almost critical, since it *really* confuses the hell out of the UI for permissions and can end up leading to permissions being granted that the admin didn't really intend. :( however, i won't get in a pushing match over the priority.
i discussed this with Heine (when i found this myself, i wanted to check with him to see if he thought it was a security issue or if i should post an issue here, and then searched the queue to find this). we decided that the simplest solution is to mimic the behavior of
user_access(), which basically always appends a,to the end of the list of permission names, and when searching always searches for$perm .','. it's a bit of a hack, but it's simple, fast, consistent with other parts of core, and it works. ;)attached patch for 5.x. 4.7.x patch coming next.
Comment #7
dww4.7.x patch. i took the liberty of removing a line of dead code (an array definition that's not used anywhere in the function) that was causing the 5.x patch to fail to apply cleanly.
Comment #8
dwweven better: use
strpos()instead ofstrstr():- faster and uses less memory
- consistent with
user_access()5.x version
Comment #9
dwwimproved 4.7.x version
Comment #10
bdragon CreditAttribution: bdragon commented#8, #9: Desk checked. Sane. No other lines reuse the variable in question. +1 from me.
The line of dead code has been dead since its creation with rev. 1.60, over 4 years ago.
Comment #11
Dries CreditAttribution: Dries commentedCommitted this patch to CVS HEAD and DRUPAL-5. Needs fixing in DRUPAL-4-7 as well.
Comment #12
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedbackported
Comment #13
(not verified) CreditAttribution: commented