wp_check_filetype is broken when checking urls with parameters

[supercleanse]

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.

[supercleanse]

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]

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

[supercleanse]

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

[wonderboymusic]

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.

[pento]

In 32172:

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

See #30377.

[pento]

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

[voldemortensen]

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

[johnbillion]

#32082 was marked as a duplicate.

[cartpauj]

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

[helen]

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.

[supercleanse]

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.

[johnbillion]

#32188 was marked as a duplicate.

[layotte]

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

[johnbillion]

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.

[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)

[layotte]

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.

[dd32]

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

[johnbillion]

Note to self: [32171]

[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.

[zeen101]

wrong account

[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?

[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.

[layotte]

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.

[atomicjack]

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. :)

[atomicjack]

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

[supercleanse]

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

[spencercameron]

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

[ianmjones]

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

[ianmjones]

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.

[ianmjones]

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.

[aaemnnosttv]

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

[paulschreiber]

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

[ocean90]

#44657 was marked as a duplicate.

[anthonyeden]

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?

[paulschreiber]

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

[SergeyBiryukov]

#54244 was marked as a duplicate.

[Ruxton]

oh my god. how on earth has this taken so long to resolve? am I going to have to manage a patch forever (according to this drawn out timeline) for a 1 line resolution?

[johnbillion]

30377.5.diff​ refreshes the patch and adds more tests, particularly ones which include URLs and paths which attempt to subvert the extension detection.

One test fails: http://example.mp3 incorrectly results in an extension of mp3, but this isn't strictly related to this change and AFAIK none of the extensions in wp_get_mime_types() are valid TLDs anyway.

[audrasjb]

Moving for 6.0 consideration
Also marking as needs-refresh since the patch doesn't apply anymore against trunk.
Anyone is welcome to submit a patch refresh.
(see ​https://wordpress.slack.com/archives/C02RQBWTW/p1646400165837319).

Trac ticket: https://core.trac.wordpress.org/ticket/30377

[audrasjb]

The above PR refreshes the last patch proposed by @johnbillion.
Adding needs-testing keyword.

[audrasjb]

One test fails: ​http://example.mp3 incorrectly results in an extension of mp3, but this isn't strictly related to this change and AFAIK none of the extensions in wp_get_mime_types() are valid TLDs anyway.

👆 this is still an issue with the current PR. @johnbillion should we just remove the failing test?

[johnbillion]

Yes let's remove that test please and get this in 👍

[audrasjb]

Alright the tests are passing now and I tested the patch successfully.
Marking for a mention in the Misc changes devnote.
Self assigning for commit.

[audrasjb]

In 52829:

Media: Allow wp_check_filetype() to support query strings in URLs.

This changeset adjusts the regex in wp_check_filetype() to support query strings in URLs.

Follow-up to [30640], [32172].

Props voldemortensen, johnbillion, layotte, dd32, atomicjack, supercleanse, spencercameron, ianmjones, audrasjb.
Fixes #30377.

Committed in https://core.trac.wordpress.org/changeset/52829

[dd32]

@audrasjb Unfortunately this causes a PHP Fatal error for ms-files.php:

Uncaught Error: Call to undefined function strip_fragment_from_url() in wp-includes/functions.php:3009
Stack trace:
#0 wp-includes/ms-files.php(32): wp_check_filetype('wp-include...')
#1 {main}
 thrown

Source: GET https://make.wordpress.org/core/files/2022/01/list-view-keyboard-focus-1.mp4
Timestamp: Tue, 08 Mar 2022 00:28:30 +0000 (1646699310)
File: wp-includes/functions.php
Line: 3009

[peterwilsoncc]

I am being a little tedious to keep the milestone report keywords clean.

I think we'd need e2e tests to test this use case as I don't think phpunit's ms-files tests use SHORTINIT.

[dd32]

#55333 is a php notice that this change started triggering on WordPress.org, but is 'unrelated'.

[audrasjb]

@dd32 I'm trying to reproduce the issue but I'm struggling a bit with ms-files.php, it's pretty hard to reproduce on a new install. Do you have an easy to reproduce use case (I mean outside w.org)?

[SergeyBiryukov]

As noted in comment:10, this was previously attempted in [30640] and reverted in [32171] as a security fix for ​WordPress 4.1.2. It would be great to confirm with the Security Team that [52829] does not reintroduce any issues.

[peterwilsoncc]

In 52832:

Media: Revert query string support for wp_check_filetype().

Revert [52829] due to fatal errors in some Multisite configurations.

Props dd32, SergeyBiryukov, audrasjb.
See #30377.

[dd32]

Replying to audrasjb:

@dd32 I'm trying to reproduce the issue but I'm struggling a bit with ms-files.php, it's pretty hard to reproduce on a new install.

Unfortunately I'm not aware of how to setup Multisite using ms-files.php, You probably need to manually flip the ms_files_rewriting site option, or perhaps even just request ms-files.php?file=relative/path/to/file.ext. I can say that strip_fragment_from_url is in canonical and not loaded in multisite SHORTINIT though.

[Ruxton]

With the knowledge of what @dd32 just mentioned, it seems the viable solution outside of moving strip_fragment_from_url out of canonical (which is excessive) is to strip the fragment and beyond with strpos, etc. as the query string was.

<?php
$frag_pos = strpos( $filename, '#' );

if ( false !== $frag_pos) {
  $filename = substr($filename,0, $frag_pos-1)
}
?>

[dd32]

Replying to Ruxton:

With the knowledge of what dd32 just mentioned, it seems the viable solution outside of moving strip_fragment_from_url out of canonical (which is excessive) is to strip the fragment and beyond with strpos, etc. as the query string was.

Using string functions to remove parts of a URL (both ?... and #...) is a potential security issue here (and why it would've been reverted prevously), as it's possible to cause it to strip more than anticipated and result in operating on the incorrect filename. You should really use wp_parse_url() instead too (It's kind of like parsing HTML with Regex, possible, but should be avoided).

Additionally, since the parameter can be a Filename, Path, or URL, you need to restrict any URL handling to URLs only, not filenames.

If a filename on disk looks like a url (You can have a filename on Linux that IS a url), you can't treat it as a URL either. If the filename came from an untrusted location, you can't treat it as a URL unless it's known to be intentionally a URL.

Pulling out two comments from above:

• Comment by me: 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)
• Comment by 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.

Even if the existing function is being passed URLs, it's not intentionally supposed to be used for URLs, and performing processing like this is doomed for failure.

[Ruxton]

All very fair, someone actually wrote a patch with what you want and you responded to it at the time, then it somehow just drifted away from that.

If patch 3 is the solution you want, should we refresh that and include the tests from John's patch? Would that get us to a better place?

I'm just trying to get this across the line. It's old, annoying and a resolution is needed.

Replying to dd32:

Pulling out two comments from above:

• Comment by me: 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)
• Comment by 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.

Even if the existing function is being passed URLs, it's not intentionally supposed to be used for URLs, and performing processing like this is doomed for failure.

[SergeyBiryukov]

Replying to Ruxton:

If patch 3 is the solution you want, should we refresh that and include the tests from John's patch? Would that get us to a better place?

Yes, I think that would be the preferred direction here.

[peterwilsoncc]

I'm moving this off the milestone, it's getting close to the beta and there hasn't been any additional patches since it was reverted earlier in this release cycle.