Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[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.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
[Form] Skip CSRF validation on form when POST max size is exceeded#19373
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fabpot commentedJul 19, 2016
You should also add the params to |
jameshalsall commentedJul 19, 2016
@fabpot updated |
| publicfunctionpreSubmit(FormEvent$event) | ||
| { | ||
| $form =$event->getForm(); | ||
| $method =$form->getConfig()->getMethod(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This intermediate var can be removed and the code inlined as it's not reused anywhere.
fabpot commentedJul 20, 2016
👍 |
| { | ||
| $contentLength =$this->getContentLength(); | ||
| $maxContentLength =$this->getPostMaxSize(); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why is the call toempty() required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I mean, we could omit the call toempty() this way:
return$maxContentLength &&$contentLength >$maxContentLength;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I don't see any benefit in doing that, it just reduces readability in my opinion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
$maxContentLength isint|null so!empty does not bring any value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I guess you mean the opposite :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would indeed removeempty. I tried very hard to never useempty and be explicit instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Removed usage ofempty
fabpot commentedAug 15, 2016
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/AIn#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.