[Security] don't do nested calls to serialize() #30006

renanbr · 2019-01-28T11:21:36Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#29951
License	MIT
Doc PR	n/a

The problem (originally reported as Symfony\Component\Security\Core\Authentication\Token\AbstractToken issue), may occur also in classes extending Symfony\Component\Security\Core\Exception\AuthenticationException

Tasks:

Skip native serializer (workaround itself)
Token test
Exception test

src/Symfony/Component/Security/Core/Exception/CustomUserMessageAuthenticationException.php

nicolas-grekas

I think this is the best approach we can achieve for 3.4.
For master, we should deprecate extending serialize/unserialize and provide another API that returns/accepts arrays instead.

nicolas-grekas · 2019-01-28T16:46:51Z

@stof I'd be happy to have your 👍 here :)

stof · 2019-01-28T16:52:19Z

@nicolas-grekas Would this @param bool $isCalledFromOverridingMethod cause issue with the DebugClassLoader when overriding the method in a child class (asking people to add it in their own class), or is the deprecation based on magic parameters restricted to interfaces ?

stof · 2019-01-28T16:55:39Z

For master, we should deprecate extending serialize/unserialize and provide another API that returns/accepts arrays instead.

I suggest looking at what the PHP RFC for __unserialize/__serialize would do for that. If it ends up being accepted, we could implement them, and provide a BC layer based on Serializable calling these methods internally, marking our Serializable as @final (so that child classes override __unserialize/__serialize instead, which would be necessary for PHP 7.4 anyway if the methods get called natively). What do you think @nicolas-grekas

nicolas-grekas · 2019-01-28T18:11:36Z

issue with the DebugClassLoader when overriding the method in a child class

That's correct, DebugClassLoader 4.2 will trigger a deprecation notice.
That might be desired as it can hint ppl to change their code in a place where they might have a bug (esp. if they extend their own token...) - but we can also decide we should not trigger the notice. Any preference?

For master:

I suggest looking at what the PHP RFC for __unserialize/__serialize would do for that. If it ends up being accepted, we could implement them, and provide a BC layer based on Serializable calling these methods internally, marking our Serializable as @Final

I thought of a slightly different plan: we should mark serialize/unserialize @final and @internal to deprecate extending them (from core) and using them (for users). Then we should provide alternative methods to basically do what the proposed __unserialize/__serialize methods do, but in a way that would be decoupled from the underlying PHP mechanism that we decide to use. That would be the most flexible in the long run IMHO.

stof · 2019-01-28T18:15:31Z

That might be desired as it can hint ppl to change their code in a place where they might have a bug (esp. if they extend their own token...) - but we can also decide we should not trigger the notice. Any preference?

But they probably cannot add an actual parameter there. We don't even expect them to do it (we expect them to add it when doing the parent call, but even that is optional as we handle the case where it is not done, so we may not even need that param)

nicolas-grekas · 2019-01-28T18:17:13Z

So, we remove the annotation and we plan for master. Works for me. OK for you with my proposal for master?

stof · 2019-01-28T18:18:37Z

So in your plan, switching to __unserialize/__serialize would also be done by marking them as final, and still telling people to override the protected methods we expose ? Fine with me.

nicolas-grekas · 2019-01-28T19:11:27Z

Correct.

@renanbr could you please remove the added docblocks?

nicolas-grekas · 2019-01-29T09:07:16Z

Thank you @renanbr.

…rekas, Renan) This PR was merged into the 3.4 branch. Discussion ---------- [Security] don't do nested calls to serialize() | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #29951 | License | MIT | Doc PR | n/a The problem (originally reported as `Symfony\Component\Security\Core\Authentication\Token\AbstractToken` issue), may occur also in classes extending `Symfony\Component\Security\Core\Exception\AuthenticationException` Tasks: - [x] Skip native serializer (workaround itself) - [x] Token test - [x] Exception test Commits ------- 10256fc skip native serialize among child and parent serializable objects 41000f1 [Security] dont do nested calls to serialize()

guilliamxavier · 2019-01-30T13:55:52Z

src/Symfony/Component/Security/Core/Exception/CustomUserMessageAuthenticationException.php

-        ]);
+        return serialize([parent::serialize(true), $this->messageKey, $this->messageData]);
+
+        return $this->doSerialize($serialized, \func_num_args() ? \func_get_arg(0) : null);


Problem: double return!

(seeing the others changes, I guess the first line should be $serialized = [...];)

thanks @guilliamxavier , see #30044

…icolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [Security\Guard] bump lowest version of security-core | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - Forgotten in #30006 so that `PostAuthenticationGuardToken` can call `AbstractToken::doSerialize()`. Commits ------- 93cfd5b [Security\Guard] bump lowest version of security-core

[Security] dont do nested calls to serialize()

41000f1

carsonbot added Status: Needs Review Security Bug labels Jan 28, 2019

nicolas-grekas added this to the 3.4 milestone Jan 28, 2019

nicolas-grekas reviewed Jan 28, 2019

View reviewed changes

src/Symfony/Component/Security/Core/Exception/CustomUserMessageAuthenticationException.php Outdated Show resolved Hide resolved

nicolas-grekas mentioned this pull request Jan 28, 2019

Error in PostAuthenticationGuardToken::serialize because of bug in PHP 7.3 #29951

Closed

nicolas-grekas approved these changes Jan 28, 2019

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 28, 2019

chalasr approved these changes Jan 28, 2019

View reviewed changes

skip native serialize among child and parent serializable objects

10256fc

nicolas-grekas merged commit 10256fc into symfony:3.4 Jan 29, 2019

fabpot mentioned this pull request Jan 29, 2019

Release v4.1.11 #30016

Merged

renanbr deleted the serialize-workaround-broadly branch January 30, 2019 12:49

guilliamxavier reviewed Jan 30, 2019

View reviewed changes

renanbr restored the serialize-workaround-broadly branch January 30, 2019 15:11

renanbr deleted the serialize-workaround-broadly branch January 30, 2019 16:59

This was referenced Feb 3, 2019

Release v3.4.22 #30067

Merged

Release v4.2.3 #30068

Merged

jmalinens mentioned this pull request Feb 4, 2019

Symfony 3.4.22 session not compatible with 3.4.21 #30073

Closed

nanasess mentioned this pull request Feb 5, 2019

Add PHP7.3 to Travis EC-CUBE/ec-cube#4045

Merged

6 tasks

nicolas-grekas mentioned this pull request Feb 17, 2019

[Security\Guard] bump lowest version of security-core #30282

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] don't do nested calls to serialize() #30006

[Security] don't do nested calls to serialize() #30006

renanbr commented Jan 28, 2019 •

edited

nicolas-grekas left a comment

nicolas-grekas commented Jan 28, 2019

stof commented Jan 28, 2019

stof commented Jan 28, 2019

nicolas-grekas commented Jan 28, 2019 •

edited

stof commented Jan 28, 2019

nicolas-grekas commented Jan 28, 2019

stof commented Jan 28, 2019

nicolas-grekas commented Jan 28, 2019 •

edited

nicolas-grekas commented Jan 29, 2019

guilliamxavier Jan 30, 2019

renanbr Jan 30, 2019

[Security] don't do nested calls to serialize() #30006

[Security] don't do nested calls to serialize() #30006

Conversation

renanbr commented Jan 28, 2019 • edited

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Jan 28, 2019

stof commented Jan 28, 2019

stof commented Jan 28, 2019

nicolas-grekas commented Jan 28, 2019 • edited

stof commented Jan 28, 2019

nicolas-grekas commented Jan 28, 2019

stof commented Jan 28, 2019

nicolas-grekas commented Jan 28, 2019 • edited

nicolas-grekas commented Jan 29, 2019

guilliamxavier Jan 30, 2019

Choose a reason for hiding this comment

renanbr Jan 30, 2019

Choose a reason for hiding this comment

renanbr commented Jan 28, 2019 •

edited

nicolas-grekas commented Jan 28, 2019 •

edited

nicolas-grekas commented Jan 28, 2019 •

edited