rest_framework.throttling sets invalid key for memcached.

[amrael]

I stumbled upon an error, "error 9 from memcached_set: SUCCESS" and it turned out that DRF creates invalid key for throttling such as :1:throttle_anon_153.191.xxx.xxx, 107.178.xxx.xxx. memcached cannot have keys containing space so I assume this is a bug.

[jpadilla]

@amrael what are your values for HTTP_X_FORWARDED_FOR and REMOTE_ADDR? It seems that this should be taking care already, but I'm seeing there could exist a couple of use cases that might not be covered.

[amrael]

@jpadilla

X-Forwarded-For: 153.191.xxx.xxx, 107.178.xxx.xxx
REMOTE_ADDR: 127.0.0.1

These are the headers of the troublesome request in question. It seems the X-Forwarded-For header is used for the key as-is.

[jpadilla]

@amrael It seems X-Forwarded-For is used as is if the NUM_PROXIES setting is set to None(it's default value). The offending code is in L184. It's using get_ident() instead of the local ident variable which would have fixed the issue. I'm submitting a fix that takes care of it in BaseThrottle.get_ident().

For now, you could either set NUM_PROXIES to an integer 0 or greater, or implement a custom throttle class.

[amrael]

@jpadilla Thanks for the commit.
But I just discovered more serious problem by reading the code. Setting NUM_PROXIES can result in IndexError thanks to this line, L35

client_addr = addrs[-min(num_proxies, len(xff))]

It's measuring the length of request.META.get('HTTP_X_FORWARDED_FOR') instead of the number of items in addrs. So IndexError will constantly be raised if I specify NUM_PROXIES to greater than one.

[tomchristie]

Indeed. That should surely be:

client_addr = addrs[-min(num_proxies, len(addrs))]

[tomchristie]

Closed by #2401.