allow user strings to be parsed with or without compile-caching

[highfalutin]

Overview

Best practice as of the Civi 5.75 security release is to use functions like CRM_Utils_String::parseOneOffStringThroughSmarty(), which wraps Smarty in a security layer, rather than calling CRM_Core_Smarty::singleton()->fetch(), which is less secure. This PR provides another function, CRM_Utils_String::parseUserStringThroughSmarty(), which uses the security layer and doesn't disable caching. Based on discussion on MM.

Before

If you wanted to parse a user-supplied string in a secure way, it would have to be compiled anew each time.

After

You can parse a user-supplied string in a secure way, and take advantage of Smarty's compiled-template caching if you want.

Comments

I'm not married to the function names here. Any ideas on how to make them clearer and shorter?

[civibot]

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...

• If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
• A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
• A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
• If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
• Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
• A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
• To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
• For more information about contributing, see CONTRIBUTING.md.

Quick links for reviewers...

• ➡️ PR Demo site (can be rebuilt with the command test this please).
• 📖 Review standards
• 🗒️ Review template (brief or verbose)

➡️ Online demo of this PR 🔗

[seamuslee001]

my only concern here is the windows element here @demeritcowboy do you have thoughts here?

[demeritcowboy]

Something else seems to have changed since 5.69. Let me get back to you...

[highfalutin]

Just to be clear, this is a very simple refactoring. The function parseOneOffStringThroughSmarty() should work exactly as before. So any problems with Windows won't have changed.

[demeritcowboy]

Yes the old function might be unchanged but the question is about the new function/usecache=true. It will try to use string:, and this is where the problem is (was). I'm in and out today but I'd like to (a) determine if there is still a problem, (b) understand what changed elsewhere, and (c) remember what (c) was.

[demeritcowboy]

Soooo, it was actually the fact that we were still registering our own string resource that made it appear that smarty3 couldn't handle string: on windows. (Long ago in olden times, we created a version of string: in smarty 2, either borrowing from smarty3, or possibly inspiring smarty3 - who knows.) If you apply this PR from 5.71, which removes the registration, to 5.69 when the problem first appeared, everything is happy, and so there's no filename-based reason for eval when you really want string.

There may be other reasons to want eval, like to avoid caching sensitive info, but I'm not sure how you would know when and when not to do that here. In the discussion that prompted this PR, assuming we would change this now back to prefer string, would you know when the info is sensitive so you could tell it NOT to cache it? Or alternatively, would you know when it's NOT sensitive?

Boring tech details of why the registration matters regarding the filename:

• It hits here https://github.com/civicrm/civicrm-packages/blob/45b39026def71038e58b5330e37879d9b64ece8a/smarty3/vendor/smarty/smarty/libs/sysplugins/smarty_resource.php#L74, which then returns our version if it's registered.
• Our version causes it to call the base class getBasename(), and so returns the entire tpl string verbatim as the "name" which gets appended: https://github.com/civicrm/civicrm-packages/blob/45b39026def71038e58b5330e37879d9b64ece8a/smarty3/vendor/smarty/smarty/libs/sysplugins/smarty_template_compiled.php#L69.
• But when using smarty's internal implementation, it overrides getBasename() to return '' for the problematic part of the filename: https://github.com/civicrm/civicrm-packages/blob/45b39026def71038e58b5330e37879d9b64ece8a/smarty3/vendor/smarty/smarty/libs/sysplugins/smarty_internal_resource_string.php#L91.
• In smarty2 it wasn't a problem because of https://github.com/civicrm/civicrm-packages/blob/45b39026def71038e58b5330e37879d9b64ece8a/Smarty/Smarty.class.php#L1936 and a hack of ours: https://github.com/civicrm/civicrm-packages/blob/45b39026def71038e58b5330e37879d9b64ece8a/Smarty/Smarty.class.php#L1680

Separate, we may want to revisit #27583 (which is where the string issues first came up and has since been updated to avoid that, but which have a different playing field now).