WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 11 months ago

#30377 reviewing defect (bug)

wp_check_filetype is broken when checking urls with parameters

Reported by: supercleanse Owned by: johnbillion
Milestone: Future Release Priority: normal
Severity: normal Version: 4.0
Component: Media Keywords: needs-testing has-patch
Focuses: Cc:
PR Number:

Description

The function in ./wp-includes/media.php named wp_check_filetype has a bug.

It works properly when checking a url such as http://example.org/coolfile.mp4 but as soon as you add parameters to it (a common practice when attempting to embed un-cached or amazon pre-signed urls) like so: http://example.org/coolfile.mp4?extra=true&params=true ... it fails to return the extension / content type.

The fix for this should be *very* easy. The preg_match in this function that looks like this currently:

$ext_preg = '!\.(' . $ext_preg . ')$!i';

could be adjusted to ignore the query string (if there is one) and just return the true extension like so:

$ext_preg = '!\.(' . $ext_preg . ')(\?.*)?$!i';

I've tested this change in my local environment and it works great.

Attachments (4)

30377.diff (1.1 KB) - added by voldemortensen 5 years ago.
30377.2.diff (462 bytes) - added by layotte 4 years ago.
30377.3.diff (4.0 KB) - added by layotte 4 years ago.
30377.4.diff (2.5 KB) - added by ianmjones 3 years ago.
wp_check_filetype now returns correct extension when URL has query params or fragment. Unit tests included.

Download all attachments as: .zip

Change History (44)

#1 @supercleanse
5 years ago

  • Severity changed from normal to major

#2 @SergeyBiryukov
5 years ago

  • Component changed from General to Media
  • Focuses ui javascript administration performance removed
  • Keywords needs-patch needs-unit-tests added
  • Milestone changed from Awaiting Review to 4.1
  • Severity changed from major to normal

#3 @supercleanse
5 years ago

For some additional info about this issue ... here's someone else who has had this problem:

http://wordpress.stackexchange.com/questions/152316/using-audio-shortcode-for-mp3-urls-with-a-query-string

@voldemortensen
5 years ago

#4 @voldemortensen
5 years ago

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

30377.diff updates the regex to correctly identify file type and adds unit tests.

#5 @supercleanse
5 years ago

That looks great.

So what are the odds that this change will be incorporated into 4.1?

I'm just asking because I figured out a nasty hack in my plugin around this issue but would like to version it so it won't run once this is in place.

Thanks, Blair

#6 @valendesigns
5 years ago

  • Keywords needs-testing added

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


5 years ago

#8 @wonderboymusic
5 years ago

  • Owner set to wonderboymusic
  • Resolution set to fixed
  • Status changed from new to closed

In 30640:

Adjust the RegEx in wp_check_filetype() to be aware that query strings are thing that exist sometimes in URLs.

Adds unit tests.

Props voldemortensen.
Fixes #30377.

#9 @pento
5 years ago

In 32172:

Revert [30640], as it was incorrectly checking some filenames. This merges [32171] in the 4.1 branch.

See #30377.

#10 @pento
5 years ago

  • Keywords needs-refresh added
  • Milestone changed from 4.1 to Future Release
  • Resolution fixed deleted
  • Status changed from closed to reopened

[30640] was reverted in [32171] and [32172].

#11 @voldemortensen
5 years ago

@pento, can I get an example or two of filetypes it was checking incorrectly so I can do testing?

#12 @johnbillion
5 years ago

#32082 was marked as a duplicate.

#13 follow-up: @cartpauj
5 years ago

This was working in 4.1, but appears to be broken again in 4.1.2 FYI

#14 in reply to: ↑ 13 @helen
5 years ago

Replying to cartpauj:

This was working in 4.1, but appears to be broken again in 4.1.2 FYI

As per above, it was reverted out of 4.1.2, so that's expected.

#15 @supercleanse
5 years ago

Is there any place I can see the urls the reverted regex was failing on?

I'm sure this regex can be adjusted to fix whatever cases it was failing on that have been submitted.

Now we're back to urls that include query strings being completely un-usable by the WordPress media players.

#16 @johnbillion
5 years ago

#32188 was marked as a duplicate.

@layotte
4 years ago

#17 @layotte
4 years ago

I just submitted a patch for this. Running into the problem when using a file from Soundcloud which has query args. Instead of a regex, I chose to get the 'path' from parse_url, which works in these cases:

file.mp3
c:\file.mp3
/usr/local/bin/file.mp3
http://lewayotte.com/file.mp3
lewayotte.com/file.mp3
http://lewayotte.com/file.mp3?test=1
lewayotte.com/file.mp3?test=1
http://lewayotte.com/file.mp3?test=1&echo=2
lewayotte.com/file.mp3?test=1&echo=2

#18 @johnbillion
4 years ago

  • Keywords needs-unit-tests added; needs-refresh removed

The problem here is that wp_check_filetype() was never intended to work with URLs, only file paths.

If we're going to explicitly allow support for URLs (which we need to do by extension of the fact that this function is used in the wp_[audio|video|playlist]_shortcode() functions), this is gonna need unit test coverage and security review.

#19 follow-up: @dd32
4 years ago

If we're going to explicitly allow support for URLs (which we need to do by extension of the fact that this function is used in the wp_[audio|video|playlist]_shortcode() functions), this is gonna need unit test coverage and security review.

I'd personally prefer to add an explicit wp_check_url_filetype() function instead, one which extracts the actual filename from the URL and passes it along to wp_check_filetype() (if appropriate, or duplicates parts of it)

@layotte
4 years ago

#20 in reply to: ↑ 19 @layotte
4 years ago

Replying to dd32:

If we're going to explicitly allow support for URLs (which we need to do by extension of the fact that this function is used in the wp_[audio|video|playlist]_shortcode() functions), this is gonna need unit test coverage and security review.

I'd personally prefer to add an explicit wp_check_url_filetype() function instead, one which extracts the actual filename from the URL and passes it along to wp_check_filetype() (if appropriate, or duplicates parts of it)

I've submitted a patch for this.

#21 @dd32
4 years ago

Sorry I never saw it, 30377.3.diff looks good to me at first glance.

#22 @johnbillion
4 years ago

  • Owner changed from wonderboymusic to johnbillion
  • Status changed from reopened to reviewing

Note to self: [32171]

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


4 years ago

#24 follow-ups: @atomicjack
4 years ago

  • Keywords needs-refresh added; has-patch removed

We pull data from a product feed, with parameters in the URL. It works perfectly in 4.1.1, but breaks after any version past that, even with the latest -3 patch.

It is related to this bug.

#25 in reply to: ↑ 24 @zeen101
4 years ago

Replying to atomicjack:

We pull data from a product feed, with parameters in the URL. It works perfectly in 4.1.1, but breaks after any version past that, even with the latest -3 patch.

It is related to this bug.

Can you describe the setup and GET request, so I can rework the patch and test?

Version 0, edited 4 years ago by zeen101 (next)

#26 in reply to: ↑ 24 ; follow-ups: @layotte
4 years ago

Replying to atomicjack:

We pull data from a product feed, with parameters in the URL. It works perfectly in 4.1.1, but breaks after any version past that, even with the latest -3 patch.

It is related to this bug.

Can you describe the setup and GET request, so I can rework the patch and test?

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

#27 follow-up: @helen
4 years ago

  • Type changed from defect (bug) to enhancement

Changing to enhancement, as wp_check_filetype() was never meant for URLs and the solution in this case is to introduce something else that does work for URLs.

#28 in reply to: ↑ 27 @layotte
4 years ago

Replying to helen:

Changing to enhancement, as wp_check_filetype() was never meant for URLs and the solution in this case is to introduce something else that does work for URLs.

The wp_check_filetype() function is called several times on URLs in wp-includes/media.php. Including the Audio shortcode (which is where I ran into this bug). I believe this should be considered a bug. Especially if the function was never meant to be used for URLs.

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

#29 in reply to: ↑ 26 @atomicjack
4 years ago

Replying to layotte:

Replying to atomicjack:

We pull data from a product feed, with parameters in the URL. It works perfectly in 4.1.1, but breaks after any version past that, even with the latest -3 patch.

It is related to this bug.

Can you describe the setup and GET request, so I can rework the patch and test?

Thanks for offering to help - currently we have a line like this:

<?php
$media_info = wp_check_filetype(basename($vebra_media_line['File_url']));

I understand it is the File_url that you will probably need? I'm waiting for an example of what gets passed into that to be sent to me, but if you can do anything with the above at all, that'd be helpful. :)

#30 in reply to: ↑ 26 @atomicjack
4 years ago

Replying to layotte:

Replying to atomicjack:

We pull data from a product feed, with parameters in the URL. It works perfectly in 4.1.1, but breaks after any version past that, even with the latest -3 patch.

It is related to this bug.

Can you describe the setup and GET request, so I can rework the patch and test?

Here's an example URL that gets passed in:

http://[BUCKET NAME].s3.amazonaws.com/[RANDOM CHARACTERS].jpg
Last edited 4 years ago by SergeyBiryukov (previous) (diff)

#31 @supercleanse
4 years ago

Yeah, our use case is for AWS urls as well ... but ours are expiring links so they include a question mark followed by a bunch of random characters.

Here is a real link we'd use ... it may not actually work because it's an expiring link but this will give you an idea of the type of URL that can be passed to this function:

https://wordup.s3.amazonaws.com/4992_IMG_8521_converted.mp4?AWSAccessKeyId=1D10DF3V7M34NWTQNBG2&Expires=1445884947&Signature=Mp8PDvQHYZJP69hdAFNgAr11X00%3D&_=1

Thanks

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

#32 @spencercameron
4 years ago

Worth noting this also breaks importing when attachment URLs contain query parameters since the importer uses wp_check_filetype().

See: https://plugins.trac.wordpress.org/browser/wordpress-importer/trunk/wordpress-importer.php#L861

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


3 years ago

@ianmjones
3 years ago

wp_check_filetype now returns correct extension when URL has query params or fragment. Unit tests included.

#34 @ianmjones
3 years ago

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

My 30377.4.diff patch allows wp_check_filetype to find the extension of a file in a URL when it also includes either query params or a fragment.

This is really important for use cases such as @supercleanse's where signed URLs are failing to instantiate the default media player due to wp_check_filetype not finding the extension.

The patch also fixes strip_fragment_from_url throwing "Undefined index: path" when a URL without a trailing slash is passed to it.

#35 @ianmjones
2 years ago

  • Type changed from enhancement to defect (bug)

30377.4.diff is still required on trunk in order to be able to use the built in media player in the admin area, and still applies cleanly to r40891.

#36 @aaemnnosttv
2 years ago

For anyone looking for a temporary workaround, I created this plugin: https://github.com/aaemnnosttv/fix-wp-media-shortcodes-with-params

#37 @paulschreiber
2 years ago

@helen @dd32 What steps need to be taken to get this fixed?

#38 @ocean90
16 months ago

#44657 was marked as a duplicate.

#39 @anthonyeden
15 months ago

Hi all,

Is there any chance someone from the Core team can look at this again? This ticket has been open for four years, and the [audio] shortcode with URL Parameters is still broken.

Are we able to merge this into core? Is there something that still needs to be done?

#40 @paulschreiber
11 months ago

@johnbillion What is blocking including this fix in a release?

Note: See TracTickets for help on using tickets.