Memcached key validation raises InvalidCacheKey with clunky message.

[Tim McCormack]

On Django 2.2.13 the code for memcache_key_warnings in django/core/cache/backends/base.py has a bad format string that results in raising an exception rather than just producing a warning. This can be reproduced with a memcached key with a space in it, e.g. "foo bar".

This code was present before the 2.2.13 release, but becomes more exposed with that release, since it begins validating cache keys.

I think it's as simple as removing the , CacheKeyWarning.

[Mariusz Felisiak]

Thanks for the report, we've already noticed this, see ​PR. It raises InvalidCacheKey for me (even if a message is not perfect), e.g.

File "django/core/cache/backends/memcached.py", line 157, in validate_key
    raise InvalidCacheKey(warning)

django.core.cache.backends.base.InvalidCacheKey: ("Cache key contains characters that will cause errors if used with memcached: '38_postgis_pull-requests-bionic_memcache:1:key with spaces and 清'", <class 'django.core.cache.backends.base.CacheKeyWarning'>)

[Carlton Gibson]

... results in raising an exception rather than just producing a warning.

The expected behaviour is to raise the InvalidCacheKey for memcached backends, addressing CVE-2020-13254.

Tim, can you post the traceback?

(How did you hit this?)

[Tim McCormack]

Ah, this was actually against the dummy backend. It was caught in tests: ​https://build.testeng.edx.org/job/edx-platform-python-pipeline-pr/18276/testReport/junit/common.djangoapps.xblock_django.tests.test_api/XBlockSupportTestCase/Run_Tests___lms_unit___test_disabled_blocks/

self = <xblock_django.tests.test_api.XBlockSupportTestCase testMethod=test_disabled_blocks>

    def setUp(self):
        super(XBlockSupportTestCase, self).setUp()
    
        # Set up XBlockConfigurations for disabled and deprecated states
        block_config = [
            ("poll", True, True),
            ("survey", False, True),
            ("done", True, False),
        ]
    
        for name, enabled, deprecated in block_config:
>           XBlockConfiguration(name=name, enabled=enabled, deprecated=deprecated).save()

common/djangoapps/xblock_django/tests/test_api.py:28: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../edx-venv-3.5/edx-venv/lib/python3.5/site-packages/config_models/models.py:110: in save
    update_fields
../edx-venv-3.5/edx-venv/lib/python3.5/site-packages/django/db/models/base.py:741: in save
    force_update=force_update, update_fields=update_fields)
../edx-venv-3.5/edx-venv/lib/python3.5/site-packages/django/db/models/base.py:790: in save_base
    update_fields=update_fields, raw=raw, using=using,
../edx-venv-3.5/edx-venv/lib/python3.5/site-packages/django/dispatch/dispatcher.py:175: in send
    for receiver in self._live_receivers(sender)
../edx-venv-3.5/edx-venv/lib/python3.5/site-packages/django/dispatch/dispatcher.py:175: in <listcomp>
    for receiver in self._live_receivers(sender)
openedx/core/lib/cache_utils.py:187: in invalidate
    TieredCache.delete_all_tiers(key)
../edx-venv-3.5/edx-venv/lib/python3.5/site-packages/edx_django_utils/cache/utils.py:226: in delete_all_tiers
    django_cache.delete(key)
../edx-venv-3.5/edx-venv/lib/python3.5/site-packages/django/core/cache/backends/dummy.py:30: in delete
    self.validate_key(key)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <django.core.cache.backends.dummy.DummyCache object at 0x7fc3211f7d30>
key = ":1:<class 'xblock_django.models.XBlockConfiguration'>.xblock_django.api.deprecated_xblocks"

    def validate_key(self, key):
        """
        Warn about keys that would not be portable to the memcached
        backend. This encourages (but does not force) writing backend-portable
        cache code.
        """
        for warning in memcache_key_warnings(key):
>           warnings.warn(warning, CacheKeyWarning)
E           TypeError: 'type' object cannot be interpreted as an integer

../edx-venv-3.5/edx-venv/lib/python3.5/site-packages/django/core/cache/backends/base.py:250: TypeError

This error only started happening in tests with the Django 2.2.12 -> 2.2.13 upgrade. I don't understand why we wouldn't have seen the InvalidCacheKey error in production, where we use memcached.

(We were in fact creating an invalid cache key, and I have a patch ready for that on our side that unblocks the upgrade, so I'm not sure how much we'll end up digging into that mystery!)

[Carlton Gibson]

Thanks for the follow up Tim.

I still don't get the error using 3.5...

Python 3.5.9 (default, Jun  4 2020, 16:47:18) 
[GCC 4.2.1 Compatible Apple LLVM 10.0.1 (clang-1001.0.46.4)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from django.core.cache.backends.base import CacheKeyWarning
>>> import warnings
>>> a_tuple = ('A string message, plus...', CacheKeyWarning)
>>> warnings.warn(a_tuple, CacheKeyWarning)
__main__:1: CacheKeyWarning: ('A string message, plus...', <class 'django.core.cache.backends.base.CacheKeyWarning'>)

There must be something else going on. Maybe a custom warning formatting?

(How else is the TypeError: 'type' object cannot be interpreted as an integer — the warning class being converted to an int? 🤔)

​The EdX change will stop the warning coming up, and is exactly the case that the patch was meant to catch.

Treating CacheKeyWarnings as errors would be good — I wonder how many folks would be seeing those and ignoring them... 😬

I think there's enough of a regression here to backport the fix.

[Tim McCormack]

I don't understand it either! I'll take a little more time to poke at it today. (For anyone who wants to reproduce this strange test error in place: Python 3.5 virtualenv, make requirements, pytest -s common/djangoapps/xblock_django/tests/test_api.py::XBlockSupportTestCase::test_authorable_blocks -- on commit 9f53525c.) The farthest I've gotten is that it's definitely coming from inside warnings.warn(...):

(Pdb) warnings.warn(("hi", str), RuntimeWarning)
*** TypeError: 'type' object cannot be interpreted as an integer

Why it doesn't happen on a fresh REPL with all dependencies loaded, I'm not sure. My best guess is that it has something to do with warning filtering, since that's configurable and appears to be what's inspecting the warning message. Probably not specifically relevant to Django or the edX code, though.

[Simon Charette]

Out of curiosity, do you get the same crash if passing category as a keyword argument? warnings.warn(warning, category=CacheKeyWarning)

[Mariusz Felisiak]

I can reproduce this error when unpacking tuple

>>> warnings.warn(*("hi", str), RuntimeWarning)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: 'type' object cannot be interpreted as an integer

because RuntimeWarning is passed as stacklevel, it must be somewhere in EdX. Nevertheless we will fix this message.

[Mariusz Felisiak]

​PR

[GitHub <noreply@…>]

In 926148e:

Fixed #31654 -- Fixed cache key validation messages.

[Mariusz Felisiak <felisiak.mariusz@…>]

In 031a082:

[3.1.x] Fixed #31654 -- Fixed cache key validation messages.

Backport of 926148ef019abcac3a9988c78734d9336d69f24e from master

[Mariusz Felisiak <felisiak.mariusz@…>]

In e8723af4:

[3.0.x] Fixed #31654 -- Fixed cache key validation messages.

Backport of 926148ef019abcac3a9988c78734d9336d69f24e from master

[Mariusz Felisiak <felisiak.mariusz@…>]

In b2b2723:

[2.2.x] Fixed #31654 -- Fixed cache key validation messages.

Backport of 926148ef019abcac3a9988c78734d9336d69f24e from master.

[Tim McCormack]

I get the error even with warnings.warn(message=("hi", dict), category=UserWarning). I suspect it depends on which warning filters are loaded in your environment.

[Chris Lamb]

Do you plan to re-release 3.0.7 and 2.2.13 to fix this, out of interest?

As an aside, I don't believe this was present before: See ​https://i.imgur.com/fZOegeI.jpg and ​https://i.imgur.com/hyc3k4J.jpg - you can see that the CacheKeyWarning was removed/added (it was factored out into eventual call to warnings.warn.