Skip to content

[Security] check token in payload instead just request #57488

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

Merged
merged 1 commit into from
Jun 25, 2024

Conversation

eltharin
Copy link
Contributor

Q A
Branch? 7.1
Bug fix? yes
New feature? no
Deprecations? no
License MIT

Replace request by getPayload for allow token to ba passed in Json for an API call

@eltharin eltharin requested a review from chalasr as a code owner June 21, 2024 16:48
@carsonbot carsonbot added this to the 7.2 milestone Jun 21, 2024
@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "7.2" but it seems your PR description refers to branch "7.1".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@eltharin eltharin changed the base branch from 7.2 to 7.1 June 21, 2024 16:49
@xabbuh
Copy link
Member

xabbuh commented Jun 21, 2024

As this is not a bugfix this PR must target the ’7.2` branch.

@eltharin
Copy link
Contributor Author

If you look the documentation for 7.1 it's written the attribute is alternative to $submittedToken = $request->getPayload()->get('token'); but it's not really
So It's a bug not a new feature.
https://symfony.com/doc/current/security/csrf.html#generating-and-checking-csrf-tokens-manually

@xabbuh
Copy link
Member

xabbuh commented Jun 21, 2024

That’s a bug in the documentation (caused by symfony/symfony-docs#19225) and needs to be fixed there.

@eltharin
Copy link
Contributor Author

no need to fix the fake bug in documentation since the real bug is fixed. One moire time it's not a new feature

@carsonbot carsonbot changed the title check token in payload instead just request [Security] check token in payload instead just request Jun 21, 2024
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand both pov, strictly speaking this is a feature, but since this class has been introduced in 7.1 #52961 by @yguedidi, it might be fine being a bit more relaxed here, afterall this might just be something we missed. Bonus: that'd make the doc simpler :)

@fabpot fabpot modified the milestones: 7.2, 7.1 Jun 25, 2024
@fabpot
Copy link
Member

fabpot commented Jun 25, 2024

Thank you @eltharin.

@fabpot fabpot merged commit c9c16e9 into symfony:7.1 Jun 25, 2024
5 of 10 checks passed
@fabpot fabpot mentioned this pull request Jun 28, 2024
@eltharin eltharin deleted the AllowPayload branch July 25, 2024 11:55
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

6 participants