Skip to content

[Serializer] Prevent GetSetMethodNormalizer from creating invalid magic method call #48233

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 13, 2022
Merged

[Serializer] Prevent GetSetMethodNormalizer from creating invalid magic method call #48233

merged 1 commit into from
Dec 13, 2022

Conversation

klaussilveira
Copy link
Contributor

@klaussilveira klaussilveira commented Nov 17, 2022 •
edited by nicolas-grekas
Loading

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

When serializing an object that has an isser for a property, but not a getter, and the object happens to have a __call magic method, the GetSetMethodNormalizer will attempt to call the magic method with the attribute. This may cause unexpected behavior, or, a fatal. It depends on the __call implementation.

This undefined behavior is caused by the use of is_callable on getAttributeValue, which will return true since there is a __call implementation. The correct behavior can be achieved by using method_exists instead. A test case has been added that illustrates the issue.

@carsonbot carsonbot added this to the 6.2 milestone Nov 17, 2022
@OskarStark OskarStark changed the title [Serializer] Prevent GetSetMethodNormalizer from creating invalid magic method call [Serializer] Prevent GetSetMethodNormalizer from creating invalid magic method call Nov 18, 2022
@xabbuh
Copy link
Member

xabbuh commented Nov 20, 2022

Should the is_callable() call in setAttributeValue() also be changed?

@xabbuh xabbuh modified the milestones: 6.2, 4.4 Nov 20, 2022
@klaussilveira
Copy link
Contributor Author

@xabbuh denormalization does not seem to be affected.

@OskarStark
Copy link
Contributor

@xabbuh denormalization does not seem to be affected.

Doe we have a test case for this? Otherwise it could be helpful to add one in this PR

@klaussilveira
Copy link
Contributor Author

I was wrong! Good catch @xabbuh. Managed to reproduce it, but had to take a different approach: method_exists does not see protected/private methods, which are expected to be set by the Normalizer during denormalization. So I'm keeping is_callable, but catching the exception when Reflection fails to recognize the method existence.

@fabpot fabpot modified the milestones: 4.4, 5.4 Nov 23, 2022
@nicolas-grekas nicolas-grekas changed the base branch from 6.3 to 5.4 December 13, 2022 17:21
@nicolas-grekas
Copy link
Member

Thank you @klaussilveira.

@nicolas-grekas nicolas-grekas merged commit 5ec600e into symfony:5.4 Dec 13, 2022
@fabpot fabpot mentioned this pull request Dec 16, 2022
Merged
This was referenced Dec 28, 2022
Merged
Merged
Merged
@xabbuh
Copy link
Member

xabbuh commented Jan 14, 2023 •
edited
Loading

Managed to reproduce it, but had to take a different approach: method_exists does not see protected/private methods, which are expected to be set by the Normalizer during denormalization. So I'm keeping is_callable, but catching the exception when Reflection fails to recognize the method existence.

@klaussilveira Sorry for being so late with my review, but I do not really see when or why a private or protected setter should be called. Do you have an example in mind?

@xabbuh xabbuh mentioned this pull request Jan 14, 2023
Merged
fabpot added a commit that referenced this pull request Jan 14, 2023
@fabpot
bug #48978 [Serializer] use method_exists() instead of catching refle…
0096175 
…ction exceptions (xabbuh)

This PR was merged into the 5.4 branch.

Discussion
----------

[Serializer] use method_exists() instead of catching reflection exceptions

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       | #48233 (comment)
| License       | MIT
| Doc PR        |

Commits
-------

0e883a9 use method_exists() instead of catching reflection exceptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark left review comments

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

@lyrixx lyrixx Awaiting requested review from lyrixx

@yceruto yceruto Awaiting requested review from yceruto

@wouterj wouterj Awaiting requested review from wouterj

@chalasr chalasr Awaiting requested review from chalasr

@xabbuh xabbuh Awaiting requested review from xabbuh

Assignees
No one assigned
Labels
Bug Serializer Status: Needs Review
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.
6 participants
@klaussilveira @xabbuh @OskarStark @nicolas-grekas @fabpot @carsonbot