[Form] Skip CSRF validation on form when POST max size is exceeded #19373
Conversation
|
You should also add the params to |
|
@fabpot updated |
| } | ||
|
|
||
| public function preSubmit(FormEvent $event) | ||
| { | ||
| $form = $event->getForm(); | ||
| $method = $form->getConfig()->getMethod(); |
There was a problem hiding this comment.
This intermediate var can be removed and the code inlined as it's not reused anywhere.
|
👍 |
| $contentLength = $this->getContentLength(); | ||
| $maxContentLength = $this->getPostMaxSize(); | ||
|
|
||
| return !empty($maxContentLength) && $contentLength > $maxContentLength; |
There was a problem hiding this comment.
Why is the call to empty() required?
There was a problem hiding this comment.
If there's no limit set on the max POST size then we can't validate whether or not it has been exceeded.
There was a problem hiding this comment.
I mean, we could omit the call to empty() this way:
return $maxContentLength && $contentLength > $maxContentLength;There was a problem hiding this comment.
I don't see any benefit in doing that, it just reduces readability in my opinion
There was a problem hiding this comment.
$maxContentLength is int|null so !empty does not bring any value.
There was a problem hiding this comment.
If it can be an int or null then empty is entirely useful as it returns true for 0 and null...
There was a problem hiding this comment.
I guess you mean the opposite :)
There was a problem hiding this comment.
I would indeed remove empty. I tried very hard to never use empty and be explicit instead.
There was a problem hiding this comment.
Removed usage of empty
|
Thank you @jameshalsall. |
…exceeded (jameshalsall) This PR was squashed before being merged into the 2.7 branch (closes #19373). Discussion ---------- [Form] Skip CSRF validation on form when POST max size is exceeded | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #19140 | License | MIT | Doc PR | N/A In #19140 the CSRF validation listener was not aware that the POST max size had exceeded, and was adding a form error message that wasn't relevant to the actual error. This introduces the `ServerParams` utility class into the `CsrfValidationListener` and checks that the POST max size has not been exceeded. If it has then it won't bother trying to validate the CSRF token. My main concern with this change is that it opens up an attack vector around tokens, but I've encapsulated the request size validation in a single method in `ServerParams` now so that the request handlers are using the same logic. Commits ------- 289531f [Form] Skip CSRF validation on form when POST max size is exceeded
In #19140 the CSRF validation listener was not aware that the POST max size had exceeded, and was adding a form error message that wasn't relevant to the actual error.
This introduces the
ServerParamsutility class into theCsrfValidationListenerand checks that the POST max size has not been exceeded. If it has then it won't bother trying to validate the CSRF token.My main concern with this change is that it opens up an attack vector around tokens, but I've encapsulated the request size validation in a single method in
ServerParamsnow so that the request handlers are using the same logic.