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

allow_extra_attributes does not throw an exception as documented #26534

Merged
merged 1 commit into from Jun 21, 2018

Conversation

Projects
None yet
6 participants
@deviantintegral
Contributor

deviantintegral commented Mar 14, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets none
License MIT
Doc PR none

The example at Deserializing an object does not actually work. It looks like this is a bug and not a docs issue. #24783 reported the same bug, but it looks like the fix at #24816 isn't complete.

Here's a failing test that copies the existing example.

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Mar 14, 2018

Hey @deviantintegral! Thanks for the report.

IIRC, this is not working because there is no metadata. allow_extra_attribute => false cannot work properly without metadata. Hence my fix was only fixing the case where the ObjectNormalizer was instantiated with a ClassMetadataFactoryInterface $classMetadataFactory argument.

Right now, I'm not sure if we can do something.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Mar 15, 2018

@dunglas

This comment has been minimized.

Member

dunglas commented Mar 15, 2018

We may throw if this flag is set but the metadata are not loaded. WDYT?

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Mar 15, 2018

Could be a solution indeed. Perhaps triggering a deprecation first and throwing on 5.0 if we fear this might break some apps (that would mean the flag wasn't working anyway and they didn't noticed, but an exception at runtime would be worse I think).

@deviantintegral: Would you like to work on this ?

@dunglas

This comment has been minimized.

Member

dunglas commented Mar 15, 2018

I would throw directly. Relying on a non-functional behavior like this one can lead to security vulnerabilities.

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Mar 15, 2018

But I don't see any security vulnerability here. If the flag is set without metadata factory, no ExtraAttributesException is raised but extra attributes will simply be ignored. This exception is only for convenience, no security purpose implied AFAIK.

Anyway, I'm fine with both.

@deviantintegral

This comment has been minimized.

Contributor

deviantintegral commented Mar 16, 2018

IIRC, this is not working because there is no metadata. allow_extra_attribute => false cannot work properly without metadata.

I'd expected that something was reflecting the target class, and looking for public properties or set methods. If neither of those existed for a given property in the data being deserialized, then an ExtraAttributesException would be thrown. I agree about the convenience factor; my aim was to use it only during early development to ensure that the data being deserialized matched the documentation provided to me.

Is there any reason why we couldn't instantiate a default metadata providing this behaviour, if allow_extra_attribute => false is set and no factory is provided?

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Mar 19, 2018

Status: needs work

@deviantintegral

This comment has been minimized.

Contributor

deviantintegral commented Mar 22, 2018

I see I need to add the legacy annotations or methods for exception tests. I will do that tomorrow.

One question; if I try to deserialize into a class that only has a private field specified, with no setter, it is not set, but no exception is thrown as it's still in the list of allowed attributes. Is there a decorator class I missed to do field access checks when reflecting the target class?

</person>
EOF;
$encoders = array(new XmlEncoder());

This comment has been minimized.

@ogizanagi

ogizanagi Mar 23, 2018

Member

No need for the xml encoder & serializer for this test. It's just about the object normalizer. See above one.

This comment has been minimized.

@deviantintegral

deviantintegral Mar 23, 2018

Contributor

Good point. I used the full ObjectNormalizer class as I think we need it's implementations to reasonably test this. Should it be copied in as a dummy class or left as is?

This comment has been minimized.

@ogizanagi

ogizanagi Mar 23, 2018

Member

This is fine with the ObjectNormalizer to me :)

// If additional attributes are not allowed, use a default class metadata factory to check against
// properties and methods.
if (isset($context[static::ALLOW_EXTRA_ATTRIBUTES]) && !$context[static::ALLOW_EXTRA_ATTRIBUTES]) {
$this->classMetadataFactory = new ClassMetadataFactory(new AnnotationLoader(new AnnotationReader()));

This comment has been minimized.

@ogizanagi

ogizanagi Mar 23, 2018

Member

An issue I see with this might be performances, as there is no cache on contrary of the CacheClassMetadataFactory used on production. If that's only for a convenience exception, it's a free penalty.

This comment has been minimized.

@deviantintegral

deviantintegral Mar 23, 2018

Contributor

We could default to an array cache, or we could leave it to the docs to say "if you are using allow_extra_attributes in production, consider caching the metadata factory". Thoughts?

This comment has been minimized.

@ogizanagi

ogizanagi Mar 23, 2018

Member

Array cache wouldn't be useful actually. ClassMetadataFactory already has its own in memory cache. So the performances impact might not be really significant in most cases actually.

This comment has been minimized.

@dunglas

dunglas Mar 26, 2018

Member

It's why I'm for throwing instead.
Everything else in the component works this way. If someone want to use this feature, it must register properly a metadata factory (and use a cached one as much as possible).

@deviantintegral

This comment has been minimized.

Contributor

deviantintegral commented Mar 26, 2018

Can someone rebuild the appveyor build? It threw Failed to decode response: zlib_decode(): data error 69 during composer update.

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Mar 26, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 14, 2018

ping @ogizanagi @dunglas, any decision about throwing or not?

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Apr 14, 2018

Alright, considering #26534 (comment), let's throw.
I still see no security implications though, so we're able to trigger a deprecation to ensure it doesn't breaks any app thinking they're using the flag properly without metadata factory, but no strong opinion.

The documentation should also benefit from a PR.

@dunglas

This comment has been minimized.

Member

dunglas commented Apr 16, 2018

Perfect.

@deviantintegral

This comment has been minimized.

Contributor

deviantintegral commented May 4, 2018

The appveyor failure is in the Process component, so I'm guessing it's unrelated to this PR. This should be ready for another review.

@fabpot

This comment has been minimized.

Member

fabpot commented May 30, 2018

Something went wrong here. @deviantintegral Can you rebase on current 3.4?

@ogizanagi ogizanagi removed the request for review from lyrixx May 30, 2018

@deviantintegral

This comment has been minimized.

Contributor

deviantintegral commented May 30, 2018

The test failures are from HttpKernel and are on the 3.4 branch too:

1) Symfony\Component\HttpKernel\Tests\Fragment\InlineFragmentRendererTest::testRenderWithTrustedHeaderDisabled
Expectation failed for method name is equal to "handle" when invoked 1 time(s)
Parameter 0 for invocation Symfony\Component\HttpKernel\HttpKernelInterface::handle(Symfony\Component\HttpFoundation\Request Object (...), 2, false) does not match expected value.
Failed asserting that two objects are equal.
--- Expected
+++ Actual
@@ @@
-            'REMOTE_ADDR' => '1.1.1.1'
+            'REMOTE_ADDR' => '127.0.0.1'
@ogizanagi

@deviantintegral : Could you also submit a PR to the symfony-docs repository?
Would be great IMHO to add a note block to hint about this.

@@ -205,6 +205,10 @@ protected function handleCircularReference($object)
protected function getAllowedAttributes($classOrObject, array $context, $attributesAsString = false)
{
if (!$this->classMetadataFactory) {
if (isset($context[static::ALLOW_EXTRA_ATTRIBUTES]) && !$context[static::ALLOW_EXTRA_ATTRIBUTES]) {
throw new LogicException('A class metadata factory must be provided in the constructor when setting ALLOW_EXTRA_ATTRIBUTES to false.');

This comment has been minimized.

@ogizanagi

ogizanagi May 30, 2018

Member

Actually should rather use the ALLOW_EXTRA_ATTRIBUTE const value here I think.

This comment has been minimized.

@deviantintegral

deviantintegral Jun 21, 2018

Contributor

Where is that constant defined? I don't see that anywhere in the project, at least on the 3.4 and 4.1 branches.

This comment has been minimized.

@ogizanagi

ogizanagi Jun 21, 2018

Member

I mean static::ALLOW_EXTRA_ATTRIBUTES. the one used in the isset above ^^

This comment has been minimized.

@deviantintegral

deviantintegral Jun 21, 2018

Contributor

Oh, I get it - the docs and so on don't use the class constant, but the actual lowercase string. I'll update.

This comment has been minimized.

@ogizanagi

ogizanagi Jun 21, 2018

Member

That's why I asked for using the const value. Here you're mentioning the constant in the error message :)

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 21, 2018

(rebase needed)

deviantintegral added a commit to deviantintegral/symfony-docs that referenced this pull request Jun 21, 2018

@deviantintegral

This comment has been minimized.

Contributor

deviantintegral commented Jun 21, 2018

Docs PR filed at symfony/symfony-docs#9948

@@ -208,7 +208,7 @@ protected function getAllowedAttributes($classOrObject, array $context, $attribu
{
if (!$this->classMetadataFactory) {
if (isset($context[static::ALLOW_EXTRA_ATTRIBUTES]) && !$context[static::ALLOW_EXTRA_ATTRIBUTES]) {
throw new LogicException('A class metadata factory must be provided in the constructor when setting ALLOW_EXTRA_ATTRIBUTES to false.');
throw new LogicException(sprintf('A class metadata factory must be provided in the constructor when setting %s to false.', static::ALLOW_EXTRA_ATTRIBUTES));

This comment has been minimized.

@ogizanagi

ogizanagi Jun 21, 2018

Member

Perhaps just wrapping the const value with double quotes

@ogizanagi

This comment has been minimized.

Member

ogizanagi commented Jun 21, 2018

@ogizanagi ogizanagi merged commit a67b650 into symfony:3.4 Jun 21, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

ogizanagi added a commit that referenced this pull request Jun 21, 2018

bug #26534 allow_extra_attributes does not throw an exception as docu…
…mented (deviantintegral)

This PR was squashed before being merged into the 3.4 branch (closes #26534).

Discussion
----------

allow_extra_attributes does not throw an exception as documented

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | none
| License       | MIT
| Doc PR        | none

The example at [Deserializing an object](https://symfony.com/doc/current/components/serializer.html#deserializing-an-object) does not actually work. It looks like this is a bug and not a docs issue. #24783 reported the same bug, but it looks like the fix at #24816 isn't complete.

Here's a failing test that copies the existing example.

Commits
-------

a67b650 allow_extra_attributes does not throw an exception as documented

This was referenced Jun 25, 2018

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jul 16, 2018

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Jul 16, 2018

minor #9948 Document the required ClassMetadataFactory (deviantintegr…
…al, javiereguiluz)

This PR was submitted for the master branch but it was merged into the 3.4 branch instead (closes #9948).

Discussion
----------

Document the required ClassMetadataFactory

Symfony PR at symfony/symfony#26534

Commits
-------

bc011c0 Reword
b05c1cc Codefence ClassMetadataFactory
a4cb67a Document the required ClassMetadataFactory
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment