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.

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

[LiveComponent] Remove CSRF tokens - rely on same-origin/CORS instead #2251

Merged
merged 1 commit into from
Oct 23, 2024
Merged

[LiveComponent] Remove CSRF tokens - rely on same-origin/CORS instead #2251

merged 1 commit into from
Oct 23, 2024

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Oct 9, 2024 •
edited by kbond
Loading

Q A
Bug fix? no
New feature? could be
Issues Fixes #2177
License MIT

This PR replaces token-based CSRF-protection by same-origin/CORS policies.
It replaces #2250

nicolas-grekas
nicolas-grekas commented Oct 9, 2024
View reviewed changes
src/LiveComponent/src/DependencyInjection/Compiler/OptionalDependencyPass.php

if (!$container->hasDefinition('test.client')) {
$container->getDefinition('ux.live_component.event_subscriber')
->setArgument(1, false);
Copy link
Member Author

@nicolas-grekas nicolas-grekas Oct 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

In test mode, we don't require the custom Accept content-type to make testing easier (and not break existing tests).

@nicolas-grekas nicolas-grekas mentioned this pull request Oct 9, 2024
Closed
@nicolas-grekas nicolas-grekas force-pushed the csrf-less branch 5 times, most recently from 799ae44 to 51bf1fb Compare October 9, 2024 11:24
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 9, 2024
Closed
@onEXHovia
Copy link
Contributor

Many developers, for various reasons(anwser stackoverflow), can add Access-Control-Allow-Origin: * header for all requests its loosens Single Origin Policy (SOP) and allows you to execute request using XMLHttpRequest and setting Accept header. Classic way with hidden input, seems like more effective or at least an additional layer against CSRF.

@smnandre smnandre mentioned this pull request Oct 11, 2024
Closed
@nicolas-grekas
Copy link
Member Author

@onEXHovia thanks for the comment but I'm not sure what you're suggesting for this PR?

smnandre added a commit to smnandre/ux that referenced this pull request Oct 12, 2024
@smnandre
[Autocomplete] Remove unused "symfony/security-csrf" dev dependency
6445355 
Spotted by @nicolas-grekas in symfony#2251
@smnandre smnandre mentioned this pull request Oct 12, 2024
Merged
@onEXHovia
Copy link
Contributor

@onEXHovia thanks for the comment but I'm not sure what you're suggesting for this PR?

If we consider this implementation and current with hidden token, I see two different scenarios. Without documentation developers may not know that live component depends on SOP in js for protections CSRF and added header Access-Control-Allow-Origin could mean potential CSRF.

On the other hand, implementation with token is more explicit. Developer disable it through the component configuration and understands that could mean potential CSRF.

Discussion in #2250

Enabling CSRF by default means enabling the session, making requests stateful.

CSRF protection is not a "feature", nobody is "using it" in the sense that it's a behavior that shouldn't have any visible effect to any normal workflow.

The same can be said about stateless requests, usually no one tries to make all requests stateless. I don't see any reason to remove the token based implementation, as a solution we could add a global configuration that configures the CSRF behavior for all components.

javiereguiluz added a commit that referenced this pull request Oct 14, 2024
@javiereguiluz
minor #2264 [Autocomplete] Remove unused "symfony/security-csrf" dev …
d159cdd 
…dependency (smnandre)

This PR was merged into the 2.x branch.

Discussion
----------

[Autocomplete] Remove unused "symfony/security-csrf" dev dependency

Spotted by `@nicolas`-grekas in #2251

Commits
-------

6445355 [Autocomplete] Remove unused "symfony/security-csrf" dev dependency
@WebMamba WebMamba mentioned this pull request Oct 20, 2024
Open
kbond
kbond approved these changes Oct 23, 2024
View reviewed changes
Copy link
Member

@kbond kbond left a comment

Choose a reason for hiding this comment

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

This is really nice! Ryan and I always wanted the possibility for stateless live components but csrf was our Achilles heel!

@carsonbot carsonbot added Status: Reviewed Has been reviewed by a maintainer and removed Status: Needs Review Needs to be reviewed labels Oct 23, 2024
@daFish
Copy link
Contributor

daFish commented Oct 23, 2024

This MR helps with #2177 and fixes the issue. My attempt to fix it in #2254 is no longer needed.

@nicolas-grekas
Copy link
Member Author

I've just added a note about CSRF / CORS in the doc.

@smnandre
Copy link
Member

LGTM :)

@nicolas-grekas can you update the CHANGELOG in the src/LiveComponent repository ?

@nicolas-grekas
Copy link
Member Author

CHANGELOG updated

@kbond
Copy link
Member

kbond commented Oct 23, 2024

Thank you Nicolas.

@kbond kbond merged commit 45f6504 into symfony:2.x Oct 23, 2024
60 checks passed
@nicolas-grekas nicolas-grekas deleted the csrf-less branch October 23, 2024 18:43
@WebMamba WebMamba mentioned this pull request Dec 19, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kbond kbond kbond approved these changes

@smnandre smnandre smnandre approved these changes

Assignees
No one assigned
Labels
LiveComponent Status: Reviewed Has been reviewed by a maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[LiveComponent] Testing forms causes exeption "There is currently no session available."
6 participants
@nicolas-grekas @onEXHovia @daFish @smnandre @kbond @carsonbot