8000 [Property Accessor] Error on reserved characters from CacheAdapter · Issue #29329 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content
8000

[Property Accessor] Error on reserved characters from CacheAdapter #29329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shavounet opened this issue Nov 26, 2018 · 3 comments
Closed

[Property Accessor] Error on reserved characters from CacheAdapter #29329

shavounet opened this issue Nov 26, 2018 · 3 comments

Comments

@shavounet
Copy link

Bug description: when using the built-in PropertyAccessor service, accessing an array property containing / in its key throws an error.

Example

  • in a standalone bin/test.php file, in a working Symfony instance
<?php
$loader = require __DIR__.'/../vendor/autoload.php';

$kernel = new AppKernel('dev', true);
$kernel->boot();

var_dump($kernel->getContainer()->get('property_accessor')->getValue(['a/b' => 'ok'], '[a/b]'));
  • execution and stack
$ php bin/test.php
PHP Fatal error:  Uncaught Symfony\Component\Cache\Exception\InvalidArgumentException: Cache key "p[a/b]" contains reserved characters {}()/\@: in /var/www/current/vendor/symfony/symfony/src/Symfony/Component/Cache/CacheItem.php:162
Stack trace:
#0 /var/www/current/vendor/symfony/symfony/src/Symfony/Component/Cache/Traits/ArrayTrait.php(45): Symfony\Component\Cache\CacheItem::validateKey('p[a/b]')
#1 /var/www/current/vendor/symfony/symfony/src/Symfony/Component/Cache/Adapter/ArrayAdapter.php(56): Symfony\Component\Cache\Adapter\ArrayAdapter->hasItem('p[a/b]')
#2 /var/www/current/vendor/symfony/symfony/src/Symfony/Component/PropertyAccess/PropertyAccessor.php(875): Symfony\Component\Cache\Adapter\ArrayAdapter->getItem('p[a/b]')
#3 /var/www/current/vendor/symfony/symfony/src/Symfony/Component/PropertyAccess/PropertyAccessor.php(160): Symfony\Component\PropertyAccess\PropertyAccessor->getPropertyPath('[a/b]')
#4 /var/www/current/bin/test.php(8): Symfony\Component\PropertyAccess\PropertyAccessor->getValue(Array, '[a/b]')
#5 {main} in /var/www/current/vendor/symfony/symfony/src/Symfony/Component/Cache/CacheItem.php on line 162

Analysis
The PropertyAccessor component uses a standard PSR6 cache to increase it's performance, but the key is validated by Symfony\Component\Cache\CacheItem::validateKey, and some characters are forbidden. The problem is that any of those characters are allowed as an array key (source). It means some properties can never be accessed through the property accessor.

As a quick workaround, we will disable the cache.

Side note: an empty array key is also not reachable (even if it's certainly a bad practice...)

Resolution proposals

  • Hash the key (but should it be protected against collisions with a mapping ?)
  • Catch the cache component error, add a warning and skip caching for those property (not ideal, but a quick and safe solution...)
  • Find a way to escape those characters

Context

  • Symfony 3.4
  • PHP 7.2
@nicolas-grekas
Copy link
Member

Thanks for reporting.
Closing as duplicate of #29293, fixed in #29313.

@shavounet
Copy link
Author

Wow that was quick :-)

@nicolas-grekas I'm looking at your fix, and I think there might be an issue... You're escaping the key only on character detection, not always, which implies that in an extremely rare circumstance (or as an exploit ?) we can have a cache collision...

$obj = new stdClass();
$obj->{"a/b"} = '1';
$obj->{"a%2Fb"} = '2';

var_dump($kernel->getContainer()->get('property_accessor')->getValue($obj, "a/b"));
var_dump($kernel->getContainer()->get('property_accessor')->getValue($obj, "a%2Fb"));

displays

/var/www/current/bin/test.php:12:
string(1) "1"
/var/www/current/bin/test.php:13:
string(1) "1"

instead of "1", then "2"

@nicolas-grekas
Copy link
Member

Agreed, fixed in #29332

nicolas-grekas added a commit that referenced this issue Nov 26, 2018
…as-grekas)

This PR was merged into the 3.4 branch.

Discussion
----------

[PropertyAccess] make cache keys encoding bijective

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29329 (comment)
| License       | MIT
| Doc PR        | -

Commits
-------

4fe566b [PropertyAccess] make cache keys encoding bijective
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants
0