Make WordPress Core

Opened 13 months ago

Closed 4 months ago

Last modified 3 months ago

#58094 closed defect (bug) (worksforme)

CSV files are prevented in PHP 8.1. due to wp_check_filetype_and_ext()

Reported by: bradshawtm's profile bradshawtm Owned by:
Milestone: Priority: normal
Severity: minor Version: 6.2
Component: Upload Keywords: php81 close needs-testing-info
Focuses: Cc:

Description

finfo_file() gives different results for the same file in different PHP versions:

PHP 7.4: text/plain
PHP 8.0: application/csv
PHP 8.1: text/csv

The first two are currently allowed by WP, but the latter is not.

Change History (14)

#1 @SergeyBiryukov
13 months ago

  • Keywords needs-patch php81 needs-unit-tests added
  • Milestone changed from Awaiting Review to 6.3

Hi there, welcome to WordPress Trac! Thanks for the ticket.

Just noting that this previously came up in comment:49:ticket:50913, and [49049] / #50913 added application/csv to the list of supported types in wp_check_filetype_and_ext(), so that it works as expected on PHP 8.0.

While the text/csv type is technically allowed as of [44438] / #45615, it looks like the function may need a new conditional to check for it properly, similar to the one added for application/csv in [49049].

The unit tests might need some updates too if there is currently an issue with text/csv that they don't catch.

#2 @bradshawtm
13 months ago

While the text/csv type is technically allowed

Yes, it's recognised in the code as a valid mime type, but the piece that's missing is that it needs a similar check to the one added in [49049], line 2947, since finfo_file() gives text/csv in PHP 8.1 and thus doesn't match any of the other checks.

Thanks!

This ticket was mentioned in PR #4370 on WordPress/wordpress-develop by @elenachavdarova.


12 months ago
#3

  • Keywords has-patch added; needs-patch removed

Include text/csv to the list of supported types in wp_check_filetype_and_ext().

Track ticket: https://core.trac.wordpress.org/ticket/58094

#4 @oglekler
10 months ago

It looks like a pure bug fix but still needs a unit test, it can be added to data provider data_wp_check_filetype_and_ext(), but it can require checking PHP version.

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


10 months ago

#6 follow-up: @felipeelia
10 months ago

I tried to reproduce the problem but I could not (sorry if I missed something :-) )

Summarizing my findings, in wp_check_filetype_and_ext() we deal with two mime types: one from wp_check_filetype() and another from finfo_file().

As @bradshawtm pointed out, finfo_file() will indeed return 'text/csv' but so will wp_check_filetype(), and adaptations are only needed when those two don't match.

Happy to work on the unit tests if I missed something and can start reproducing the problem.

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


9 months ago

#8 @oglekler
9 months ago

  • Milestone changed from 6.3 to 6.4

Due to lack of unit test update and proper testing, I am moving this into 6.4 milestone.

#9 in reply to: ↑ 6 @SergeyBiryukov
9 months ago

  • Keywords close added; needs-unit-tests removed
  • Milestone changed from 6.4 to Awaiting Review

Replying to felipeelia:

As @bradshawtm pointed out, finfo_file() will indeed return 'text/csv' but so will wp_check_filetype(), and adaptations are only needed when those two don't match.

Thanks for testing! Same here, I can't seem to reproduce the issue.

In my testing on PHP 8.1+:

  • Both finfo_file() and wp_check_filetype() return text/csv. The function then proceeds to check whether the type matches the real type, and whether it is allowed. Both of these conditions are satisfied, so the correct values are returned (the proper_filename key is not relevant here, it's only used for images):
    array(3) {
      ["ext"]=>
      string(3) "csv"
      ["type"]=>
      string(8) "text/csv"
      ["proper_filename"]=>
      bool(false)
    }
    
  • This is confirmed by a unit test for wp_check_filetype_and_ext(), which has a test case for 'text/csv' that currently passes on PHP 8.1+.
  • This can also be confirmed by uploading a CSV file in the admin, which works as expected in my testing.

Note that text/csv is an allowed type as of WordPress 3.0: [13962] / #12757.

So I don't see any issues here at the moment. The steps to reproduce on a clean install would be helpful if any changes are still needed.

Last edited 9 months ago by SergeyBiryukov (previous) (diff)

@SergeyBiryukov commented on PR #4370:


9 months ago
#10

Thanks for the PR! Closing this for now, as the related issue cannot be reproduced, see comment 9 on #58094.

#11 @hellofromTonya
9 months ago

  • Keywords needs-testing-info reporter-feedback added; has-patch removed

Changing the keywords as the patch was closed and the issue has not been reproducible.

  • Removed has-patch as the patch was closed.
  • Added reporter-feedback and needs-testing-info as the steps to produce are needed to continue investigating.

I did not add php-compatibility focus to denote an incompatibility, as currently the code seems correct and compatible.

#12 @hellofromTonya
4 months ago

  • Milestone Awaiting Review deleted
  • Resolution set to worksforme
  • Status changed from new to closed

Closing this ticket, as there are multiple reports of can't reproduce and it's been 5+ months with no follow-up.

Hey @bradshawtm, does this problem persists today?

  • If yes, does it happen on a clean install without plugins and using a default theme?
  • If yes, please reopen this ticket and share the steps of how to reproduce it. The steps help contributors investigate.

#13 @bradshawtm
3 months ago

I'm not sure what changed, but I can't reproduce it now so it must have been some weird config on my end. Apologies for the delay and the wasted time!

#14 @SergeyBiryukov
3 months ago

  • Keywords reporter-feedback removed

No worries, thanks for the follow-up!

Note: See TracTickets for help on using tickets.