Problem/Motivation

In #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols we added :variable placeholders so that URLs can be secured from bad protocols. However this is not possible in Twig {% trans %}, which has the equivalent of @ and % only.

Proposed resolution

Add a Twig filter and placeholder for {% trans %}.

Remaining tasks

Commit

User interface changes

None.

API changes

A new filter and placeholder for {% trans %}.

Data model changes

None.

Why this should be an RC target

#2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols which introduced this on the PHP side was an RC/release blocking critical. It should have already added support for :placeholder in Twig. Not allowing Twig templates to use the URL filtering like the rest of the Drupal codebase may lead to security issues and translatable string inconsistencies (Eg. a string properly using the :placeholder in PHP will not be reusable in templates because Twig's %trans% lacks support for it).

Files: 
CommentFileSizeAuthor
#21 trans_is_unable-2575275-21.patch4.93 KBcilefen
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,470 pass(es). View
#15 trans_is_unable-2575275-15.patch4.93 KBcilefen
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch trans_is_unable-2575275-15.patch. Unable to apply patch. See the log in the details link for more information. View
#11 interdiff.txt1.4 KBlauriii
#11 trans_is_unable-2575275-11.patch5.26 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,361 pass(es). View
#7 2575275-7.patch5.37 KBstefan.r
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,111 pass(es), 1 fail(s), and 0 exception(s). View
#7 interdiff-2-7.txt922 bytesstefan.r
#2 trans_is_unable-2575275-2.patch5.36 KBlauriii
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,972 pass(es). View

Comments

lauriii created an issue. See original summary.

lauriii’s picture

Status: Active » Needs review
FileSize
5.36 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,972 pass(es). View
stefan.r’s picture

  1. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -346,6 +348,21 @@ public function escapePlaceholder($env, $string) {
    +   * Provides way to escape URLs.
    

    we also strip URLs of dangerous protocols - could we come up with something more descriptive than "escape URLs"?

  2. +++ b/core/lib/Drupal/Core/Template/TwigExtension.php
    @@ -346,6 +348,21 @@ public function escapePlaceholder($env, $string) {
    +    return $this->escapeFilter($env, UrlHelper::stripDangerousProtocols($string));
    

    Could this just call Html::escape always, for consistency with ":"?

alexpott’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -134,6 +135,7 @@ public function getFilters() {
+      new \Twig_SimpleFilter('url_filter', array($this, 'urlFilter'), array('is_safe' => array('html'), 'needs_environment' => TRUE)),

@@ -346,6 +348,21 @@ public function escapePlaceholder($env, $string) {
+    return $this->escapeFilter($env, UrlHelper::stripDangerousProtocols($string));

I agree with @stefan.r - I don't think this should do a regular escape (which is conditional depending on safeness) - should't it always escape to be the same as SafeMarkup::format() and TranslatableString()?

dawehner’s picture

While it would be nice to have feature parity I'm not necessarily convinced that this is critical, why? We are in an HTML. How high is the chance that you generate an HTML inside {% trans %} and not use the proper template engine and then put strings around that.

In other words, this does feel more like major.

catch’s picture

Priority: Critical » Major
Issue tags: +SafeMarkup, +Needs issue summary update

Yes this feels major in the same way that #2569041: Figure out what to do about attribute filtering in Twig and #2567743: Add protocol filtering to Attribute are. Downgrading for now. If there's a core use case we're missing could use a reference.

stefan.r’s picture

Status: Needs work » Needs review
FileSize
922 bytes
5.37 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,111 pass(es), 1 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 7: 2575275-7.patch, failed testing.

The last submitted patch, 7: 2575275-7.patch, failed testing.

cilefen’s picture

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -346,6 +348,21 @@ public function escapePlaceholder($env, $string) {
+  public function urlFilter($env, $string) {
+    return Html::escape($env, UrlHelper::stripDangerousProtocols($string));
+  }

Html::escape takes only one parameter. Do we actually need the twig environment here? Perhaps we do.

lauriii’s picture

Status: Needs work » Needs review
FileSize
5.26 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 114,361 pass(es). View
1.4 KB
stefan.r’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs issue summary update +Needs change record

Looks good! Seems useful to have this available in Twig as well and have feature parity with SafeMarkup::format()

stefan.r’s picture

Assigned: lauriii » Unassigned

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: trans_is_unable-2575275-11.patch, failed testing.

cilefen’s picture

FileSize
4.93 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch trans_is_unable-2575275-15.patch. Unable to apply patch. See the log in the details link for more information. View

This is a reroll.

stefan.r’s picture

Status: Needs work » Needs review
stefan.r’s picture

Status: Needs review » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 15: trans_is_unable-2575275-15.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 15: trans_is_unable-2575275-15.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
4.93 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,470 pass(es). View
joelpittet’s picture

This looks like it does the trick. My only concern:

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -154,6 +155,7 @@ public function getFilters() {
+      new \Twig_SimpleFilter('url_filter', array($this, 'urlFilter'), array('is_safe' => array('html'))),

There is a bit of a naming collision calling a filter a filter...

|strip_dangerous_protocols may be a bit long?

stefan.r’s picture

It also escapes though. Just to clarify, what does this collide with?

joelpittet’s picture

|filter the pipe is a filter in twig, so it it's like calling your method method(). In that sense of collision with the language syntax.

stefan.r’s picture

So could we do |url or is that taken?

joelpittet’s picture

I'd expect that would convert a string into a Url object or something of the sort.

Though I also think nobody will use this... or use it wrong. Sorry to be pessimistic here.

Cottser’s picture

|url_escape
|escape_url

Cottser’s picture

Or if it's not really escaping, url_strip, strip_url. ¯\_(ツ)_/¯

stefan.r’s picture

I would just keep this as is (url_filter)... given that |url is not appropriate, #28 not technically correct and |url_escape_and_strip_dangerous_protocols is a bit silly.

Maybe |url_xss?

joelpittet’s picture

hey that's not bad |url_xss! |url_filter_xss even?

joelpittet’s picture

Then I can create my contrib module "xss attack" with |url_xss still:) And it will generate XSS attacks. Of course will require bad judgment:P

lauriii’s picture

I'm happy with |url_filter.

Gábor Hojtsy’s picture

I agree url_filter looks odd. The placeholder is not called placeholder_filter or placeholder_escape either. I opened #2592573: The :placeholder translation placeholder type not supported in Twig since there was no mention of this issue at #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols. Closing that and connecting this to #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols so the world finds it.

Gábor Hojtsy’s picture

Cottser’s picture

Thank you @Gábor Hojtsy!

stefan.r’s picture

The current patch still says url_filter so this will need a reroll with the new name, joelpittet suggested url_filter_xss, are we OK with that?

xjm’s picture

Status: Needs review » Needs work

Thanks for tagging this for rc target triage! To have committers consider it for inclusion in RC, we should add a statement to the issue summary of why we need to make this change during RC, including what happens if we do not make the change and what any disruptions from it are. We can add a section <h3>Why this should be an RC target</h3> to the summary.

Gábor Hojtsy’s picture

Issue summary: View changes
Status: Needs work » Needs review

@xjm: hope this helps (updated the summary):

Why this should be an RC target

#2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols which introduced this on the PHP side was an RC/release blocking critical. It should have already added support for :placeholder in Twig. Not allowing Twig templates to use the URL filtering like the rest of the Drupal codebase may lead to security issues and translatable string inconsistencies (Eg. a string properly using the :placeholder in PHP will not be reusable in templates because Twig's %trans% lacks support for it).

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +security

Also given that we are only arguing about the name of the filter and we don't agree on it, we can have committers decide if they like it the way it is in the patch. The implementation is complete.

effulgentsia’s picture

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -154,6 +155,7 @@ public function getFilters() {
       // ... These filters should only
       // be used in "trans" tags.
       // @see TwigNodeTrans::compileString()
+      new \Twig_SimpleFilter('url_filter', array($this, 'urlFilter'), array('is_safe' => array('html'))),

I'm somewhat concerned about this going in before there's resolution in #2569041: Figure out what to do about attribute filtering in Twig. For example:

+++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.trans.html.twig
@@ -96,3 +96,11 @@
+  {% trans %}
+    Kittens are awesome: {{ url|url_filter }}
+  {% endtrans %}

But in #2569041-10: Figure out what to do about attribute filtering in Twig, there's a proposal to do Kittens are awesome: {{ url|e('url') }} instead for when that text is not inside of {% trans %}. However, the comment after that points out that that wouldn't be quite right either, at least given current Twig docs about the 'url' escape strategy not being suitable for full URLs.

I think it would be great for us to use the same syntax for outputting a URL whether we're inside of {% trans %} or not.

a string properly using the :placeholder in PHP will not be reusable in templates because Twig's %trans% lacks support for it

That's a reasonable argument for getting this in ahead of that one. On the other hand, do we know of any real case where this has happened or is likely to happen? If addressing this use-case isn't super-urgent, I'd still rather perhaps expand the scope of this issue to also cover outputting URLs in Twig outside of translations. Note that #2569041: Figure out what to do about attribute filtering in Twig is currently scoped to attributes in general, not only URLs, but I think this issue should not be expanded to anything other than URLs.

Gábor Hojtsy’s picture

@effulgentsia: so we thought it was critical for our PHP API to have URL filtering as a separate thing (ref: #2565895: Add a new :placeholder to SafeMarkup::format() for URLs that handles bad protocols). We apparently don't think that the same thing is critical in our templating API. Does that mean that template builders will be fine to use whatever they want and we are not concerned for what will come out of that security-wise with Drupal 8.0.0?

effulgentsia’s picture

Re #41: well currently, since we don't have an agreed-upon answer for how to filter URLs in Twig in #2569041: Figure out what to do about attribute filtering in Twig, it's the responsibility of preprocess functions to do that filtering, such as in template_preprocess_aggregator_item(). Since preprocess functions don't know if the template will print the URL within {% trans %} or not, it has to be their responsibility to filter. So long as the preprocess functions are filtering the URL anyway, there's no security problem with Twig templates outputting them within {% trans %} sections without any further filtering. However, the fact that this results in the translation string being registered in locale with "@" rather than ":" is a problem for purposes of reusability of that string in non-Twig contexts. And it would mean that if at some later point, we fix that issue and remove filtering from preprocess functions, then the Twig templates will need to be updated and that will be another break of locale strings.

Gábor Hojtsy’s picture

@effulgentsia: I find it odd that we have so different solutions in TranslatableMarkup (same as Drupal.t()) vs. {%trans%}, I think they would be used well if they would be consistent. It is not just we use different markers for placeholders, which is coming from Twig anyway, but that we don't even support the same set of security features. We worked hard in Drupal 8 to align Drupal.t() and t() in terms of features and it would be said if {%trans%} will not provide the same set of security features.

I do understand that requiring URL filtering to happen in preprocess functions in the workaround for now.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -rc target triage +rc target

Discussed with @catch, @xjm, @effulgentsia and agreed this should be rc target. We can leave the non trans case for #2569041: Figure out what to do about attribute filtering in Twig.

I think we should make the new filter behave like the placeholder filter and use $this->escapeFilter() to do the escaping.

stefan.r’s picture

So what still needs to happen here?

effulgentsia’s picture

I think #44 is asking for:

+++ b/core/lib/Drupal/Core/Template/TwigExtension.php
@@ -367,6 +369,19 @@ public function escapePlaceholder($env, $string) {
+    return Html::escape(UrlHelper::stripDangerousProtocols($string));

to be changed to:

return UrlHelper::filterBadProtocol($this->escapeFilter($env, $string));

I don't know if I agree with that though. escapeFilter() is designed to support a string that might already have rendered markup, or a render array, but URLs are not HTML, so I don't see why we'd want to spend the extra CPU cycles on supporting that. @alexpott: what's your reasoning for wanting that?

Gábor Hojtsy’s picture

Issue tags: -sprint, -rc target

So we did not figure out this or #2569041: Figure out what to do about attribute filtering in Twig either in time for an RC. Removing from D8MI sprint due to inactivity. :(

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Title: {% trans %} is unable to use URL escaping » {% trans %} (and other parts of templates) are unable to use URL escaping
Issue tags: +Triaged D8 major, +Needs followup
Related issues: +#2569041: Figure out what to do about attribute filtering in Twig

The core committers (@xjm, @alexpott, @catch, @effulgentsia, @Cottser) and theme maintainers (@lauriii, @joelpittet) discussed this issue today and agreed that this issue (as well as the related, broader issue #2569041: Figure out what to do about attribute filtering in Twig) are major. I'm also expanding the scope a little, since this is not limited to only the {% trans %} tag and since according to @alexpott this issue will necessarily make the fix available for other parts of templates as well.

We also discussed whether this issue should go into 8.1.x or 8.2.x. There were two reasons:

  1. This change implies changes to translatable strings in order to use the URL escaping.
  2. This change adds a method to TwigExtension.

For the first point, @alexpott pointed out that we don't need to break strings with this issue -- that can be followup work filed against 8.2.x. (Adding a followup for that.)

For the second, we disagreed somewhat. @catch pointed out that semver says we "may" increment the minor version number for "private" API additions, and must for public API additions, so it was up to our policy to decide whether this was an API change. I don't think TwigExtension is "private" so to me this would not be consistent, and it also doesn't really fit under https://www.drupal.org/core/d8-allowed-changes#minor. On the other hand, though, this addition does not really risk disruption because there is not much chance of anyone subclassing this or using the API directly. (This is unlike adding a method to e.g. an entity type definition, because that could easily have a name collision with a subclass, so it makes sense to restrict those changes to minor versions.)

Still, for me, this is an addition, even if we consider the lack of support/parity with TranslatableMarkup a bug. In any case, though, it is definitely a major issue (whether bug or task, 8.1.x or 8.2.x). We agreed to leave it filed against 8.1.x for now.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joelpittet’s picture

Issue tags: +Twig

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.