Skip to content
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] Return default value instead of deferring to lower prio resolvers when using #[CurrentUser] and no user is found #49034

Merged
merged 1 commit into from
Jan 19, 2023

Conversation

Seldaek
Copy link
Member

@Seldaek Seldaek commented Jan 19, 2023

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

The issue here is that if you have an action with fooAction(string $email, #[CurrentUser] User $user = null) i.e. you want the current user if logged in but otherwise null, if there is no user logged in the EntityValueResolver gets called after UserValueResolver, and if you also have another param which would allow it to try to resolve a User it will do so as it is a mapped entity. But if it gives you a random user here instead of the logged user this is BAD :)

@carsonbot carsonbot added this to the 6.2 milestone Jan 19, 2023
@Seldaek Seldaek force-pushed the patch-31 branch 2 times, most recently from 3eb06be to 8cfe338 Compare January 19, 2023 13:16
@nicolas-grekas nicolas-grekas changed the title [Security] Return the default value instead of deferring to lower prio resolvers when UserValueResolver is nullable [Security] Return default value instead of deferring to lower prio resolvers when using #[CurrentUser] and no user is found Jan 19, 2023
…solvers when using #[CurrentUser] and no user is found
@nicolas-grekas
Copy link
Member

Thank you @Seldaek.

@nicolas-grekas nicolas-grekas merged commit ec73043 into symfony:6.2 Jan 19, 2023
@fabpot fabpot mentioned this pull request Jan 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants