#58094 closed defect (bug) (worksforme)
CSV files are prevented in PHP 8.1. due to wp_check_filetype_and_ext()
Reported by: | 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
@
20 months ago
- Keywords needs-patch php81 needs-unit-tests added
- Milestone changed from Awaiting Review to 6.3
#2
@
20 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.
20 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
@
17 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.
17 months ago
#6
follow-up:
↓ 9
@
17 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.
17 months ago
#8
@
17 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
@
17 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 willwp_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()
andwp_check_filetype()
returntext/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 (theproper_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 passed 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.
@SergeyBiryukov commented on PR #4370:
17 months ago
#10
Thanks for the PR! Closing this for now, as the related issue cannot be reproduced, see comment 9 on #58094.
#11
@
16 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
andneeds-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
@
11 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.
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 inwp_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 forapplication/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.