Skip to content

Conversation

nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Oct 8, 2024

Q A
Bug fix? no
New feature? no
Issues -
License MIT

CSRF protection doesn't need a CSRF token manager: because the listener checks for a custom content-type, and because setting a custom content-type can only be done using JS, receiving a request with the expected content-type already means that the request was sent in compliance with the same-origin policy (or it's broader version: CORS).

Enabling CSRF by default means enabling the session, making requests stateful.
This PR makes it easier to be stateless by default (when generating the component at least).

I wondered about removing all the CSRF-related stuff in this bundle but thought this would be a too bold move for now :)

@nicolas-grekas
Copy link
Member Author

Help welcome for tests ;) (once you confirm this makes sense to you)

@smnandre
Copy link
Member

smnandre commented Oct 9, 2024

As i see it, either we say "CSRF checks for live component had .."

  • zero interest, so let's remove them everywhere / for everyone
  • sometimes, and then we cannot change the default like that (as it would silentely remove them even for people currently usng them)

Or am i missing something here ?

@smnandre
Copy link
Member

smnandre commented Oct 9, 2024

.... so this is because the CRSF 7.2 recent changes have a side effect: they enable "crsf" for everyone right ?

@nicolas-grekas
Copy link
Member Author

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.
As said, in the description, I'm challenging the need for any token-based CSRF protection at all.

Then, the motivation for this PR is not related to that other PR on Symfony. The motivation is that we were trying to make a page stateless (to achieve "cache-controle: public"), and we discovered that a live component we were using for a search widget what using the session, just to store CSRF tokens.

Statefulness is something we should prevent by default IMHO. It's orthogonal to HTTP, so we'd better align the defaults with the protocol.

@nicolas-grekas
Copy link
Member Author

Here is the more involving patch I had in mind: #2251
Closing here as I don't see the need to keep the current CSRF infrastructure.

@nicolas-grekas nicolas-grekas deleted the cors-csrf branch October 9, 2024 11:09
kbond added a commit that referenced this pull request Oct 23, 2024
…n/CORS instead (nicolas-grekas)

This PR was merged into the 2.x branch.

Discussion
----------

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

| 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

Commits
-------

e3c0ef5 [LiveComponent] Remove CSRF tokens - rely on same-origin/CORS instead
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants