Refactor use of SafeMarkup in HWLDFWordAccumulator

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.

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.

the other issue has a test too

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.

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?

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

Retitling since the other bug was resolved.

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.

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

woops!

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

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

Thank you both!

Thank you both!

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!

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.

Also posted a small followup: #2509448: Further refactor use of SafeMarkup in HWLDFWordAccumulator.