[Csrf] Moved CSRF component outside Security namespace #36423
Conversation
664d687 to
ab71cd1
Compare
| @@ -11,7 +11,7 @@ | |||
|
|
|||
| namespace Symfony\Bridge\Twig\Extension; | |||
|
|
|||
| use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface; | |||
| use Symfony\Component\Csrf\CsrfTokenManagerInterface; | |||
There was a problem hiding this comment.
ALL similar changes are BC breaks
There was a problem hiding this comment.
Are you sure about this? There is a class_alias between the old and the new namespace.
I just tested this by linking the new CSRF component, and the old CSRF component + twig bridge into the vendor/ directory (+ autoload config). Then this code works as expected (without error, but with the deprecation):
<?php
require_once 'vendor/autoload.php';
use Symfony\Component\Security\Csrf\CsrfTokenManager;
use Symfony\Bridge\Twig\Extension\CsrfRuntime;
$manager = new CsrfTokenManager();
$runtime = new CsrfRuntime($manager);
echo $runtime->getCsrfToken('foo');* CSRF not directly related on the rest of Security (nor has dependencies on it) * CSRF is usable in any form or request * CSRF is configured through the FrameworkBundle * This allows re-introducing the `symfony/security` component (see symfony#33558)
ab71cd1 to
bca5f15
Compare
|
I just found #34004 (comment) . Would it make sense to only deprecate CSRF and not introduce |
|
@wouterj even if that's true ... do you think that 100% of web apps are going to be updated immediately to stop using CSRF? Looks unlikely ... so I think we should go on with this PR. |
|
If browsers implement all that is needed to protect against csrf, there's nothing to upgrade. I think this is worth considering yes: planning to drop the component altogether. |
|
The plan could be to update the documentation first, then deprecate in 5.4 or even 6.1. |
|
@javiereguiluz the nice thing about deprecation is that there is no need for immediate change. The CSRF component will be around until at least November 2024 if we deprecate it now (and given that it does only depend on 1 exception class from security, we can also add Symfony 6 support to it, extending it's life even longer). If we're considering to remove the component in Symfony 6 (or at least: no longer install it in core components), I don't think it makes sense to do the change in this PR. When in the 5.x lifecycle we're deprecating this component wouldn't really matter for this. (I agree that first updating the docs and then deprecating might be a better plan) |
|
I agree with the deprecation of the CSRF feature in 5.1. |
|
Unfortunately, for software targetting enterprise, IE 11 is not dead yet. And so CSRF is not dead yet either there. |
|
According to https://caniuse.com/#feat=same-site-cookie-attribute , IE 11 on Windows 10 does support the SameSite cookie attribute. Windows 8(.1) reaches end of life at January 10 2023. So this is within eol for 5.4, and I think it's fair to release a 5.4 version that supports |
|
Well, even the EOL defined by Microsoft does not reflect the reality of enterprises, due to the time it takes them to migrate. Windows 7 reached its EOL in January 2020, but over the last 90 days, my stats for Incenteev reveal that my pages views for Windows users are still 33% for Windows 7, 10% for Windows 8.x and 57% for Windows 10 (and things get even worse if I restrict stats to page views done with IE, as Windows 7 then reaches 70%). |
|
And deprecating CSRF in 5.1 means that you consider that CSRF is dead in May 2020 (when 5.1 is released), not in January 2023. And that's definitely not the case in the enterprise world. |
|
Ah, "just because symfony/form 6 assumes that CSRF is not an issue anymore would be a bad news " is a very valid point imho. The integration of CSRF lives in all other components, so deprecating the CSRF component would mean removing the integrations from the other components in 6.0. In any case, I think what still applies is to close this PR and revisit it for 5.4: We either deprecate & drop CSRF or we move CSRF to the root component namespace. |
|
Ok, let's close this PR then and revisit the issue when we will work on 5.4. |
This PR moves the CSRF component outside the Security namespace.
Advantages
symfony/securitycomponent in a future version (see [Security] AuthenticatorManager to make "authenticators" first-class security #33558)Considerations
For renaming classes, I tried using the same techniques as where done in the Messenger component (ref [Messenger] Move Transports to separate packages #35422). I need to test it in a real application to see if the BC layers are working as expected.
I think this requires changes to the subtree split configuration.
The
symfony/security-csrfshould probably be marked as deprecated on packagist.The
TokenNotFoundExceptionused to extendSymfony\Component\Security\Core\Exception\RuntimeException. I've changed this to\RuntimeException, to remove the dependency on the (completely irrelevant)symfony/security-corepackage. Previously this was considered a BC break ([Security] Remove dependency on security-core from security-csrf #16494), but as we're now moving all classes anyway, I think this is probably the only moment we can make such a change.