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.
Summary:
In Views, in Administrative titles of fields/filters/sort's etc.: Some Characters, namely ampersand, single- and double-quotes get double encoded, so you end up with "bug's" & more
instead of "bug's & more":
Screenshot
Steps to reproduce:
- Install Views 7.x
- go to a view, eg. /admin/structure/views/view/content
- edit a field's (or filter-/sort-criteria's) administrative title to something with special chars, eg
bug's
- see the result
bug's
(source isbug's
Comment | File | Size | Author |
---|---|---|---|
#48 | ViewsDisplay-not-equal-operator-double-encoded.png | 30.25 KB | ao2 |
#40 | 2200731-40.patch | 2.54 KB | maijs |
#35 | views-edit-form-double-enc.png | 30.45 KB | maijs |
#35 | 2200731-35.patch | 1.03 KB | maijs |
#30 | after.png | 13.76 KB | maris.abols |
Comments
Comment #1
sirtetAlso applies to Views 7.x 3.7
Comment #2
sidharthapThanks @Sirtet for reporting this. Please update the steps to replicate the issue.
Comment #3
sirtetComment #4
Anonymous (not verified) CreditAttribution: Anonymous commentedI notice other special characters are handled properly, but the apostrophe isn't. Could this be an edge case?
Comment #5
maijs CreditAttribution: maijs at Wunder commentedThe way I see it is that field names are sanitized with
SafeMarkup::checkPlain()
in:HandlerBase::adminLabel()
ViewEditForm::getFormBucket()
and field values are sanitized with
SafeMarkup::checkPlain()
in:FieldPluginBase::adminSummary()
,SortPluginBase::adminSummary()
, etc.ViewEditForm::getFormBucket()
hence the double escaping.
Proposed solution
My suggestion is to let
adminLabel()
andadminSummary()
do their sanitizing and sanitize the field names and field values withXss::filterAdmin
inViewEditForm::getFormBucket()
. After field name and field values are sanitized, link text should be considered safe and therefore added to the list of safe strings usingSafeMarkup::set()
so that link generator does not sanitize the link text again. This way the output is sanely sanitized (without double escaping) even if label and summary methods fail to sanitize the strings at all.I also added a filter to the view provided by
views_ui_test
module in order to have an admin label which indicates a comparison operator "<
" or ">
" which also shouldn't be double escaped.Comment #6
maijs CreditAttribution: maijs commentedChanging the issue component to
views_ui.module
.Comment #7
maijs CreditAttribution: maijs commentedRelated issue added. The patch in #5 includes the fix that is provided in #2473907: Tests not being run by testbot due to missing summary line to have test coverage for the proposed changes.
Comment #8
LewisNyman CreditAttribution: LewisNyman at Wunder commentedThis issue needs manual testing and screenshots, this is a good novice task.
Comment #9
Sam MooreManually tested the patch in #5 against D8 dev.
Edited the No Results Behavior field's administrative name on admin/structure/views/view/content to use a few special characters, including a right single quote.
Looks like it works - see attached screenshots.
Novice contributor here - LMK if I should change the issue metadata.
Comment #10
Manjit.Singhmanually test #5 , I have changed the admin title of some fields, the changes looks good to me.. :) Please check screenshot as well.
Comment #11
ijf8090 CreditAttribution: ijf8090 commentedTested with Simply test/Firefox, confirming apostrophes are now correctly escaped. See attached image.
Think issue can be closed
Comment #12
LewisNyman CreditAttribution: LewisNyman at Wunder commentedComment #13
YesCT CreditAttribution: YesCT commentedComment #14
YesCT CreditAttribution: YesCT commentedThanks @Sam Moore @Manjit.Singh @ijf8090 for doing those screenshots.
So that more people see your hard work, next time please embed screenshots either by updating the issue summary, or embedding them in your comment.
I think this still needs a code review though.
Comment #16
jaxxed CreditAttribution: jaxxed at Wunder commentederror: patch failed: core/modules/views_ui/src/Tests/XssTest.php:8
error: patch failed: core/modules/views_ui/src/ViewEditForm.php:1072
Comment #17
Manjit.SinghComment #18
Manjit.Singhrerolled a patch.
Comment #19
dawehnerThis is certainly not the right approach. We don't want to have SafeMarkup::set() there.
Comment #22
mikgreen CreditAttribution: mikgreen commentedThe actual problem as it's reported is fixed elsewhere.
There is a similar problem in modal windows. I'm researching that right now. I think I'll use this issue for that.
https://www.evernote.com/l/AELpgYDAtslNGojeqJQVilCAEjOcggZIQ90B/image.png
Comment #23
mikgreen CreditAttribution: mikgreen at Wunder commentedComment #24
dawehnerI remember that this is maybe actually escaped in javascript.
Comment #25
mikgreen CreditAttribution: mikgreen at Wunder commentedThis is fixed by #2560641.
https://www.drupal.org/node/2560641
Instead of SafeMarkup::checkPlain() now it uses $handler->adminLabel(TRUE);
So I will close this issue.
Comment #26
sirtetAs i understand, the fix (D8 core) is something that can't be backported (to views)7.x, but needs a different approach.
I can't move the issue to views, also not sure if a new issue should be created, or if this should be moved over.
Comment #27
dawehnerEscaping is done in JS, so it should stay there, but alex might have a total different opinion.
Comment #28
maris.abols CreditAttribution: maris.abols commentedAs last patch fails and problem still occurs, I will try to find out solution.
Comment #29
maris.abols CreditAttribution: maris.abols commentedComment #30
maris.abols CreditAttribution: maris.abols commentedI created small change for #18 and created patch that fixes wrong char in views field list. Inspired from this patch: https://www.drupal.org/node/2568149#comment-10389499
Comment #31
maris.abols CreditAttribution: maris.abols commentedComment #33
sirtetSo, if this is now about views 7.x, should the issue not be moved to views?
Comment #35
maijs CreditAttribution: maijs at Wunder commentedThis issue is related to 8.x and I believe for outstanding issue for 7.x a separate issue needs to be created in Views module project.
@mikgreen correctly observed in #25 that the problem with original issue (admin titles being double encoded in views edit page) has been solved in #2560641: Remove all usages SafeMarkup::checkPlain() from render arrays. On the other hand the admin label in field (filter, sort, etc.) edit window (opened with JS) is still double encoded.
Fortunately, @dawehner's patch in #27 sort of fixes the problem.
I'd rather utilize built-in
\Drupal\Component\Utility\Html::decodeEntities()
utility method for decoding. Updated patch is provided.Comment #38
maijs CreditAttribution: maijs at Wunder commentedLatest patch passed test after re-queueing.
Comment #39
dawehnerIt fixes the problem but its sadly not possible at all to test that :(
Comment #40
maijs CreditAttribution: maijs at Wunder commentedWebTestBase::drupalGetAjax()
can test that admin label is not double encoded in field edit modal window. Also, there'ssa_contrib_2013_035
view inviews_ui_test
module that has fields with special characters in their admin label settings. Why not test against those?Comment #43
maijs CreditAttribution: maijs at Wunder commentedAdding related issue #2568149: Field title is rendering special characters as html entites in Views field edit modal frame which specifically addresses the issue with admin label encoding in modal window.
Comment #44
mikgreen CreditAttribution: mikgreen at Wunder commentedUnfortunatelly the original issue is back. I'm dereferencing this issue from the other issue - which is about modal titles.
This issues is about field titles in fields UI.
- In Fields (but not NOT in Add fields)
- In Filter Criteria (and in Add filter criteria)
- Sort Criteria (and in Add sort criteria)
I don't know what's causing it now.
Comment #45
mikgreen CreditAttribution: mikgreen at Wunder commentedI got broken again when this was commited:
#2506479: Replace !placeholder with @placeholder for non URLs in t() in Views, except for t() output that is used as an attribute value
Comment #48
ao2 CreditAttribution: ao2 as a volunteer commentedHi, I still get this issue in drupal 8.2.5, in particular if I choose the "Is not one of" operator in the filter criterion.
Look at the bottom part of the attached screenshot:
The patch from comment #40 does not apply cleanly so I haven't tested it yet.
Thanks,
Antonio
Comment #49
BerdirThis issue was specifically about titles in modals, which was fixed by #2207247: Dialog titles double escaped for views handlers and delete forms. I'm closing this as a duplicate of that issue now. I suggest you create a new issue for the bug you found.