dev/core#1328 Add define to optionally enable escape-on-output

[eileenmcnaughton]

Overview

Add define to optionally enable escape-on-output

This PR + fixing the use of isset in the template layer are the pre-requisites for
turning escape on output on and off. Keeping this part in an unmerged branch is
tricky as the templates need to be recompiled when you switch to a branch
which does not have this function

I envisage us starting to use this define in our dev environments fairly soon
as it's working well locally for me on
#21935

Before

Only smarty variables specifically piped to escape are escaped

After

If you add the following define

define('CIVICRM_SMARTY_DEFAULT_ESCAPE', TRUE);

Then all variables are escaped.

Technical Details

Note that with this PR if you uncomment the above define your site will crash as long as there are calls to isset in the smarty templates. All of those calls have been removed in #21935 - but not with adequate care (and with some errors) . Merging this helps the process of working through those and the enotice issues since it makes it easier to switch back & forth between having default esaping on & off

Comments

#21979 is open as one PR removal of isset in the template & a bunch of those would need to be merged

https://lab.civicrm.org/dev/core/-/issues/1328#note_66924

[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

[eileenmcnaughton]

@seamuslee001 @demeritcowboy it would make it easier to work on this if we could merge this one - since the presence or not of the function can be a bit of a pain between branches - given it is compiled into the templates

[demeritcowboy]

What is the security team's take on the whole approach? i.e. is there concept approved? That's been my main hesitation from single handedly approving the various PRs.

[eileenmcnaughton]

@demeritcowboy yeah - I chatted with @seamuslee001 & @totten today & explained where it was at & the various aspects. I think the mood was generally 'yeah ok' - I do hope that once the basics are working we might get some early adoption in dev environments from people like @pfigel & @adixon - the key parts are

1. no change unless people set that define
2. the default escape function has some carve-outs to not break awfully
3. it changes how we handle things in civi in terms of not using isset in templates & reducing use of empty and also adding use of |smarty nodefaults to specify that default escaping (or modifiers in general) should not apply to a variable

[eileenmcnaughton]

I just added a comment here too https://lab.civicrm.org/dev/core/-/issues/1328#note_66924

[demeritcowboy]

Ok thanks.
Noting the site will also crash if you don't clear templates_c after setting the setting, separate from isset().

One thing is that it seems to do double-escaping for any template that already has |escape.

Related but separate, it should probably check that escape isn't already in the default_modifiers list before adding it.

Otherwise I don't see any issues.

[adixon]

I love the concept! Looking forward to trying it out.

[eileenmcnaughton]

@demeritcowboy I've fixed it to check for the modifier

On the double escape - I guess we just need to add smarty:nodefaults - we can't remove the existing escapes

[demeritcowboy]

Sorry what I meant was suppose $this->default_modifiers already contains something like escape:"html". Then everything would end up with double-escaping.

[eileenmcnaughton]

@demeritcowboy but then why would you add the define if you had some other method adding in a default modifier?

[demeritcowboy]

Ahh.