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

WP-r44443: Upload: Fix upload failures of common text file types. #354

Merged
merged 2 commits into from Jun 18, 2019

Conversation

@PaoloFalomo
Copy link
Contributor

commented Jan 30, 2019

This adds some special case handling in 'wp_check_filetype_and_ext()' that prevents some common file types from being blocked based on mismatched MIME checks, which were made more strict in WordPress 5.0.1.

Merges https://core.trac.wordpress.org/changeset/44438, https://core.trac.wordpress.org/changeset/44439, https://core.trac.wordpress.org/changeset/44441, and https://core.trac.wordpress.org/changeset/44442 to the 4.9 branch.

WP:Props Kloon, birgire, tellyworth, joemcgill.
See https://core.trac.wordpress.org/ticket/45615.


Merges https://core.trac.wordpress.org/changeset/44443 / WordPress/wordpress-develop@d9a6440 to ClassicPress.

WP-r44443: Upload: Fix upload failures of common text file types.
This adds some special case handling in 'wp_check_filetype_and_ext()' that prevents some common file types from being blocked based on mismatched MIME checks, which were made more strict in WordPress 5.0.1.

Merges https://core.trac.wordpress.org/changeset/44438, https://core.trac.wordpress.org/changeset/44439, https://core.trac.wordpress.org/changeset/44441, and https://core.trac.wordpress.org/changeset/44442 to the 4.9 branch.

WP:Props Kloon, birgire, tellyworth, joemcgill.
See https://core.trac.wordpress.org/ticket/45615.

----
Merges https://core.trac.wordpress.org/changeset/44443 / WordPress/wordpress-develop@d9a6440 to ClassicPress.
@nylen

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

I'd like to understand more about what issues this PR fixes. Do you have an example of a file which fails to upload before this PR, then works fine after this PR is applied?

@Mte90

This comment has been minimized.

Copy link
Collaborator

commented Jan 31, 2019

As written here https://make.wordpress.org/core/2018/12/13/backwards-compatibility-breaks-in-5-0-1/ for safety reason they added this checks but few plugins was broken after that.
So they are working on another fix that will be released for 5.2 https://core.trac.wordpress.org/ticket/40175

@Mte90

This comment has been minimized.

Copy link
Collaborator

commented Mar 16, 2019

I am testing this and right now in ClassicPress is not possible to upload the files of this pr.
I get this error:
immagine
We should enable to upload csv and the other formats, I will test the patch anyway right now.

@Mte90

This comment has been minimized.

Copy link
Collaborator

commented Mar 16, 2019

With the patch the csv file was uploaded
immagine

@Mte90

This comment has been minimized.

Copy link
Collaborator

commented Mar 16, 2019

As I can see the dxfp is part of the unit tests but is not used (maybe an error?) but with that patch the csv are uploaded without issue,

@nylen

This comment has been minimized.

Copy link
Member

commented Mar 26, 2019

@Mte90 thanks for the additional context, so this change is basically "fix CSV uploads on some servers, and a few other related cases where files are occasionally detected as plain text."

File upload verification is always going to be a mess as long as it relies on the fileinfo extension, which can return different results on different servers.

Still, this seems like a good change to keep up with WP. According to https://core.trac.wordpress.org/ticket/45615#comment:39 and https://core.trac.wordpress.org/ticket/45615#comment:42 dfxp files are not meant to be supported out of the box.

There's one more thing I'd like us to do in this PR, which is https://core.trac.wordpress.org/ticket/45707 (adding a new filter parameter that makes it easier to override file upload behavior, if needed). I'll add this commit to the PR now.

In the future we should also watch https://core.trac.wordpress.org/ticket/40175 (big one) and https://core.trac.wordpress.org/ticket/45633 for other changes to upload verification.

WP-r44677: Media: Add a `$real_mime` parameter to the `wp_check_filet…
…ype_and_ext` filter.

This allows more accurate filtering of the filename and extension given to uploaded files.

WP:Props desrosj, Tkama.
Fixes https://core.trac.wordpress.org/ticket/45707.

Conflicts:
  src/wp-includes/functions.php
----
Merges https://core.trac.wordpress.org/changeset/44677 / WordPress/wordpress-develop@1d2153b to ClassicPress.
@Mte90

This comment has been minimized.

Copy link
Collaborator

commented Mar 28, 2019

I am following both tickets, do you need any tests before merge this pr?

@nylen

This comment has been minimized.

Copy link
Member

commented Apr 1, 2019

If @invisnet has time to look at this PR this week I think that would be a good idea.

Some additional context:

Due to recent hardening of upload file validation, some files (CSV and RTF for example) may fail validation in WP 4.9.x. More info: https://make.wordpress.org/core/2018/12/13/backwards-compatibility-breaks-in-5-0-1/

This was relaxed a bit in a later WP 5.x version (but not in 4.9.x) by allowing "a few common file types [that] are occasionally detected as text/plain" to pass validation. This PR ports the changes to relax validation to ClassicPress.

I was able to confirm the changes work as expected. On my system, the 3 new test files added here fail to upload before these changes and pass after these changes, because they are covered by the new conditions added here:

{"filename":"test.vtt","type_claimed":"text/vtt","type_detected":"text/plain"}
{"filename":"test.csv","type_claimed":"text/csv","type_detected":"text/plain"}
{"filename":"test.rtf","type_claimed":"application/rtf","type_detected":"text/rtf"}

We haven't had any reports of this as a bug in ClassicPress.

@nylen

This comment has been minimized.

Copy link
Member

commented Jun 18, 2019

I have been thinking about how we decide whether to accept changes from WP. We can't accept all of them, there are just too many and a lot of them wouldn't make sense anyway. I did come up with something more concrete than that though:

  • The change impacts existing ClassicPress users. (We haven't had any reports of users failing to upload CSV or similar files.)
  • The change is not going to break any other use cases. (This code is far from ideal from a security perspective but it doesn't appear to introduce any new issues.)
  • ✔️ The change has automated tests.
  • ✔️ I understand the change very well, or I can ask questions of someone who understands it very well. (See above comments.)

Overall, I think this is good enough to accept.

@nylen

nylen approved these changes Jun 18, 2019

@nylen nylen merged commit b86cee8 into ClassicPress:develop Jun 18, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.