Smarty - Convert {ts} block to compile-time plugin#36121
Conversation
|
🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷 Introduction for new contributors...
PR commands & links...
|
c10d442 to
2adb0b2
Compare
|
Really nice how you tracked through the compiler process. This example feels quite surprising: In the simplest terms... I only expect IMHO, Smarty's design for escaping is... probably more mature than
I wonder if it might make more sense to imitate Smarty behavior -- apply filtering to the variable-data used by {* ONE VARIABLE, ONE KIND OF DATA *}
{* Use default escaping *}
{ts 1=$name}Hello %1{/ts}
{* Use explicit escaping *}
{ts 1=$name|escape:"html"}Hello %1{/ts}
{* Use explicit escaping *}
{setfilter escape:"html"}
{ts 1=$name}Hello %1{/ts}
{/setfilter}{* MULTIPLE VARIABLES, MULTIPLE KIND OF DATA*}
{ts 1=$name|escape:"html" 2="target=_blank url='https://example.com'"|nofilter}<A %2>Hello %1</a>{/ts}
{setfilter escape:"html"}
{ts 1=$name 2="target=_blank url='https://example.com'"|nofilter}<A %2>Hello %1</a>{/ts}
{/setfilter} |
Yea, that's a tough one... that example text In that case our brains have a different intuition and the output looks correct. Note that the I do like your idea of applying filters directly to each variable e.g. |
2adb0b2 to
ceb83e1
Compare
|
@totten actually that already works. Added the new test case for it. |
|
@totten so the test failure in CI is: What this is saying is that if you globally turn on escaping and you also escape the individual variable in {ts} then it will cause double-escaping of the variable. My answer to this test is that should be the expected behavior and the scenario being tested is mistaken. If you ask it to escape the entire string AND the variables within it, then the variables will come out double-escaped, so you shouldn't do that. I understand this is a contractual change... however, none of our templates yet use autoescaping, so we still have time to change the contract. And IMO it's a good change that will make Civi more secure in the long-run. We want auto-escaping to apply to entire {ts} blocks automatically, that's the safest default, and it can still be bypassed with |
This replaces the old runtime block plugin (block.ts.php) with a pair of
compile-time plugins so that {ts} respects the template-wide escape_html
setting and any enclosing {setfilter} block, just like {$variable} does.
How it works
------------
On the opening {ts ...} tag, the compiler emits ob_start() so that the
literal text between the tags is captured at runtime. On the closing
{/ts} tag, the compiler emits the call to _ts(ob_get_clean(), $params)
and then wraps the result in exactly the same modifier / escape_html
logic that PrintExpressionCompiler applies to {$variable} output.
Because all of this is resolved at compile time, {setfilter escape}
and $smarty->escape_html both work transparently with no changes to
existing .tpl files.
The explicit {ts escape='html'} attribute is still honored: if it is
present, the plugin forces htmlspecialchars() regardless of the filter
state, and sets the "raw output" flag so that escape_html auto-escaping
does not double-escape the result (matching Smarty's own behaviour for
{$var|escape:'html'}).
ceb83e1 to
2333ec3
Compare
|
Long timeframe on this PR; I guess because there's a lot to feedback on....
Yep, I didn't make it up. :) Just reporting on how Smarty already addressed that.
To my brain, that example N.B. The
It sets the default filter for variables. It has no impact on literal text. Your intuition might be more pleased by a different tag. Hypothetically, you could implement a tag {filter escape:html}Hello {$first_name} {$last_name}. All of this will be escaped at once.{/filter}
{filter escape:html}This & will be escaped.{/filter}But note the distinction...
So there's a design/aesthetic question about what behavior is desirable, and then a question about what behavior is being exhibited.
|
|
Thanks for your feedback @totten First, a clarification:
The test shows that this PR will cause double-escaping with global escaping ON and variables escaped inside a ... now to the question of whether we should: You've raised 2 procedural objections
And a design objection
I see what you mean about
Unsafe HTML Examples These may seem obvious but imagine the translated string contains these special characters instead of the source string, making the gotcha invisible to developers: <button title="{ts}Clicking "delete" cannot be undone.{/ts}">Delete</button><button onclick="alert('{ts}Don't click me!{/ts}')">Delete</button>Unsafe JS Examples <script>
let greeting = '{ts}It's nice to meet you.{/ts}';
</script><div data-options='{"label": "{ts}I'm "very" safe, trust me bro.{/ts}"}'></div>Unsafe SQL Examples INSERT INTO civicrm_note (note)
VALUES ('{ts}User's note{/ts}');To address your procedural objections:
IMHO, the opposite is true. If we make escaping to
I disagree. I think it will push you to be intentional about designating a string as markup. By our status-quo in Civi templates, |
Arg, right. @colemanw - OK, so your list of examples persuades me that there is a problem -- where ts-total-escape is responsive; and where basic Smarty isn't responsive. I just want to restate the problem with an even more pointed example (which helps me). Consider just one string,
The translator should only see this string once in Transifex. Even if the translator is knowledgable+patient about escaping, it's not possible for one translation to work in all those contexts. And it's a good thing that we only have one copy of that string. (It would be silly to translate the same string 5 times because each one has slightly different encoding.) So thanks, I agree that's also an important angle. (Still need to figure out a way that reconciles the competing angles here.) |
|
@totten yes exactly. This feels like yet another one of those times when CIvi has been at a crossroads and we're faced with a design choice. Do we make it: A. Unsafe by default but easier in the short-run Maybe I'm over-reacting because of all the other times we've chosen A and lived to regret it, but something inside me is screaming FOR GOD'S SAKE CHOOSE B THIS TIME! As your 5 examples show, the safe choice is for If we don't do that and go with option A, then yes what's happening is that a specifically-encoded string (e.g. html or js or sql encoded) is being sent to transifex but with the encoding context stripped away and it's left for the translator to guess the encoding and craft their string appropriately. In practice, that usually works out ok, and it usually doesn't bite us really hard and break the UI or crash the server or open up security vulnerabilities... 🙈🙈🙈 |
Here it is: https://gist.github.com/colemanw/2306860feaae1bf8502542f33488176f |
|
Your PR has been merged. |
2 participants
Overview
Toward moving dev/core#1328 forward, this adjusts our Smarty
{ts}block to respect the template-wideescape_htmlsetting and any enclosing{setfilter}block, just like{$variable}does.Technical Details
On the opening
{ts ...}tag, the compiler emitsob_start()so that the literal text between the tags is captured at runtime. On the closing{/ts}tag, the compiler emits the call to_ts(ob_get_clean(), $params)and then wraps the result in exactly the same modifier / escape_html logic that PrintExpressionCompiler applies to{$variable}output.Because all of this is resolved at compile time,
{setfilter escape}and$smarty->escape_htmlboth work transparently with no changes to existing.tplfiles. Since we don't (yet) use either of those settings, there should be no functional change to any templates, but it makes way for us to start using those settings with consistent results.The explicit
{ts escape='html'}attribute is still honored: if it is present, the plugin forceshtmlspecialchars()regardless of the filter state, and sets the "raw output" flag so thatescape_htmlauto-escaping does not double-escape the result (matching Smarty's own behaviour for{$var|escape:'html'}).