Opened 2 months ago

Closed 2 months ago

Last modified 2 months ago

#31654 closed Bug (fixed)

Memcached key validation raises InvalidCacheKey with clunky message.

Reported by: Tim McCormack Owned by: felixxm
Component: Core (Cache system) Version: 2.2
Severity: Release blocker Keywords: memcached
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Change History (14)

comment:1 Changed 2 months ago by felixxm

Owner: changed from nobody to felixxm
Status: newassigned
Summary: Memcached key validation raises TypeError on invalid charactersMemcached key validation raises InvalidCacheKey with clunky message.
Triage Stage: UnreviewedAccepted

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'>)

comment:2 Changed 2 months ago by 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?)

comment:3 Changed 2 months ago by 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!)

comment:4 Changed 2 months ago by Carlton Gibson

Severity: NormalRelease blocker

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.

comment:5 Changed 2 months ago by 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 with pip 20.0.2, 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.

(Edit: Specified pip version.)

Last edited 2 months ago by Tim McCormack (previous) (diff)

comment:6 Changed 2 months ago by Simon Charette

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

comment:7 Changed 2 months ago by felixxm

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.

comment:8 Changed 2 months ago by felixxm

Has patch: set

comment:9 Changed 2 months ago by GitHub <noreply@…>

Resolution: fixed
Status: assignedclosed

In 926148e:

Fixed #31654 -- Fixed cache key validation messages.

comment:10 Changed 2 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 031a082:

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

Backport of 926148ef019abcac3a9988c78734d9336d69f24e from master

comment:11 Changed 2 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In e8723af4:

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

Backport of 926148ef019abcac3a9988c78734d9336d69f24e from master

comment:12 Changed 2 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In b2b2723:

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

Backport of 926148ef019abcac3a9988c78734d9336d69f24e from master.

comment:13 Changed 2 months ago by 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.

comment:14 Changed 2 months ago by 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.

Note: See TracTickets for help on using tickets.
Back to Top