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

[FrameworkBundle] Automatically enable the CSRF protection if CSRF manager exists #25151

Merged
merged 1 commit into from
Nov 24, 2017

Conversation

sroze
Copy link
Contributor

@sroze sroze commented Nov 24, 2017

Q A
Branch? 3.4
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ø
License MIT

This will automatically enable the CSRF protection if CsrfTokenManagerInterface exists.

@sroze
Copy link
Contributor Author

sroze commented Nov 24, 2017

Travis failure is not related :)

@xabbuh xabbuh added this to the 3.4 milestone Nov 24, 2017
@fabpot
Copy link
Member

fabpot commented Nov 24, 2017

Thank you @sroze.

@fabpot fabpot merged commit fd43406 into symfony:3.4 Nov 24, 2017
fabpot added a commit that referenced this pull request Nov 24, 2017
… if CSRF manager exists (sroze)

This PR was merged into the 3.4 branch.

Discussion
----------

[FrameworkBundle] Automatically enable the CSRF protection if CSRF manager exists

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ø
| License       | MIT

This will automatically enable the CSRF protection if `CsrfTokenManagerInterface` exists.

Commits
-------

fd43406 Automatically enable the CSRF protection if CSRF manager exists
This was referenced Nov 30, 2017
fabpot added a commit that referenced this pull request Dec 14, 2017
This PR was merged into the 3.4 branch.

Discussion
----------

Fixing wrong class_exists on interface

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | none
| License       | MIT
| Doc PR        | symfony/symfony-docs#8873 already does not mention changing anything in the config

This was a bug introduced in #25151 on the 3.4 branch. It's... pretty self-explanatory I hope :).

Cheers!

Commits
-------

be75bd9 Fixing wrong class_exists on interface
@nicolas-grekas
Copy link
Member

nicolas-grekas commented Dec 14, 2017

Is this really a good idea?
All tests are broken now, unless we patch them to explicitly disable CSRF.
see https://travis-ci.org/symfony/symfony/jobs/316664330
Not a real issue on the tests side (except that someone needs to fix them actually),
but is this really the DX we want?
ping @weaverryan

@Tobion
Copy link
Member

Tobion commented Dec 15, 2017

Auto-enabling CSRF without it working because session are not enabled, sounds like a bad DX

LogicException: CSRF protection needs sessions to be enabled.

How about only enabling it when session is enabled as well? Or even better, we implement #13464

@fabpot
Copy link
Member

fabpot commented Dec 15, 2017

I'm going to revert this change as it was broken anyway before the fix today, so it never worked. That will give us some time to implement it properly.

fabpot added a commit that referenced this pull request Dec 15, 2017
…otection if CSRF manager exists (sroze)"

This reverts commit d5f0428, reversing
changes made to e52825e.
fabpot added a commit that referenced this pull request Dec 15, 2017
* 3.4:
  Revert "bug #25151 [FrameworkBundle] Automatically enable the CSRF protection if CSRF manager exists (sroze)"
  Revert "bug #25502 Fixing wrong class_exists on interface (weaverryan)"
fabpot added a commit that referenced this pull request Dec 15, 2017
* 4.0:
  Revert "bug #25151 [FrameworkBundle] Automatically enable the CSRF protection if CSRF manager exists (sroze)"
  Revert "bug #25502 Fixing wrong class_exists on interface (weaverryan)"
@sroze sroze deleted the enable-csrf-if-token-manager-exists branch December 15, 2017 10:14
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

7 participants