[meta] !placeholder causes strings to be escaped and makes the sanitization API harder to understand

I'm +1 on this issue. But one thing just itches at me that may be an missunderstanding on my part. I think one of the use-cases that @effulgentsia mentioned that may still be valid is strings that are not going to HTML.

Examples: non-HTML emails, JSON, CLI.

There may be a better approach for these all but I'd like to know more about the indented use of ! for those cases.

If we can avoid putting markup in strings in the first place and not early rendering in the render system we may have a chance at providing CLI or email specific rendered output. #dreamsbig

Deprecate !, possibly by making it just a copy of @.

I started on #2509218: Ensure that SafeString objects can be used in non-HTML contexts.

For emails I'd almost prefer to provide a completely separate API that has nothing to do with SafeMarkup.

I agree, but t() is still our API for translation, whether we're translating for HTML or plain text. But I'll post an idea on how to handle that in that issue.

Note: #2409477: Various ! placeholders throughout core are now showing escaped HTML (especially <a href="!..."> examples) exists. I sense some overlap.

Closed on behalf of #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests

I am sorry later I found that this issue is parent of #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests. I am reopening this issue.

Status update on #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests:
Based on advice from from @xjm and conversations with @joelpittet, the large patch that was created there has been broken into smaller pieces. Comment #93 has the suggested approach, and is how we are starting.
Children issues addressing those listed modules have been created, and they appear as children of this issue. Not on that list, #2551743: Replace !placeholder references in Config with @placeholder is being re-purposed since ConfigImporter & tests is a definable chunk.
We should aim to manually test each one of the new patches, which is something originally missing from the conglomerated patch. Also, because the patches are largely coming from the bigger patch, we need to make sure each patch stays in scope for the individual issue.
At the suggestion of @joelpittet, #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests should only deal with leftovers that are too tedious to chunk out individually, and thus has been left open. The patch there should be considerably smaller than the original.

So before we start committing all of these I think we really have to consider whether it is worth it... see #2560783-16: Replace !placeholder with :placeholder for URLs in hook_help() implementations and the fact that #2558791: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness will resolve many of the issues with !

I'd prefer leave hook_help() out of scope because re-translate help strings is serious pita

@andypost It's find and replace ! with @. And this whole issue will do that. Where does this happen, maybe it can be automated?

After talking to @alexpott I think this issue and its children are postponed on #2558791: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness.

Where ! is being used to prevent double escaping (even though it won't) we need to be really careful and add tests - see #2564321: file_save_htaccess() generates error logs which are escaped incorrectly

Created #2564353: Remove !placeholder usage from SafeMarkup::format() to handle some fixes that we should do regardless of #2558791: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness and that removes all ! from SafeMarkup::format() in the code base.

though, as mentioned above, it's hard to have a hard BC break for magically named array key patterns in an optional function parameter

Couldn't we have that hard BC break through an extra check inside the placeholder processing code with an assert/trigger_error/exception in case any !placeholders are still used? Any code analysis would then just have to flag all usages of array keys starting with "!"

Re proposal 1) in the Issue Summary

Revert the change that skips marking strings containing !placeholder as safe

Is still my preferred solution. I appreciate the intent behind the change had great intentions of ensuring safety everywhere in SafeMarkup::format() but as mentioned in the "Pros", it forces people to look for creative solutions to put raw data into the template.

In contrib we've seen SafeMarkup::set() used when the output had hard-coded assets(devel module) came from a third party tool (IIRC kint). And the same or another contrib module while it was being deprecated used SafeMarkup::format() to accomplish the same thing and even when we forced the second parameter to be an array tried committing SafeMarkup::format('haha still safe sucker', []). Personally I'd be using SafeString::create() even though it's internal or make my own and extend SafeStringInterface.

Also to add to my appreciation for the issue that removed !placeholder safeness, it has given us a good hard look(though laborious) at how we are using these placeholders, found bugs in core and re-evaluate why we are using !placeholder when we don't need to.

What this would mean, should we go this route, is that contrib will likely abuse !placeholder some more and the security team will have the same work they did in D7 for that, but I don't think us "slowing people down" will prevent that sec task as people will find workarounds which will still be on the sec team plate. IMO

re #27 I still have hopes for this issue too. (look at the green children!)
#2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list

@effulgentsia also had an earlier issue for a different proposal

I think you mean #2509218-4: Ensure that SafeString objects can be used in non-HTML contexts. The patch in that comment makes ! behave exactly like @:

• If $options['html'] isn't FALSE, then both would checkPlain() if not already safe and the result would be marked safe.
• If $options['html'] is FALSE, then neither would perform any escaping and the result would not be marked safe.

At which point, ! could be marked deprecated, since it would just be an artifact from the days when auto-escaping didn't exist so we needed callers to clarify when to escape and when not to. I still believe that to be a better option than this issue's proposed resolution #1 (but not necessarily better than some of the other options).

Looking through some of the examples in #2558791-89: "!"-prefixed tokens should Xss::filterAdmin() but not affect safeness, the only cases I see where that would not give you the desired result is where a !foo value is set to a PHP concatenation of safe variables (i.e., using the . operator). Can someone here point out an example from any of those files (D7 or D8) that contradicts this claim? And if I'm right about that, then do we really want to do the proposed resolution #1 and reopen a very common XSS vector just to reintroduce BC for that concatenation pattern from D7?

Re #27 isn't that what @alexpott proposed in #2509218-8: Ensure that SafeString objects can be used in non-HTML contexts as well?

As to #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list I still think this is possible to implement right after RC or even before if we raise the priority or get some more attention to that issue, it's just blocked on the children and on finding a resolution to this current issue. As soon as these are solved we'll be very close to having something reviewable and green there.

I believe @alexpott's opinion about the feasibility of that issue for 8.0.0 changed as soon as we had a green patch there 11 days ago which was not as complicated as expected (and the required changes for contrib also seem to be small). Also the majority of the effort has already been done in the child issues.

Some thoughts on the options:

• Option 1 I don't think SafeMarkup::format() should become a way to bypass sanitization and filtering. We have twig templates and SafeString objects for that. Note that bypassing is not the same using t() or SafeMarkup::format() for non-HTML output.
• Option 2 I think we should do this as a matter of course. I think HEAD is full of bad examples of !placeholder and potential XSS - doing thins will fix that.
• Option 3 Is a good idea as long as we have a URL placeholder - because we need to provide a way to safely add URLs to string - it is a super common use case.
• Option 4 I think this is a complete no-go for two reasons. XSS filtering is not free - it does 4 preg_replace() before it's even started to chop up the string. XSS filtering the entire string will make changes to things over that the !placeholder.
• Option 5 I think the solution of making t() and SafeMarkup::format() return an object is way better because then we can just add a method that returns the unescaped string. The other massive advantage of doing that is the removal of the entire safe list which will make whether some is santized at the Twig level completely predictable and knowable by debugging or dsm()'ing.
• Option 6 I disagree about this being insidious especially as we could mitigate with an assert to tell the developer that this is now for URLs and doing this would make less work for translators. BUT a new placeholder is fine too and will make it easier to do the work now.
• Option 7 if we don't do 6 we have do this.

Some other random thoughts:

• We have to have a way of safely putting a URL in a t()'d string
• We need to use t() in non-HTML contexts
• SafeMarkup::format() should not be a way of munging HTML together

So given all of that my plan would be to do the following:

1. Add a new placeholder for URL - I suggest ~ because it is identifiable and on all keyboards and as far as I know has no meaning Drupal.
2. Make all URLs use ~placeholder
3. Make t() and SafeMarkup::format return objects
4. Use these objects to get strings for non-HTML
5. Remove !placeholder because then we don't have to decide what to do if it is present. If we don't want to do that we should make it do the same as @placeholder

So I think #31 1-5 sounds like a good plan.

The only modification I'd make is I think we should consider deprecating !placeholder (either for 8.0.x with the intention to remove during RC, or for 9.0.x) rather than completely removing it. It could stay in core with exactly the same behaviour as the new URL placeholder just to give contrib/custom code a bit more time to update. I'm not tied to this idea though, if people really want to do a hard break that's OK with me too. I'd also be fine with changing the meaning of !placeholder to be for URLs as an even softer break. But I can see that completely doing away with it forces us to get to an actually consistent state in core very quickly and a avoids a lot of ah um about whether we actually got the conversions done or not.

JustAChris in #11:

We should aim to manually test each one of the new patches, which is something originally missing from the conglomerated patch.

in any of the 'Replace !placeholder' issues:

Manually test the update and post screen shot after patch, review source for any difference in escaping.

What screenshots?
I suggest only those places where the replacement content can be modified, for example by configuration text or translation. Example code: $this->t('@name field is required.', array('@name' => $elements['#title']))) No screenshots required where the replacement content is a URL or the string is part of a test.

review source
Assume that with "review source", you mean the HTML output.

@Sutharsan: The direction of this plan has changed since comment #11, thus most of the child issues created at that point shouldn't be worked on right now while the new plan is determined. I'm sorry that I failed to marked those children issues as postponed, will do that now.

We have to have a way of safely putting a URL in a t()'d string

Why is this a need? We don't have a way to do so either in Twig or elsewhere in PHP. For example, aggregator-item.html.twig prints out <a href="{{ url }}">{{ title }}</a>, which means we are required to (and do) have a template_preprocess_aggregator_item() that calls $variables['url'] = UrlHelper::stripDangerousProtocols($item->getLink());. Similarly, all code that sets a user-input URL on a $attributes value must also explicitly protocol filter: e.g., template_preprocess_form() calls $element['#attributes']['action'] = UrlHelper::stripDangerousProtocols($element['#action']);. So, why do we feel that t() must implement protocol filtering rather than making it the caller's responsibility? Or in other words, why must t() be more auto-URL-protocol-safe than Twig and $attributes are?

If it truly is a need though, what if we make both @ and ! have the same behavior (per #29), but give them both some additionally smart behavior. I.e., parse t()'s first argument for if a placeholder is being assigned into a URI attribute value (potentially defining that the same way that Xss::attributes() does), and if so, then protocol filter it. Though I would then ask: if we do this, should we also put similar intelligence into Attribute::createAttributeValue() and into Twig compilation?

So, why do we feel that t() must implement protocol filtering rather than making it the caller's responsibility?

So for me, I'd say that we automatically check for external URLs to avoid open redirects now per #2507831: Harden redirect responses to make external URIs opt in (was SA-CORE-2015-002 foward-port).

And we automatically apply escaping via Twig.

So attribute filtering at this point feels like the exception rather than the rule.

Also Xss::filter() does apply protocol filtering to attributes, the specific problem we have with t() and the filterXssAdmin() issue is that the URL is used as an attribute but does not run through attribute filtering even if you use @. It's easy to get wrong.

Or in other words, why must t() be more auto-URL-protocol-safe than Twig and $attributes are?

I think we should consider adding it to https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Template%... and similar too, yes.

If it truly is a need though, what if we make both @ and ! have the same behavior (per #29), but give them both some additionally smart behavior. I.e., parse t()'s first argument for if a placeholder is being assigned into a URI attribute value (potentially defining that the same way that Xss::attributes() does), and if so, then protocol filter it.

Is there a reason to do this vs. just protocol filtering everything?

@xjm that seems to cover it mostly for contrib as well:

https://www.drupal.org/files/issues/d7-contrib-exclamationmark_0.txt
https://www.drupal.org/files/issues/d8-contrib-exclamationmark_0.txt

Is there a reason to do this vs. just protocol filtering everything?

Yes, we don't want to strip words that precede colons in title (and some other) attributes and non-attribute text. Such as:

$title = 'Llama: a domesticated South American camelid';
t('More<span class="visually-hidden"> posts about @title</span>', ['@title' => $title]);
t('<a href="@url" title="@title">More info</a> about ...', ['@title' => $title, ...]);

URLs not from user input that do not need to be sanitized, but have already been generated via the URL generator and so should not be escaped a second time

The URL generator doesn't "escape" (in the sense of Html::escape()) a first time, so there's no "second" time to speak of. The URL generator does stripDangerousProtocols() in some cases, so if we want t() to protocol strip in some cases, then we'd have to address the question of whether it's ok to stripDangerousProtocols() twice. I think it's ok functionally, but perhaps feels wasteful.

URLs from user input that have already been sanitized including stripping bad protocols, and so should not be escaped a second time

In Drupal 7, this was done via check_url(), so if we continued to support check_url() in D8, I would say this should be handled by making check_url() return a SafeString. But, check_url() is now deprecated in D8, so I don't know what the specific use case of already Html::escape()'d-but-not-a-SafeString is. Anyone have a concrete example?

Placeholders for non-HTML output

I think either $options['html'] = FALSE or t() returning an object with different methods for html and non-html output is superior to continuing to do this with a placeholder prefix.

Embedding HTML that does not need to be sanitized

Why does it not need to be sanitized? Because it doesn't use any <script> tags or other questionable markup? Or because it does, but the caller trusts it (e.g., something returned by krumo or another trusted library / data source)? I think the former can be done like this:

t('Some HTML follows: @html', ['@html' => drupal_render(['#markup' => $script_free_html])]);

And the latter perhaps like this if we introduce a '#raw' or '#verbatim' render API support?

t('Some HTML follows: @html', ['@html' => drupal_render(['#raw' => $html_with_trusted_javascript])]);

@effulgentsia re #35 I tend to agree with your here about letting the caller filter their own potentially bad URLs and I think maybe we are over-engineering this !url problem. The only real problem I see is we have 300K patch that mostly does these urls for hook_help() and it's likely we won't wrap each argument in UrlHelper::stripDangerousProtocols() BUT at the same time those are not variables, they are hard coded URLs with 0 chance of being vulnerabilities. So yes I'm onboard with letting the caller do it.

@catch re #36 and @xjm #38 The biggest problem with this auto-magical filtering I believe is performance as @alexpott mentioned regarding preg_replace() in #31 Yet as mentioned in #38 it has other side effects of previously escaped or filtered, but half of that problem is performance too.
Maybe we can do some kind of hybrid of parsing t() mentioned in #35 in an assert() to alert developers, but we'd still need to know if the value was previously sanitized so that may be tricky...

@stefan.r++ thanks for raw data to parse:)

I think we should go for the quickest win here - and ideally somethign that gives us a result similar to Drupal 7, so #1 from the issue summary. Just pass it through.

We can work to remove use of these placeholders as we wrap up 8.0.x, but let's not try to invent a whole new system at this point.

Is there an issue started for that resolution?

I'm just wondering if we should organize a call to discuss about this? I'm +1 for #31, however given the tight schedule we might want to consider, if it would be possible to separate the critical parts & BC breaking parts to the post RC phase. We could keep improving rest of it during the RC phase. I'm happy to work on this but right now I feel like this is kind of stuck.

Yeah I think we should really do #7, it adds more semantic meaning, is the cleanest solution for many of those conversions
and also important help the developer. We discussed about that on the critical call and decided to start with #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols

One more thing I wanted to add is as we discussed this on the criticals call, IF we can automate the updates to the localize.drupal.org strings (reupload fixed strings as suggestions for the teams). That is possible to do after RC1.

One more thing I wanted to add is as we discussed this on the criticals call, IF we can automate the updates to the localize.drupal.org strings (reupload fixed strings as suggestions for the teams). That is possible to do after RC1.

While talking with catch later we also thought a bit about also updating the strings on the Drupal installations itself using hook_update_N()/hook_post_update_NAME()
but I'm not entirely sure at that point, whether its needed given that we can sync translations ... do we care about custom made translations?

Yeah if we want to keep custom translations we do need update functions as well yes. Which is one more good reason to have an easy scriptable change pattern.

Went over this with @xjm, @effulgentsia, @pwolanin, @Gábor Hojtsy and a few others. I believe these are the actionable parts of this now for people who are looking to get this polished off sooner than later:

• One way or another, we want to get rid of as many instances of ! in strings as possible before string freeze. That's at #2506445: Replace !placeholder with @placeholder in t() and format_string() for non-URLs in tests
• #2509218: Ensure that SafeString objects can be used in non-HTML contexts would allow a BC layer in place while we work through what we ultimately want to do
• #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list Sorry, I forget why this is good, but alexpott wants it, so. :)
• #2545972: Remove all code usages SafeMarkup::checkPlain() and rely more on Twig autoescaping This is another big chunk out of the big meta issue to remove SafeMarkup methods and leads to more secure code.
• #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols This one doesn't have consensus yet, so is a bit risky to work on, but having at least a patch here might make other issues earlier, not sure.

Went over this with @xjm, @effulgentsia, @pwolanin, @Gábor Hojtsy and a few others. I believe these are the actionable parts of this now for people who are looking to get this polished off sooner than later:

While I totally agree with every dedicated step you listed and its advantages, this list doesn't make it clear which part of those are actually critical,
and which parts needs / can be resolved in order to resolve this particular critical issue. Did you talked about those points?

One point for me which is critical is that we announce when the string freeze actually happens so translators from > 100 languages can actually rely on it or at least
partially can.

I'll give a more detailed answer to #50 this weekend, but for now, I'll give the short answer of: removing all core usages of !placeholder is very likely Critical. That means #49.1 and #49.5 will likely need to be promoted to Critical. #49.3 and #49.4 would also be very, very good to do, even if they end up not being strictly Critical, and there's a chance they will be. #49.2 might be moot once #49.3 is done: I'll work on an issue summary for that issue to clarify that this weekend.

Just a note that at xjm's request, I've sent around a Doodle to most of the people involved in this and various sub-issues to try and get alignment on a direction on Mon/Tues.

While reviewing this I found #2567741: Attribute/drupal_attributes() docs do not mention protocol filtering on URLs and opened an issue to fix the currently incorrect docs on those functions.

Also opened #2567743: Add protocol filtering to the core Attribute component which would make the current docs accurate. Although I think we should work on both independently - the docs one will be quicker.

@all see research I did yesterday regarding Option 7 in the issue summary.
#2565895-46: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols

Oops, I guess I added the tag to an old page (prior to your #57 update).

Ooookay.

Had a massive hangout with the following people:

• Alex Bronstein (effulgentsia)
• Nathaniel Catchpole (catch)
• Lauri Eskola (lauriii)
• Jess M (xjm)
• Joël Pittet (joelpittet)
• Alex Pott (alexpott)
• Scott Reeves (Cottser)
• Erik Stielstra (Sutharsan)
• Daniel Wehner (dawehner)
• Peter Wolanin (pwolanin)

(If I missed anyone, sorry, I was taking notes, not paying attention to the hangout ;))

On said hangout we discussed a number of questions:

• Are we cool with the stuff in #63 that has already been decided?
• Should we merely deprecate !placeholder, or should we remove it? And is doing whatever we do there critical?
• Should we make t() return a TranslationWrapper, and if so, is it critical?
• Should we introduce a :placeholder for URLs, and if so, is it critical?
• Are the various sanitization context issues that catch has found hardening, or release-blocking?

Here's the summary from the call. Also adding this to the issue summary:

• No objections were raised about the three items decided by the core committers prior to the call:
  • We will not ship D8 with !placeholder continuing to behave as it does in HEAD (sometimes causing the string to be marked unsafe if !placeholder is not already independently safe).
  • We will not restore the D7 behavior of !placeholder, given the different expectations D8 vs. D7 developers will have regarding auto-handling of security.
  • We will not change !placeholder to automatically Xss::adminFilter() the whole string nor Xss::adminFilter(!placeholder) by itself.
• (various issues) On the question of deprecating !placeholder or removing it, consensus was to remove it entirely (critical issue). We can write a smoke test (don't have to commit it) that checks all drupalGet()/drupalPost() calls for unexpected escaping bugs. Announce ! is deprecated ASAP, update change records/docs.
• #2557113: Make t() return a TranslationWrapper object to remove reliance on a static, unpredictable safe list On the question of making t() return a TranslationWrapper object, consensus was to do it (critical issue). We can add a temporary BC layer that t() only returns TW in cases where HEAD adds it to the SafeMarkup safe list. Until we finish ! placeholder removal, the behavior will be unchanged for those cases where ! would have caused the string to be marked unsafe. In the TW issue, add this temporary BC layer, and create a postponed follow-up to remove the BC layer once ! is gone. Make sure the change record documents things like the #alt / $attributes case, the optgroup case, impacts on assertIdentical() for objects, etc.; anything we had to do to get tests passing.
• #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols On question of providing a :url placeholder for URLs, consensus was to do it (critical issue). Increases surface area of the API, but for good reason, because URLs are their own string format and XSS vector, and it also removes the need to understand the different URL methods when creating a t() string. We do need to make sure documentation is very thorough, including what the cases to use :placeholder are (which attributes etc. to use it with), and what the cases that are NOT covered by :placeholder nor @placeholder are (other attributes/internal parts of tags). Keep ":" as the placeholder.
• A number of other sanitization context concerns were raised by catch, including:
  • #2567741: Attribute/drupal_attributes() docs do not mention protocol filtering on URLs
  • #2567743: Add protocol filtering to the core Attribute component
  • #2569041: Figure out what to do about attribute filtering in Twig
  • #2567257: hook_tokens() $sanitize option incompatible with Html sanitisation requirements
• Consensus was that none of those problems are critical, they are all major hardening that could be done before, during, or after RC. However, catch would like to investigate esp. the token sanitization one a bit more before making that a hard call.

Added a smoke test issue.

I've created #2570093: Replace !placeholder with @placeholder where needed in JavaScript as we did not yet cover the !placeholder in JavaScript.

During planning for replacing !placeholder it was accepted that we would break translation strings. Given that all of the critical portions of these updates have been committed, here is a listing of the affected strings and their impacts on the various languages: #2572771-11: List translation string changes following replacement of !placeholder and @placeholder and discuss ways to assist translation teams
Linking this here for visibility in case any translation teams find it helpful.

All of the issues here are either closed already, or JavaScript issues which are fine existing in their own right. Closing this out.