Cache key contains reserved characters aka IPv6 support

[drzraf]

I'm using the following RateLimit for the Route of one of my controllers:
@RateLimit(methods={"PUT", "POST"}, limit=35, period=3600)

and use the following storage backend:

## config/services.yaml
    rate_limit_cache_storage:
        class: Symfony\Component\Cache\Simple\FilesystemCache
        arguments: ['ratelimit', 0, '%kernel.cache_dir%']

## config/packages/noxlogic_rate_limit.yaml
noxlogic_rate_limit:
    storage_engine: simple_cache
    simple_cache_service: rate_limit_cache_storage
    rate_response_code: 429
    rate_response_message: 'You exceeded the rate limit'

Sadly, once I started getting IPv6 flowing my controller started failing badly:

Uncaught PHP Exception Symfony\Component\Cache\Exception\InvalidArgumentException:
"Cache key "PUT.POST.api_foo.2800:xxx:yyyy:zzzz:d141:zzzz:xxx:yyyy" contains reserved characters "{}()/\@:"." at
vendor/symfony/cache/CacheItem.php line 177
{"exception":"[object] (Symfony\\Component\\Cache\\Exception\\InvalidArgumentException(code: 0): Cache key \"PUT.POST.api_foo.2800:xxx:yyyy:zzzz:d141:zzzz:xxx:yyyy\" contains reserved characters \"{}()/\\@:\". at vendor/symfony/cache/CacheItem.php:177)"} []

This is really bad because:

1. This basically kept requests from fullfiling even though such an exceptions must have failed safely (A failure of the RateLimit is not something that must block normal processing!).
2. I've no (simple) way to create filesystem-compatible cache-key for IPv6 without rolling a custom cache-key generator.

[drzraf]

Friendly ping.
It's a serious issue and although it's does not show up under the default configuration, it's likely to have grave consequence for affected users.

[jaytaph]

I think we can sanitize the key first. We have to check if this can cause any issues (I think not though).

Also, I agree we should it should fail without error.

[jaytaph]

Fixed the key issue by sanitizing any keys that will be send to redis.

Still need to fix the safe failure

[drzraf]

Go for #116
Curious about your solution for failsafe.

[drzraf]

@jaytaph : Do you think we should open another issue about this?

[drzraf]

Note: About the changelog. The issue wasn't so much about Redis but about Symfony simple_cache restriction. (My backend was a Unix filesystem which handle colons)

[jaytaph]

I see now that this is indeed not directly a Redis issue, but a symfony/cacheItem. This means the premise for this bugfix seems wrong.

I would suggest the following:

• check if we still need the redis sanitization and revert back the redis sanitizationif not needed.

Further, since we support simple_cache driver which in itself uses a Psr\SimpleCache\CacheInterface. By itself it doesn't know or does anything with keys. In your case, it's the symfony implementation that restricts the key characters from Symfony\Contracts\Cache\ItemInterface::RESERVED_CHARACTERS.

I'm not sure if sanitization then needs to be in the rate limiter, or in this case you would need to have a wrapper around it that does the sanitation for it. I can imagine we would run into issues when other implementation would use other reserved characters, which we would in turn need to sanitize as well.

@drzraf @goetas maybe you have an idea about this?

[drzraf]

I think handling this in this bundle makes sense since forbidding characters (what symfony/cacheItem does) is different from substituting characters (what symfony/cacheItem may not do).
The bundle knows how safe it is to sanitize and it could be done easily as it's been for Redis in the PR.
I'm sorry I didn't ensured, while reading #116, the sanitization applied to all backends.

[drzraf]

ping