[wp-trac] [WordPress Trac] #60788: Content-Disposition support in download_url() seems broken

[WordPress Trac]

#60788: Content-Disposition support in download_url() seems broken
-------------------------------------------------+-------------------------
 Reporter:  siliconforks                         |       Owner:  (none)
     Type:  defect (bug)                         |      Status:  new
 Priority:  normal                               |   Milestone:  Awaiting
                                                 |  Review
Component:  HTTP API                             |     Version:  6.4.3
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch 2nd-opinion needs-testing  |     Focuses:
  needs-unit-tests                               |
-------------------------------------------------+-------------------------
Description changed by SergeyBiryukov:

Old description:

> In https://core.trac.wordpress.org/changeset/51939 a change was made to
> resolve ticket https://core.trac.wordpress.org/ticket/38231 to make
> `download_url()` use the `Content-Disposition` header to specify the name
> of the downloaded file.  I realize I'm a bit late to the party here since
> the ticket was opened more than 7 years ago and the change was made more
> than 2 years ago, but I think that this change was flawed and needs to be
> reconsidered (and possibly reverted entirely).  The way it was
> implemented seems fundamentally broken and has the effect of making
> `download_url()` basically impossible to use reliably.
>
> Consider the following simple PHP script named `echo.php`:
>
> {{{#!php
> <?php
>
> header( 'Content-Type: text/plain' );
> header( 'Content-Disposition: attachment; filename="echo.txt"' );
> if ( isset( $_GET['echo'] ) ) {
>         echo $_GET['echo'];
> }
> }}}
>
> This script just takes a query string parameter `echo` and outputs it, in
> plain text format, with suggested filename `echo.txt`.
>
> Now consider the following file, `download.php`, which is intended to be
> used with WP-CLI:
>
> {{{#!php
> <?php
>
> $foo = download_url( 'http://example.test/echo.php?echo=foo' );
> echo file_get_contents( $foo );
> echo "\n";
> unlink( $foo );
>
> $bar = download_url( 'http://example.test/echo.php?echo=bar' );
> echo file_get_contents( $bar );
> echo "\n";
> unlink( $bar );
> }}}
>
> If you run this, it works as expected:
>
> {{{
> $ wp eval-file download.php
> foo
> bar
> }}}
>
> Now consider the following file, `download-bad.php`:
>
> {{{#!php
> <?php
>
> $foo = download_url( 'http://example.test/echo.php?echo=foo' );
> $bar = download_url( 'http://example.test/echo.php?echo=bar' );
>
> echo file_get_contents( $foo );
> echo "\n";
>
> echo file_get_contents( $bar );
> echo "\n";
>
> unlink( $foo );
> unlink( $bar );
> }}}
>
> If you try to run this, the results are not good:
>
> {{{
> $ wp eval-file download-bad.php
> bar
> bar
> PHP Warning:  unlink(...tmp/echo.txt): No such file or directory...
> }}}
>
> You might argue that the code for `download-bad.php` is flawed, and
> should be rewritten to look more like `download.php`, and I would agree.
> However, note that `download-bad.php` would have worked fine before the
> change https://core.trac.wordpress.org/changeset/51939 was made.
> Furthermore, even `download.php` is likely to display buggy behavior if
> two users attempt to run it at the same time.  I don't think there is any
> reliable way to use `download_url()` in a manner that will protect
> against concurrent access (short of adding some kind of external locking
> mechanism).  Consider the following PHP files, `foo.php`...
>
> {{{#!php
> <?php
>
> $foo = download_url( 'http://example.test/echo.php?echo=foo' );
> sleep( 10 );
> echo file_get_contents( $foo );
> echo "\n";
> sleep( 10 );
> @ unlink( $foo );
> }}}
>
> ...and `bar.php`:
>
> {{{#!php
> <?php
>
> sleep( 5 );
> $bar = download_url( 'http://example.test/echo.php?echo=bar' );
> sleep( 10 );
> echo file_get_contents( $bar );
> echo "\n";
> @ unlink( $bar );
> }}}
>
> Now try running them both at the same time:
>
> {{{
> $ wp eval-file foo.php & wp eval-file bar.php
> bar
> bar
> }}}
>
> Obviously the output might vary because there's a race condition here,
> but I've added `sleep()` calls in such a way that the output will usually
> be `bar` and `bar`.
>
> I'm not really sure what should be done with `download_url()`, but as it
> currently stands it seems essentially impossible to use in a reliable
> manner.  Personally I would probably recommend just removing all
> `Content-Disposition` support entirely.  That would technically be a
> backward compatibility break, but I'm not sure anyone is actually using
> this functionality?  To me it doesn't really even make any sense as
> currently implemented.  Why would you want a temporary file stored under
> a fixed name?
>
> It looks like the `Content-Disposition` feature was originally proposed
> as a solution to the problem that `download_url()` uses `wp_tempnam()` in
> such a way that it can create very long filenames for some URLs, as
> described in https://core.trac.wordpress.org/ticket/34938 - but that
> doesn't make much sense either:
>
> 1. The change to use the `Content-Disposition` header bears no
> resemblance to the change that was originally requested in
> https://core.trac.wordpress.org/ticket/34938
> 2. Adding support for the `Content-Disposition` header doesn't actually
> fix the original problem (what if the URL being downloaded doesn't have a
> `Content-Disposition` header?)
> 3. Even if the URL being downloaded has a `Content-Disposition` header,
> the way the `Content-Disposition` support was implmented means that it
> still creates a file based on the URL first, then renames it later, so
> the first file could still have a filename which is too long
> 4. Another change was made in
> https://core.trac.wordpress.org/changeset/37598 which should (mostly)
> address the original problem
>
> That having been said, the original patch
> https://core.trac.wordpress.org/attachment/ticket/38231/38231.diff
> actually makes a lot more sense than what was finally committed.  The
> original patch takes the `Content-Disposition` filename and feeds it as
> input into `wp_tempnam()`, so you will get a random filename which should
> prevent the issues in the examples I've given.  Somehow, in the multiple
> iterations of the patch, the call to `wp_tempnam()` got lost, so the
> current code just uses the `Content-Disposition` filename directly
> instead of using a random filename based on the `Content-Disposition`
> filename.
>
> I suppose the behavior could be changed to use a random filename based on
> the `Content-Disposition` filename, as was originally intended, but that
> would also technically be a backward compatibility break (assuming anyone
> is actually using this functionality and expecting the current behavior).
>
> To sum up - I'm not sure exactly why this `Content-Disposition`
> functionality was added in the first place, but it should probably just
> be removed.  If it can't be removed, then the behavior should be changed
> so that it uses a random filename based on the `Content-Disposition`
> filename rather than using the `Content-Disposition` filename directly.
> If that can't be done either, perhaps there could be another argument
> added to the `download_url()` function to turn the `Content-Disposition`
> functionality on or off?  Ideally the default would be off.

New description:

 In [51939] a change was made to resolve ticket #38231 to make
 `download_url()` use the `Content-Disposition` header to specify the name
 of the downloaded file.  I realize I'm a bit late to the party here since
 the ticket was opened more than 7 years ago and the change was made more
 than 2 years ago, but I think that this change was flawed and needs to be
 reconsidered (and possibly reverted entirely).  The way it was implemented
 seems fundamentally broken and has the effect of making `download_url()`
 basically impossible to use reliably.

 Consider the following simple PHP script named `echo.php`:

 {{{#!php
 <?php

 header( 'Content-Type: text/plain' );
 header( 'Content-Disposition: attachment; filename="echo.txt"' );
 if ( isset( $_GET['echo'] ) ) {
         echo $_GET['echo'];
 }
 }}}

 This script just takes a query string parameter `echo` and outputs it, in
 plain text format, with suggested filename `echo.txt`.

 Now consider the following file, `download.php`, which is intended to be
 used with WP-CLI:

 {{{#!php
 <?php

 $foo = download_url( 'http://example.test/echo.php?echo=foo' );
 echo file_get_contents( $foo );
 echo "\n";
 unlink( $foo );

 $bar = download_url( 'http://example.test/echo.php?echo=bar' );
 echo file_get_contents( $bar );
 echo "\n";
 unlink( $bar );
 }}}

 If you run this, it works as expected:

 {{{
 $ wp eval-file download.php
 foo
 bar
 }}}

 Now consider the following file, `download-bad.php`:

 {{{#!php
 <?php

 $foo = download_url( 'http://example.test/echo.php?echo=foo' );
 $bar = download_url( 'http://example.test/echo.php?echo=bar' );

 echo file_get_contents( $foo );
 echo "\n";

 echo file_get_contents( $bar );
 echo "\n";

 unlink( $foo );
 unlink( $bar );
 }}}

 If you try to run this, the results are not good:

 {{{
 $ wp eval-file download-bad.php
 bar
 bar
 PHP Warning:  unlink(...tmp/echo.txt): No such file or directory...
 }}}

 You might argue that the code for `download-bad.php` is flawed, and should
 be rewritten to look more like `download.php`, and I would agree.
 However, note that `download-bad.php` would have worked fine before the
 change [51939] was made.  Furthermore, even `download.php` is likely to
 display buggy behavior if two users attempt to run it at the same time.  I
 don't think there is any reliable way to use `download_url()` in a manner
 that will protect against concurrent access (short of adding some kind of
 external locking mechanism).  Consider the following PHP files,
 `foo.php`...

 {{{#!php
 <?php

 $foo = download_url( 'http://example.test/echo.php?echo=foo' );
 sleep( 10 );
 echo file_get_contents( $foo );
 echo "\n";
 sleep( 10 );
 @ unlink( $foo );
 }}}

 ...and `bar.php`:

 {{{#!php
 <?php

 sleep( 5 );
 $bar = download_url( 'http://example.test/echo.php?echo=bar' );
 sleep( 10 );
 echo file_get_contents( $bar );
 echo "\n";
 @ unlink( $bar );
 }}}

 Now try running them both at the same time:

 {{{
 $ wp eval-file foo.php & wp eval-file bar.php
 bar
 bar
 }}}

 Obviously the output might vary because there's a race condition here, but
 I've added `sleep()` calls in such a way that the output will usually be
 `bar` and `bar`.

 I'm not really sure what should be done with `download_url()`, but as it
 currently stands it seems essentially impossible to use in a reliable
 manner.  Personally I would probably recommend just removing all `Content-
 Disposition` support entirely.  That would technically be a backward
 compatibility break, but I'm not sure anyone is actually using this
 functionality?  To me it doesn't really even make any sense as currently
 implemented.  Why would you want a temporary file stored under a fixed
 name?

 It looks like the `Content-Disposition` feature was originally proposed as
 a solution to the problem that `download_url()` uses `wp_tempnam()` in
 such a way that it can create very long filenames for some URLs, as
 described in #34938 - but that doesn't make much sense either:

 1. The change to use the `Content-Disposition` header bears no resemblance
 to the change that was originally requested in #34938
 2. Adding support for the `Content-Disposition` header doesn't actually
 fix the original problem (what if the URL being downloaded doesn't have a
 `Content-Disposition` header?)
 3. Even if the URL being downloaded has a `Content-Disposition` header,
 the way the `Content-Disposition` support was implmented means that it
 still creates a file based on the URL first, then renames it later, so the
 first file could still have a filename which is too long
 4. Another change was made in [37598] which should (mostly) address the
 original problem

 That having been said, the original patch
 https://core.trac.wordpress.org/attachment/ticket/38231/38231.diff
 actually makes a lot more sense than what was finally committed.  The
 original patch takes the `Content-Disposition` filename and feeds it as
 input into `wp_tempnam()`, so you will get a random filename which should
 prevent the issues in the examples I've given.  Somehow, in the multiple
 iterations of the patch, the call to `wp_tempnam()` got lost, so the
 current code just uses the `Content-Disposition` filename directly instead
 of using a random filename based on the `Content-Disposition` filename.

 I suppose the behavior could be changed to use a random filename based on
 the `Content-Disposition` filename, as was originally intended, but that
 would also technically be a backward compatibility break (assuming anyone
 is actually using this functionality and expecting the current behavior).

 To sum up - I'm not sure exactly why this `Content-Disposition`
 functionality was added in the first place, but it should probably just be
 removed.  If it can't be removed, then the behavior should be changed so
 that it uses a random filename based on the `Content-Disposition` filename
 rather than using the `Content-Disposition` filename directly.  If that
 can't be done either, perhaps there could be another argument added to the
 `download_url()` function to turn the `Content-Disposition` functionality
 on or off?  Ideally the default would be off.

--

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