WordPress.org

Make WordPress Core

Opened 3 months ago

Last modified 4 weeks ago

#47752 new defect (bug)

Fix upload of .srt files

Reported by: afercia Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version: 5.0.1
Component: Upload Keywords: has-screenshots has-patch has-unit-tests 2nd-opinion
Focuses: Cc:
PR Number:

Description

See #45615, #45622, [44438], [44439], and [44443].

Files with .srt extension are meant for video subtitles (captions), much like .vtt files. After the changes to make the mime type check stricter in WordPress 5.0.1 (backported to 4.9.9, etc.), uploading .srt files can fail because of mismatched MIME type check. Actually, .vtt can be served as text/plain depending on the server configuration.

Before WordPress 5.0.2, .srt files could be uploaded without issues.

For example, on a standard VVV install, the upload fails. Test file from the mediaelement-files GitHub repo:
https://github.com/mediaelement/mediaelement-files/blob/master/mediaelement.srt

Attachments (3)

srt.png (33.6 KB) - added by afercia 3 months ago.
Screenshot 2019-07-22 at 5.55.35 PM.png (172.2 KB) - added by dkarfa 3 months ago.
47752.diff (2.8 KB) - added by killerbishop 7 weeks ago.
Patch for #47752 - includes fix and unit test

Download all attachments as: .zip

Change History (14)

@afercia
3 months ago

#2 @dkarfa
3 months ago

True, I try in VVV and got
`“mediaelement.srt” has failed to upload.
Sorry, this file type is not permitted for security reasons.` - Error

#3 @killerbishop
3 months ago

I've tested this specific issue using vvv - from what I can tell - the mime type on this file according to finfo is text/html. Expectation from the wp_check_filetype_and_ext() is that it should be text/plain. It's also not in the array of choices in that call. Before pushing a patch - if something comes back as text/html with extension .srt - should this be allowed or is text/html purposefully ignored for security reasons here?

#4 @killerbishop
3 months ago

I would say this might be a part of this bigger looking issue: #40175

#5 follow-up: @itowhid06
8 weeks ago

https://pasteboard.co/IuDT57I.png

I was able to upload .srt files without any problem. screenshot attached.

#6 in reply to: ↑ 5 @killerbishop
8 weeks ago

@itowhid06 - I was able to get some SRT files to work as well. The issue is with specific SRT files, such as the one @afercia posted originally:

https://github.com/mediaelement/mediaelement-files/blob/master/mediaelement.srt

This file is failing the check from fileinfo which is why I was thinking this is part of a bigger issue that was being mentioned in #40175. The bundled magic files with PHP apparently think that file is not a valid SRT - or at least this is true in PHP 7.3.

#7 @afercia
8 weeks ago

I think it depends on your server's MIME types configuration. Testing should first check which is the MIME type a .srt file is served with. Re: potential fix, see [44438] which just allowed .vtt files.

#8 @killerbishop
7 weeks ago

@afercia - I agree that the environment variable MAGIC can cause the system version to be used, but by default, it uses the built-in magic file of PHP. On my install MAGIC is not set, so my testing is against whatever the PHP version provides.

I updated to the latest upstream/master branch and tested the change from #44438 which is merged already. On my install of vvv, it is not functioning still with the file mediaelement.srt. For PHP 5.6.40 and 7.2.21, it returns the MIME type text/html which does not appear like it will fall into the changes from #44438. The likely reason that it is calling it HTML is the fact the SRT file in question actually has HTML tags in the content.

@killerbishop
7 weeks ago

Patch for #47752 - includes fix and unit test

#9 @killerbishop
7 weeks ago

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

I've uploaded a patch for this along with a unit test that uses a copy of the file referenced in this issue. If text/html is detected as the MIME type and the file extension is .srt then it will be allowed through.

#10 @azaozz
4 weeks ago

  • Keywords 2nd-opinion added
  • Milestone changed from 5.3 to Future Release

If text/html is detected as the MIME type and the file extension is .srt then it will be allowed through.

Wondering how "safe" are the .srt files that contain HTML tags.

  • What happens if a user downloads such file directly in the browser?
  • Shouldn't handling of .srt files match handling of other text/html files?
  • By default HTML files are not allowed. If WP needs an exception for .srt files that contain tags, how can we ensure they are "safe for use"?

It seems that we shouldn't allow .srt files that contain HTML tags. Moving to future release for further review/investigation.

#11 @afercia
4 weeks ago

Good point. Although I'm not sure I understand how this differs from the .vtt files case: they may contain HTMl as well.

Note: See TracTickets for help on using tickets.