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

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

Merged
merged 2 commits into from Jan 29, 2019

Conversation

Projects
None yet
6 participants
@renanbr
Copy link

renanbr commented Jan 28, 2019

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
@nicolas-grekas
Copy link
Member

nicolas-grekas left a comment

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.

@renanbr renanbr force-pushed the renanbr:serialize-workaround-broadly branch from 3c7c7e3 to d11cc26 Jan 28, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 28, 2019

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

@stof

This comment has been minimized.

Copy link
Member

stof commented Jan 28, 2019

@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

This comment has been minimized.

Copy link
Member

stof commented Jan 28, 2019

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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 28, 2019

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

This comment has been minimized.

Copy link
Member

stof commented Jan 28, 2019

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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 28, 2019

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

@stof

This comment has been minimized.

Copy link
Member

stof commented Jan 28, 2019

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

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 28, 2019

Correct.

@renanbr could you please remove the added docblocks?

@nicolas-grekas nicolas-grekas force-pushed the renanbr:serialize-workaround-broadly branch from d11cc26 to 10256fc Jan 29, 2019

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 29, 2019

Thank you @renanbr.

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

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

nicolas-grekas added a commit that referenced this pull request Jan 29, 2019

bug #30006 [Security] don't do nested calls to serialize() (nicolas-g…
…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()

@fabpot fabpot referenced this pull request Jan 29, 2019

Merged

Release v4.1.11 #30016

@renanbr renanbr deleted the renanbr:serialize-workaround-broadly branch Jan 30, 2019

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

This comment has been minimized.

@guilliamxavier

guilliamxavier Jan 30, 2019

Contributor

Problem: double return!

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

This comment has been minimized.

@renanbr

renanbr Jan 30, 2019

Author

thanks @guilliamxavier , see #30044

@renanbr renanbr restored the renanbr:serialize-workaround-broadly branch Jan 30, 2019

@renanbr renanbr deleted the renanbr:serialize-workaround-broadly branch Jan 30, 2019

This was referenced Feb 3, 2019

@nanasess nanasess referenced this pull request Feb 5, 2019

Open

[WIP] Add PHP7.3 to Travis #4045

6 of 6 tasks complete

chalasr added a commit that referenced this pull request Feb 18, 2019

minor #30282 [Security\Guard] bump lowest version of security-core (n…
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment