Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[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

Show file tree
Hide file tree
Changes from all commits
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
1 change: 1 addition & 0 deletions 1 src/Symfony/Bundle/FrameworkBundle/Resources/config/form_csrf.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
<argument>%form.type_extension.csrf.field_name%</argument>
<argument type="service" id="translator.default" />
<argument>%validator.translation_domain%</argument>
<argument type="service" id="form.server_params" />
</service>
</services>
</container>
12 changes: 10 additions & 2 deletions 12 src/Symfony/Component/Form/Extension/Csrf/EventListener/CsrfValidationListener.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
use Symfony\Component\Form\FormEvents;
use Symfony\Component\Form\FormError;
use Symfony\Component\Form\FormEvent;
use Symfony\Component\Form\Util\ServerParams;
use Symfony\Component\Security\Csrf\CsrfToken;
use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
use Symfony\Component\Translation\TranslatorInterface;
Expand Down Expand Up @@ -68,14 +69,19 @@ class CsrfValidationListener implements EventSubscriberInterface
*/
private $translationDomain;

/**
* @var ServerParams
*/
private $serverParams;

public static function getSubscribedEvents()
{
return array(
FormEvents::PRE_SUBMIT => 'preSubmit',
);
}

public function __construct($fieldName, $tokenManager, $tokenId, $errorMessage, TranslatorInterface $translator = null, $translationDomain = null)
public function __construct($fieldName, $tokenManager, $tokenId, $errorMessage, TranslatorInterface $translator = null, $translationDomain = null, ServerParams $serverParams = null)
{
if ($tokenManager instanceof CsrfProviderInterface) {
$tokenManager = new CsrfProviderAdapter($tokenManager);
Expand All @@ -89,13 +95,15 @@ public function __construct($fieldName, $tokenManager, $tokenId, $errorMessage,
$this->errorMessage = $errorMessage;
$this->translator = $translator;
$this->translationDomain = $translationDomain;
$this->serverParams = $serverParams ?: new ServerParams();
}

public function preSubmit(FormEvent $event)
{
$form = $event->getForm();
$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/HeahDude/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')) {
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]))) {
Expand Down
12 changes: 10 additions & 2 deletions 12 src/Symfony/Component/Form/Extension/Csrf/Type/FormTypeCsrfExtension.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\FormView;
use Symfony\Component\Form\FormInterface;
use Symfony\Component\Form\Util\ServerParams;
use Symfony\Component\OptionsResolver\Options;
use Symfony\Component\OptionsResolver\OptionsResolver;
use Symfony\Component\Security\Csrf\CsrfTokenManagerInterface;
Expand Down Expand Up @@ -55,7 +56,12 @@ class FormTypeCsrfExtension extends AbstractTypeExtension
*/
private $translationDomain;

public function __construct($defaultTokenManager, $defaultEnabled = true, $defaultFieldName = '_token', TranslatorInterface $translator = null, $translationDomain = null)
/**
* @var ServerParams
*/
private $serverParams;

public function __construct($defaultTokenManager, $defaultEnabled = true, $defaultFieldName = '_token', TranslatorInterface $translator = null, $translationDomain = null, ServerParams $serverParams = null)
{
if ($defaultTokenManager instanceof CsrfProviderInterface) {
$defaultTokenManager = new CsrfProviderAdapter($defaultTokenManager);
Expand All @@ -68,6 +74,7 @@ public function __construct($defaultTokenManager, $defaultEnabled = true, $defau
$this->defaultFieldName = $defaultFieldName;
$this->translator = $translator;
$this->translationDomain = $translationDomain;
$this->serverParams = $serverParams;
}

/**
Expand All @@ -89,7 +96,8 @@ public function buildForm(FormBuilderInterface $builder, array $options)
$options['csrf_token_id'] ?: ($builder->getName() ?: get_class($builder->getType()->getInnerType())),
$options['csrf_message'],
$this->translator,
$this->translationDomain
$this->translationDomain,
$this->serverParams
))
;
}
Expand Down
5 changes: 1 addition & 4 deletions 5 src/Symfony/Component/Form/Extension/HttpFoundation/HttpFoundationRequestHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,7 @@ public function handleRequest(FormInterface $form, $request = null)
// Mark the form with an error if the uploaded size was too large
// This is done here and not in FormValidator because $_POST is
// empty when that error occurs. Hence the form is never submitted.
$contentLength = $this->serverParams->getContentLength();
$maxContentLength = $this->serverParams->getPostMaxSize();

if (!empty($maxContentLength) && $contentLength > $maxContentLength) {
if ($this->serverParams->hasPostMaxSizeBeenExceeded()) {
// Submit the form, but don't clear the default values
$form->submit(null, false);

Expand Down
5 changes: 1 addition & 4 deletions 5 src/Symfony/Component/Form/NativeRequestHandler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,7 @@ public function handleRequest(FormInterface $form, $request = null)
// Mark the form with an error if the uploaded size was too large
// This is done here and not in FormValidator because $_POST is
// empty when that error occurs. Hence the form is never submitted.
$contentLength = $this->serverParams->getContentLength();
$maxContentLength = $this->serverParams->getPostMaxSize();

if (!empty($maxContentLength) && $contentLength > $maxContentLength) {
if ($this->serverParams->hasPostMaxSizeBeenExceeded()) {
// Submit the form, but don't clear the default values
$form->submit(null, false);

Expand Down
22 changes: 22 additions & 0 deletions 22 src/Symfony/Component/Form/Tests/Extension/Csrf/EventListener/CsrfValidationListenerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

namespace Symfony\Component\Form\Tests\Extension\Csrf\EventListener;

use Symfony\Component\Form\Form;
use Symfony\Component\Form\FormBuilder;
use Symfony\Component\Form\FormEvent;
use Symfony\Component\Form\Extension\Csrf\EventListener\CsrfValidationListener;
Expand Down Expand Up @@ -72,4 +73,25 @@ public function testStringFormData()
// Validate accordingly
$this->assertSame($data, $event->getData());
}

public function testMaxPostSizeExceeded()
{
$serverParams = $this
->getMockBuilder('\Symfony\Component\Form\Util\ServerParams')
->disableOriginalConstructor()
->getMock()
;

$serverParams
->expects($this->once())
->method('hasPostMaxSizeBeenExceeded')
->willReturn(true)
;

$event = new FormEvent($this->form, array('csrf' => 'token'));
$validation = new CsrfValidationListener('csrf', $this->tokenManager, 'unknown', 'Error message', null, null, $serverParams);

$validation->preSubmit($event);
$this->assertEmpty($this->form->getErrors());
}
}
13 changes: 13 additions & 0 deletions 13 src/Symfony/Component/Form/Util/ServerParams.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,19 @@ public function __construct(RequestStack $requestStack = null)
$this->requestStack = $requestStack;
}

/**
* Returns true if the POST max size has been exceeded in the request.
*
* @return bool
*/
public function hasPostMaxSizeBeenExceeded()
{
$contentLength = $this->getContentLength();
$maxContentLength = $this->getPostMaxSize();

return $maxContentLength && $contentLength > $maxContentLength;
}

/**
* Returns maximum post size in bytes.
*
Expand Down
Morty Proxy This is a proxified and sanitized view of the page, visit original site.