✅ [#135] Make cookie parsing and serialization more robust

Handle some edge cases around malformed cookie strings that may
cause crashes. While our own library is not suspected to generate
invalid cookie strings, there are ways to bypass input validation
of the models and even the cookie value may be set by javascript
or people running fuzzers, leading to the cookie value being
untrusted input that must be sanitized properly.

Author: sergei-maertens

.gitignore

@@ -22,6 +22,7 @@ dist/
 htmlcov/
 reports/
 testapp/*.db
+.hypothesis
 
 # frontend tooling / builds
 js/node_modules/

cookie_consent/util.py

@@ -1,24 +1,64 @@
 # -*- coding: utf-8 -*-
 import datetime
-from typing import Union
+import logging
+from typing import Dict, Union
 
 from .cache import all_cookie_groups, get_cookie, get_cookie_group
 from .conf import settings
 from .models import ACTION_ACCEPTED, ACTION_DECLINED, LogItem
 
+logger = logging.getLogger(__name__)
 
-def parse_cookie_str(cookie):
-    dic = {}
+COOKIE_GROUP_SEP = "|"
+KEY_VALUE_SEP = "="
+
+
+def parse_cookie_str(cookie: str) -> Dict[str, str]:
     if not cookie:
-        return dic
-    for c in cookie.split("|"):
-        key, value = c.split("=")
-        dic[key] = value
-    return dic
+        return {}
+
+    bits = cookie.split(COOKIE_GROUP_SEP)
+
+    def _gen_pairs():
+        for possible_pair in bits:
+            parts = possible_pair.split(KEY_VALUE_SEP)
+            if len(parts) == 2:
+                yield parts
+            else:
+                logger.debug("cookie_value_discarded", extra={"value": possible_pair})
+
+    return dict(_gen_pairs())
+
+
+def _contains_invalid_characters(*inputs: str) -> bool:
+    # = and | are special separators. They are unexpected characters in both
+    # keys and values.
+    for separator in (COOKIE_GROUP_SEP, KEY_VALUE_SEP):
+        for value in inputs:
+            if separator in value:
+                logger.debug("skip_separator", extra={"value": value, "sep": separator})
+                return True
+    return False
+
+
+def dict_to_cookie_str(dic) -> str:
+    """
+    Serialize a dictionary of cookie-group metadata to a string.
+
+    The result is stored in a cookie itself. Note that the dictionary keys are expected
+    to be cookie group ``varname`` fields, which are validated against a slug regex. The
+    values are supposed to be ISO-8601 timestamps.
+
+    Invalid key/value pairs are dropped.
+    """
 
+    def _gen_pairs():
+        for key, value in dic.items():
+            if _contains_invalid_characters(key, value):
+                continue
+            yield f"{key}={value}"
 
-def dict_to_cookie_str(dic):
-    return "|".join(["%s=%s" % (k, v) for k, v in dic.items() if v])
+    return "|".join(_gen_pairs())
 
 
 def get_cookie_dict_from_request(request):
@@ -171,6 +211,7 @@ def is_cookie_consent_enabled(request):
         return enabled
 
 
+# Deprecated
 def get_cookie_string(cookie_dic):
     """
     Returns cookie in format suitable for use in javascript.

pyproject.toml

@@ -48,6 +48,7 @@ tests = [
     "pytest",
     "pytest-django",
     "pytest-playwright",
+    "hypothesis",
     "tox",
     "isort",
     "black",

tests/test_util.py

@@ -1,10 +1,13 @@
 # -*- coding: utf-8 -*-
 from datetime import datetime
 
+from django.http import parse_cookie
 from django.test import TestCase
 from django.test.client import RequestFactory
 from django.test.utils import override_settings
 
+from hypothesis import example, given, strategies as st
+
 from cookie_consent.conf import settings
 from cookie_consent.models import Cookie, CookieGroup
 from cookie_consent.util import (
@@ -127,3 +130,25 @@ def test_get_accepted_cookies(self):
         self.request.COOKIES[settings.COOKIE_CONSENT_NAME] = cookie_str
         cookies = get_accepted_cookies(self.request)
         self.assertIn(self.cookie, cookies)
+
+
+@example({"": "|"})
+@example({"": "="})
+@given(
+    cookie_dict=st.dictionaries(
+        keys=st.text(min_size=0),
+        values=st.text(min_size=0),
+    )
+)
+def test_serialize_and_parse_cookie_str(cookie_dict):
+    serialized = dict_to_cookie_str(cookie_dict)
+    parsed = parse_cookie_str(serialized)
+
+    assert len(parsed.keys()) <= len(cookie_dict.keys())
+
+
+@given(cookie_str=st.text(min_size=0))
+def test_parse_cookie_str(cookie_str: str):
+    parsed = parse_cookie_str(cookie_str)
+
+    assert isinstance(parsed, dict)