mzur / kirby-uniform

A versatile Kirby plugin to handle web form actions.
https://kirby-uniform.readthedocs.io
MIT License
252 stars 40 forks source link

file upload mime type validation #139

Closed jklue closed 7 years ago

jklue commented 7 years ago

When I require a mime type for the file upload field, I get the following error:

in_array() expects parameter 2 to be array, string given

It shows the error coming from the following line...

return in_array(f::mime($name), $allowed);

in block of code...

 v::$validators['mime'] = function ($value, $allowed) {
        if (is_string($value)) {
          $name = $value;
        } elseif (is_array($value) && array_key_exists('tmp_name', $value)) {
          // This is for uploaded files from $_FILES
          $name = $value['tmp_name'];
        }
        if (isset($name)) {
          return in_array(f::mime($name), $allowed);
        }
        return false;
    };

Part of my controller looks like this:

            'resume' => [
                'rules' => [
                    'file',
                    'mime' => ['application/pdf'],
                    'filesize' => 2000000,
                ],
                'message' => [
                    'Please choose a file.',
                    'Please choose a PDF.',
                    'Please choose a file that is smaller than 5 MB.',
                ],
            ],

What am I doing wrong?

mzur commented 7 years ago

Your code should work. What do you get when you insert a var_dump($allowed);die; above the in_array line? What PHP, Kirby and Kirby Toolkit version do you use?

jklue commented 7 years ago

The var_dump($allowed) returns the following:

/Users/luke/Sites/vd/taverninthesquare.com/code/site/plugins/uniform/vendor/mzur/kirby-form/src/helpers.php:45:string 'application/pdf' (length=15)

PHP: MAMP PRO 7.1.0 Kirby: 2.5.5 Kirby Toolkit: 2.5.5

mzur commented 7 years ago

Dang, there actually was an error in the way the mime validator handles its arguments. You can update Uniform to v3.2.1 and it will work as expected. If you don't want to update you can do it like this:

'mime' => [['application/pdf']],

Thanks for reporting this :+1:

jklue commented 7 years ago

Thanks for the quick update @mzur! I ran kirby plugin:update mzur/kirby-uniform and it said I'm now on version 3.0.1. I hope that’s the right version.

I'm not getting the error anymore, but I'm also not getting a success. Now the form silently fails without indicating success. Is there some other debugging trick in this instance--maybe look at the mail logs on a test server? (when I don't validate the uploaded file the form will successfully submit)

mzur commented 7 years ago

Yep, I always forget to update the version number in the package.json... (why didn't they use Composer from the start??)

Do you have Kirby's debug mode enabled in the config? Also, you can check the MAMP server logs. I've downloaded the recent Plainkit and installed Uniform v3.2.1 and this works:

<?php
$form = new Uniform\Form([
     'file' => [
         'rules' => [
            'required',
            'file',
            'mime' => ['application/pdf'],
            'filesize' => 2000000
        ],
         'message' => [
            'Please select a file.',
            'Please select a file.',
            'Please select a PDF.',
            'Please select a PDF smaller than 2 MB.',
        ]
     ],
 ]);

 if (r::is('POST')) {
     $form->uploadAction(['fields' => [
        'file' => ['target' => kirby()->roots()->content()],
     ]]);
 }
?>

<?php snippet('header') ?>

<h1><?php echo $page->title()->html() ?></h1>
<form enctype="multipart/form-data" action="<?php echo $page->url() ?>" method="POST">
    <input type="file" name="file" required/>
    <?php echo csrf_field() ?>
    <?php echo honeypot_field() ?>
    <input type="submit" value="Submit">
</form>
<?php if ($form->success()): ?>
    Success!
<?php else: ?>
    <?php snippet('uniform/errors', ['form' => $form]) ?>
<?php endif; ?>

<?php snippet('footer') ?>
jklue commented 7 years ago

I'm just learning about Composer and really loving it! Especially when it comes time to deploy this to the server--submodules wasn't cutting it.

Thanks for the test case. I had renamed the input name "file" to "resume" and changed it throughout, but only when I use the name "file" for the input and for validation and upload does it seem to work.

The only error I get now is when I try to upload a file that is waaaay over the max, like 169mb PDF. I get "The CSRF token was invalid" every time in this edge case. (I have debug mode enabled)

mzur commented 7 years ago

The code works for me even when I change the input field name from file to resume. Can you double check that the input field name is set correctly in the template, in the validation rules array and in the action options array?

If you try to upload a very big file, PHP's post_max_size configuration may abort the request. You'll see a warning like this:

PHP Warning:  POST Content-Length of xxxxxxx bytes exceeds the limit of yyyy bytes in Unknown on line 0

Because of this the $_POST array is empty and does not contain the CSRF token, hence the error message. Unfortunately you cannot catch such an error in PHP. What you can do is validate the file size with JavaScript before you allow the upload.

jklue commented 7 years ago

Thanks. I was able to get it to work using resume. And it validates well on file type or if no file was selected. Now I can't get the filesize limit to work at low levels. I'll start fresh with the plainkit and work up from there.

jklue commented 7 years ago

It works now when I move the ->uploadAction to be the first $form action in the chain. Before I was doing an emailAction, logAction, and emailSelectAction first.

mzur commented 7 years ago

Note that the filesize must be specified in kb rather than bytes. Also, there is PHP's upload_max_filesize configuration (in addition to post_max_size) that may play a role. I just tested the upload with a preceding logAction and it works. Does it work for you if you perform the logAction first, too?

jklue commented 7 years ago

Yes, I can confirm it works even if the logAction is first. There was a bug somewhere else in my code. Thanks for staying with this one!