WordPress.org

Make WordPress Core

Opened 11 months ago

Closed 10 months ago

Last modified 8 months ago

#45615 closed defect (bug) (fixed)

CSV Mime Type fails upload

Reported by: Kloon Owned by: joemcgill
Milestone: 5.0.3 Priority: normal
Severity: major Version: 5.0.1
Component: Upload Keywords: has-unit-tests
Focuses: Cc:
PR Number:

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

Download all attachments as: .zip

Change History (75)

@Kloon
11 months ago

Add csv to text/plain mime types

#1 @swissspidy
11 months ago

  • Keywords has-patch needs-testing added

#2 @swissspidy
11 months ago

  • Version changed from trunk to 5.0.1

#3 @batesy87
11 months 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
11 months ago

Related #45622 for .vtt files

@birgire
11 months ago

#6 @birgire
11 months 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.


11 months ago

#8 @iandunn
11 months ago

#45630 was marked as a duplicate.

#9 @SergeyBiryukov
11 months ago

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

#10 @3omar7adidy
11 months ago

#45655 was marked as a duplicate.

#11 follow-up: @pento
11 months ago

  • Milestone changed from 5.0.2 to 5.0.3

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


11 months ago

#13 follow-up: @clearsite
11 months 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
11 months 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
11 months ago

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

#16 in reply to: ↑ 11 @linuxhombr3
11 months ago

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

#17 @clearsite
11 months 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 Clearsite Webdesigners.

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


11 months ago

#19 @joemcgill
11 months ago

#45699 was marked as a duplicate.

#20 @joemcgill
11 months ago

#45671 was marked as a duplicate.

#21 @joemcgill
11 months ago

#45622 was marked as a duplicate.

#22 @joemcgill
11 months 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
11 months ago

Tests for csv, tsv, vtt, dfxp files

#23 @tellyworth
11 months 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
11 months 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
11 months 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 11 months ago by Mahesh901122 (previous) (diff)

#26 @iandunn
11 months ago

@shodan01 mentioned that .gpx also stopped working.

#27 @birgire
11 months 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 11 months ago by birgire (previous) (diff)

#28 @iandunn
11 months ago

#45771 was marked as a duplicate.

#29 @audrasjb
11 months ago

#45786 was marked as a duplicate.

#30 @audrasjb
11 months ago

#45665 was marked as a duplicate.

#31 @audrasjb
11 months ago

#45787 was marked as a duplicate.

#32 @audrasjb
11 months 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.


11 months ago

@tellyworth
11 months ago

Incremental fix and unit tests

#34 follow-up: @tellyworth
11 months 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.


11 months ago

@joemcgill
11 months ago

#36 in reply to: ↑ 34 @joemcgill
11 months 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 11 months ago by joemcgill (previous) (diff)

#37 @harmr
11 months 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
10 months 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
10 months ago

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

#39 @tellyworth
10 months 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
10 months 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
10 months ago

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

@joemcgill
10 months ago

@joemcgill
10 months ago

#42 @joemcgill
10 months 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
10 months ago

Add special casing for RTF files

#43 @joemcgill
10 months ago

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

@joemcgill
10 months ago

Ensure test files are included in the patch.

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


10 months ago

#45 @harmr
10 months 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
10 months 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
10 months 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
10 months ago

In 44441:

PHPCS: Fix formatting issues.

Fixes formatting issues introduced in [44438].

See #45615.

@joemcgill
10 months ago

#49 @joemcgill
10 months 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
10 months 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
10 months 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 10 months ago by joemcgill (previous) (diff)

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


10 months ago

#53 @joemcgill
10 months 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.

Version 0, edited 10 months ago by joemcgill (next)

#54 @linuxhombr3
10 months 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
10 months 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
10 months 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
10 months 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
10 months 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.


10 months ago

#60 follow-up: @arunsathiya
10 months 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
10 months 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
9 months 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.


9 months ago

#64 @desrosj
8 months ago

#46375 was marked as a duplicate.

Note: See TracTickets for help on using tickets.