Skip to content

Commit

Permalink
feature #54443 [Security] Add support for dynamic CSRF id with Expres…
Browse files Browse the repository at this point in the history
…sion in `#[IsCsrfTokenValid]` (yguedidi)

This PR was merged into the 7.1 branch.

Discussion
----------

[Security] Add support for dynamic CSRF id with Expression in `#[IsCsrfTokenValid]`

| Q             | A
| ------------- | ---
| Branch?       | 7.1
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | continuation of #52961 from Hackday
| License       | MIT

Use case is for example on a list page with delete action per item, and you want a CSRF token per item, so in the template you have something like the following:

```twig
{# in a loop over multiple posts #}
<form action="{{ path('post_delete', {post: post.id}) }}" method="POST">
    <input type="hidden" name="_token" value="{{ csrf_token('delete-post-' ~ post.id) }}">
    ...
</form>
```

The new feature will allow:
```php
#[IsCsrfTokenValid(new Expression('"delete-post-" ~ args["post"].id'))]
public function delete(Request $request, Post $post): Response
{
    // ... delete the post
}
```

Maybe this need more tests but need help identify which test cases are useful.
Hope this can pass before the feature freeze

Commits
-------

8f99ca5 Add support for dynamic CSRF id in IsCsrfTokenValid
  • Loading branch information
nicolas-grekas committed Apr 5, 2024
2 parents 4c1d8eb + 8f99ca5 commit 695bd1a
Show file tree
Hide file tree
Showing 7 changed files with 85 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Component\DependencyInjection\Compiler\CompilerPassInterface;
use Symfony\Component\DependencyInjection\ContainerBuilder;
use Symfony\Component\DependencyInjection\ContainerInterface;
use Symfony\Component\DependencyInjection\Reference;
use Symfony\Component\Security\Csrf\TokenStorage\ClearableTokenStorageInterface;
use Symfony\Component\Security\Http\EventListener\CsrfProtectionListener;
Expand All @@ -35,6 +36,10 @@ public function process(ContainerBuilder $container): void

private function registerCsrfProtectionListener(ContainerBuilder $container): void
{
if (!$container->hasDefinition('cache.system')) {
$container->removeDefinition('cache.security_is_csrf_token_valid_attribute_expression_language');
}

if (!$container->has('security.authenticator.manager') || !$container->has('security.csrf.token_manager')) {
return;
}
Expand All @@ -45,6 +50,7 @@ private function registerCsrfProtectionListener(ContainerBuilder $container): vo

$container->register('controller.is_csrf_token_valid_attribute_listener', IsCsrfTokenValidAttributeListener::class)
->addArgument(new Reference('security.csrf.token_manager'))
->addArgument(new Reference('security.is_csrf_token_valid_attribute_expression_language', ContainerInterface::NULL_ON_INVALID_REFERENCE))
->addTag('kernel.event_subscriber');
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,7 @@ public function load(array $configs, ContainerBuilder $container): void
$container->removeDefinition('security.expression_language');
$container->removeDefinition('security.access.expression_voter');
$container->removeDefinition('security.is_granted_attribute_expression_language');
$container->removeDefinition('security.is_csrf_token_valid_attribute_expression_language');
}

if (!class_exists(PasswordHasherExtension::class)) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -303,5 +303,12 @@
->set('cache.security_is_granted_attribute_expression_language')
->parent('cache.system')
->tag('cache.pool')

->set('security.is_csrf_token_valid_attribute_expression_language', BaseExpressionLanguage::class)
->args([service('cache.security_is_csrf_token_valid_attribute_expression_language')->nullOnInvalid()])

->set('cache.security_is_csrf_token_valid_attribute_expression_language')
->parent('cache.system')
->tag('cache.pool')
;
};
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@

namespace Symfony\Component\Security\Http\Attribute;

use Symfony\Component\ExpressionLanguage\Expression;

#[\Attribute(\Attribute::IS_REPEATABLE | \Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::TARGET_FUNCTION)]
final class IsCsrfTokenValid
{
public function __construct(
/**
* Sets the id used when generating the token.
* Sets the id, or an Expression evaluated to the id, used when generating the token.
*/
public string $id,
public string|Expression $id,

/**
* Sets the key of the request that contains the actual token value that should be validated.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
namespace Symfony\Component\Security\Http\EventListener;

use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\ExpressionLanguage\Expression;
use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\Security\Core\Exception\InvalidCsrfTokenException;
Expand All @@ -26,6 +29,7 @@ final class IsCsrfTokenValidAttributeListener implements EventSubscriberInterfac
{
public function __construct(
private readonly CsrfTokenManagerInterface $csrfTokenManager,
private ?ExpressionLanguage $expressionLanguage = null,
) {
}

Expand All @@ -37,9 +41,12 @@ public function onKernelControllerArguments(ControllerArgumentsEvent $event): vo
}

$request = $event->getRequest();
$arguments = $event->getNamedArguments();

foreach ($attributes as $attribute) {
if (!$this->csrfTokenManager->isTokenValid(new CsrfToken($attribute->id, $request->request->getString($attribute->tokenKey)))) {
$id = $this->getTokenId($attribute->id, $request, $arguments);

if (!$this->csrfTokenManager->isTokenValid(new CsrfToken($id, $request->request->getString($attribute->tokenKey)))) {
throw new InvalidCsrfTokenException('Invalid CSRF token.');
}
}
Expand All @@ -49,4 +56,18 @@ public static function getSubscribedEvents(): array
{
return [KernelEvents::CONTROLLER_ARGUMENTS => ['onKernelControllerArguments', 25]];
}

private function getTokenId(string|Expression $id, Request $request, array $arguments): string
{
if (!$id instanceof Expression) {
return $id;
}

$this->expressionLanguage ??= new ExpressionLanguage();

return (string) $this->expressionLanguage->evaluate($id, [
'request' => $request,
'args' => $arguments,
]);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,11 @@
* file that was distributed with this source code.
*/

namespace EventListener;
namespace Symfony\Component\Security\Http\Tests\EventListener;

use PHPUnit\Framework\TestCase;
use Symfony\Component\ExpressionLanguage\Expression;
use Symfony\Component\ExpressionLanguage\ExpressionLanguage;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Event\ControllerArgumentsEvent;
use Symfony\Component\HttpKernel\HttpKernelInterface;
Expand Down Expand Up @@ -86,6 +88,37 @@ public function testIsCsrfTokenValidCalledCorrectly()
$listener->onKernelControllerArguments($event);
}

public function testIsCsrfTokenValidCalledCorrectlyWithCustomExpressionId()
{
$request = new Request(query: ['id' => '123'], request: ['_token' => 'bar']);

$csrfTokenManager = $this->createMock(CsrfTokenManagerInterface::class);
$csrfTokenManager->expects($this->once())
->method('isTokenValid')
->with(new CsrfToken('foo_123', 'bar'))
->willReturn(true);

$expressionLanguage = $this->createMock(ExpressionLanguage::class);
$expressionLanguage->expects($this->once())
->method('evaluate')
->with(new Expression('"foo_" ~ args.id'), [
'args' => ['id' => '123'],
'request' => $request,
])
->willReturn('foo_123');

$event = new ControllerArgumentsEvent(
$this->createMock(HttpKernelInterface::class),
[new IsCsrfTokenValidAttributeMethodsController(), 'withCustomExpressionId'],
['123'],
$request,
null
);

$listener = new IsCsrfTokenValidAttributeListener($csrfTokenManager, $expressionLanguage);
$listener->onKernelControllerArguments($event);
}

public function testIsCsrfTokenValidCalledCorrectlyWithCustomTokenKey()
{
$request = new Request(request: ['my_token_key' => 'bar']);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\Security\Http\Tests\Fixtures;

use Symfony\Component\ExpressionLanguage\Expression;
use Symfony\Component\Security\Http\Attribute\IsCsrfTokenValid;

class IsCsrfTokenValidAttributeMethodsController
Expand All @@ -24,6 +25,16 @@ public function withDefaultTokenKey()
{
}

#[IsCsrfTokenValid(new Expression('"foo_" ~ args.id'))]
public function withCustomExpressionId(string $id)
{
}

#[IsCsrfTokenValid(new Expression('"foo_" ~ args.slug'))]
public function withInvalidExpressionId(string $id)
{
}

#[IsCsrfTokenValid('foo', tokenKey: 'my_token_key')]
public function withCustomTokenKey()
{
Expand Down

0 comments on commit 695bd1a

Please sign in to comment.