Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 4 years ago

#45615 closed defect (bug) (fixed)

CSV Mime Type fails upload

Reported by: kloon's profile Kloon Owned by: joemcgill's profile joemcgill
Milestone: 5.0.3 Priority: normal
Severity: major Version: 5.0.1
Component: Upload Keywords: has-unit-tests
Focuses: Cc:

Description

CSV is an allowed upload type, however since WP 5.0.1 the mime type is now checked against the file. CSV files can however have text/csv as well as text/plain mime types, WP only caters for the first.

Attachments (11)

csv_mime_type.diff (671 bytes) - added by Kloon 6 years ago.
Add csv to text/plain mime types
45615-2.diff (1.5 KB) - added by birgire 6 years ago.
45615-tests.diff (4.1 KB) - added by tellyworth 6 years ago.
Tests for csv, tsv, vtt, dfxp files
45615.3.diff (9.0 KB) - added by tellyworth 6 years ago.
Incremental fix and unit tests
45615.4.diff (6.6 KB) - added by joemcgill 6 years ago.
45615.5.diff (12.3 KB) - added by tellyworth 6 years ago.
Rearrange tests, and add tests for adding unsupported types by filter
45615.6.diff (4.1 KB) - added by joemcgill 6 years ago.
45615.7.diff (2.5 KB) - added by joemcgill 6 years ago.
45615.8.diff (2.9 KB) - added by joemcgill 6 years ago.
Add special casing for RTF files
45615.9.diff (25.6 KB) - added by joemcgill 6 years ago.
Ensure test files are included in the patch.
45615.diff (1.9 KB) - added by joemcgill 6 years ago.

Download all attachments as: .zip

Change History (75)

@Kloon
6 years ago

Add csv to text/plain mime types

#1 @swissspidy
6 years ago

  • Keywords has-patch needs-testing added

#2 @swissspidy
6 years ago

  • Version changed from trunk to 5.0.1

#3 @batesy87
6 years ago

Has mime type checking been retrospectively patched into older versions of WP?
Two sites running 4.8.8 and 4.9.9 both seem to have this issue

#5 @birgire
6 years ago

Related #45622 for .vtt files

@birgire
6 years ago

#6 @birgire
6 years ago

  • Keywords has-unit-tests added; needs-testing removed

45615-2.diff adds a test with a sample.csv with sample data from https://www.w3.org/TR/2016/NOTE-tabular-data-primer-20160225/

This ticket was mentioned in Slack in #forums by ipstenu. View the logs.


6 years ago

#8 @iandunn
6 years ago

#45630 was marked as a duplicate.

#9 @SergeyBiryukov
6 years ago

  • Component changed from Filesystem API to Upload
  • Milestone changed from Awaiting Review to 5.0.2

#10 @3omar7adidy
6 years ago

#45655 was marked as a duplicate.

#11 follow-up: @pento
6 years ago

  • Milestone changed from 5.0.2 to 5.0.3

This ticket was mentioned in Slack in #forums by clorith. View the logs.


6 years ago

#13 follow-up: @clearsite
6 years ago

for those in need of a solution NOW, like, everyone, because, well, we DEPEND on this;

https://gist.github.com/rmpel/e1e2452ca06ab621fe061e0fde7ae150

#14 in reply to: ↑ 13 @linuxhombr3
6 years ago

Replying to clearsite:

for those in need of a solution NOW, like, everyone, because, well, we DEPEND on this;

https://gist.github.com/rmpel/e1e2452ca06ab621fe061e0fde7ae150

Thanks a ton! Please send feedback for those of you implementing this. Just tested on my staging server and it worked like a charm as an interim solution!!!

Thanks Team!!

#15 @linuxhombr3
6 years ago

Team - i just implemented this in mu-plugins with no change. Help please!!!

#16 in reply to: ↑ 11 @linuxhombr3
6 years ago

Replying to pento: Hi there! Are these actually available yet buddy?

#17 @clearsite
6 years ago

For those using or trying at least to use my mu-plugin option, please join the discussion on GitHub, I'll try to answer questions as quickly as possible.

@linuxhombr3 ; I'm not affiliated with any 'team' other than my co-workers, in any case not with the WordPress team.
This mu-plugin is not created, endorsed or maintained by Automattic, or any other WordPress affiliated development team other than myself and my co-workers at Internetbureau Clearsite.

Last edited 4 years ago by clearsite (previous) (diff)

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 years ago

#19 @joemcgill
6 years ago

#45699 was marked as a duplicate.

#20 @joemcgill
6 years ago

#45671 was marked as a duplicate.

#21 @joemcgill
6 years ago

#45622 was marked as a duplicate.

#22 @joemcgill
6 years ago

  • Owner set to joemcgill
  • Status changed from new to assigned

I'm assigning myself, but am coordinating with @tellyworth on this and any tickets marked as duplicates to ensure we cover as many reported use cases affected as possible.

@tellyworth
6 years ago

Tests for csv, tsv, vtt, dfxp files

#23 @tellyworth
6 years ago

attachment:45615-tests.diff incorporates unit tests taken from some of the other tickets, plus a couple more, to cover various text/* file types. It also includes a couple of extra cases for misnamed files.

#24 @edemir206
6 years ago

I have a problem with XLS, mime file is correct: application/vnd.ms-excel but keeps saying "not allowed for security reasons"

No problem with XLSX.

WP Multisite (Directory): 5.0.2

#25 @Mahesh901122
6 years ago

Getting different REAL MIME type from function finfo_file(). Looks like this is the PHP version bug. PHP bug ticket id 75380 All other related of MIME.

Testing MIME of file with PHP Code:

echo 'PHP Version: ' . phpversion() . "<br/><br/>";
echo 'file.vtt | ' . mime_content_type( 'file.vtt') . "<br/>";
echo 'file.xml | ' . mime_content_type( 'file.xml') . "<br/>";

On Localhost:

PHP Version: 7.2.4
file.vtt | text/plain
file.xml | text/xml

On Live

PHP Version: 7.0.32-4+ubuntu16.04.1+deb.sury.org+1
file.vtt | text/plain
file.xml | application/xml
Last edited 6 years ago by Mahesh901122 (previous) (diff)

#26 @iandunn
6 years ago

@shodan01 mentioned that .gpx also stopped working.

#27 @birgire
6 years ago

As I understand it:

If we associate a given file extension to multiple mime types in wp_get_mime_types(), then wp_check_filetype() will only pick up the first match, because of the break in the foreach loop when a match is confirmed (src).

So having e.g.

'txt|asc|c|cc|h|srt|vtt' => 'text/plain',

before

'vtt' => 'text/vtt',

will mean that the .vtt file type will only be expected to have the text/plain mime type, regardless of other possible matches.

It also seems that finfo_file() is not consistent determining the real mime type info depending on the system setup.

So simply assigning a file extension to multiple mime types in wp_get_mime_types() will not solve the issue here. An actual support for multiple mime types or mime type "fallbacks" seems to be needed.


In the meanwhile here's a suggestion for a plugin workaround to support a fallback mime type of a given file extension:

/**
 * Support for 'text/plain' as the secondary mime type of .vtt files,
 * in addition to the default 'text/vtt' support.
 */
add_filter( 'wp_check_filetype_and_ext', 'wpse323750_secondary_mime', 99, 4 );    

function wpse323750_secondary_mime( $check, $file, $filename, $mimes ) {
    if ( empty( $check['ext'] ) && empty( $check['type'] ) ) {
        // Adjust to your needs!
        $secondary_mime = [ 'vtt' => 'text/plain' ];

        // Run another check, but only for our secondary mime and not on core mime types.
        remove_filter( 'wp_check_filetype_and_ext', 'wpse323750_secondary_mime', 99, 4 );
        $check = wp_check_filetype_and_ext( $file, $filename, $secondary_mime );
        add_filter( 'wp_check_filetype_and_ext', 'wpse323750_secondary_mime', 99, 4 );
    }
    return $check;
}

and it's possible to extend this further for multiple fallbacks, see here.

Last edited 6 years ago by birgire (previous) (diff)

#28 @iandunn
6 years ago

#45771 was marked as a duplicate.

#29 @audrasjb
6 years ago

#45786 was marked as a duplicate.

#30 @audrasjb
6 years ago

#45665 was marked as a duplicate.

#31 @audrasjb
6 years ago

#45787 was marked as a duplicate.

#32 @audrasjb
6 years ago

I just closed #45665 and #45787 as duplicates. FYI, they were dealing with .aac, .m4a, and fonts files.

This ticket was mentioned in Slack in #core-media by joemcgill. View the logs.


6 years ago

@tellyworth
6 years ago

Incremental fix and unit tests

#34 follow-up: @tellyworth
6 years ago

attachment:45615.3.diff includes a mildly hacky fix for a few file types like CSV that are often misidentified by fileinfo as text/plain.

It also includes a test and partial fix for #45633 - when coupled with the patch on that ticket I believe it should be fixed also. (The patch on that ticket alone did not fix the problem in my testing, see the unit tests in this diff).

This does not fix other file types that have been reported as problematic, like aac and fonts. I haven't yet been able to reproduce those, most likely because I don't have sample files that are specific enough. A similar approach to the text/plain fix might work for those, but we need sample files in order to test.

This ticket was mentioned in Slack in #core-media by tellyworth. View the logs.


6 years ago

@joemcgill
6 years ago

#36 in reply to: ↑ 34 @joemcgill
6 years ago

Replying to tellyworth:

The approach in 45615.3.diff is ok for 5.0.3, and then we can follow with a broader refactor to make this less "hacky" as you put it. I don't think we should take this occasion to add support for .json files, even though the workaround for #45633 is mildly annoying for now. I've modified your approach slightly in that regard and to also add back support for vtt files when they're reported as text/plain by the server, which should fix #45622.

I think we can take a similar approach to add support for other miscellaneous types that are commonly misidentified.

Last edited 6 years ago by joemcgill (previous) (diff)

#37 @harmr
6 years ago

Any chance that the fix for 5.0.3 can also re-add gpx file upload support again? We are currently getting lots of tickets because of this issue. A possible workaround for us would be to change the registered mimetype in our plugin from

application/gpx+xml

to

application/gpx

but we would rather like to prevent this, as formerly uploaded gpx files would not be selectable via media library filter anymore for example.

#38 @tellyworth
6 years ago

@harmr can you elaborate on this please? Your plugin is, what, filtering mime_types to add 'gpx' => 'application/gpx+xml'? At what point does that go wrong now, and how? Can you provide a minimal sample of a gpx file that we can test with?

@tellyworth
6 years ago

Rearrange tests, and add tests for adding unsupported types by filter

#39 @tellyworth
6 years ago

attachment:45615.5.diff moves the unit tests for vtt to a separate function, and adds tests for dfxp and json. Those types are unsupported by default, but can be added by filtering upload_mimes - see test_wp_check_filetype_and_ext_unsupported_filtered() for a trivial example. I think this is the right approach for obscure file types that don't need to be supported by core by default.

However, note that the test.dfxp and test.json cases currently fail their filtered tests, meaning adding those types to upload_mimes via a filter is not sufficient to allow them. We'd need either less strict logic or specific exceptions for them in the text/plain block in wp_check_filetype_and_ext(). I haven't found a simple solution.

#40 @audrasjb
6 years ago

Hello,

Just to say that I tested 45615.5.diff on 5.0.2 with several different files and it's ok on my side. Thanks for the patch!

#41 @wpuserhunger
6 years ago

When will this fix be available in the next core update? Is this slotted for version 5.0.3?

@joemcgill
6 years ago

@joemcgill
6 years ago

#42 @joemcgill
6 years ago

For 5.0.3, I don't think we should remove vtt and dfxp files from the list of supported mime types. 45615.6.diff leaves those file types in the array of supported mime types, removes the tests requiring a filter for these types. I've also adjusted the json test to expect a text/plain file type because that test was failing for me. I don't think we even really need that test for 5.0.3, though I agree with the idea to include tests for a suggested filter overrides for this functionality, generally.

45615.7.diff is the same as 45615.6.diff but removes the unsupported file type unit tests for now. I think this is a good iterative approach for 5.0.3 that fixes a few of the common errors for files being misidentified as text/plain.

@joemcgill
6 years ago

Add special casing for RTF files

#43 @joemcgill
6 years ago

45615.8.diff adds another clause for RTF files, addressing #45671.

@joemcgill
6 years ago

Ensure test files are included in the patch.

This ticket was mentioned in Slack in #core by webdevmattcrom. View the logs.


6 years ago

#45 @harmr
6 years ago

@tellyworth: We use the following code to register the GPX mimetype in our plugin Maps Marker Pro:

add_filter('upload_mimes', array($this, 'add_upload_mime_types'));
public function add_upload_mime_types($mimes) {
    $mimes['gpx'] = 'application/gpx+xml';
    return $mimes;
}

filtering for gpx files in the media library is added via

add_filter('post_mime_types', array($this, 'add_post_mime_types'));
public function add_post_mime_types($post_mime_types) {
    $post_mime_types['application/gpx+xml'] = array(
        __('GPX tracks', 'mmp'),
        __('Manage GPX tracks', 'mmp'),
        _n_noop('GPX track <span class="count">(%s)</span>', 'GPX tracks <span class="count">(%s)</span>', 'mmp')
    );

    return $post_mime_types;
}

A minimal sample GPX file can be found at https://demo.mapsmarker.com/wp-content/uploads/2019/01/gpx-sample.zip

thanks a lot!

#46 @joemcgill
6 years ago

In 44438:

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.

Props Kloon, birgire, tellyworth, joemcgill.
See #45615.

#47 @joemcgill
6 years ago

In 44439:

Upload: Add test files for phpunit.

This is a follow up to [44438], which missed adding the test files.

See #45615.

#48 @joemcgill
6 years ago

In 44441:

PHPCS: Fix formatting issues.

Fixes formatting issues introduced in [44438].

See #45615.

@joemcgill
6 years ago

#49 @joemcgill
6 years ago

45615.diff moves the new tests to the section that only runs when not in multisite, since these file types are not usually allowed in multisite.

#50 @joemcgill
6 years ago

In 44442:

Upload: Don't run some upload tests on multisite.

This moves several unit tests added in [44438] so they aren't run during multisite tests.

See #45615.

#51 @joemcgill
6 years ago

In 44443:

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 [44438], [44439], [44441], and [44442] to the 4.9 branch.

Props Kloon, birgire, tellyworth, joemcgill.
See #45615.

[Note: typo – the branch should have been 5.0]

Last edited 6 years ago by joemcgill (previous) (diff)

This ticket was mentioned in Slack in #core by ocean90. View the logs.


6 years ago

#53 @joemcgill
6 years ago

  • Milestone changed from 5.0.3 to 5.1

WordPress 5.0.3-RC (and the final release, pending no further changes are needed) will contain a partial fix for these issues and we'll pick up additional fixes in 5.1. I'm updating the milestone to reflect this plan.

The current changes in r44443 should fix most cases where users were seeing text files like CSV, RTF, and VTT getting blocked.

Last edited 6 years ago by joemcgill (previous) (diff)

#54 @linuxhombr3
6 years ago

  • Keywords needs-patch added; has-patch removed
  • Severity changed from normal to major

In 5.0.3, will this be fixed for CSV product file imports in WooCommerce or not?

Also, WHEN is the release scheduled?

#55 follow-up: @pento
6 years ago

  • Keywords needs-patch removed
  • Milestone changed from 5.1 to 5.0.3
  • Resolution set to fixed
  • Status changed from assigned to closed

@linuxhombr3: WordPress 5.0.3 is due to be released tomorrow.

I'm moving this ticket back to 5.0.3, we can open new tickets for followup fixes in 5.1.

#56 in reply to: ↑ 55 @linuxhombr3
6 years ago

Replying to pento:

@linuxhombr3: WordPress 5.0.3 is due to be released tomorrow.

I'm moving this ticket back to 5.0.3, we can open new tickets for followup fixes in 5.1.

Great news. Thank you much and I'll be standing-by, considering I'm sure this has been battle-tested prior to release.

Thank you and maybe you guys can write a simple 'uploader unit tester' in PHP to attempt to use the CLI to upload CSVs as well as automate plugin-side imports so that if that fails you know you have an issue.

My two cents having required such a scope of testing for a platform as dynamic as WordPress.

Thanks!
@Linuxhombr3

PS: I'd be happy to consult.

#57 @harmr
6 years ago

Unfortunately GPX upload is still broken with 5.0.3 - do I understand the comments right here that a broader fix is planned for 5.1?

#58 @joemcgill
6 years ago

@harmr I'm planning on a broader fix either via #40175 or by making it easier for implementers to filter wp_check_filetype_and_ext() via #45707.

This ticket was mentioned in Slack in #core-media by desrosj. View the logs.


6 years ago

#60 follow-up: @arunsathiya
6 years ago

.m4a still seems to be broken on WordPress 5.0.3. Trying to understand if this is being planned in a separate fix, or what the status is. Thanks for checking!

#61 in reply to: ↑ 60 @flips01
6 years ago

Replying to arunsathiya:

.m4a still seems to be broken on WordPress 5.0.3. Trying to understand if this is being planned in a separate fix, or what the status is. Thanks for checking!

I too have this question. I reported this with bug #45665, still not working.

#62 in reply to: ↑ description @sporkme
6 years ago

Replying to Kloon:

CSV is an allowed upload type, however since WP 5.0.1 the mime type is now checked against the file. CSV files can however have text/csv as well as text/plain mime types, WP only caters for the first.

What is the status of this bug in relation to adding a filter for additional mime types? We're on 5.0.3 and we add .dwg and .skp files and these are still failing. New ticket for each mime-type now or what? Is there a way to easily see what mime type WP thinks it's seeing? It would be nice if the mime type were shown to the user in the error message...

This ticket was mentioned in Slack in #core by killua99. View the logs.


6 years ago

#64 @desrosj
6 years ago

#46375 was marked as a duplicate.

Note: See TracTickets for help on using tickets.