Merge commit from fork

andres-torres-marroquin · web-flow · commit 829cbd9cb5ef · 2024-09-03T17:51:13.000+01:00
* Added security for email templates that used Jinja2

* Added security for email templates that used Jinja2

* Added security for email templates that used Jinja2

* QA to migration

* Update CHANGELOG.md

* Update src/fides/api/alembic/migrations/versions/ff81f8e7d6ae_migrate_messaging_template_to_use_new_.py

* QA

* Update migrations
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -38,6 +38,9 @@ The types of changes are:
 - TCF Optimized for performance on initial load by offsetting most experience data until after banner is shown [#5230](https://github.com/ethyca/fides/pull/5230)
 - Updates to support DynamoDB schema with Tokenless IAM auth [#5240](https://github.com/ethyca/fides/pull/5240)
 
+### Security
+- Removed Jinja2 for email templates, the variables syntax changed from `{{variable_name}}` to `__VARIABLE_NAME__` [CVE-2024-45053](https://github.com/ethyca/fides/security/advisories/GHSA-c34r-238x-f7qx)
+
 ### Developer Experience
 - Sourcemaps are now working for fides-js in debug mode [#5222](https://github.com/ethyca/fides/pull/5222)
 
diff --git a/src/fides/api/alembic/migrations/versions/eef4477c37d0_changes_the_email_template_variables_.py b/src/fides/api/alembic/migrations/versions/eef4477c37d0_changes_the_email_template_variables_.py
@@ -0,0 +1,47 @@
+"""Changes the email template variables syntax from {{variable_name}} to __VARIABLE_NAME__.
+
+Revision ID: eef4477c37d0
+Revises: cc37edf20859
+Create Date: 2024-09-03 12:47:54.708196
+
+"""
+from alembic import op
+
+
+# revision identifiers, used by Alembic.
+revision = 'eef4477c37d0'
+down_revision = 'cc37edf20859'
+branch_labels = None
+depends_on = None
+
+
+VARIABLES = ["minutes", "days", "denial_reason", "code", "download_link"]
+JSON_VARIABLES = ["subject", "body"]
+
+
+def upgrade():
+    for json_variable in JSON_VARIABLES:
+        for variable in VARIABLES:
+            statement = f"""
+            UPDATE messaging_template
+            SET content = jsonb_set(
+                content,
+                '{{{json_variable}}}',
+                to_jsonb(REPLACE(content ->> '{json_variable}', '{{{{{variable}}}}}', '__{variable.upper()}__'))
+            );
+            """
+            op.execute(statement)
+
+
+def downgrade():
+    for json_variable in JSON_VARIABLES:
+        for variable in VARIABLES:
+            statement = f"""
+            UPDATE messaging_template
+            SET content = jsonb_set(
+                content,
+                '{{{json_variable}}}',
+                to_jsonb(REPLACE(content ->> '{json_variable}', '__{variable.upper()}__', '{{{{{variable}}}}}'))
+            );
+            """
+            op.execute(statement)
diff --git a/src/fides/api/models/messaging_template.py b/src/fides/api/models/messaging_template.py
@@ -19,8 +19,8 @@
     MessagingActionType.SUBJECT_IDENTITY_VERIFICATION.value: {
         "label": "Subject identity verification",
         "content": {
-            "subject": "Your one-time code is {{code}}",
-            "body": "Your privacy request verification code is {{code}}. Please return to the Privacy Center and enter the code to continue. This code will expire in {{minutes}} minutes.",
+            "subject": "Your one-time code is __CODE__",
+            "body": "Your privacy request verification code is __CODE__. Please return to the Privacy Center and enter the code to continue. This code will expire in __MINUTES__ minutes.",
         },
     },
     MessagingActionType.PRIVACY_REQUEST_RECEIPT.value: {
@@ -41,14 +41,14 @@
         "label": "Privacy request denied",
         "content": {
             "subject": "Your privacy request has been denied",
-            "body": "Your privacy request has been denied. {{denial_reason}}.",
+            "body": "Your privacy request has been denied. __DENIAL_REASON__.",
         },
     },
     MessagingActionType.PRIVACY_REQUEST_COMPLETE_ACCESS.value: {
         "label": "Access request completed",
         "content": {
             "subject": "Your data is ready to be downloaded",
-            "body": "Your access request has been completed and can be downloaded at {{download_link}}. For security purposes, this secret link will expire in {{days}} days.",
+            "body": "Your access request has been completed and can be downloaded at __DOWNLOAD_LINK__. For security purposes, this secret link will expire in __DAYS__ days.",
         },
     },
     MessagingActionType.PRIVACY_REQUEST_COMPLETE_DELETION.value: {
diff --git a/src/fides/api/service/messaging/message_dispatch_service.py b/src/fides/api/service/messaging/message_dispatch_service.py
@@ -5,7 +5,6 @@
 
 import requests
 import sendgrid
-from jinja2 import Environment
 from loguru import logger
 from sendgrid.helpers.mail import Content, Email, Mail, Personalization, TemplateId, To
 from sqlalchemy.orm import Session
@@ -384,9 +383,11 @@ def _render(template_str: str, variables: Optional[Dict] = None) -> str:
     """Helper function to render a template string with the provided variables."""
     if variables is None:
         variables = {}
-    jinja_env = Environment()
-    template = jinja_env.from_string(template_str)
-    return template.render(variables)
+
+    for key, value in variables.items():
+        template_str = template_str.replace(f"__{key.upper()}__", str(value))
+
+    return template_str
 
 
 def _build_email(  # pylint: disable=too-many-return-statements
diff --git a/tests/fixtures/application_fixtures.py b/tests/fixtures/application_fixtures.py
@@ -401,8 +401,8 @@ def property_b(db: Session) -> Generator:
 def messaging_template_with_property_disabled(db: Session, property_a) -> Generator:
     template_type = MessagingActionType.SUBJECT_IDENTITY_VERIFICATION.value
     content = {
-        "subject": "Here is your code {{code}}",
-        "body": "Use code {{code}} to verify your identity, you have {{minutes}} minutes!",
+        "subject": "Here is your code __CODE__",
+        "body": "Use code __CODE__ to verify your identity, you have __MINUTES__ minutes!",
     }
     data = {
         "content": content,
@@ -422,8 +422,8 @@ def messaging_template_with_property_disabled(db: Session, property_a) -> Genera
 def messaging_template_no_property_disabled(db: Session) -> Generator:
     template_type = MessagingActionType.SUBJECT_IDENTITY_VERIFICATION.value
     content = {
-        "subject": "Here is your code {{code}}",
-        "body": "Use code {{code}} to verify your identity, you have {{minutes}} minutes!",
+        "subject": "Here is your code __CODE__",
+        "body": "Use code __CODE__ to verify your identity, you have __MINUTES__ minutes!",
     }
     data = {
         "content": content,
@@ -443,8 +443,8 @@ def messaging_template_no_property_disabled(db: Session) -> Generator:
 def messaging_template_no_property(db: Session) -> Generator:
     template_type = MessagingActionType.SUBJECT_IDENTITY_VERIFICATION.value
     content = {
-        "subject": "Here is your code {{code}}",
-        "body": "Use code {{code}} to verify your identity, you have {{minutes}} minutes!",
+        "subject": "Here is your code __CODE__",
+        "body": "Use code __CODE__ to verify your identity, you have __MINUTES__ minutes!",
     }
     data = {
         "content": content,
@@ -466,8 +466,8 @@ def messaging_template_subject_identity_verification(
 ) -> Generator:
     template_type = MessagingActionType.SUBJECT_IDENTITY_VERIFICATION.value
     content = {
-        "subject": "Here is your code {{code}}",
-        "body": "Use code {{code}} to verify your identity, you have {{minutes}} minutes!",
+        "subject": "Here is your code __CODE__",
+        "body": "Use code __CODE__ to verify your identity, you have __MINUTES__ minutes!",
     }
     data = {
         "content": content,
diff --git a/tests/ops/api/v1/endpoints/test_messaging_endpoints.py b/tests/ops/api/v1/endpoints/test_messaging_endpoints.py
@@ -1944,8 +1944,8 @@ def payload(self) -> List[Dict[str, Any]]:
             {
                 "type": "subject_identity_verification",
                 "content": {
-                    "body": "Your privacy request verification code is {{code}}. Please return to the Privacy Center and enter the code to continue. You have {{minutes}} minutes.",
-                    "subject": "Your code is {{code}}",
+                    "body": "Your privacy request verification code is __CODE__. Please return to the Privacy Center and enter the code to continue. You have __MINUTES__ minutes.",
+                    "subject": "Your code is __CODE__",
                 },
             },
         ]
@@ -1979,8 +1979,8 @@ def test_put_messaging_templates(
                 {
                     "type": "subject_identity_verification",
                     "content": {
-                        "body": "Your privacy request verification code is {{code}}. Please return to the Privacy Center and enter the code to continue. You have {{minutes}} minutes.",
-                        "subject": "Your code is {{code}}",
+                        "body": "Your privacy request verification code is __CODE__. Please return to the Privacy Center and enter the code to continue. You have __MINUTES__ minutes.",
+                        "subject": "Your code is __CODE__",
                     },
                     "label": "Subject identity verification",
                 }
@@ -2015,8 +2015,8 @@ def test_put_messaging_templates_missing_values(
                 {
                     "type": "subject_identity_verification",
                     "content": {
-                        "body": "Your privacy request verification code is {{code}}. Please return to the Privacy Center and enter the code to continue. This code will expire in {{minutes}} minutes.",
-                        "subject": "Your one-time code is {{code}}",
+                        "body": "Your privacy request verification code is __CODE__. Please return to the Privacy Center and enter the code to continue. This code will expire in __MINUTES__ minutes.",
+                        "subject": "Your one-time code is __CODE__",
                     },
                     "label": "Subject identity verification",
                 }
@@ -2202,8 +2202,8 @@ def test_delete_messaging_template_by_id_success(
         # Creating new config, so we don't run into issues trying to clean up a deleted fixture
         template_type = MessagingActionType.SUBJECT_IDENTITY_VERIFICATION.value
         content = {
-            "subject": "Here is your code {{code}}",
-            "body": "Use code {{code}} to verify your identity, you have {{minutes}} minutes!",
+            "subject": "Here is your code __CODE__",
+            "body": "Use code __CODE__ to verify your identity, you have __MINUTES__ minutes!",
         }
         data = {
             "content": content,
diff --git a/tests/ops/service/messaging/message_dispatch_service_test.py b/tests/ops/service/messaging/message_dispatch_service_test.py
@@ -109,8 +109,8 @@ def test_email_dispatch_mailgun_success(
     Test scenario:
     ✅︎ Property-specific messaging is enabled
     ❌ No template configured for action type
-    
-    Result: Email is not sent. An explicit messaging template with matching action type is needed to send emails for 
+
+    Result: Email is not sent. An explicit messaging template with matching action type is needed to send emails for
     property-specific messaging
     """
 
@@ -140,7 +140,7 @@ def test_email_dispatch_property_specific_templates_enabled_no_template(
     Test scenario:
     ❌ Property-specific messaging is disabled
     ✅︎ Has template configured for action type
-    
+
     Result: Email sent the template configured with matching action type.
     """
 
@@ -178,7 +178,7 @@ def test_email_dispatch_property_specific_templates_disabled_with_template(
     Test scenario:
     ❌ Property-specific messaging is disabled
     ❌ No template configured for action type
-    
+
     Result: Email sent with default messaging template.
     """
 
@@ -217,7 +217,7 @@ def test_email_dispatch_property_specific_templates_disabled_no_template(
     ✅︎ Has template configured for action type
     ❌ No property id attached to template
     ❌ No property id in request
-    
+
     Result: Email not sent. There was no explicit property id linked to the template with matching action type.
     """
 
@@ -250,7 +250,7 @@ def test_email_dispatch_property_specific_templates_enabled_with_template_no_pro
     ✅︎ Has template configured for action type
     ✅︎ Default property id attached to template
     ❌ No property id in request
-    
+
     Result: Email sent using template linked to default property id. If no property id was received, we assume
     the default property id to look up the associated messaging template.
     """
@@ -294,7 +294,7 @@ def test_email_dispatch_property_specific_templates_enabled_with_template_has_pr
     ✅︎ Has template configured for action type
     ❌ No property attached to template
     ✅ Default property id in request
-    
+
     Result: Email not sent. There was no explicit property id linked to the template with matching action type.
     """
 
@@ -328,7 +328,7 @@ def test_email_dispatch_property_specific_templates_enabled_with_template_no_pro
    ✅︎ Has template configured for action type
    ✅ Property attached to template
    ✅ Matching property id in request
-   
+
    Result: Email sent using template with with property id
    """
 
diff --git a/tests/ops/service/messaging/test_message_template_render.py b/tests/ops/service/messaging/test_message_template_render.py
@@ -0,0 +1,64 @@
+import pytest
+
+from fides.api.service.messaging.message_dispatch_service import _render
+
+
+@pytest.mark.unit
+class TestMessageTemplateRender:
+
+    def test_template_render(self):
+        """
+        Test that a template is rendered correctly with the given variables.
+        """
+        template_str = """
+        Your privacy request has been denied.
+        __DENIAL_REASON__
+        """
+        variables = {
+            "denial_reason": "Accounts with an unpaid balance cannot be deleted."
+        }
+
+        expected_rendered_template = """
+        Your privacy request has been denied.
+        Accounts with an unpaid balance cannot be deleted.
+        """
+
+        rendered_template = _render(template_str, variables)
+        assert rendered_template == expected_rendered_template
+
+    def test_template_render_unsafe(self):
+        """
+        Test that a template with unsafe code is not rendered and raises a SecurityError.
+        """
+        template_str = """
+        Your privacy request has been denied.
+        *bb*
+        {% for s in ().__class__.__base__.__subclasses__() %}{% if "warning" in s.__name__ %}{{s()._module.__builtins__['__import__']('os').popen("env").read() }}{% endif %}
+        {% endfor %}
+        __CONFIG__
+        *aa*
+        """
+
+        expected_rendered_template = """
+        Your privacy request has been denied.
+        *bb*
+        {% for s in ().__class__.__base__.__subclasses__() %}{% if "warning" in s.__name__ %}{{s()._module.__builtins__['__import__']('os').popen("env").read() }}{% endif %}
+        {% endfor %}
+        123
+        *aa*
+        """
+
+        variables = {
+            "config": "123",
+        }
+
+        rendered_template = _render(template_str, variables)
+        assert rendered_template == expected_rendered_template
+
+        template_str = "your privacy request has been denied. __CONFIG.security.app_encryption_key__"
+        variables = {
+            "config": "123",
+        }
+
+        rendered_template = _render(template_str, variables)
+        assert rendered_template == template_str
diff --git a/tests/ops/service/messaging/test_messaging_crud_service.py b/tests/ops/service/messaging/test_messaging_crud_service.py
