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).
Comment | File | Size | Author |
---|---|---|---|
#21 | trans_is_unable-2575275-21.patch | 4.93 KB | cilefen |
#15 | trans_is_unable-2575275-15.patch | 4.93 KB | cilefen |
#11 | interdiff.txt | 1.4 KB | lauriii |
#11 | trans_is_unable-2575275-11.patch | 5.26 KB | lauriii |
| |||
#7 | 2575275-7.patch | 5.37 KB | stefan.r |
Comments
Comment #2
lauriiiComment #3
stefan.r CreditAttribution: stefan.r commentedwe also strip URLs of dangerous protocols - could we come up with something more descriptive than "escape URLs"?
Could this just call Html::escape always, for consistency with ":"?
Comment #4
alexpottI 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()
andTranslatableString()
?Comment #5
dawehnerWhile 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.
Comment #6
catchYes 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.
Comment #7
stefan.r CreditAttribution: stefan.r commentedComment #10
cilefen CreditAttribution: cilefen commentedHtml::escape takes only one parameter. Do we actually need the twig environment here? Perhaps we do.
Comment #11
lauriiiComment #12
stefan.r CreditAttribution: stefan.r commentedLooks good! Seems useful to have this available in Twig as well and have feature parity with SafeMarkup::format()
Comment #13
stefan.r CreditAttribution: stefan.r commentedComment #15
cilefen CreditAttribution: cilefen commentedThis is a reroll.
Comment #16
stefan.r CreditAttribution: stefan.r commentedComment #17
stefan.r CreditAttribution: stefan.r commentedComment #21
cilefen CreditAttribution: cilefen commentedComment #22
joelpittetThis looks like it does the trick. My only concern:
There is a bit of a naming collision calling a filter a filter...
|strip_dangerous_protocols
may be a bit long?Comment #23
stefan.r CreditAttribution: stefan.r commentedIt also escapes though. Just to clarify, what does this collide with?
Comment #24
joelpittet|filter
the pipe is a filter in twig, so it it's like calling your methodmethod()
. In that sense of collision with the language syntax.Comment #25
stefan.r CreditAttribution: stefan.r commentedSo could we do |url or is that taken?
Comment #26
joelpittetI'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.
Comment #27
star-szr|url_escape
|escape_url
Comment #28
star-szrOr if it's not really escaping, url_strip, strip_url. ¯\_(ツ)_/¯
Comment #29
stefan.r CreditAttribution: stefan.r commentedI 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?
Comment #30
joelpittethey that's not bad
|url_xss
!|url_filter_xss
even?Comment #31
joelpittetThen I can create my contrib module "xss attack" with
|url_xss
still:) And it will generate XSS attacks. Of course will require bad judgment:PComment #32
lauriiiI'm happy with
|url_filter
.Comment #33
Gábor HojtsyI 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.
Comment #34
Gábor HojtsyComment #35
star-szrThank you @Gábor Hojtsy!
Comment #36
stefan.r CreditAttribution: stefan.r commentedThe 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?
Comment #37
xjmThanks 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.Comment #38
Gábor Hojtsy@xjm: hope this helps (updated the summary):
Comment #39
Gábor HojtsyAlso 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.
Comment #40
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI'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:
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.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.
Comment #41
Gábor Hojtsy@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?
Comment #42
effulgentsia CreditAttribution: effulgentsia at Acquia commentedRe #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.Comment #43
Gábor Hojtsy@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.
Comment #44
alexpottDiscussed 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.
Comment #45
stefan.r CreditAttribution: stefan.r commentedSo what still needs to happen here?
Comment #46
effulgentsia CreditAttribution: effulgentsia at Acquia commentedI think #44 is asking for:
to be changed to:
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?
Comment #47
Gábor HojtsySo 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. :(
Comment #49
xjmThe 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:
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.Comment #51
joelpittet