[Form] Added a cause on when a Csrf Error has occurred on CsrfValidationListener #28564

gmponos · 2018-09-23T12:12:32Z

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

The Csrf error that it is added as a FormError does not have a cause making it hard to filter it out.

I am not sure if this is a bug or a feature.
@xabbuh on the issue you said something about CsrfValidationError. I didn't quite get that. In the current PR you can see what I was thinking about. Let me know your thoughts and continue the discussion here.

xabbuh · 2018-09-23T13:55:29Z

src/Symfony/Component/Form/Extension/Csrf/EventListener/CsrfValidationListener.php

@@ -66,7 +67,8 @@ public function preSubmit(FormEvent $event)
                    $errorMessage = $this->translator->trans($errorMessage, array(), $this->translationDomain);
                }

-                $form->addError(new FormError($errorMessage));
+                $cause = new ConstraintViolation($errorMessage, null, array(), null, '', null, null, 'csrf');
+                $form->addError(new FormError($errorMessage, null, array(), null, $cause));


Using a ConstraintViolation here adds a hard dependency on the Validator component. This is not necessary. A cause of a form error can be arbitrary objects. We should add a new class for that.

I hear what you are saying but I am a little bit confused.

If I understood correctly you are suggesting this:

$cause = new CsrfValidationError($some_arguments_or_no_arguments_at_all); $form->addError(new FormError($errorMessage, null, array(), null, $cause));

The reason I went with my implementation is because of this line

The function above implies that all causes are ConstraintViolation objects. Personally I can fulfill my requirements with your suggestion but IMHO I think

On this current PR the cause should be a simple string.

$cause argument on FormError object should either accept strings, objects that implement the __toString function or a ConstraintViolation object.

Besides filtering ConstraintViolations the function findByCodes should also be able to filter causes that are string or objects that implement __toString function.

If you still believe that what you are saying is the way to go I will change my PR accordingly. Let me know.

btw I do not suggest to implement all these changes at the current PR.

Hello, any feedback on this... sorry if I am pushing things.

For now, I suggest to only add the new class that does not depend on the Validator component. IMO we should discuss your other ideas in different issues/PRs afterwards. :)

I believe I have applied the changes :)

xabbuh · 2018-09-30T10:35:36Z

src/Symfony/Component/Form/Extension/Csrf/EventListener/CsrfValidationListener.php

@@ -66,7 +66,7 @@ public function preSubmit(FormEvent $event)
                    $errorMessage = $this->translator->trans($errorMessage, array(), $this->translationDomain);
                }

-                $form->addError(new FormError($errorMessage));
+                $form->addError(new FormError($errorMessage, null, array(), null, new CsrfValidationError()));


I think it might be good to pass the CsrfToken instance as well as the submitted token to CsrfValidationError as that would ease debugging IMO.

Hm... I have my doubts about this... CsrfToken does not always exists. Check the last failing build. It will need some refactoring there making the code I little bit more complex. Do you want me to proceed with the refactoring?

xabbuh · 2018-09-30T10:36:50Z

src/Symfony/Component/Form/Extension/Csrf/EventListener/CsrfValidationError.php

+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\Component\Form\Extension\Csrf\EventListener;


Maybe move the class one level up and rename it to ValidationError so that the FQCN reads Symfony\Component\Form\Extension\Csrf\ValidationError?

xabbuh · 2018-09-30T10:37:15Z

I left two minor comments, but apart from that the changes look good to me. Great work! 👍

ro0NL · 2018-09-30T11:40:53Z

src/Symfony/Component/Form/Extension/Csrf/EventListener/CsrfValidationListener.php

                $errorMessage = $this->errorMessage;

                if (null !== $this->translator) {
                    $errorMessage = $this->translator->trans($errorMessage, array(), $this->translationDomain);
                }

-                $form->addError(new FormError($errorMessage));
+                $form->addError(new FormError($errorMessage, null, array(), null, new ValidationError($csrfToken)));


isnt $csrfToken the actual cause here? what's the point of getCause()->getCsrfToken() vs. getCause() 🤔

I added a comment here. If I understand correctly it seems that CSRF token does not always exist. So I can not set CsrfToken object directly as cause.

now it's declared at L63 :)

ro0NL · 2018-09-30T11:53:23Z

src/Symfony/Component/Form/Extension/Csrf/EventListener/CsrfValidationListener.php

@@ -59,14 +60,15 @@ public function preSubmit(FormEvent $event)
        if ($form->isRoot() && $form->getConfig()->getOption('compound') && !$postRequestSizeExceeded) {
            $data = $event->getData();

-            if (!isset($data[$this->fieldName]) || !$this->tokenManager->isTokenValid(new CsrfToken($this->tokenId, $data[$this->fieldName]))) {
+            $csrfToken = new CsrfToken($this->tokenId, $data[$this->fieldName]);


$data[$this->fieldName] ?? null

then there's always a valid token object for a cause

It doesn't feel right to me this way. Waiting for @xabbuh feedback.

@ro0NL has a good point here. Just having the CsrfToken class as the cause sounds good to me.

xabbuh · 2018-09-30T13:26:44Z

src/Symfony/Component/Form/Extension/Csrf/EventListener/CsrfValidationListener.php

                $errorMessage = $this->errorMessage;

                if (null !== $this->translator) {
                    $errorMessage = $this->translator->trans($errorMessage, array(), $this->translationDomain);
                }

-                $form->addError(new FormError($errorMessage));
+                $form->addError(new FormError($errorMessage, null, array(), null, $csrfToken));


the second argument (message template) should be $errorMessage too

gmponos · 2018-09-30T13:39:09Z

Thank you for the review.. Let's hope the build will pass... Can you check now 😄

ro0NL

looks good to me (with minor comment)

ro0NL · 2018-09-30T20:06:11Z

src/Symfony/Component/Form/CHANGELOG.md

@@ -7,6 +7,7 @@ CHANGELOG
 * deprecated the `$scale` argument of the `IntegerToLocalizedStringTransformer`
 * added `Symfony\Component\Form\ClearableErrorsInterface`
 * deprecated calling `FormRenderer::searchAndRenderBlock` for fields which were already rendered
+ * added a cause when a CsrfError has occurred


... CSRF error ...

…ionListener

fabpot · 2018-10-01T05:27:59Z

Thank you @gmponos.

@xabbuh

… on CsrfValidationListener (gmponos) This PR was squashed before being merged into the 4.2-dev branch (closes #28564). Discussion ---------- [Form] Added a cause on when a Csrf Error has occurred on CsrfValidationListener | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28427 | License | MIT | Doc PR | symfony/symfony-docs The Csrf error that it is added as a `FormError` does not have a cause making it hard to filter it out. 1. I am not sure if this is a bug or a feature. 2. @xabbuh on the issue you said something about `CsrfValidationError`. I didn't quite get that. In the current PR you can see what I was thinking about. Let me know your thoughts and continue the discussion here. Commits ------- 162d0be [Form] Added a cause on when a Csrf Error has occurred on CsrfValidationListener

fabpot · 2018-10-01T05:29:58Z

Something went wrong when merging this PR. @gmponos Can you resubmit it again? Sorry for the trouble.

gmponos · 2018-10-01T07:31:30Z

@fabpot done... I have also squashed the commits. I hope that doesn't cause more trouble.

@gmponos

…e FormError object (gmponos) This PR was merged into the 4.2-dev branch. Discussion ---------- When a CSRF occures on a Form submit add a cause on the FormError object | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #28427 | License | MIT | Doc PR | symfony/symfony-docs This is a resubmitted PR of this: #28564 > Something went wrong when merging this PR. @gmponos Can you resubmit it again? Sorry for the trouble. Commits ------- e54e94c When a CSRF occures on a Form submit add a cause on the FormError object

carsonbot added Status: Needs Review Form Bug Feature labels Sep 23, 2018

xabbuh requested changes Sep 23, 2018

View reviewed changes

carsonbot added Status: Needs Work and removed Status: Needs Review labels Sep 23, 2018

nicolas-grekas removed the Bug label Sep 23, 2018

nicolas-grekas added this to the next milestone Sep 23, 2018

xabbuh reviewed Sep 30, 2018

View reviewed changes

gmponos mentioned this pull request Sep 30, 2018

[Form] FormErrorIterator do not filter only with Constraint causes #28649

Closed

ro0NL reviewed Sep 30, 2018

View reviewed changes

xabbuh reviewed Sep 30, 2018

View reviewed changes

ro0NL approved these changes Sep 30, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Sep 30, 2018

ro0NL reviewed Sep 30, 2018

View reviewed changes

xabbuh approved these changes Sep 30, 2018

View reviewed changes

fabpot approved these changes Oct 1, 2018

View reviewed changes

[Form] Added a cause on when a Csrf Error has occurred on CsrfValidat…

162d0be

…ionListener

fabpot force-pushed the issue#28427 branch from b22cbd0 to 162d0be Compare October 1, 2018 05:27

gmponos requested a review from dunglas as a code owner October 1, 2018 05:27

carsonbot added Status: Needs Review and removed Status: Reviewed labels Oct 1, 2018

fabpot merged commit 162d0be into symfony:master Oct 1, 2018

gmponos mentioned this pull request Oct 1, 2018

When a CSRF occures on a Form submit add a cause on the FormError object #28656

Merged

nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Added a cause on when a Csrf Error has occurred on CsrfValidationListener #28564

[Form] Added a cause on when a Csrf Error has occurred on CsrfValidationListener #28564

gmponos commented Sep 23, 2018 •

edited by nicolas-grekas

Loading

xabbuh Sep 23, 2018

gmponos Sep 23, 2018 •

edited

Loading

gmponos Sep 23, 2018

gmponos Sep 28, 2018

xabbuh Sep 28, 2018

gmponos Sep 29, 2018

xabbuh Sep 30, 2018

gmponos Sep 30, 2018

xabbuh Sep 30, 2018

xabbuh commented Sep 30, 2018

ro0NL Sep 30, 2018

gmponos Sep 30, 2018

ro0NL Sep 30, 2018

ro0NL Sep 30, 2018 •

edited

Loading

gmponos Sep 30, 2018

xabbuh Sep 30, 2018

xabbuh Sep 30, 2018

gmponos commented Sep 30, 2018

ro0NL left a comment

ro0NL Sep 30, 2018

gmponos Sep 30, 2018

fabpot commented Oct 1, 2018

fabpot commented Oct 1, 2018

gmponos commented Oct 1, 2018

[Form] Added a cause on when a Csrf Error has occurred on CsrfValidationListener #28564

[Form] Added a cause on when a Csrf Error has occurred on CsrfValidationListener #28564

Conversation

gmponos commented Sep 23, 2018 • edited by nicolas-grekas Loading

Choose a reason for hiding this comment

gmponos Sep 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Sep 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Sep 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmponos commented Sep 30, 2018

ro0NL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Oct 1, 2018

fabpot commented Oct 1, 2018

gmponos commented Oct 1, 2018

gmponos commented Sep 23, 2018 •

edited by nicolas-grekas

Loading

gmponos Sep 23, 2018 •

edited

Loading

ro0NL Sep 30, 2018 •

edited

Loading