Problem/Motivation

When you display a config diff on admin/config/development/configuration/sync/diff/ , or make a page in a module that uses the \Drupal\Core\Diff\DiffFormatter class to display a diff, you will see a lot of double-escaped   entities displayed on the page. (Note: this was originally #2314513: Add test coverage for DiffFormatter not double escaping   but that has been marked as a duplicate of this issue.)

DiffFormatter also is generating the double-escaped nbsp characters in two methods:

  protected function contextLine($line) {
    return array(
      ' ',
      array(
        'data' => $line,
        'class' => 'diff-context',
      ),
    );
  }

  protected function emptyLine() {
    return array(
      ' ',
      ' ',
    );
  }

And for some reason they are being check_plained.

There is another problem with the SafeMarkup from DIffFormatter as well (which was the original intent of this issue): DiffFormatter uses the \Drupal\Component\Diff\WordLevelFormatter class, which in turn uses \Drupal\Component\Diff\Engine\HWLDFWordAccumulator. This class has a line in its HWLDFWordAccumulator::_flushLine() method that says:

    array_push($this->lines, SafeMarkup::set($this->line));

It is unlikely that this is the correct place to do SafeMarkup. Probably any SafeMarkup calls should be higher-level.

Proposed resolution

TBD

Remaining tasks

Code fix and tests.

There is a proposed test for this in comment #5, and an issue marked as duplicate also has a test specifically for the config diff display, see patch on #2314513: Add test coverage for DiffFormatter not double escaping  .

User interface changes

Config diffs and other diffs using DiffFormatter will not have double-escaped entities in them.

API changes

TBD, probably not.

Files: 
CommentFileSizeAuthor
#19 Screen Shot 2015-06-07 at 12.16.23 PM.png375.11 KBpeezy
#17 document_use_of-2280963-14_0.patch1.43 KBcwells
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,264 pass(es). View
#15 document_use_of-2280963-14.patch1.43 KBcwells
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php. View
#13 safemarkup-in-hwldfwordaccumulator-2280963-13.patch916 bytesmlncn
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,709 pass(es). View

Comments

crowdcg’s picture

Status: Active » Postponed
xjm’s picture

Status: Postponed » Active
jibran’s picture

Issue tags: +SafeMarkup
jhodgdon’s picture

Note that #2314513: Add test coverage for DiffFormatter not double escaping   is set as a duplicate of this issue.

So I'm assuming that this issue is about the   entries that are showing if you use the \Drupal\Core\Diff\DiffFormatter class.

These are coming from two places:

(1)

  protected function contextLine($line) {
    return array(
      ' ',

(2)

  protected function emptyLine() {
    return array(
      ' ',
      ' ',
    );
  }

The only use of this class in Core is I think in the Config system to make diffs. I am also using it for this purpose in my contrib/sandbox module https://www.drupal.org/sandbox/jhodgdon/2391835

If someone wants to make a test for this problem, while avoiding Config, here are some ideas:

a) Make two arrays of lines of text, $a and $b. For instance you could do:

$a = array(
  'hi',
  'hello',
  'hum',
);

$b = array(
   'hi',
   'greetings',
   'more greetings',
   'hum',
);

b) Call

use Drupal\Component\Diff\Diff;

   $diff = new Diff($a, $b);

to compute the differences between the arrays.

c) To make this into a table of differences for display:

   $build['#attached']['library'][] = 'system/diff';

    $build['diff'] = array(
      '#type' => 'table',
      '#header' => array(
        array('data' => t('Lines A'), 'colspan' => '2'),
        array('data' => t('Lines B'), 'colspan' => '2'),
      ),
      '#rows' => $this->diffFormatter->format($diff),
    );

where diffFormatter comes from:

      $container->get('diff.formatter')

in the create() function.

d) Display this table on a page, or run it through drupal_render() or whatever it's called these days.

Hopefully that will be enough for someone to go on if they want to make a test for this.

jhodgdon’s picture

And I apologize if #5 is not what this issue is about. chx marked that other issue as a duplicate so I assumed so. If not we should unmark the other issue as not a duplicate and go from there and fix that issue.

larowlan’s picture

the other issue has a test too

jhodgdon’s picture

Title: DiffEngine and SafeMarkup » DiffFormatter displays double-escaped   characters; SafeMarkup should not be used in HWLDFWordAccumulator
Issue summary: View changes

Yes. The other issue has a test that verifies that admin/config/development/configuration/sync/diff/ does not have   showing up on it. My suggestion in #5 is to make a more generic test that specifically tests DiffFormatter and does not depend on Config Manager module.

So... This title and summary were not exactly illuminating to me. So I did some research, and updated the issue summary and title.

I am still not sure that #2314513: Add test coverage for DiffFormatter not double escaping   is a duplicate of this issue. This one was filed about the use of SafeMarkup buried deep within the HWLDFWordAccumulator class. Which is likely a problem, but I don't think it is the cause of the double-escaped nbsp entities we are seeing in config diffs.

Updating summary and title. Hopefully it will be clearer.

jhodgdon’s picture

My sandbox Config Revert module, which uses the DiffFormatter class, is no longer showing the nbsp characters double-escaped. So, the duplicate issue may have vanished. I haven't looked at the code to see what changed, and I'm not sure that Core is even using this class any more?

andypost’s picture

First half of problem was fixed in #2318081: remove ' ' from Drupal/Core/Diff/DiffFormatter
So now it's harder to test what is autoescaped

xjm’s picture

Title: DiffFormatter displays double-escaped   characters; SafeMarkup should not be used in HWLDFWordAccumulator » SafeMarkup should not be used in HWLDFWordAccumulator

Retitling since the other bug was resolved.

mlncn’s picture

Title: SafeMarkup should not be used in HWLDFWordAccumulator » Document use of SafeMarkup in HWLDFWordAccumulator
Assigned: Unassigned » mlncn
Status: Active » Needs review
FileSize
916 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,709 pass(es). View

As predicted and then noted by jhodgdon, and confirmed by andypost, this use of SafeMarkup was not the cause of the escaping issues. Further, it is a correct use of SafeMarkup (in that what it is marking is undeniably safe) and turning the incremental creation of a diff line into a template or even an inline template would force these onto WordLevelDiff and upstream.

joelpittet’s picture

Title: Document use of SafeMarkup in HWLDFWordAccumulator » Refactor use of SafeMarkup in HWLDFWordAccumulator
Status: Needs review » Needs work
+++ b/core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php
@@ -46,8 +46,9 @@ protected function _flushGroup($new_tag) {
-      // @todo This is probably not the right place to do this. To be
-      //   addressed in https://www.drupal.org/node/2280963.
+      // This is safe because lines are built only in _flushGroup above, where
+      // input is run through Drupal\Component\Utility\SafeMarkup::checkPlain,
+      // or out of an empty string or a non-breaking space.

I think we can rework the lines above to have them safe when they get here.

cwells’s picture

Assigned: mlncn » cwells
Status: Needs work » Needs review
Issue tags: +Twig, +D8 Accelerate
FileSize
1.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Invalid PHP syntax in core/lib/Drupal/Component/Diff/Engine/HWLDFWordAccumulator.php. View

Status: Needs review » Needs work

The last submitted patch, 15: document_use_of-2280963-14.patch, failed testing.

cwells’s picture

Status: Needs work » Needs review
FileSize
1.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 96,264 pass(es). View

woops!

joelpittet’s picture

Assigned: cwells » Unassigned
Issue tags: +Needs manual testing

Thanks this looks great. Needs some manual testing but otherwise it should be good.

peezy’s picture

The patch has passed manual testing. In the screenshot below, the upper image shows CMI's diff markup before applying the path; the lower image shows the identical markup after applying the patch.
Manual Test Diff

Manual testing steps

Do these steps both with HEAD and with the patch applied:

  1. Clean install of Drupal 8.
  2. Make note of your website's Site Name at /admin/config/system/site-informatio n
  3. Run drush config-export (cex)
  4. Manually change the Site Name in /sites/youre_site/files/config_some_hash/staging/system.site.yml.
  5. Visit /admin/config/development/configuration and click the button labelled "View Differences."
  6. The table inside the dialog box that appears after clicking the button will contain classes such as "diff-context."
  7. Compare the output above in HEAD and with the patch applied. Confirm that there is no double-escaping.
  8. If there is any user or calling code input in the string, submit
    alert('XSS');

    and ensure that it is sanitized.

joelpittet’s picture

Status: Needs review » Reviewed & tested by the community

Thank you both!

joelpittet’s picture

Thank you both!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

As sad as it is to have a dependency on SafeMarkup in the Diff component I can't see a way around it. Especially considering we're potentially mixing markup with things that need to be escaped at this point. I tested this by exporting configuration and change the list of allowed tags in filter.format.basic_html.yml. It works nicely.

This is a non disruptive change that removes a call to SafeMarkup::set() - therefore permitted during beta. Committed 53065cb and pushed to 8.0.x. Thanks!

  • alexpott committed 53065cb on 8.0.x
    Issue #2280963 by cwells, mlncn, peezy, joelpittet: Refactor use of...
xjm’s picture

The summary indicates that #2314513: Add test coverage for DiffFormatter not double escaping   had a test for this, but the test never got added to this patch. I've reopened #2314513: Add test coverage for DiffFormatter not double escaping   to look into adding it.

xjm’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.