Hello!
If I put a single quote in a field name, the tooltip that shows it (for instance, the sort tooltip), have an entity (') instead of a quote (').
You can see it in the attachment.
To reproduce this bug, you just have to enter a single quote (') in a sortable field name and hover the sort link with the mouse to see the tooltip.
Thanks to fix this problem.

CommentFileSizeAuthor
#8 1773828-8-fix-double-escaping-quotes-in-sort.patch1.16 KBsylvainm
#5 Screen Shot 2012-09-17 at 12.03.49.png23.06 KBsphism
#5 views_1773828-5.patch1.06 KBsphism
#4 test_issue_1773828.txt2.95 KBsphism
#4 Screen Shot 2012-09-17 at 11.01.12.png24.79 KBsphism
drupalViewsBug.png6.61 KBglcp

Comments

glcp’s picture

glcp commented

Here are the changes needed to fix this bug:
in the file views/theme/theme.inc, around line 498, change this line:

$label = check_plain(!empty($fields[$field]) ? $fields[$field]->label() : '');

by this one:

$label = htmlspecialchars(!empty($fields[$field]) ? $fields[$field]->label() : '', ENT_COMPAT | ENT_HTML401, 'UTF-8');

and
around line 509, change this line:

$title = t('sort by @s', array('@s' => $label));

by this one:

$title = t('sort by ') . $label;

We should check if others character could be in the label. This fix works only for the ' character. We also need to fix the translation files if there are any.
Thanks to fix this bug.

sphism’s picture

sphism commented

Hi, I'll try to make a patch for this.

The first change looks good but the second one

$title = t('sort by ') . $label;

You shouldn't concatenate a translated field onto the label like that, you should do it like they've done:

$title = t('sort by @s', array('@s' => $label));

However where that @ sign is - you can use a couple of different signs to do slightly different things, which may accomplish what you are trying to achieve.

http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/forma...

sphism’s picture

sphism commented
Assigned: Unassigned » sphism

Having a look into this now

sphism’s picture

sphism commented
Title: Quotes in sort tooltip are shown as entity » Quotes in sort tooltip and 'sort by' drop down are shown as html code
StatusFileSize
new Screen Shot 2012-09-17 at 11.01.12.png24.79 KB
new test_issue_1773828.txt2.95 KB

Here's a views export of a simple view to demonstrate the issue.

It lists all content title's in a table, the title field has been renamed to Title's and made sortable. The issue is that the tooltip says Title's

Also the sort order has been exposed with the name Title's and we get a similar issue in the "Sort By" drop down list Title's.

I've attached a screenshot showing both these

sphism’s picture

sphism commented
Status: Active » Needs review
StatusFileSize
new views_1773828-5.patch1.06 KB
new Screen Shot 2012-09-17 at 12.03.49.png23.06 KB

This patch fixes both the issue's in #4 and the screen shot show's the fixes.

Screen Shot 2012-09-17 at 12.03.49.png

Note that I've used the title of:

Title's <> "

to check for single/double quotes and less/greater than. It's interesting that the less/greater than is removed from the tooltip, but remain in the drop down (is that an XSS issue?)

I'm wrapping the title outputs in htmlspecialchars_decode($title, ENT_QUOTES) so that single and double quotes are turned back into regular text.

Essentially this means you have a check_plain, to encode it then the htmlspecialchars_decode to decode it again. I believe that this will verify UTF-8 and stop XSS.

If there's a better way to do this then let me know, but I think this is correct.

sphism’s picture

sphism commented
Version: 7.x-3.3 » 7.x-3.x-dev

Changing version to 7.x-3.x-dev to see if my patch gets tested...

ecrown’s picture

ecrown commented
Issue summary: View changes

the patch at #5 worked for me was having the same issue on a multilingual site

sylvainm’s picture

sylvainm commented
StatusFileSize
new 1773828-8-fix-double-escaping-quotes-in-sort.patch1.16 KB

I think you don't need to use htmlspecialchars_decode, as the problem is that the $label is escaped twice.
I tried to remove the double escaping and did not succeed to have an XSS with the attached patch.
What do you think about ?

chris matthews’s picture

chris matthews commented
Assigned: sphism » Unassigned

The 4 year old patch in #8 to views_plugin_exposed_form.inc and theme.inc applied cleanly to the latest views 7.x-3.x-dev and if still relevant needs to be reviewed.

Checking patch plugins/views_plugin_exposed_form.inc...
Hunk #1 succeeded at 245 (offset 30 lines).

Checking patch theme/theme.inc...
Hunk #1 succeeded at 522 (offset 13 lines).

Applied patch plugins/views_plugin_exposed_form.inc cleanly.
Applied patch theme/theme.inc cleanly.