8000 [Form] Skip CSRF validation on form when POST max size is exceeded by jameshalsall · Pull Request #19373 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Form] Skip CSRF validation on form when POST max size is exceeded #19373

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.

Sign up for GitHub

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

Closed
wants to merge 6 commits into from
Closed

[Form] Skip CSRF validation on form when POST max size is exceeded #19373

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
317e7c3
Skip CSRF validation on form when POST max size is exceeded
jameshalsall Jul 17, 2016
ac408e1
Make sure the form method is POST when checking request size
jameshalsall Jul 17, 2016
0e48952
Fix coding standards violations
jameshalsall Jul 17, 2016
87150cd
Pass ServerParams from FormTypeCsrfExtension to CsrfValidationListener
jameshalsall Jul 19, 2016
cbcf8fd
Remove unused variable assignment
jameshalsall Jul 20, 2016
3c4b8a7
Remove usage of empty() in form ServerParams util
Jul 29, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Remove unused variable assignment
  • Loading branch information
@jameshalsall
jameshalsall committed Jul 20, 2016
commit cbcf8fdc907bc3820e45274fdfef7e78cbd2ce8c
3 changes: 1 addition & 2 deletions src/Symfony/Component/Form/Extension/Csrf/EventListener/CsrfValidationListener.php
10000
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,7 @@ public function __construct($fieldName, $tokenManager, $tokenId, $errorMessage,
public function preSubmit(FormEvent $event)
{
$form = $event->getForm();
$method = $form->getConfig()->getMethod();
$postRequestSizeExceeded = $method === 'POST' && $this->serverParams->hasPostMaxSizeBeenExceeded();
$postRequestSizeExceeded = $form->getConfig()->getMethod() === 'POST' && $this->serverParams->hasPostMaxSizeBeenExceeded();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it also work for PUT methods?

Copy link
Contributor
@HeahDude HeahDude Jul 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And PATCH. Please also use a yoda style condition.

But it'd better be:

!in_array($form->getConfig()->getMethod(), array('GET', 'HEAD', 'TRACE'), true) && $this->serverParams->hasPostMaxSizeBeenExceeded()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PUT and PATCH are fetched from php://inputstream, not via $_POST - I don't believe they're affected by the post_max_size ini setting

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True you are right 😉 Maybe add a comment for that clarifying we only need to do it for POST?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remain consistent since this is the logic in the request handler, see https://github.com/HeahDu 10000 de/symfony/blob/master/src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php#L60, so null will be submitted anyway and the token won't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@HeahDude very good point, I wonder whether some of the logic in the ServerParams around POST max size would be better being moved to HttpFoundation? cc/ @nicolas-grekas

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic is the same for the native request handler too (ref https://github.com/HeahDude/symfony/blob/master/src/Symfony/Component/Form/NativeRequestHandler.php#L68), so I suggest to keep things simple here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

POST request size is not exclusive to forms, so adding too much logic into a Form component class doesn't make much sense to me.

Would be better to move the logic around max POST (and PATCH, PUT) to HttpFoundation and then use that component here instead.

Sent from my iPhone

On 29 Jul 2016, at 14:02, Jules Pietri notifications@github.com wrote:

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

 }

 public function preSubmit(FormEvent $event)
 {
     $form = $event->getForm();

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fabpot do you think it's worth me moving the logic for checking if max POST size is exceeded to HttpFoundation component or is this good to merge as is?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is good as is.


if ($form->isRoot() && $form->getConfig()->getOption('compound') && !$postRequestSizeExceeded) {
$data = $event->getData();
Expand Down
0