i just stumbled into a minor, problem: the admin/settings/upload page is currently double-escaping output. theme('placeholder') already does a check_plain(), and apparently some other layer (perhaps FAPI field sets?) is doing another check_plain(). the result is that whenever any user roles have permission to upload files, this page has stray <em> tags sitting as output, instead of the expected behavior. screen-shot (from a local 4.6) site attached. this bug exists in 4.7 and HEAD, too, so it should be fixed on all branches. removing the theme('placeholder') seems like an obvious choice, but i'm not 100% sure where the 2nd escaping is coming from. until i'm absolutely certain taking out that theme('placeholder') isn't going to introduce an XSS exploit, i'm not touching it. ;) therefore, i'm creating the issue so that either a) someone who already knows all the answers can just do the right thing or b) i'll look at this again when it's not 4am and i have a chance to actually dig deeper.

screenshot attached for the interested reader. note the <em> tags around the role names in the "Settings for [role]" fieldset titles.

CommentFileSizeAuthor
#7 upload.module_43.patch1.34 KBdww
#6 upload.module_42.patch1.09 KBmagico
#4 upload.module_41.patch1.1 KBmagico
upload-settings-double-escape.png160.22 KBdww

Comments

magico’s picture

magico commented
Version: 4.6.7 » 4.6.9
Priority: Minor » Normal

I confirm this bug.
Must confirm if this happens in HEAD.

magico’s picture

magico commented
Version: 4.6.9 » 5.x-dev
Status: Active » Fixed
dww’s picture

dww
we/he/they
commented
Version: 5.x-dev » 4.6.x-dev
Status: Fixed » Active

great, glad to hear it's now fixed. ;) however, the bug still exists back in 4.6.x, so if 4.6.12 is going to happen, it might be worth fixing.

magico’s picture

magico commented
Status: Active » Needs review
StatusFileSize
new upload.module_41.patch1.1 KB

@dww could you review my patch? Thanks

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

theme('placeholder') just calls check_plain(). the issue here is that form_group() *also* calls check_plain():

DRUPAL-4-6 branch:
includes/common.inc ~line 1009:

function form_group($legend, $group, $description = NULL) {
  return '<fieldset>' . ($legend ? '<legend>'. check_plain($legend) .'</legend>' : '') . $group . ($description ? '<div class="description">'. $description .'</div>' : '') . "</fieldset>\n";
}

so, just rip out the theme('placeholder'), but don't replace with check_plain().

magico’s picture

magico commented
Status: Needs work » Needs review
StatusFileSize
new upload.module_42.patch1.09 KB

When the label is set to the form_group, it is given to it the following array('%role' => theme('placeholder', $role)) which results in the following be called from theme.inc

function theme_placeholder($text) {
  return '<em>'. check_plain($text) .'</em>';
}

I just followed the code present in drupal 5 upload.module that has t('Settings for @role', array('@role' => $role)) that explains that @ are converted directly to check_plain()

In the end, I just tried to do the correct code port from the most recent branch. Anyway what you are suggesting is correct, and of course I did a patch as you requested.

Thanks

dww’s picture

dww
we/he/they
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new upload.module_43.patch1.34 KB

minor gripe: the patch should have been from the root of core, not inside the modules directory. re-rolled for that, but otherwise, this is the identical patch. reviewed, tested, works. RTBC.

gerhard killesreiter’s picture

gerhard killesreiter commented
Status: Reviewed & tested by the community » Closed (won't fix)

4.6 is dead and buried.