Placeholder injection mitigation unsafe placeholders

CHANGELOG.md

@@ -1,9 +1,5 @@
 # CHANGELOG
 
-## 100.0.0
-
-* `notification_type` is now a required argument of `clients.redis.daily_limit_cache_key` (apps have already been updated)
-
 ## 99.8.0
 
 * Add new version of GOV.UK brand to email template, behind a flag

notifications_utils/clients/redis/__init__.py

@@ -3,11 +3,11 @@
 from .request_cache import RequestCache  # noqa: F401 (unused import)
 
 
-def daily_limit_cache_key(service_id, notification_type):
+def daily_limit_cache_key(service_id, notification_type=None):
     yyyy_mm_dd = datetime.utcnow().strftime("%Y-%m-%d")
 
     if not notification_type:
-        raise TypeError("notification_type is required")
+        return f"{service_id}-{yyyy_mm_dd}-count"
 
     return f"{service_id}-{notification_type}-{yyyy_mm_dd}-count"
 

notifications_utils/field.py

@@ -1,4 +1,5 @@
 import re
+from enum import StrEnum, auto
 
 from markupsafe import Markup
 from ordered_set import OrderedSet
@@ -7,6 +8,7 @@
     escape_html,
     strip_and_remove_obscure_whitespace,
     unescaped_formatted_list,
+    url,
 )
 from notifications_utils.insensitive_dict import InsensitiveDict
 
@@ -16,17 +18,46 @@ def __init__(self, body):
         # body shouldn’t include leading/trailing brackets, like (( and ))
         self.body = body.lstrip("(").rstrip(")")
 
+    class Types(StrEnum):
+        BASE = auto()
+        MAKE_SAFE = auto()
+        CONDITIONAL = auto()
+
+    # order matters as the placeholder type will be the first type that returns a match
+    # conditional shoud come first because no escaping should happen on conditional
+    # placeholders as they don't contain vulnerable input
+    extended_type_pattern = {
+        Types.CONDITIONAL: re.compile(r".*\?\?.*"),
+        Types.MAKE_SAFE: re.compile(r".*::make_safe$"),
+    }
+
+    @property
+    def type(self):
+        for type, pattern in self.extended_type_pattern.items():
+            if re.match(pattern, self.body):
+                return type
+        return self.Types.BASE
+
     @classmethod
     def from_match(cls, match):
         return cls(match.group(0))
 
     def is_conditional(self):
-        return "??" in self.body
+        return self.type == self.Types.CONDITIONAL
+
+    def is_make_safe(self):
+        return self.type == self.Types.MAKE_SAFE
 
     @property
     def name(self):
         # for non conditionals, name equals body
-        return self.body.split("??")[0]
+        match self.type:
+            case self.Types.BASE:
+                return self.body
+            case self.Types.MAKE_SAFE:
+                return self.body.split("::make_safe")[0]
+            case self.Types.CONDITIONAL:
+                return self.body.split("??")[0]
 
     @property
     def conditional_text(self):
@@ -69,6 +100,7 @@ class Field:
     conditional_placeholder_tag = "<span class='placeholder-conditional'>&#40;&#40;{}??</span>{}&#41;&#41;"
     placeholder_tag_no_brackets = "<span class='placeholder-no-brackets'>{}</span>"
     placeholder_tag_redacted = "<span class='placeholder-redacted'>hidden</span>"
+    placeholder_tag_make_safe = "<span class='placeholder-unsafe'>&#40;&#40;{}</span>::make_safe&#41;&#41;"
 
     def __init__(
         self,
@@ -119,6 +151,9 @@ def format_placeholder(self, placeholder):
         if placeholder.is_conditional():
             return self.conditional_placeholder_tag.format(placeholder.name, placeholder.conditional_text)
 
+        if placeholder.is_make_safe():
+            return self.placeholder_tag_make_safe.format(placeholder.name)
+
         return self.placeholder_tag.format(placeholder.name)
 
     def replace_match(self, match):
@@ -131,6 +166,19 @@ def replace_match(self, match):
         if placeholder.is_conditional():
             return placeholder.get_conditional_body(replacement)
 
+        if placeholder.is_make_safe():
+            return self.sanitise_replacement_make_safe(replacement)
+
+        return replacement
+
+    def sanitise_replacement_make_safe(self, replacement: str):
+        # if the replacement contains a link consider it all compromised
+        if re.search(url, replacement):
+            return ""
+        # escape markdown-specific characters
+        markdown_characters = r"`*_(){}[]<>#+-.!|"
+        for character in markdown_characters:
+            replacement = replacement.replace(character, f"\\{character}")
         return replacement
 
     def get_replacement(self, placeholder):
@@ -181,6 +229,7 @@ class PlainTextField(Field):
     conditional_placeholder_tag = "(({}??{}))"
     placeholder_tag_no_brackets = "{}"
     placeholder_tag_redacted = "[hidden]"
+    placeholder_tag_make_safe = "(({}::make_safe))"
 
 
 def str2bool(value):

tests/clients/test_redis.py

@@ -10,12 +10,8 @@
 @pytest.mark.parametrize(
     "kwargs, expected_cache_key",
     (
-        pytest.param({}, "{sample_service.id}-2016-01-01-count", marks=pytest.mark.xfail(raises=TypeError)),
-        pytest.param(
-            {"notification_type": None},
-            "{sample_service.id}-2016-01-01-count",
-            marks=pytest.mark.xfail(raises=TypeError),
-        ),
+        ({}, "{sample_service.id}-2016-01-01-count"),
+        ({"notification_type": None}, "{sample_service.id}-2016-01-01-count"),
         ({"notification_type": "sms"}, "{sample_service.id}-sms-2016-01-01-count"),
         ({"notification_type": "letter"}, "{sample_service.id}-letter-2016-01-01-count"),
         ({"notification_type": "email"}, "{sample_service.id}-email-2016-01-01-count"),

tests/test_field.py

@@ -83,9 +83,21 @@ def test_returns_a_string_without_placeholders(content):
             {"placeholder": {"key": "value"}},
             "before {'key': 'value'} after",
         ),
-        ("((warning?))", {"warning?": "This is not a conditional"}, "This is not a conditional"),
-        ("((warning?warning))", {"warning?warning": "This is not a conditional"}, "This is not a conditional"),
-        ("((warning??This is a conditional warning))", {"warning": True}, "This is a conditional warning"),
+        (
+            "((warning?))",
+            {"warning?": "This is not a conditional"},
+            "This is not a conditional",
+        ),
+        (
+            "((warning?warning))",
+            {"warning?warning": "This is not a conditional"},
+            "This is not a conditional",
+        ),
+        (
+            "((warning??This is a conditional warning))",
+            {"warning": True},
+            "This is a conditional warning",
+        ),
         (
             "((warning??This is a conditional warning\nwith line break))",
             {"warning": True},
@@ -124,7 +136,11 @@ def test_replacement_of_placeholders(field_class, template_content, data, expect
 @pytest.mark.parametrize(
     "template_content,data,expected",
     [
-        ("((code)) is your security code", {"code": "12345"}, "12345 is your security code"),
+        (
+            "((code)) is your security code",
+            {"code": "12345"},
+            "12345 is your security code",
+        ),
         (
             "((code)) is your security code",
             {},
@@ -149,7 +165,10 @@ def test_optional_redacting_of_missing_values(template_content, data, expected):
     "content,expected",
     [
         ("((colour))", "<span class='placeholder'>&#40;&#40;colour&#41;&#41;</span>"),
-        ("the quick ((colour)) fox", "the quick <span class='placeholder'>&#40;&#40;colour&#41;&#41;</span> fox"),
+        (
+            "the quick ((colour)) fox",
+            "the quick <span class='placeholder'>&#40;&#40;colour&#41;&#41;</span> fox",
+        ),
         (
             "((article)) quick ((colour)) ((animal))",
             "<span class='placeholder'>&#40;&#40;article&#41;&#41;</span> quick <span class='placeholder'>&#40;&#40;colour&#41;&#41;</span> <span class='placeholder'>&#40;&#40;animal&#41;&#41;</span>",  # noqa
@@ -166,8 +185,14 @@ def test_optional_redacting_of_missing_values(template_content, data, expected):
                 <span class='placeholder'>&#40;&#40;animal&#41;&#41;</span>
             """,
         ),
-        ("the quick (((colour))) fox", "the quick (<span class='placeholder'>&#40;&#40;colour&#41;&#41;</span>) fox"),
-        ("((warning?))", "<span class='placeholder'>&#40;&#40;warning?&#41;&#41;</span>"),
+        (
+            "the quick (((colour))) fox",
+            "the quick (<span class='placeholder'>&#40;&#40;colour&#41;&#41;</span>) fox",
+        ),
+        (
+            "((warning?))",
+            "<span class='placeholder'>&#40;&#40;warning?&#41;&#41;</span>",
+        ),
         (
             "((warning? This is not a conditional))",
             "<span class='placeholder'>&#40;&#40;warning? This is not a conditional&#41;&#41;</span>",
@@ -184,6 +209,10 @@ def test_optional_redacting_of_missing_values(template_content, data, expected):
             "((warning?? This warning is ?? questionable))",
             "<span class='placeholder-conditional'>&#40;&#40;warning??</span> This warning is ?? questionable&#41;&#41;",  # noqa
         ),
+        (
+            "Unsafe placeholder: ((name::make_safe))",
+            "Unsafe placeholder: <span class='placeholder-unsafe'>&#40;&#40;name</span>::make_safe&#41;&#41;",
+        ),
     ],
 )
 def test_formatting_of_placeholders(content, expected):
@@ -214,6 +243,40 @@ def test_handling_of_missing_values(content, values, expected):
     assert str(Field(content, values)) == expected
 
 
+@pytest.mark.parametrize(
+    "content, values, expected",
+    [
+        (
+            "My name is ((name))",
+            {"name": "Geoff()[]{}"},
+            "My name is Geoff()[]{}",
+        ),
+        (
+            "My name is ((name::make_safe))",
+            {"name": "Geoff()[]{}"},
+            r"My name is Geoff\(\)\[\]\{\}",
+        ),
+        (
+            "Escaped link: ((link::make_safe))",
+            {"link": "https://www.google.com"},
+            "Escaped link: ",
+        ),
+        (
+            "My name is ((name::make_safefoobar))",
+            {"name::make_safefoobar": "Geoff"},
+            "My name is Geoff",
+        ),
+        (
+            "My name is ((show_name??name::unsafefoobar))",
+            {"show_name": True},
+            "My name is name::unsafefoobar",
+        ),
+    ],
+)
+def test_placeholder_types_render_as_expected(content, values, expected):
+    assert str(Field(content, values)) == expected
+
+
 @pytest.mark.parametrize(
     "value",
     [