Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optional defaults for SecurityPolicy #3902

Open
wants to merge 2 commits into
base: 2.x
Choose a base branch
from

Conversation

YSaxon
Copy link
Contributor

@YSaxon YSaxon commented Oct 30, 2023

This is an implementation of Defaults for the Twig Security Policy.

The idea is that there should be a set of known safe values that a user can whitelist. The goals for the pull request were:

  • It should be easy to use the defaults
  • It should be easy to NOT use the defaults
  • It should be easy to include all the values in the defaults plus some of your own. I've seen implementations of defaults where you need to take it or leave it; if you like the defaults but want to add some extra values in your project, you have no choice but to reject the defaults and just copy them into your own policy.

I've implemented this via a special value or "token", meant to be referred to as SecurityPolicyDefaults::INCLUDE_DEFAULTS, which, if included as a value in any of the whitelist config arguments, will be replaced and merged with the actual hardcoded default values.

So for instance if you want to trust all of the default filters, plus your own custom filters format_number and format_currency and 'format_currency', you can initialize your SecurityPolicy with $allowedFilters = ['format_number', 'format_currency', SecurityPolicyDefaults::INCLUDE_DEFAULTS], and the actual allowedFilters will end up as ['format_number', 'format_currency','abs','batch','capitalize', 'date', 'date_modify', ...].

Note that I haven't done extensive research for the present list of defaults. They are intended as a proof-of-concept sample for the purpose of the pull request, and should definitely be reviewed thoroughly before actually accepting and merging.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant