[FrameworkBundle] Auto-enable CSRF if the component + session are loaded #25508

nicolas-grekas · Dec 15, 2017

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	symfony/recipes#262
License	MIT
Doc PR	-

By binding CSRF and session default state, we provide better DX, but we also provide a way for bundles to enable session on its own: they just need to require "symfony/security-csrf".
Yes, that's a side effect, but I think that's a nice one for 3.4/4.0.
Of course, we might do better in 4.1, but for bug fix only releases, LGTM.

sroze · Dec 15, 2017

Sorry for not picking the session before. If it's the behaviour we want (and I believe yes), I can help with changing the tests to reflect that @nicolas-grekas.

stof · Dec 15, 2017

@nicolas-grekas can you check why deps=high and deps=low are failing here ?

sroze · Dec 15, 2017

Travis is 💚 and AppVeyor's issues are not related.

weaverryan · Dec 16, 2017

I really want to help fix the session issue... but this strikes me as weird:

It's weird that the session config key has a special case specifically for CSRF.
If I require symfony/security, won't I have CSRF? And so won't my sessions suddenly be enabled?
It IS convenient that libraries could require security-csrf as a way to auto-enable sessions, but that seems like a hack.

Sorry to not be on board... I'm just not convinced yet...

javiereguiluz · Dec 18, 2017

Without knowing the internals of this, I agree with Ryan that requiring security-csrf just to auto-enable sessions look like a hack.

nicolas-grekas · Dec 18, 2017

This is not the main purpose of this PR. It's main purpose is enabling CSRF protection when security-csrf is installed.

weaverryan · Dec 19, 2017

This is not the main purpose of this PR. It's main purpose is enabling CSRF protection when security-csrf is installed.

@nicolas-grekas Ok - fair enough then - it makes more sense looking at it through that lense. But, what about this:

If I require symfony/security, won't I have CSRF? And so won't my sessions suddenly be enabled?

Is that not a problem?

nicolas-grekas · Dec 19, 2017

There is a discussion somewhere about discouraging to use this meta repo. Should remove the issue.

weaverryan · Dec 19, 2017

There is a discussion somewhere about discouraging to use this meta repo. Should remove the issue.

@nicolas-grekas Hmm, ok... I'm going to be really annoying 😇. Should we remove the security alias then? What package should we tell users to install when they're starting with security (and we don't know yet if they are building a pure API or a form login)?

Another issue is that simply enabling the session means that it will store sessions into the var/cache/dev/sessions directory (see

symfony/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

Lines 405 to 417 in 5f72ecf

    
           ->scalarNode('storage_id')->defaultValue('session.storage.native')->end() 
        
           ->scalarNode('handler_id')->defaultValue('session.handler.native_file')->end() 
        
           ->scalarNode('name')->end() 
        
           ->scalarNode('cookie_lifetime')->end() 
        
           ->scalarNode('cookie_path')->end() 
        
           ->scalarNode('cookie_domain')->end() 
        
           ->booleanNode('cookie_secure')->end() 
        
           ->booleanNode('cookie_httponly')->defaultTrue()->end() 
        
           ->booleanNode('use_cookies')->end() 
        
           ->scalarNode('gc_divisor')->end() 
        
           ->scalarNode('gc_probability')->defaultValue(1)->end() 
        
           ->scalarNode('gc_maxlifetime')->end() 
        
           ->scalarNode('save_path')->defaultValue('%kernel.cache_dir%/sessions')->end()

). But that's not what we want - we want them to use the default Flex config, which sets handler_id to null and thus allows PHP to handle storage).

I don't see the smooth solution yet :/.

weaverryan · Dec 21, 2017

I'm here to complicate things more :). The SecurityBundle has a dependency on all of symfony/security :). So the idea of only installing the piece you need (e.g. security-guard) is a non-starter (as well as my issue above about session configuration).

nicolas-grekas · Dec 22, 2017

OK, if this is not obvious, this PR should not be merged :)

nicolas-grekas · Jan 7, 2018

Reopening as I think this should be merged when #25699 will be.

sroze · Jan 7, 2018

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

@@ -449,7 +450,7 @@ private function addSessionSection(ArrayNodeDefinition $rootNode)
            ->children()
                ->arrayNode('session')
                    ->info('session configuration')
-                    ->canBeEnabled()
+                    ->{!class_exists(FullStack::class) && interface_exists(CsrfTokenManagerInterface::class) ? 'canBeDisabled' : 'canBeEnabled'}()


I'm not sure how yet but I believe we could find a better way than relying on a Symfony\Component\Security class to enable the session automatically. Maybe a meta-package containing a Symfony\Meta\RequiresSession class? 🤷‍♂️

I think we should not go in that direction. If enabling the session has no downside (with #25699), we should just enable them by default. But I wouldn't change the default in 3.4. Thus this PR, which is back in the game as is for me.

Yes, you're right that it costs nothing. The interface_exists check is what is bothering me. Can we check on the SessionInterface interface directly instead of the CsrfTokenManagerInterface?

weaverryan · Jan 7, 2018

+1 to reopen! But I think the logic should be changed: we should never automatically (in the Configuration class) enable the session... because it will default to trying to write to a var/cache directory that may not exist (so the user will get an error).

Could we auto-enable csrf if the code exists AND if the session was enabled by the user?

nicolas-grekas · Jan 8, 2018

Could we auto-enable csrf if the code exists AND if the session was enabled by the user?

here we are :)

weaverryan

Definite +1: csrf_protection enables itself if the session is enabled & CSRF code exists.

…on* are loaded

nicolas-grekas · Jan 10, 2018

Just removed 2nd commit, which belongs to another PR.

xabbuh · Jan 11, 2018

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

@@ -229,6 +231,11 @@ public function load(array $configs, ContainerBuilder $container)
            $this->registerRequestConfiguration($config['request'], $container, $loader);
        }

+        if (null === $config['csrf_protection']['enabled']) {


Will this ever be reached? I would have expected the treatNullLike() call above to result in the value here being true.

verified: yes! "defaultNull" applies after "treatNullLike" (which applies only to explicit user values)

maybe we should add a test for this :)

that's a test for the Config component :)

I meant for different configs here, but it will become quite hard as the FullStack class is always available.

treatNullLike is on the csrf_protection node, not on the enabled node.

…sion* are loaded (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [FrameworkBundle] Auto-enable CSRF if the component *+ session* are loaded | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/recipes#262 | License | MIT | Doc PR | - By binding CSRF and session default state, we provide better DX, but we also provide a way for bundles to enable session on its own: they just need to require "symfony/security-csrf". Yes, that's a side effect, but I think that's a nice one for 3.4/4.0. Of course, we might do better in 4.1, but for bug fix only releases, LGTM. Commits ------- 9e8231f [FrameworkBundle] Automatically enable the CSRF if component *+ session* are loaded

stof · Jan 16, 2018

then we again have the issue that installing the CSRF component is not enough to be protected (as it requires enabling session manually first and nothing will warn you). People using forms might not even see that they don't have csrf protection enabled on them.

nicolas-grekas · Jan 18, 2018

in Flex, the session is enabled by default, so that this is less an issue
I think the real best solution is to not need the session at all for CSRF protection - aka double submit strategy

nicolas-grekas added this to the 3.4 milestone Dec 15, 2017

carsonbot added Status: Needs Review FrameworkBundle Bug labels Dec 15, 2017

nicolas-grekas mentioned this pull request Dec 15, 2017

What to do if a bundle (recipe) needs session? symfony/recipes#262

Closed

nicolas-grekas force-pushed the auto-surf branch from a7d011e to 226a2cc Compare December 15, 2017 05:51

carsonbot removed the Status: Needs Review label Dec 15, 2017

ogizanagi approved these changes Dec 15, 2017

View reviewed changes

carsonbot added the Status: Reviewed label Dec 15, 2017

sroze force-pushed the auto-surf branch 2 times, most recently from b271567 to 54eef22 Compare December 15, 2017 11:12

stof approved these changes Dec 15, 2017

View reviewed changes

nicolas-grekas closed this Dec 22, 2017

nicolas-grekas deleted the auto-surf branch December 22, 2017 21:31

Nyholm mentioned this pull request Dec 23, 2017

Removing csrf_protection now that is is auto-enabled symfony/recipes#314

Closed

nicolas-grekas restored the auto-surf branch January 7, 2018 10:40

nicolas-grekas reopened this Jan 7, 2018

sroze reviewed Jan 7, 2018

View reviewed changes

nicolas-grekas force-pushed the auto-surf branch from 54eef22 to 74025a1 Compare January 8, 2018 19:25

nicolas-grekas force-pushed the auto-surf branch 2 times, most recently from ec93d0f to 1ab06c0 Compare January 8, 2018 19:28

weaverryan approved these changes Jan 8, 2018

View reviewed changes

nicolas-grekas changed the title ~~[FrameworkBundle] Automatically enable the CSRF protection *and session* if CSRF manager exists~~ pick 8a8ef52 [FrameworkBundle] Automatically enable the CSRF if component *+ session* are loaded Jan 9, 2018

[FrameworkBundle] Automatically enable the CSRF if component *+ sessi…

9e8231f

…on* are loaded

nicolas-grekas force-pushed the auto-surf branch from 1ab06c0 to b2eaeff Compare January 9, 2018 13:59

nicolas-grekas changed the title ~~pick 8a8ef52 [FrameworkBundle] Automatically enable the CSRF if component *+ session* are loaded~~ [FrameworkBundle] Automatically enable the CSRF if component *+ session* are loaded Jan 9, 2018

nicolas-grekas force-pushed the auto-surf branch 2 times, most recently from 53ba38a to 9e8231f Compare January 10, 2018 17:16

nicolas-grekas changed the title ~~[FrameworkBundle] Automatically enable the CSRF if component *+ session* are loaded~~ [FrameworkBundle] Auto-enable CSRF if the component *+ session* are loaded Jan 10, 2018

Tomsgu mentioned this pull request Jan 10, 2018

missing dependency, non-existent service "security.csrf.token_manager". FriendsOfSymfony/FOSUserBundle#2705

Closed

xabbuh reviewed Jan 11, 2018

View reviewed changes

xabbuh approved these changes Jan 11, 2018

View reviewed changes

nicolas-grekas merged commit 9e8231f into symfony:3.4 Jan 16, 2018

nicolas-grekas deleted the auto-surf branch January 16, 2018 14:44

This was referenced Jan 29, 2018

Release v3.4.4 #25956

Merged

Release v4.0.4 #25957

Merged

nicolas-grekas mentioned this pull request Aug 28, 2024

[symfony/framework-bundle] Remove "csrf_protection" line as its enabled by default symfony/recipes#1336

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[FrameworkBundle] Auto-enable CSRF if the component *+ session* are loaded #25508

[FrameworkBundle] Auto-enable CSRF if the component *+ session* are loaded #25508

Uh oh!

Conversation

nicolas-grekas commented Dec 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sroze commented Dec 15, 2017

Uh oh!

stof commented Dec 15, 2017

Uh oh!

sroze commented Dec 15, 2017

Uh oh!

weaverryan commented Dec 16, 2017

Uh oh!

javiereguiluz commented Dec 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Dec 18, 2017

Uh oh!

weaverryan commented Dec 19, 2017

Uh oh!

nicolas-grekas commented Dec 19, 2017

Uh oh!

weaverryan commented Dec 19, 2017

Uh oh!

weaverryan commented Dec 21, 2017

Uh oh!

nicolas-grekas commented Dec 22, 2017

Uh oh!

nicolas-grekas commented Jan 7, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weaverryan commented Jan 7, 2018

Uh oh!

nicolas-grekas commented Jan 8, 2018

Uh oh!

weaverryan left a comment

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas commented Jan 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolas-grekas Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Jan 16, 2018

Uh oh!

nicolas-grekas commented Jan 18, 2018

Uh oh!

Uh oh!

[FrameworkBundle] Auto-enable CSRF if the component + session are loaded #25508

[FrameworkBundle] Auto-enable CSRF if the component + session are loaded #25508

nicolas-grekas commented Dec 15, 2017 •

edited

Loading

javiereguiluz commented Dec 18, 2017 •

edited

Loading

nicolas-grekas Jan 11, 2018 •

edited

Loading