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.Documentation of hook perm says "The permissions in the array do not need to be wrapped with the function t(), ...".
Wrapping the permission with t() can cause a privilege escalation:
function example_perm() {
return array(t('administer example'), t('use example'));
}
Now translate 'administer example' into 'use example'. Any user who may 'use example' can now also 'administer example'.
The attached patch (for Drupal 6) says "The permissions in the array must not be wrapped with the function t(), since this could lead to privilege escalation. The string extractor takes care of extracting permission names defined in the perm hook for translation."
The documentation for user_access() could use a similar hint.
Drupal 7 might not be affected because Developers are unlikely to translate array keys.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 310168again.patch | 1.64 KB | arianek |
| #6 | 310168again.patch | 1.52 KB | jhodgdon |
| #5 | 310168new.patch | 1.51 KB | jhodgdon |
| #3 | 310168.patch | 1.23 KB | jhodgdon |
| hook_perm.patch | 965 bytes | traxer | |
Comments
Comment #1
add1sun CreditAttribution: add1sun commentedMoving to correct queue for API docs.
Comment #2
jhodgdonI reviewed this, and agree it is a good idea. So I went ahead and committed it to D6 doc (contrib repository).
I think we do need a patch for D7, which I'll attach in a few minutes, because the current wording is misleading.
Comment #3
jhodgdonHere is a patch for D7 to clear up what should and shouldn't be translated. Also fixes 80-character wrapping issue in this function's doc.
Comment #5
jhodgdonHere's a new patch that should apply.
I moved stuff around so that all information pertinent to @return would stay together, and would be at the bottom of the function doc.
Comment #6
jhodgdonSorry, I was missing a newline in that patch.
Comment #8
jhodgdonSetting back to Needs Review in hopes that the patch will get retested. The test bot was apparently malfunctioning (I had about 7 patches fail in that time period, all were doc, none broke the HEAD install I am pretty sure).
Comment #9
arianek CreditAttribution: arianek commentedapplied this patch to D7 HEAD and got:
i am not advanced enough to understand the actual issue, but re-rolling the patch for the testing bot.
Comment #10
jhodgdonSetting to "Novice", as a novice should be able to review this doc-only patch.
Comment #11
JuliaKM CreditAttribution: JuliaKM commentedThis patch applies cleanly and the description is easy-to-understand.
Comment #12
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks.