Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow override of CSRF name #170

Merged
merged 2 commits into from
Jun 16, 2022

Conversation

villermen
Copy link
Contributor

@villermen villermen commented Mar 10, 2022

Allows the csrf_options.name option passed to a Csrf element to override the element's own name for the validator, instead of discarding the option entirely.

Fixes #169.

Q A
Documentation no
Bugfix yes
BC Break no¹
New Feature no
RFC no
QA no

¹ Technically yes for people who had the name CSRF option specified before (previously a no-op).

@villermen villermen marked this pull request as ready for review March 10, 2022 12:53
@Slamdunk
Copy link
Contributor

Weird that in the first commit, with the test but without the fix, 2 build passed the tests 🤔

WDYT may have happened?

@driehle
Copy link
Contributor

driehle commented Mar 29, 2022

@villermen I agree that the current implementation has the "issue" (might as well be a feature for some) that the token is unique for all forms where the element is named identically. Usually I name the element csrf, so the token will be the same for all forms. However, this can already be solved with the current implementation by providing your own session container. Please see the following Form class which I use as a base class for all of my forms:

<?php

declare(strict_types=1);

namespace Application\Utility\Form;

use Laminas\Form\Element\Csrf;
use Laminas\Session\Container;

class Form extends \Laminas\Form\Form
{
    public function addCsrfElement($timeout = 600): void
    {
        $parts = explode('\\', static::class);
        $name = 'csrf' . $parts[0] . (count($parts) > 1 ? $parts[array_key_last($parts)] : '');
        $session = 'CsrfToken_' . $parts[0] . (count($parts) > 1 ? '_' . $parts[array_key_last($parts)] : '');

        $this->add([
            'type' => Csrf::class,
            'name' => 'csrf',
            'options' => [
                'csrf_options' => [
                    'name' => $name,
                    'session' => new Container($session),
                    'timeout' => $timeout,
                ],
            ],
        ]);
    }
}

However, looking at your changes I see that my option csrf_options.name indeed never changed anything and it was solely csrf_options.session which did the trick.

@villermen
Copy link
Contributor Author

@froschdesign It just feels very wrong to me that specifying the CSRF validator's options via csrf_options leaves out the most important option. The name option is documented to be exactly what you're looking for after reading https://docs.laminas.dev/laminas-form/v3/element/csrf/#multiple-csrf-elements-must-be-uniquely-named and then seeing https://github.com/laminas/laminas-validator/blob/bdd503adc83d814a5c94e598ea0eb9fc7ca56339/src/Csrf.php#L53, whereas the session option is not documented at all.

If the name is discarded as a feature I'd prefer it if the option were removed/not parsed altogether so it can not give the impression of improved security where there is none.

@driehle
Copy link
Contributor

driehle commented Apr 5, 2022

Yeah, if I remember correctly, I found the session option only by reverse engineering. I'd definetly prefer the name option as well, as that one is clearly mentioned on the docs for exactly this purpose. I vote for merging this PR.

@froschdesign froschdesign added this to the 3.2.0 milestone Jun 15, 2022
@froschdesign
Copy link
Member

@Slamdunk
I would release this as a new functionality. Do you agree with that?

@Slamdunk
Copy link
Contributor

Go for it 💪

@froschdesign froschdesign merged commit bfc12a8 into laminas:3.2.x Jun 16, 2022
@froschdesign
Copy link
Member

@villermen
Thank you for your time and this contribution! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CSRF element overwrites name option with element name
4 participants