fix(backends): require an int ttl and delta before any backend I/O - #262
Merged
Merged
Conversation
validate_ttl accepted floats and bools. On Redis, increment(ttl=1.5) created the counter before EXPIRE failed, leaving a counter that never expires behind a misleading "not a counter" error. Only an int up to MAX_TTL (2**31 - 1) is accepted now, and increment checks delta the same way. Memcached rejects expiries past 2038-01-19, which it used to drop silently, and maps only non-numeric values to "not a counter". @cache checks ttl at decoration time. Closes #229
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #229.
Problem
validate_ttl()only rejected values<= 0, so non-integer TTLs slipped through and each backend handled them differently:Truewas taken as one second.1.5worked on the memory backend only.10**400passed validation.The worst case, reproduced against a live Redis:
increment("c", ttl=1.5)on a missing key ranINCRBYbeforeEXPIREfailed. Redis does not roll back a failed script, so the counter stayed at 1 with no TTL (TTL= -1), a permanent lockout for a rate limiter. The error said the key was "not a counter", because the Redis error text ("value is not an integer or out of range") matched the mapping for a non-counter value. Memcached gave the same misleading message.While reproducing this I found three related problems, also fixed here:
set(ttl=11 years)is stored, whileset(ttl=12 years)succeeds and the item is already gone.delta(e.g.2**64,1.5) also reached the server and was reported as "not a counter" on both Redis and Memcached.@cache(ttl=1.5)would fail in the backend on every request, and fix(cache): serve uncached instead of 500 when the backend fails #259's fail-open would hide that the route never caches.Fix
validate_ttlacceptsNoneor anintfrom 1 to the newMAX_TTL = 2**31 - 1(about 68 years).float,booland other types raiseTypeError.ValueError.INCRBY.validate_deltarequires anintin the signed 64-bit range. It is called byincrementon every backend and the base fallback._expiry()raisesValueErrorfor an expiry past 2038-01-19.incrementconverts the TTL before any I/O.MemcacheClientErrors about non-numeric values map to "not a counter"; other client errors propagate unchanged.@cacheraisesCacheXErrorat decoration time for a non-intttlor one aboveMAX_TTL, next to the existing negative check.timedeltais not accepted. That would widen everyttl: int | Nonesignature, so the docs say to useint(td.total_seconds()).Compatibility
A float TTL that used to work on the memory backend now raises
TypeError. On Redis and Memcached it already failed. Internal callers already passint: the session manager's TTL isint(...).Tests
tests/backends/test_ttl_contract.py: the bad-TTL table now covers float, integral float, bool, str,MAX_TTL + 1and10**400(besides 0 and -1) on every backend,CacheManagerandStateManager, all before any I/O. Newtest_backends_reject_invalid_delta.test_redis_increment_with_a_float_ttl_leaves_no_counter.test_expiry_up_to_2038_is_sent_as_a_timestamp(boundary);test_ttl_past_2038_is_rejected_before_io[set|set_if_absent|increment];test_memcached_increment_only_maps_non_numeric_errors_to_not_a_counter.tests/test_cache.py:test_invalid_ttl_is_rejected_at_decoration[float|bool|too-large].Mutation checks, run with the old backends plus only the
MAX_TTLconstant:non-numericguard fails only its test.@cachecheck fails the three new decoration cases.The full suite passes against live Redis and Memcached: 983 passed, 99.96% coverage. Both docs builds pass with
--strict.Docs
incrementbullet documentsdelta;### Securityentry.