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.
Problem/Motivation
I have a view with aggregation enabled. When I rewrite the output of a field and add HTML markup, it is escaped (i.e. the markup is shown). If I turn aggregation off, the field is rendered as expected.
This only happens if the Aggregation Type is NOT set to group results together.
Proposed resolution
Remaining tasks
User interface changes
API changes
Data model changes
Comment | File | Size | Author |
---|---|---|---|
#25 | aggr_html_is_escaped-2714045-21.patch | 3.73 KB | Lendude |
#23 | aggr_html_is_escaped-2714045-23.patch | 4.84 KB | Lendude |
#23 | interdiff-2714045-21-23.txt | 1.91 KB | Lendude |
#21 | aggr_html_is_escaped-2714045-21.patch | 3.73 KB | Lendude |
#21 | interdiff-2714045-4-21.txt | 2.13 KB | Lendude |
Comments
Comment #2
cilefen CreditAttribution: cilefen commentedComment #3
joelpittetThanks for creating this issue @ExTexan. I reproduced the error in 8.2.x:
It seems to only happen if I change the aggregation settings on the field as well (in my case I added SUM or AVG)
Included the export of the config in the yaml file attached.
Comment #4
joelpittetHere's a fix and some tests. The tests only patch should fail once or twice and the patch will resolve this.
Comment #6
dawehnerOne could use
$this->assertSubString()
here.Comment #8
suzymasriI'm having a look at this @DrupalConDublin.
Comment #9
anouschka42 CreditAttribution: anouschka42 commentedI am having at look at this @DrupalCon Dublin
Comment #10
slowflyer CreditAttribution: slowflyer commentedComment #11
slowflyer CreditAttribution: slowflyer commentedI did a manual review.
I've been able to reproduce the error.
The patch applied fixed the error.
regards,
slowflyer
Comment #12
anouschka42 CreditAttribution: anouschka42 commented@slowflyer hey how did you reproduce this, we are currently trying to reproduce this error on 8.1 8.2 and 8.3 and can't.
Comment #13
alexpottDiscussed with @timplunkett, @dawehner, @cilefen and @xjm. We agreed that this is a major issue because the views render pipeline should not cause escaping and is core views functionality.
Comment #14
anouschka42 CreditAttribution: anouschka42 commentedok so I have managed to reproduce this, apologies.. I had not set the aggregation type to SUM. If i do, I can reproduce this. So I am now testing the patch again to see if it fixes the issue.
Comment #15
anouschka42 CreditAttribution: anouschka42 commentedI have managed to reproduce the issue now and have tested the patch views_html_is_escaped-2714045-4.patch against 8.2 and it seems to fix the issue for all Aggregation type settings.
Comment #16
anouschka42 CreditAttribution: anouschka42 as a volunteer commentedThis looks good to me
Comment #17
anouschka42 CreditAttribution: anouschka42 as a volunteer commentedComment #18
alexpott@anouschka42 thanks for reviewing this issue. It'd be great to see before and after screenshots of your manual testing. Also given the security implications of:
I think we need a views subsystem maintainer to review this patch before committing.
Comment #19
lomasr CreditAttribution: lomasr at gai Technologies Pvt Ltd for gai Technologies Pvt Ltd commentedI created a simple view with content type name & count of Nid. I enabled aggregation in order to get total number of nid for a content type. The patch at #4 worked for me . Please see before & after screenshots.
Comment #20
anouschka42 CreditAttribution: anouschka42 as a volunteer commentedManually tested against 8.2. I adjusted the content view to rewrite the title of an article with a h1 tag and enabled aggregation.
See screenshots. without patch
with patch #4 applied
So patch #4 worked for me. Thanks :)
Comment #21
LendudeWell lets at least add a test to make sure it is safe. So something like this?
Comment #22
dawehner@Lendude
That's a great expansion
One thing I am wondering: Should
renderAltered
mark it as safe somehow? Note:\Drupal\views\Plugin\views\field\FieldPluginBase::tokenizeValue
has another call to it. Is there a similar kind of bug in there as well?Comment #23
Lendude@dawehner so maybe something like this? Alters the return value of
renderAltered
so bit of an API change.... not sure if that's worth it.Comment #25
LendudeYeah the impact of changing what renderAltered does is way bigger then this issue, so lets not go there. Reuploading #21, I think that is sufficient for this.
@dawehner the call in
\Drupal\views\Plugin\views\field\FieldPluginBase::tokenizeValue
is:$value = strip_tags($this->renderAltered($fake_item, $tokens));
pretty sure we don't have to worry about any HTML tags after that :)
Comment #26
joelpittetI'd RTBC this if it wasn't so close to my original patch. Need someone from the views subsystem to review this I guess. Thank you @Lendude for improving the tests!
Comment #27
dawehnerI like the test coverage applied here.
Comment #28
alexpottCommitted and pushed 7c43cce to 8.3.x and c30e795 to 8.2.x. Thanks!
I've checked the output of
$this->renderAltered()
and I can't see anyway that the output is not run through XSS filtering. Since the input text is coming from the admin interface I think this is an appropriate tactic for making safe and 'administer views' is restricted access. I've updated the comment to reflect this.Fixed on commit.