Skip to content

[Security] Use instanceof NullToken in voters #17141

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
Closed
l-vo wants to merge 1 commit into from
Closed

[Security] Use instanceof NullToken in voters #17141

l-vo wants to merge 1 commit into from

Conversation

@l-vo
Copy link
Contributor

@l-vo l-vo commented Aug 10, 2022

To test if the user is not logged.

@carsonbot carsonbot added this to the 6.0 milestone Aug 10, 2022
@l-vo l-vo force-pushed the use_null_token_comparision_in_voters branch from 4d375e2 to 1d8564d Compare August 10, 2022 12:49
@l-vo l-vo requested a review from xabbuh as a code owner August 10, 2022 12:49
@javiereguiluz javiereguiluz requested a review from wouterj August 10, 2022 13:39
@carsonbot carsonbot changed the title Use instanceof NullToken in voters [Security] Use instanceof NullToken in voters Aug 11, 2022
OskarStark
OskarStark reviewed Sep 5, 2022
View reviewed changes
OskarStark
OskarStark reviewed Sep 5, 2022
View reviewed changes
OskarStark
OskarStark reviewed Sep 5, 2022
View reviewed changes
@l-vo l-vo force-pushed the use_null_token_comparision_in_voters branch from 1d8564d to 38a9ee1 Compare September 5, 2022 15:28
@l-vo
Copy link
Contributor Author

l-vo commented Sep 5, 2022

@OskarStark changes applied, thank you :)

OskarStark
OskarStark reviewed Sep 7, 2022
View reviewed changes
@l-vo
Use instanceof NullToken in voters
5b148a3 
To test if the user is not logged.
@l-vo l-vo force-pushed the use_null_token_comparision_in_voters branch from 38a9ee1 to 5b148a3 Compare September 8, 2022 16:37
@javiereguiluz
Copy link
Member

javiereguiluz commented Oct 4, 2022

Sorry to ping you again @chalasr but could you please review if this security-related proposal is correct? Thanks.

@OskarStark OskarStark changed the title [Security] Use instanceof NullToken in voters [Security] Use instanceof NullToken in voters Oct 4, 2022
@chalasr
Copy link
Member

chalasr commented Oct 4, 2022

(No worry @javiereguiluz, don't hesitate!)

I'm not totally sure about this change. Technically, the current code is correct as it covers the NullToken case as well as any eventual "unauthenticated" custom token (ref symfony/symfony#42650).
With that in mind and given "use NullToken but only in voters" makes it way more complicated, I think it's better to keep the example it as-is.

Having @wouterj's point of view would be good though.

@l-vo
Copy link
Contributor Author

l-vo commented Oct 4, 2022

Indeed... Actually I'm not sure about my change anymore 😁

@javiereguiluz
Copy link
Member

javiereguiluz commented Oct 5, 2022

OK, let's close this then. Thank you all for the reviews 🙏

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

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

@wouterj wouterj Awaiting requested review from wouterj

Assignees

No one assigned

Labels

Security Status: Needs Review

Projects

None yet

Milestone

6.0

Development

Successfully merging this pull request may close these issues.

5 participants

@l-vo @javiereguiluz @chalasr @OskarStark @carsonbot