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

double encoded admin-title

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 is bug's
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sirtet’s picture

Title: Field's administrative titles are double encoded » Views Field's administrative titles are double encoded

Also applies to Views 7.x 3.7

sidharthap’s picture

Thanks @Sirtet for reporting this. Please update the steps to replicate the issue.

sirtet’s picture

Issue summary: View changes
Anonymous’s picture

Issue summary: View changes
FileSize
30.16 KB

I notice other special characters are handled properly, but the apostrophe isn't. Could this be an edge case?

maijs’s picture

Version: 8.0-alpha9 » 8.0.x-dev
Status: Active » Needs review
FileSize
4.11 KB
76.49 KB

The 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() and adminSummary() do their sanitizing and sanitize the field names and field values with Xss::filterAdmin in ViewEditForm::getFormBucket(). After field name and field values are sanitized, link text should be considered safe and therefore added to the list of safe strings using SafeMarkup::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.

maijs’s picture

Component: views.module » views_ui.module

Changing the issue component to views_ui.module.

maijs’s picture

Related 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.

LewisNyman’s picture

This issue needs manual testing and screenshots, this is a good novice task.

Sam Moore’s picture

Manually 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.

Manjit.Singh’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
78.17 KB

manually test #5 , I have changed the admin title of some fields, the changes looks good to me.. :) Please check screenshot as well.

ijf8090’s picture

Issue summary: View changes
FileSize
40.39 KB

Tested with Simply test/Firefox, confirming apostrophes are now correctly escaped. See attached image.

Think issue can be closed

LewisNyman’s picture

YesCT’s picture

Issue summary: View changes
YesCT’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs review

Thanks @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.

jaxxed’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

error: patch failed: core/modules/views_ui/src/Tests/XssTest.php:8
error: patch failed: core/modules/views_ui/src/ViewEditForm.php:1072

Manjit.Singh’s picture

Assigned: Unassigned » Manjit.Singh
Manjit.Singh’s picture

Assigned: Manjit.Singh » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.86 KB

rerolled a patch.

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/views_ui/src/ViewEditForm.php
@@ -1059,13 +1059,13 @@ public function getFormBucket(ViewUI $view, $type, $display) {
       $description = Xss::filterAdmin($handler->adminSummary());
-      $link_text = $field_name . (empty($description) ? '' : " ($description)");
+      $link_text = SafeMarkup::set($field_name . (empty($description) ? '' : " ($description)"));
       $link_attributes = array('class' => array('views-ajax-link'));
       if (!empty($field['exclude'])) {
         $link_attributes['class'][] = 'views-field-excluded';

This is certainly not the right approach. We don't want to have SafeMarkup::set() there.

The last submitted patch, 18: views-double_escaping-2200731-18.patch, failed testing.

The last submitted patch, 18: views-double_escaping-2200731-18.patch, failed testing.

mikgreen’s picture

Assigned: Unassigned » mikgreen

The 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

mikgreen’s picture

Status: Needs work » Active
dawehner’s picture

I remember that this is maybe actually escaped in javascript.

mikgreen’s picture

Status: Active » Closed (cannot reproduce)

This 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.

sirtet’s picture

Issue summary: View changes
Status: Closed (cannot reproduce) » Active
Issue tags: -Novice
FileSize
39.17 KB

As 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.

dawehner’s picture

Status: Active » Needs review
FileSize
821 bytes

Escaping is done in JS, so it should stay there, but alex might have a total different opinion.

maris.abols’s picture

Assigned: mikgreen » maris.abols

As last patch fails and problem still occurs, I will try to find out solution.

maris.abols’s picture

Status: Needs review » Needs work
maris.abols’s picture

I 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

maris.abols’s picture

Assigned: maris.abols » Unassigned
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 30: views-double_escaping-2200731-30.patch, failed testing.

sirtet’s picture

So, if this is now about views 7.x, should the issue not be moved to views?

The last submitted patch, 30: views-double_escaping-2200731-30.patch, failed testing.

maijs’s picture

Status: Needs work » Needs review
FileSize
1.03 KB
30.45 KB

This 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.

Administrative title double encoded

Fortunately, @dawehner's patch in #27 sort of fixes the problem.

if (isset($this->dialogOptions['title'])) {
  // Javascript dialogs will handle the escaping for us.
  $this->dialogOptions['title'] = html_entity_decode($this->dialogOptions['title']);
}

I'd rather utilize built-in \Drupal\Component\Utility\Html::decodeEntities() utility method for decoding. Updated patch is provided.

Status: Needs review » Needs work

The last submitted patch, 35: 2200731-35.patch, failed testing.

maijs queued 35: 2200731-35.patch for re-testing.

maijs’s picture

Status: Needs work » Needs review

Latest patch passed test after re-queueing.

dawehner’s picture

It fixes the problem but its sadly not possible at all to test that :(

maijs’s picture

WebTestBase::drupalGetAjax() can test that admin label is not double encoded in field edit modal window. Also, there's sa_contrib_2013_035 view in views_ui_test module that has fields with special characters in their admin label settings. Why not test against those?

/**
   * Tests that the administration title of a field displayed in field edit
   * modal window is not double encoded.
   */
  public function testFieldEditModalAdminTitle() {
    $this->drupalGet('admin/structure/views/view/sa_contrib_2013_035');
    $this->drupalGetAjax('admin/structure/views/ajax/handler/sa_contrib_2013_035/page_1/field/title_1');
    $admin_title = Json::encode('Configure field: <script>alert("XSS")</script>');
    $this->assertRaw($admin_title, 'The administrative title of a field in modal window is correct.');
  }

Status: Needs review » Needs work

The last submitted patch, 40: 2200731-40.patch, failed testing.

Status: Needs work » Needs review

maijs queued 40: 2200731-40.patch for re-testing.

maijs’s picture

mikgreen’s picture

Unfortunatelly 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.

mikgreen’s picture

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ao2’s picture

Hi, 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:
 "Is not one of" operator in filter criteria is double-encoded

The patch from comment #40 does not apply cleanly so I haven't tested it yet.

Thanks,
Antonio

Berdir’s picture

Status: Active » Closed (duplicate)

This 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.