Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PHP file upload error from backend shows success #428

Open
torohill opened this issue Jun 23, 2020 · 3 comments
Open

PHP file upload error from backend shows success #428

torohill opened this issue Jun 23, 2020 · 3 comments

Comments

@torohill
Copy link
Contributor

If there is a PHP file upload error when handling a file upload (eg. https://www.php.net/manual/en/features.file-upload.errors.php) the frontend still shows success.

Reproduction steps:

  1. Create a page with a file upload field.
  2. Change the permissions on PHP's upload_tmp_dir directory to cause a permission error.
  3. Select a file for upload, which gets uploaded via AJAX before the form is submitted.
  4. Note the tick mark that indicates a successful upload.
  5. Use the browser dev tools to see that the response from the server for the POST to task:file-upload was {"status":"error","message":"Unable to upload file filename.txt: "}.

I took a look at the code in the handleError function in app/fields/file.js line 206 and it simple returns true. This code should probably display a failure and show the error message returned from the backend.

Also, the error message returned from the backend doesn't look to be complete. I will submit a pull request to fix this in a bit.

@torohill
Copy link
Contributor Author

For a little more detail on step 2 from above:

  • By default both sessions and upload temporary files are stored in /tmp.
  • Making /tmp not writable by PHP causes session creation to fail, which then makes it impossible to test file uploads.
  • When I changed upload_tmp_dir to a directory other than /tmp and made it not writable PHP automatically defaulted back to /tmp because of the permissions error, with a warning that the system temporary directory was used for a file upload.

I ended up doing the following to reliably reproduce the error:

  • Change session.save_path to something other than /tmp.
  • Leave upload_tmp_dir set to empty, which defaults to the system temporary directory (ie. /tmp).
  • Change the permissions on /tmp to make it not writable by PHP.

@ricardo118
Copy link
Contributor

this seems like a lot of loops to trigger an error that ive never seen come up? The problems plugin will also error when /tmp isnt writeable and its a default plugin that comes with grav

@torohill
Copy link
Contributor Author

I had an issue with PHP generating file upload errors in production, in that case it was large file uploads and PHP failing to write to disk. After investigating I saw that the Form class was using $upload_errors when it wasn't defined, and the frontend wasn't displaying the error returned from the backend.

The reproduction steps above are simply the best I could come up with to reliably reproduce the error in a dev environment. Any set of circumstances which causes PHP to generate one of the file upload errors (https://www.php.net/manual/en/features.file-upload.errors.php) will trigger the same behaviour.

torohill added a commit to torohill/grav-plugin-form that referenced this issue Jun 23, 2020
rhukster pushed a commit that referenced this issue Jul 9, 2020
* Add php upload error messages to form class. #428

* Move php file upload errors messages to language file. #428

* Add docs comment to language variable.

* Add unknown fileupload error instead of defaulting to ok.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants