[Security] Implement support for CSRF tokens in logout URL's #3007
Conversation
|
Odd that Travis CI reported a build failure for PHP 5.3.2, but both 5.3 and 5.4 passed: http://travis-ci.org/#!/jmikola/symfony/builds/463356 My local machine passes as well. |
| @@ -303,6 +312,18 @@ private function createFirewall(ContainerBuilder $container, $id, $firewall, &$a | |||
| foreach ($firewall['logout']['handlers'] as $handlerId) { | |||
| $listener->addMethodCall('addHandler', array(new Reference($handlerId))); | |||
| } | |||
|
|
|||
| // register with LogoutUrlHelper | |||
There was a problem hiding this comment.
How about adding this to the existing security helper? It might also be nice to have for login (also not directly related to this PR).
There was a problem hiding this comment.
I considered it, but since the main focus is for URL generation it didn't seem to jive with the role check method. The existing SecurityHelper class is concerned only with reading from the context, and it didn't seem proper to tack on request and router services for a feature that will likely be used by only a minority of users.
I also wanted to demonstrate a model for having the Twig extension compose the templating helper.
|
@schmittjoh: Please let me know your thoughts on the last commit. I think it would be overkill to add support for another handler service and/or error page just for logout exceptions. Perhaps as an alternative, we might just want to consider an invalid CSRF token on logout imply a false return value for |
|
@schmittjoh: ping |
|
@jmikola: Instead of merging symfony/master, can you rebase? |
|
Will do. |
This will facilitate adding additional options for CSRF protection. Additionally, a unit test for existing behavior was added.
This adds several new options to the logout listener, modeled after the form_login listener: * csrf_parameter * intention * csrf_provider The "csrf_parameter" and "intention" have default values if omitted. By default, "csrf_provider" is empty and CSRF validation is disabled in LogoutListener (preserving BC). If a service ID is given for "csrf_provider", CSRF validation will be enabled. Invalid tokens will result in an InvalidCsrfTokenException being thrown before any logout handlers are invoked.
…F tokens As each firewall is configured, its logout listener (if any) will be registered with the LogoutUrlHelper service. In a template, this helper may be used to generate relative or absolute URL's to a particular firewall's logout path. A CSRF token will be appended to the URL as necessary. The Twig extension composes the helper service to avoid code duplication (see: #2999).
Using "securitybundletest" as the default environment for the functional test's kernel causes a PHP fatal error redeclaring the class "appSecuritybundletestDebugProjectContainer" when multiple tests (with unique names) are executed. In lieu of forcing tests to specify their own environment explicitly, we can simply append the test name into the environment. Note: this bug may be related to PHPUnit executing multiple tests within the same process.
On the advice of @schmittjoh, this commit adds a LogoutException class for use by LogoutListener if the CSRF token is invalid. The handling in the Security component's ExceptionListener is modeled after AccessDeniedException, which gets wrapped in an AccessDeniedHttpException in the absence of handler service or error page (I didn't think it was appropriate to re-use those for LogoutException).
After rebasing, my merge commits disappeared. Is this normal? |
|
Are you sure they disappeared ? Diverging from the remote branch is logical (you rewrote the history and so changed the commit id) but are you sure it does not have the commits on top of master ? Try If your commut are there, you simply need to force the push for the logout-csrf branch (take care to push only this branch during the force push to avoid messing all others as git won't warn you when asking to force) |
|
ah sorry, you talked only about the merge commit. Yeah it is normal. When reapplying your commits on top of master, the merge commit are not kept as you are reapplying the changes linearly on top of the other branch (and deleting the merge commit was the reason why @fabpot asked you to rebase instead of merging btw) |
|
The merge commits are not present in I'll force-push the changes to my PR brange. IIRC, GitHub is smart enough to preserve inline diff comments, provided they were made through the PR and not on the original commits. |
|
That worked well. In the future, I think I'll stick to merging upstream in and then rebasing afterwards. Resolving conflicts is much easier during a merge than interactive rebase. |
|
@fabpot @schmittjoh: Is there anything else I can do for this PR? I believe the exception was the only outstanding question (see: this comment). |
Commits ------- 49a8654 [Security] Use LogoutException for invalid CSRF token in LogoutListener a96105e [SecurityBundle] Use assertCount() in tests 4837407 [SecurityBundle] Fix execution of functional tests with different names 66722b3 [SecurityBundle] Templating helpers to generate logout URL's with CSRF tokens aaaa040 [Security] Allow LogoutListener to validate CSRF tokens b1f545b [Security] Refactor LogoutListener constructor to take options c48c775 [SecurityBundle] Add functional test for form login with CSRF token Discussion ---------- [Security] Implement support for CSRF tokens in logout URL's ``` Bug fix: no Feature addition: yes Backwards compatibility break: no Symfony2 tests pass: yes Fixes the following tickets: - Todo: - ``` [](http://travis-ci.org/jmikola/symfony) This derived from #3006 but properly targeting on the master branch. This exposes new configuration options to the logout listener to enable CSRF protection, as already exists for the form login listener. The individual commits and their extended messages should suffice for explaining the logical changes of the PR. In addition to changing LogoutListener, I also created a templating helper to generate logout URL's, which includes a CSRF token if necessary. This may or may not using routing, depending on how the listener is configured since both route names or hard-coded paths are valid options. Additionally, I added unit tests for LogoutListener and functional tests for both CSRF-enabled form logins and the new logout listener work. Kudo's to @henrikbjorn for taking the time to document CSRF validation for form login listeners (see [here](http://henrik.bjrnskov.dk/symfony2-cross-site-request-forgery/)). The [Logout CSRF Protection](http://www.yiiframework.com/wiki/190/logout-csrf-protection/) article on the Yii Framework wiki was also helpful in drafting this. --------------------------------------------------------------------------- by jmikola at 2011-12-31T07:50:31Z Odd that Travis CI reported a build failure for PHP 5.3.2, but both 5.3 and 5.4 passed: http://travis-ci.org/#!/jmikola/symfony/builds/463356 My local machine passes as well. --------------------------------------------------------------------------- by jmikola at 2012-02-06T20:05:30Z @schmittjoh: Please let me know your thoughts on the last commit. I think it would be overkill to add support for another handler service and/or error page just for logout exceptions. Perhaps as an alternative, we might just want to consider an invalid CSRF token on logout imply a false return value for `LogoutListener::requiresLogout()`. That would sacrifice the ability to handle the error separately (which a 403 response allows us), although we could still add logging (currently done in ExceptionListener). --------------------------------------------------------------------------- by jmikola at 2012-02-13T17:41:33Z @schmittjoh: ping --------------------------------------------------------------------------- by fabpot at 2012-02-14T23:36:22Z @jmikola: Instead of merging symfony/master, can you rebase? --------------------------------------------------------------------------- by jmikola at 2012-02-15T00:00:49Z Will do. --------------------------------------------------------------------------- by jmikola at 2012-02-15T00:05:48Z ``` [avocado: symfony] logout-csrf (+9/-216) $ git rebase master First, rewinding head to replay your work on top of it... Applying: [SecurityBundle] Add functional test for form login with CSRF token Applying: [Security] Refactor LogoutListener constructor to take options Applying: [Security] Allow LogoutListener to validate CSRF tokens Applying: [SecurityBundle] Templating helpers to generate logout URL's with CSRF tokens Applying: [SecurityBundle] Fix execution of functional tests with different names Applying: [SecurityBundle] Use assertCount() in tests Using index info to reconstruct a base tree... Falling back to patching base and 3-way merge... Applying: [Security] Use LogoutException for invalid CSRF token in LogoutListener [avocado: symfony] logout-csrf (+7) $ git st # On branch logout-csrf # Your branch and 'origin/logout-csrf' have diverged, # and have 223 and 9 different commit(s) each, respectively. # nothing to commit (working directory clean) [avocado: symfony] logout-csrf (+7) $ ``` After rebasing, my merge commits disappeared. Is this normal? --------------------------------------------------------------------------- by stof at 2012-02-15T00:15:07Z Are you sure they disappeared ? Diverging from the remote branch is logical (you rewrote the history and so changed the commit id) but are you sure it does not have the commits on top of master ? Try ``git log master..logout-scrf`` If your commut are there, you simply need to force the push for the logout-csrf branch (take care to push only this branch during the force push to avoid messing all others as git won't warn you when asking to force) --------------------------------------------------------------------------- by stof at 2012-02-15T00:17:09Z ah sorry, you talked only about the merge commit. Yeah it is normal. When reapplying your commits on top of master, the merge commit are not kept as you are reapplying the changes linearly on top of the other branch (and deleting the merge commit was the reason why @fabpot asked you to rebase instead of merging btw) --------------------------------------------------------------------------- by jmikola at 2012-02-15T00:18:00Z The merge commits are not present in `git log master..logout-csrf`. Perhaps it used those merge commits when rebasing, as there were definitely conflicts resolved when I originally merged in symfony/master (@fabpot had made his own changes to LogoutListener). I'll force-push the changes to my PR brange. IIRC, GitHub is smart enough to preserve inline diff comments, provided they were made through the PR and not on the original commits. --------------------------------------------------------------------------- by jmikola at 2012-02-15T00:19:38Z That worked well. In the future, I think I'll stick to merging upstream in and then rebasing afterwards. Resolving conflicts is much easier during a merge than interactive rebase. --------------------------------------------------------------------------- by jmikola at 2012-02-23T18:46:13Z @fabpot @schmittjoh: Is there anything else I can do for this PR? I believe the exception was the only outstanding question (see: [this comment](#3007 (comment))).
|
Will do. I need to do a PR against the Symfony2 documentation as well. |
This derived from #3006 but properly targeting on the master branch.
This exposes new configuration options to the logout listener to enable CSRF protection, as already exists for the form login listener. The individual commits and their extended messages should suffice for explaining the logical changes of the PR.
In addition to changing LogoutListener, I also created a templating helper to generate logout URL's, which includes a CSRF token if necessary. This may or may not using routing, depending on how the listener is configured since both route names or hard-coded paths are valid options.
Additionally, I added unit tests for LogoutListener and functional tests for both CSRF-enabled form logins and the new logout listener work.
Kudo's to @henrikbjorn for taking the time to document CSRF validation for form login listeners (see here). The Logout CSRF Protection article on the Yii Framework wiki was also helpful in drafting this.