Admin: sandbox jinja2 templates in emails

[functionzz]

This PR removes the ability for users with admin access to conduct RCE via jinja2 templates within Pontoon onboarding and user inactivity emails. It uses SandboxedEnvironment to prevent access to dangerous commands.

Fixes #3985.

Note: access to settings has been restricted to INACTIVE_CONTRIBUTOR_PERIOD, INACTIVE_TRANSLATOR_PERIOD and INACTIVE_MANAGER_PERIOD. Therefore, we must subsequently edit the onboarding and inactivity emails in the PROD and DEV dbs to reflect that, or else we risk crashing Pontoon.

Note 2: The above note expanded: all template tags in PROD and DEV must be of the format {placeholder}. full_url is no longer used within email templates, and jinja templating brackets ({{}}) are replaced with single brackets ({}).

[mathjazz]

I'm afraid this is not the right fix for #3985, which sadly has a misleading title.

The sandbox reduces risk but doesn't eliminate it, and it does so at the cost of ongoing complexity and trust in a security boundary that has already failed before. Eliminating the execution model entirely is a fundamentally stronger position.

Considering we still want editors to write template-like content in the DB, we should use a logic-less engine that has no concept of code execution.

We should probably be good with using str.format_map():

body = template_str.format_map({
    "INACTIVE_CONTRIBUTOR_PERIOD": settings.INACTIVE_CONTRIBUTOR_PERIOD,
    "homepage_url": request.build_absolute_uri(reverse("pontoon.homepage")),,
})

Alternatively, we could look into something more advanced like Chevron.

[functionzz]

I see what you mean - we want to eliminate SandboxedEnvironment entirely?

[mathjazz]

Yeah.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 56.63717% with 49 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.84%. Comparing base (0766832) to head (629ca29).

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[functionzz]

Just spent the last couple of hours debugging this - apparently the removal of jinja2 SandboxedEnvironment initialization in messaging somehow affects the way request.context was used for a specific test I wrote for #2133, which in turn was a flawed test because using request.context in such a way does not sit well with Jinja2, but was frailly working until the messaging related Jinja2 environment was removed.

I'm including the test change in this PR as its small and makes more sense to explain it here rather than merge seperately.

[mathjazz]

This moves in the right direction by removing Jinja execution for DB email content. 👏

Note that format_map() still allows attribute access and currently receives a Django model instance. Please replace it as suggested.

We should also update the template files accordingly.

First in the repo:
https://github.com/functionzz/pontoon/tree/eb266b7b8a870461e8e7baecdafba7932ce6720a/pontoon/messaging/templates/messaging/emails/content

Then in the DB.

[mathjazz]

Are these changes related to the PR?

[functionzz]

Yes, but in an indirect way. This test was failing due to reasons I posted about above, I'm including it in this PR so the test passes and there is a paper trail.

[mathjazz]

I just tested locally without this change and the tests are passing.

[functionzz]

The remote tests will not pass if the change is not applied. At least that's what I tested recently.

Just quoting myself because I didn't put this comment in the right place - If I push the old code it fails the pytest in CI.

[mathjazz]

I just tested locally without this change and the tests are passing.

[functionzz]

The remote tests will not pass if the change is not applied. At least that's what I tested recently.

[mathjazz]

The remote tests will not pass if the change is not applied. At least that's what I tested recently.

Could you please push? I'd like to look into it.

[mathjazz]

Could you please rebase against upstream/main?

[flodolo]

Like @mathjazz, this PR passes for me locally without any changes 🤔

[flodolo]

Correction after more ☕ : it fails locally as well, I was only running the emails test, not admin 😳

Looks like the removal of jinja_env = Jinja2.get_default().env is what triggered the failure in another place, so Jamie's original fix was probably the correct one?

[mathjazz]

Forgot to update this yesterday: The PR changes when the default Jinja backend is initialized, which seems to change what Django’s test client captures. so the fix here is to assert rendered behavior instead of Jinja internals.