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

[Mail] Renderer email content twig templates in a sandbox #13347

Merged
merged 16 commits into from Oct 27, 2022

Conversation

dvesh3
Copy link
Collaborator

@dvesh3 dvesh3 commented Oct 11, 2022

Changes in this pull request

Mail twig templates should be rendered in a sandbox with restricted Twig security policy.
Resolves https://github.com/pimcore/planning/issues/102

Additional info

please see https://twig.symfony.com/doc/2.x/api.html#sandbox-extension

@dvesh3 dvesh3 added the Bug label Oct 11, 2022
@dvesh3 dvesh3 added this to the 10.5.8 milestone Oct 11, 2022
@github-actions
Copy link

github-actions bot commented Oct 11, 2022

Review Checklist

  • Target branch (10.5 for bug fixes, others 11.x)
  • Tests (if it's testable code, there should be a test for it - get help)
  • Docs (every functionality needs to be documented, see here)
  • Migration incl. install.sql (e.g. if the database schema changes, ...)
  • Upgrade notes (deprecations, important information, migration hints, ...)
  • Label
  • Milestone

lib/Mail.php Outdated Show resolved Hide resolved
Copy link
Member

@fashxp fashxp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • I would suggest to extract the allowed tags, filters and functions to a Symfony config ... with quite restrictive default config values
  • Also consider other places like text layout component in data objects

@dvesh3 dvesh3 requested a review from fashxp October 11, 2022 14:56
@dvesh3 dvesh3 marked this pull request as ready for review October 11, 2022 14:57
Co-authored-by: Sebastian Blank <blank@data-factory.net>
Co-authored-by: Jacob Dreesen <j.dreesen@neusta.de>
@fashxp fashxp modified the milestones: 10.5.8, 10.5.9 Oct 12, 2022
Copy link
Member

@fashxp fashxp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should try to provide more reasonable error messages:

right now when opening a data object with a bad template I get
image

and when opening preview in class editor, we get just a 500 at all
image

@fashxp fashxp modified the milestones: 10.5.8, 10.5.9 Oct 12, 2022
@dvesh3
Copy link
Collaborator Author

dvesh3 commented Oct 14, 2022

we should try to provide more reasonable error messages:

@fashxp done by rethrowing a message with context in mailer and setting the error message to the text layout html.

@dvesh3 dvesh3 requested review from fashxp and brusch October 14, 2022 13:07
…tom security policy to whitelist object properties and methods execution by default #13347
Copy link
Member

@fashxp fashxp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me now.
don't forget to create a security advisory as stated here...


$policy = new SecurityPolicy($tags, $filters, $functions);
$sandbox = new SandboxExtension($policy);
$this->twig->addExtension($sandbox);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this dynamic approach could cause some issues in case the Twig environment was already initialized previously, then addExtension() will throw an exception.
I'd propose to register SandboxExtension as a service here:

Pimcore\Twig\Extension\DocumentHelperExtensions: ~

It will the get automatically registered as a Twig extension and here, we can just use enableSandbox() and disableSandbox() depending on how the Twig env was requested. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's a good catch. I registered it as a service now.

@dvesh3 dvesh3 requested a review from brusch October 27, 2022 11:31
Copy link
Member

@brusch brusch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@dvesh3 dvesh3 merged commit 43aa34e into 10.5 Oct 27, 2022
13 checks passed
@dvesh3 dvesh3 deleted the mail_twig_rendering_sandbox branch October 27, 2022 13:09
@blankse blankse mentioned this pull request Oct 28, 2022
dvesh3 added a commit that referenced this pull request Dec 20, 2022
* [Web2Print] Render twig templates in a sandbox - follow up to #13347

* Update doc/Development_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md

Co-authored-by: Jacob Dreesen <j.dreesen@neusta.de>

* Update doc/Development_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md

* Update doc/Development_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md

* Update doc/Development_Documentation/23_Installation_and_Upgrade/09_Upgrade_Notes/README.md

Co-authored-by: Sebastian Blank <blank@data-factory.net>

Co-authored-by: Jacob Dreesen <j.dreesen@neusta.de>
Co-authored-by: Sebastian Blank <blank@data-factory.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants