[wp-trac] [WordPress Trac] #38231: Allow download_url to respect content-disposition header

[WordPress Trac]

#38231: Allow download_url to respect content-disposition header
--------------------------------------+------------------------------
 Reporter:  cklosows                  |       Owner:  johnjamesjacoby
     Type:  enhancement               |      Status:  assigned
 Priority:  normal                    |   Milestone:  5.9
Component:  HTTP API                  |     Version:  4.7
 Severity:  normal                    |  Resolution:
 Keywords:  has-patch has-unit-tests  |     Focuses:
--------------------------------------+------------------------------

Comment (by dd32):

 Replying to [comment:5 psrpinto]:
 > [attachment: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`

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/38231#comment:7>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform