Merge pull request #149 from django-commons/issue/135-crash-cookie-str-true

Make cookie parsing and serialization more robust

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)