Improve file upload validation

[marcus-at-localhost]

I noticed an issue with missing csrf tokens[1], when uploading larger files through a public facing form. [2] After finally looking into the php error log I saw this warning: PHP Warning: POST Content-Length of 19119038 bytes exceeds the limit of 8388608 bytes in Unknown on line 0 and with upping the allowed filesize/post size in php.ini

upload_max_filesize = 64M
post_max_size = 192M

I was good to go.

Unfortunately there is no indication of those limits outside of kirby in the plugin, so I would propose a check for these values, maybe behind the debug flag like kirby itself is doing: https://github.com/getkirby/kirby/blob/04127160ed1ab8dc277763cd8ba4ebc5a097bc18/src/Api/Api.php#L747-L756

if (empty($files) === true) {
    $postMaxSize       = Str::toBytes(ini_get('post_max_size'));
    $uploadMaxFileSize = Str::toBytes(ini_get('upload_max_filesize'));

    if ($postMaxSize < $uploadMaxFileSize) {
        throw new Exception(t('upload.error.iniPostSize'));
    } else {
        throw new Exception(t('upload.error.noFiles'));
    }
}

I would have sent a pull request, but I'm not sure where to place that check.

Maybe this is something to consider. Thanks!

[1] Just for the record, check for token happens here https://github.com/mzur/kirby-form/blob/38ce34bdf51079e099d71131aafbc1a543bd7ea4/src/Form.php#L163 - also csrf tokens dont make so much sense for public facing forms if no authentication is involved. [2] With version 3.1.0 the token was present in the header though, but the whole form was empty :)

[mzur]

Thanks for reporting this. The upload/POST size limits are basic PHP configuration and IMO an extra check is out of scope of this package. If anything, this could be done by Kirby itself. But you report this in context of CSRF tokens. How are the upload size limits and CSRF validation related?

[marcus-at-localhost]

Sorry, I missed your response and I'm a bit out of the loop. I mentioned the csrf tokens as part of the context how I discovered this bug, because it was not obvious why I got an error message that the token is invalid. And the reason is that the upload silently fails if files are too big.

And Kirby is already doing the check of those ini values as referenced above, but I guess only for it's internal api. Your plugin is (correct my if I'm wrong) a way to simplify the process of doing this manually: https://getkirby.com/docs/cookbook/forms/file-uploads, correct?

Checking for valid files is up to the user, but since transmitting and validation of forms is wrapped in your plugin, there is no way for me to hook into this process (if I remember right).

So I think you are right that the responsibility to validate files, is on Kirby CMS, at the same time (according to their example) they give me all the tools to do that, so I would argue this should be part of your plugin, since it abstracts away the manual way of validating uploaded files. Not?

I hope that makes sense :)

[mzur]

You have a good point there, thanks!

The additional validation could be implemented here (where all previous validation attempts have failed). If the current validation rule is file then all the error codes could be checked and an appropriate validation message appended to the errors array. Alternatively, this could throw en exception, which would basically disclose this information in debug mode only. Check if errors like these could expose some sensitive information to decide between the two methods.

Anyone wanting to implement this, just let me know here and have a go. @marcus-at-localhost What about you?

[marcus-at-localhost]

I'll give it a shot as soon as possible. Thanks for adding some context, that helps!