Attribute/drupal_attributes() docs do not mention protocol filtering on URLs

DrupalCon Extended Sprints.

In #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols we also mention on* and style attributes - could be worth repeating here

drupal_attributes() has been removed in D8, so the attached patch only adds documentation for the Attribute class.

1. s/an URI/a URI/
2. Should we also rewrite the immediately preceding paragraph in case someone reads that without reading the addition in this patch, I feel like it still hints at safeness a bit too much

Thanks for the feedback, I merged both paragraphs into one (since they both concern security), and replaced "sanitize" with "escape" in the first part (since sanitization implies completely safe, which is not the case for URIs that require protocol filtering).

Previous patch file was incomplete.

Looks great :)

"the entire attribute string is marked safe for output" is not actually true. The object itself implements SafeStringInterface, so it will not be escaped by Twig.

No
+ * protocol filtering is applied, so when using user-entered input as a value
+ * for a URI attribute (href, src, ...), UrlHelper::stripDangerousProtocols()
+ * should be used to ensure dangerous protocols (such as 'javascript:') are
+ * removed.

As in the other issue, I'm still a little iffy on having the advice in the documentation contradict what Drupal actually does itself in Xss::attributes() (apply protocol-filtering to all attributes, except "title", "alt", and "data-*").

On the other hand, we know that's more conservative than it needs to be and in practice telling people to manually run UrlHelper::stripDangerousProtocols() on almost every attribute is overkill, so I guess it's OK but is there a way we can be more specific here about how you know when you really really need to run it? Like a reference somewhere to what we mean by "URI attribute"? (Not necessarily a blocker for this issue though - documenting it as above is already leaps and bounds better than not documenting it at all.)

Slightly reworded after talking to @pwolanin.

@David_Rothstein: I changed it to read "No protocol filtering is applied, so when using user-entered input as a value for an attribute that expects an URI (href, src, ...)," ... which might be clearer? I don't think it's feasable to give an exhaustive list here like the on at http://stackoverflow.com/questions/2725156/complete-list-of-html-tag-att...

Like a reference somewhere to what we mean by "URI attribute"

As far as HTML5 is concerned, likely every attribute listed in http://www.w3.org/html/wg/drafts/html/master/index.html#attributes-1 that has the word URL in it. But I'd be a bit wary of explicitly mentioning them all here, we only really know that href works. In another issue it could be interesting to do the protocol filtering in Attribute itself, based on this list of attributes...

What I don't see is any attributes that might be a URL, or might not be a URL, it's usually pretty clear. So marking RTBC, David_Rothstein if you object feel free to set back to NR.

The final version looks good to me too (though it should say "a URI" rather than "an URI").

Maybe in the backport we should add similar documentation to format_string() here too, since the new placeholder added in #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols may not make sense to backport (it could be done, but we couldn't actually use it consistently in core without breaking lots of translations).

Patch ported to D7 (not sure if I'm correct).

This looks pretty good to me... Part of it is a bit confusing though.

The first sentence in this paragraph starts with "Attribute values...". So far, so good. Then the next one starts in about the attribute names. Then the third sentence, about "No protocol filtering" is about the values again. Then the fourth is about the names again.

Can we rearrange this so that the parts about the values are together, and the parts about the names are together? This would make much more sense. Thanks!