WIP - optin escape on output for smarty

[eileenmcnaughton]

Overview

WIP - optin escape on output for smarty.

This adds a 'strict mode' - it is not expected to be used in strict mode on live sites at the moment but allows devs to turn it on

Before

Status quo

After

No change unless you add

define('CIVICRM_SMARTY_DEFAULT_ESCAPE', TRUE);

(or use env to set the same)
AND delete templates_c

If you do you get a 'mostly functional' site with escape on output enabled - with a bunch of holes punched through.

Anything actually changed during escaping is logged to the civi log

Technical Details

The key parts to this are

1. get rid of isset in templates - even though we just added it .... Isset is not compatible with variables being escaped. I did a really rough find & replace here but it will need a more careful pass to get merged. We can pick these off
2. the right way to mark that a smarty variable should not be put through escape-on-output is this

{$shortCut.title|smarty:nodefaults}

That works in smarty 2 & smarty 3 and says 'do not apply any default modifiers'. We currently don't have any default modifiers but default -escaping is a default modifier

3. override the default escape function with one that does a series of early exits & only reaches the default function if they don't apply. The exits cover non-string values and common html strings. Over time these could be whittled down as we use approach 2 instead -especially in non-quick-form places link links.

Comments

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[eileenmcnaughton]

test this please

[demeritcowboy]

The find/replace is backwards I think (isset would become !empty), although I'm not clear yet on what this is actually about. For example I don't see what's wrong with the original here: https://github.com/civicrm/civicrm-core/pull/21935/files#diff-6c8cfb09c10a673dc77c3bfef2a71abca2cd0d1860752ace4929f89021ea3fdcL11

[eileenmcnaughton]

@demeritcowboy yeah - I only searched for isset as a quick & dirty - I guess all the `!isset ones will be wrong.

isset doesn't work on the output from a function - & if you enable filtering by default that is what you are working with. Filter on output means every variable used in a smarty template is escaped - & hence is the output of an escaping function. In order to not white screen I carved out a whole lotta exceptions - but at this stage the site is loading for me with it & I can edit contacts & a few other things which is further than I've ever gotten before. Those issets will need to be picked off 1 by 1 though.

[demeritcowboy]

Ahhhh.

[demeritcowboy]

Should this be appending to the default_modifiers array, in case it's already set?

[eileenmcnaughton]

yeah makes sense

[colemanw]

I'm surprised by the behavior you're describing because it's different than what I'm used to in Angular templates. In Angular-js, when you write something like <p>{{ myFn() + myVar }}</p> it will evaluate the expression and then escape the final output, e.g. it will execute myFn() and concatenate it with the contents of myVar and then that final string is escaped before being printed out.
With what you're describing, smarty doesn't escape the output of expressions it escapes assigned variables. Is that right?

[demeritcowboy]

Smarty doesn't escape anything, the |escape modifier, which is compiled by smarty into a php function call, is what does the escaping, so then calling isset on that is an error.

[demeritcowboy]

Just adding some thoughts for when this gets to dev-docs stage:

A file that looks like https://github.com/civicrm/civicrm-core/pull/21939/files where the new default is now about half of the instances leads to an awkward looking file, and difficult for devs to know what to use when and makes it security-issue-prone. It sounds like the desired order from best to worst would be:

1. Let it use the (new) default of escape on output where possible.
2. Assign variables to the template so you don't have to use isset/empty and can then let it do 1.
3. Use smarty:nodefaults as a last-resort.

[eileenmcnaughton]

@demeritcowboy that sounds right - that file is an outlier in terms of awkwardness though - the array is compiled in multiple places making 2 quite tricky & there is very little in that file that is not at risk of not being set.

[eileenmcnaughton]

I don't think this PR adds value now - it was conceptual but the parts are addressed elsewhere