Allow download_url to respect content-disposition header

[cklosows]

In #34938 it was proposed to use a filter in the wp_tempnam function which was rejected in favor of ignore the query string when downloading a file. This, however can still propose a problem when someone is the function to download a tokenized file where the path can be long.

In the case where the headers of the response provide a content-disposition header including a file name, it would be ideal to have the download_url function respect this file name and move the file name (which by default is the token in the path) to the file name requested.

[johnjamesjacoby]

Let's try to make this happen 5.8.

[psrpinto]

Unit tests

[psrpinto]

Check return values and other improvements

[psrpinto]

Test that the file is actually created

[psrpinto]

38231.1.diff​ improves on @cklosows's original patch by:

1. Explicitly checking the return value of preg_match()
2. Returning an error when the call to wp_tempnam() fails
3. Returning an error when the call to rename() fails

38231-tests.1.diff​ adds an extra assertion to make sure the file is actually created.

I've also now manually tested this and can confirm the Content-Disposition header is respected when calling download_url().

[desrosj]

Today is feature freeze for the 5.8 release. This one is really close! But unfortunately the time for including in 5.8 has passed.

I'm going to punt to 5.9 instead of Future Release since there is a recent momentum, and @psrpinto has done a great job iterating on the approaches here so far.

[dd32]

Replying to psrpinto:

38231.1.diff​ improves on @cklosows's original patch by:

1. Explicitly checking the return value of preg_match()
2. Returning an error when the call to wp_tempnam() fails
3. Returning an error when the call to rename() fails

Hi @psrpinto! Thanks for the improved patch :)

In general, WordPress tries to use the most basic loose boolean checking it can, so for example (I'm being nit-picky here, since you're new to Core patches, this is intended to be constructive feedback)

$content_disposition = wp_remote_retrieve_header( $response, 'Content-Disposition' );
if ( ! empty( $content_disposition ) && 1 === preg_match( '/filename="([^ ]+)"/', $content_disposition, $matches ) ) {

can be simplified to simply:

$content_disposition = wp_remote_retrieve_header( $response, 'Content-Disposition' );
if ( $content_disposition && preg_match( '/filename="([^ ]+)"/', $content_disposition, $matches ) ) {

as a) We only want to know that $content_disposition is 'something' the preg_match will validate it further and b) any truthful value return is acceptable.

I personally think in the event that the Content-Disposition filename can't be used, it should be something like this, what do you think?

set tmpName = create random name tmp file
if ( disposition header is set ) {
   set tmpNameDisposition = create disposition-named tmp file
   if ( can create tmpNameDisposition AND can rename tmpName to tmpNameDisposition ) {
      set tmpName = tmpNameDisposition
      continue on with processing
   } else {
      continue on like no error was encountered, tmpName is still set to random
   }
}
/// .. continue on

So that if the header is invalid, can't be created on the filesystem, or something else odd the function can still succeed without erroring out. As this function is used within the WordPress update process, failing on some edgecase here could be disastrous.
In other words, treating this as a progressive enhancement if possible but retaining existing behaviour in the event that it can't be used.

Other edgecases worth considering here:

• filename="file.zip" and filename=file.zip are both valid but the patch only handles the former
• What if the filename in the URL and in the Content-Disposition header match? For example https://wordpress.org/wordpress-5.7.2.zip

[psrpinto]

Refactor according to @dd32's suggestions

[psrpinto]

Add a test for Content-Disposition=filename=foo.txt (i.e. without quotes)

[psrpinto]

Thanks for the feedback @dd32! I agree with pretty much all your points so I've now refactored the code to reflect your suggested changes.

Please do let me know if you have further suggestions, they will be very welcome and I will be happy to consider them.

---

What if the filename in the URL and in the Content-Disposition header match?

Filenames are generated through wp_tempnam() so there can be no collisions, since that method makes sure the filename is unique. So even if the filename in the URL and in the Content-Disposition header are the same, the temporary files that are created will have unique names.

---

Changes in 38231.2.diff​:

• No longer check that $content_disposition is not empty before the call to preg_match(). If it is empty, it won't match, so that check is unnecessary. (wp_remote_retrieve_header() returns either the header or an empty string).
• Treat Content-Disposition as "progressive enhancement", falling back to original randomly-generated filename in case of error.
• Support the case where the header contains no quotes: Content-Disposition: filename=foo.txt

Changes in 38231-tests.2.diff​

• Add test for the case where the header contains no quotes: Content-Disposition: filename=foo.txt

[dd32]

38231-tests.2.diff​ Looks good to me @psrpinto! Thanks

The only thought I have is that the unlink could be moved to the else to avoid a file_exists() call, but that's all I got :) I'll leave that up to you or whomever commits this.

if ( tmpfname_disposition && rename ) {
   set ...
} else if ( tmpfname_disposition ) {
   unlink tmpfname_disposition
}