Add protocol filtering to the core Attribute component

This sounds like a great idea. We already do Html::escape() on all values and attribute names. Though this may not be the best stategy. Twig actually has a strategy for attributes:
http://twig.sensiolabs.org/doc/filters/escape.html html_attr. I would suggest we use that to it's advantage as well as tag it on to AttributeValueBase. The name part can be left Html::escape() i'd assume.

If we want to strip dangerous protocols, it may be good to use Twig's html_attr strategy in place of Html::escape() inside UrlHelper::filterBadProtocol() at the very least I'd suspect, as well.

Xss::filter() needs the context of the tag I believe to do it's job effectively. Though we have context in Attribute to know the name of the attribute and effectively be smart about which attributes need something like.
UrlHelper::stripDangerousProtocols() eg: href,src,action,etc. And which ones don't eg. title

Hope that helps.

Oh and we should probably let twig apply the 'html_attr' strategy and not do it in Attribute as a step here. So the only domain thing this will deal with the dangerous protocol domain.

Just have to make our escapeFilter a bit smarter about SafeStringIntrface escaping strategy, which was inevitable.

@joelpittet - doing this all in twig would be a pretty dramatic code change.

Also, I don't think that actually meets the goal of this issue - it seems html_attr is just a much broader conversion of characters to entities:
http://stackoverflow.com/questions/12038048/difference-between-escapehtm...

In contrast, the suggestion here is to do protocol filtering. We can't escape those or they would break href, and src for example.

The tricky part of this issue is that I'm not sure we have the attribute name at the place we need it.

I think it just needs to do this?

Probably need to write some new tests for it - let's see if existing ones fail.

+++ b/core/lib/Drupal/Core/Template/AttributeString.php
@@ -30,7 +31,20 @@ class AttributeString extends AttributeValueBase {
-    return Html::escape($this->value);
...
+      return UrlHelper::filterBadProtocol($this->value);

Per #2565895-21: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols, I really don't think we should use filterBadProtocol() on something whose API is defined as needing to be in plain-text to begin with (which this is, since we currently do an unconditional Html::escape() on it). Instead, I think we should do Html::escape(UrlHelper::stripDangerousProtocols($this->value)); (without the Html::decodeEntities() that filterBadProtocol() does).

Note there's a test in #2567795: Introduce a : placeholder which works like ! for now that includes a URL fragment on which Html::decodeEntities() would be incorrect to do, so perhaps we can add a similar test for that here as well.

@effulgentsia - per catch, I was trying to mimic what Xss::filter does, which is to call UrlHelper::filterBadProtocol()

The @param of Xss::filter() explicitly says that the parameter it receives is: "The string with raw HTML in it." So yes, that then needs to decode HTML entities, because what it was passed is expected to be HTML encoded. The input to Attribute, however, is explicitly plain-text, not HTML. Html::decodeEntities() is a function you call on HTML, not on plain text.

This is, on purpose, a test only patch.

+++ b/core/lib/Drupal/Core/Template/AttributeString.php
@@ -30,7 +31,15 @@ class AttributeString extends AttributeValueBase {
+    if (substr($this->name, 0, 5) === 'data-' || in_array($this->name, ['title', 'alt', 'callback'])) {
+      return Html::escape($this->value);

micro opt, I would swap the two conditions around.

+++ b/core/tests/Drupal/Tests/Core/Template/AttributeTest.php
@@ -448,7 +448,7 @@ public function providerTestAttributesWithUrls() {
-    $data['xss-in-style'] = [['style' => 'list-style-image: url(javascript:alert(0))'], ' style="alert(0)"'];
+    $data['xss-in-style'] = [['style' => 'list-style-image: url(javascript:alert(0))'], ' style="alert(0))"'];

The expected output I don't get. Do we really want two "))" closed?

+++ b/core/lib/Drupal/Core/Template/AttributeString.php
@@ -33,7 +33,7 @@ class AttributeString extends AttributeValueBase {
-    if (substr($this->name, 0, 5) === 'data-' || in_array($this->name, ['title', 'alt', 'value'])) {
+    if (substr($this->name, 0, 5) === 'data-' || in_array($this->name, ['title', 'alt', 'value', 'name'])) {
       return Html::escape($this->value);

Can we expand the test coverage to include an example for alt/value/name, just to be 100% sure?

+++ b/core/lib/Drupal/Core/Template/AttributeString.php
@@ -30,7 +31,14 @@ class AttributeString extends AttributeValueBase {
+    // @see Xss::attributes()
+    if (substr($this->name, 0, 5) === 'data-' || in_array($this->name, ['title', 'alt', 'value', 'name', 'property', 'typeof', 'rel', 'about', 'content', 'datatype', 'datatype_callback'])) {
+      return Html::escape($this->value);

Given how many we already have I'm curious whether this needs to be configurable, much like allowed protocols ...

I would rather see us using an isset() instead of much slower in_array()

What is the correct way for someone to legitimately create attributes for something that are considered dangerous? I would have assumed that using Attribute() was the API for this.

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -706,7 +706,7 @@ protected function createPlaceholder(array $element) {
-    $attributes['callback'] = $placeholder_render_array['#lazy_builder'][0];
+    $attributes['data-callback'] = $placeholder_render_array['#lazy_builder'][0];
     $attributes['arguments'] = UrlHelper::buildQuery($placeholder_render_array['#lazy_builder'][1]);
     $attributes['token'] = hash('crc32b', serialize($placeholder_render_array));
     $placeholder_markup = SafeMarkup::format('<drupal-render-placeholder@attributes></drupal-render-placeholder>', ['@attributes' => $attributes]);

This could (and IMO should) just not use Attribute, and do a SafeString::create(). This is internal in the Renderer anyway.

We should do that, because this is what the preceding comment says:

    // Generate placeholder markup. Note that the only requirement is that this
    // is unique markup that isn't easily guessable. The #lazy_builder callback
    // and its arguments are put in the placeholder markup solely to simplify
    // debugging.

Prefixing it with data- makes it less developer-friendly.

@Wim Leers
Do you mind doing that conversion in its own issue?

I'd like to add a few test assertions to this if it's ok with you all?

1. I'd like to close this and move the tests here #2531824: Attribute class to check safe strings before escaping (has tests)
2. And make two more tests from this comment #2545056-11: Attribute class does not sanitize for "javascript:alert('XSS')" and close that issue too as a duplicate of this

Ça joue?

Adding the mailto: and escape bypassing examples from #2545056-11: Attribute class does not sanitize for "javascript:alert('XSS')"

Wrong assertion it should just strip the protocol not the value.

1. +++ b/core/lib/Drupal/Core/Template/AttributeString.php
   @@ -30,7 +32,17 @@ class AttributeString extends AttributeValueBase {
   +    // Whitelist 'title', 'alt', and all data- attributes.
   

   Needs some doc updates

2. +++ b/core/lib/Drupal/Core/Template/AttributeString.php
   @@ -30,7 +32,17 @@ class AttributeString extends AttributeValueBase {
   +    else if (substr($this->name, 0, 5) === 'data-' || in_array($this->name, ['title', 'alt', 'value', 'name', 'property', 'typeof', 'rel', 'about', 'content', 'datatype', 'datatype_callback', 'datetime', 'mailto', 'media', 'sizes'])) {
   

   Wait, so does that actually mean that Xss::attributes() is wrong at the moment?
   Ah yeah, see #2544110: XSS attribute filtering is inconsistent and strips valid attributes

#63-2, one early idea on #2544110: XSS attribute filtering is inconsistent and strips valid attributes was to add setter/getters for its internal lists. If we do that, then Attributes could get the whitelist that way.

#52 lost the changes I made in #41.

But @dawehner asked in #42 to do that in a separate issue. So here we go: #2569371: Update Renderer::createPlaceholder() to not use Attribute and SafeMarkup::format(). That's an easy patch to get in.

This needs a reroll, but I am not sure which hunk for the callback attribute name is correct here.

#71: just callback, #2569371: Update Renderer::createPlaceholder() to not use Attribute and SafeMarkup::format() removed the need here to prefix it with data-.

Done.

(This is a straight reroll with just the conflict with #2569371: Update Renderer::createPlaceholder() to not use Attribute and SafeMarkup::format() resolved, making this patch slightly simpler.)

This is blocked on #2569485: Add AttributeSafeStringInterface and UriAttributeSafeStringInterface, which was in turn blocked on #2576533: Rename SafeStringInterface to MarkupInterface and move related classes. The latter has landed, so we're still blocked on one issue here.

I agree this is important and would help prevent some XSS vulnerabilities.
I rebased the latest patch and adjusted some things to make tests pass.

However, I see two potential problems with the solution:

• It would require maintaining a long list of attributes in which protocols should not be filtered (see #2544110: XSS attribute filtering is inconsistent and strips valid attributes).
  This is an existing problem with the XSS filter, but filtering attributes everywhere makes it more visible.
• Using MarkupInterface to explicitly bypass the filter seems a bit weird semantically, because an attribute value is not really markup.
  (When this was proposed the interface was still called SafeStringInterface.)

Also filtering the style attribute would break too many places in core where it is used (because every CSS rule contains the : character) and I don't think it is the job stripDangerousProtocols() to remove dangerous URLs in CSS.
Removing dangerous CSS could be implemented later in #3264121: Allow inline style to certain html elements despite of "Limit allowed HTML tags and correct faulty HTML" filter turned on .

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I rebased the MR.

It would be so nice if the tests didn’t change. Are the test changes really necessary? Adding more expected assertions is fine, but changing the provider values reduces confidence in the change.

Thanks for picking this up, it's been sitting for a while.

The tests have to change (well at least the expected values) because attribute values that were not sanitized before now have their protocol removed.
However, I adjusted the MR a bit to limit the changes in tests.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I merged the latest 11.x.

Retitling because I keep forgetting what this is.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I merged the latest 11.x.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I merged the latest 11.x.

So this seems to be toughing a number of different modules and components. Should this be broken up?

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I merged the latest 11.x.

So this seems to be toughing a number of different modules and components. Should this be broken up?

Most of the changes are because these parts of the code add an attribute with a dangerous protocol and don't expect it to be filtered, so now they need to wrap the attribute in a Markup object to explicitly prevent it from being filtered.
I guess theses changes could be committed separately but they won't do anything or make a lot of sense on their own.

Ok, may take a while to get to then just an fyi

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I merged the changes from #3537801: Fatal error when using a Twig defined variable as Attribute value.

I left a bunch of comments on the MR. The issue summary needs to be updated too because the proposed resolution only refers to the title, alt, and data-* attributes. The MR excludes a lot more than that from protocol filtering. Please be certain to mention the other issue from which the current list of excluded attributes was taken.

I applied the suggestions.

Unfortunately, the new function short description doesn't comply with the documentation standards. I left a suggestion for it, along with a couple of other things I noticed while reading through the MR again. I'm sorry I didn't notice those before.

I applied the suggestions.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I merged the latest main.