ping @bschussek

The "csrf_protection" setting is form specific, while the CSRF services are independent of forms. So your solution only works if you create a second configuration setting.

@stof @bschussek Ok, so what about this:

• always load security.xml, no configuration for it, security-core is a hard dependency of FrameworkBundle so we can do this
• only load security_csrf if the form csrf protection is enabled

I think this makes the most sense for now.

@asm89 It does not. The security_csrf services are completely independent from the Form component.

@bschussek csrf_protection in the FrameworkBundle config is not form-specific. Otherwise, it would be nested inside form

@bschussek Ok, I see what you mean. I was referring too much to the "old" situation.

Any suggestions for naming the new configuration option? I'll add it + tests etc. That will mean that if you enable csrf protection for forms, you will also have to enable the security csrf stuff and session, but that's fine (and default). :)

@stof Yes, that was what I thought, hence my suggestion above. In the "old" situation I guess "csrf_protection" only had impact on forms, but now you also get access to a token generator etc.

What's the benefit of making this configurable?

@asm89 the CSRF token generator was also exposed outside forms previously

I pushed a new commit, now the secure random generator service is always created, the csrf stuff only if you enable it in the configuration.

@bschussek We don't use it. By always loading security_csrf.xml by default you're always forcing me to use sessions which I also don't use. Now it's all configurable.

@bschussek As the CSRF system has a dependency on the session, and the session system can be disabled (if you are building a stateless API for instance), it is important to allow disabling it (otherwise you forbid disabling the session)

That's a good point then. The commit is still missing the changes to the configuration class, as well as a test case. Also, the setting should default to true if the form CSRF setting is enabled (for simplicity, and for BC).

@bschussek which configuration change ? The config already exists as of 2.1 (or maybe even 2.0 but this is too old for me to remember). Your refactoring made it being ignored.

@asm89 in 2.3, there is a LogicException being thrown when enabling the CSRF protection without enabling the session. You should also restore it

@stof Sorry, my bad. I forgot that these two settings are already decoupled. 👍 then

@stof Ok, so I'll restore the LogicException. Anything else this PR needs?

@asm89 Actually, you only need to move the exception. It is still thrown by the configuration of form in case the CSRF protection is enabled. The exception should be thrown by the registration of the CSRF system itself

@stof @bschussek @fabpot Ok, we have to change the default configuration of the framework bundle. Currently we try to do the following by default:

• csrf on / session off / form off

But since csrf depends on session, this makes no sense. We can do one of the following:

• csrf off / session off / form off
• csrf on / session on / form _off_0

For now I'll pushed the version where csrf is off by default. Note that the configuration shipped with symfony standard enables csrf, forms and sessions by default.

I just came up with another option. We can/should introduce a new configuration option under the form key to enable CSRF protection for forms, this can be enabled by default if you enable forms. So:

# this will throw an exception that it needs csrf_protection to be enabled
framework:
    form: ~

# this will throw an exception that it needs session to be enabled
framework:
    csrf_protection: ~
    form: ~

# this is fine
framework:
    csrf_protection: ~
    form: ~
    session: ~

# this is also fine
framework:
    form:
        csrf_protection: false

I think by introducing an extra configuration setting csrf_protection under form, we can come closest to how it worked before + now you explicitly disable csrf_protection under form.

@asm89 Please don't change the default value in the Configuration class, keeping the same than in 2.3. Anything else would be a BC break.

@stof ok, but then the default configuration will throw a LogicException
(since csrf is ON by default and session is not)...
Op 9 okt. 2013 18:05 schreef "Christophe Coevoet" notifications@github.com
het volgende:

@asm89 https://github.com/asm89 Please don't change the default value
in the Configuration class, keeping the same than in 2.3. Anything else
would be a BC break.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/9252#issuecomment-25983794
.

ah sorry, the default is indeed not working anymore because of csrf_protection being enabled by default but not session. But as 2.3 loads the CSRF only when enabling forms, the default was working previously. We indeed need to change some default even if now fully BC (the only way to be fully BC would be to make the CSRF dependent on form again, which looks like a weird choice)

Your proposal looks good to me. It is technically as small BC break (you have to list csrf_protection explicitly) but it was already listed explicitly in the standard edition since 2.0 so most people should already have it and not be impacted by the BC break.

@fabpot I think this one is good to go. Should I mention the change in the changelog + add a note somewhere that the deprecated setting should be removed in 3.0?

👍 looks good

@asm89 Can you add a note in the changelog? Also, anything that needs to be updated in the official documentation? Anything that needs to be tweaked in the default configuration of the Symfony SE (even if things are commented there)?

@fabpot symfony SE config is fine and does not need to be updated.

Docs update PR: symfony/symfony-docs#3114

@fabpot What file do you want the message in? The CHANGELOG files are generated afaik. Deprecation notice in UPGRADING-2.4 and a removal notice in UPGRADING-3.0?

@asm89 not the changelog of the components between minor versions. The only generated changelogs are the changelogs for maintenance releases at the root of the repo