Skip to content

HTTP API: Use the actual HTTP status code in download_url() error codes #13602

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
hinnerk-a wants to merge 1 commit into
base: trunk
Choose a base branch
from
+114 −1
Open

HTTP API: Use the actual HTTP status code in download_url() error codes #13602

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion src/wp-admin/includes/file.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1150,6 +1150,9 @@ function wp_handle_sideload( &$file, $overrides = false, $time = null ) {
* @since 2.5.0
* @since 5.2.0 Signature Verification with SoftFail was added.
* @since 5.9.0 Support for Content-Disposition filename was added.
* @since 7.2.0 The error code for a non-200 response now reflects the actual
* HTTP status code (`http_403`, `http_500`, ...) instead of always
* being `http_404`.
*
* @param string $url The URL of the file to download.
* @param int $timeout The timeout for the request to download the file.
Expand Down Expand Up @@ -1217,7 +1220,13 @@ function download_url( $url, $timeout = 300, $signature_verification = false ) {

unlink( $tmpfname );

return new WP_Error( 'http_404', trim( wp_remote_retrieve_response_message( $response ) ), $data );
/*
* Name the error after the actual status code, e.g. `http_403` or `http_500`.
* A response without a status code keeps the historical `http_404`.
*/
$error_code = $response_code ? 'http_' . $response_code : 'http_404';

return new WP_Error( $error_code, trim( wp_remote_retrieve_response_message( $response ) ), $data );
}

$content_disposition = wp_remote_retrieve_header( $response, 'Content-Disposition' );
Expand Down
104 changes: 104 additions & 0 deletions tests/phpunit/tests/admin/includesFile.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public function test_get_home_path() {

/**
* @ticket 43329
* @ticket 60564
*
* @covers ::download_url
*/
Expand All @@ -45,6 +46,7 @@ public function test_download_url_non_200_response_code() {
$error = download_url( 'test_download_url_non_200' );

$this->assertWPError( $error );
$this->assertSame( 'http_418', $error->get_error_code() );
$this->assertSame(
array(
'code' => 418,
Expand Down Expand Up @@ -84,6 +86,108 @@ public function __return_5() {
return 5;
}

/**
* Tests that the error code of a non-200 response reflects the actual HTTP status code.
*
* @ticket 60564
* @dataProvider data_download_url_should_use_the_response_code_in_the_error_code
*
* @covers ::download_url
*
* @param int|string $response_code The HTTP status code returned by the mocked request.
* @param string $expected_error_code The expected `WP_Error` code.
*/
public function test_download_url_should_use_the_response_code_in_the_error_code( $response_code, $expected_error_code ) {
$filter = static function ( $response, $parsed_args ) use ( $response_code ) {
file_put_contents( $parsed_args['filename'], 'Error body' );

return array(
'response' => array(
'code' => $response_code,
'message' => 'Error message',
),
);
};

add_filter( 'pre_http_request', $filter, 10, 2 );

$error = download_url( 'test_download_url_non_200' );

remove_filter( 'pre_http_request', $filter );

$this->assertWPError( $error );
$this->assertSame( $expected_error_code, $error->get_error_code() );
$this->assertSame( 'Error message', $error->get_error_message() );
$this->assertSame(
array(
'code' => $response_code,
'body' => 'Error body',
),
$error->get_error_data()
);
}

/**
* Data provider for test_download_url_should_use_the_response_code_in_the_error_code().
*
* @return array[]
*/
public function data_download_url_should_use_the_response_code_in_the_error_code() {
return array(
'403 Forbidden' => array(
'response_code' => 403,
'expected_error_code' => 'http_403',
),
'404 Not Found' => array(
'response_code' => 404,
'expected_error_code' => 'http_404',
),
'406 Not Acceptable' => array(
'response_code' => 406,
'expected_error_code' => 'http_406',
),
'500 Internal Server Error' => array(
'response_code' => 500,
'expected_error_code' => 'http_500',
),
'503 Service Unavailable' => array(
'response_code' => 503,
'expected_error_code' => 'http_503',
),
);
}

/**
* Tests that a response without a status code keeps the historical `http_404` error code.
*
* A `pre_http_request` filter may short-circuit the request with a response
* that carries no `response` array at all, in which case
* `wp_remote_retrieve_response_code()` returns an empty string.
*
* @ticket 60564
*
* @covers ::download_url
*/
public function test_download_url_should_fall_back_to_http_404_without_a_response_code() {
$filter = static function ( $response, $parsed_args ) {
file_put_contents( $parsed_args['filename'], 'Error body' );

return array(
'headers' => array(),
'body' => '',
);
};

add_filter( 'pre_http_request', $filter, 10, 2 );

$error = download_url( 'test_download_url_non_200' );

remove_filter( 'pre_http_request', $filter );

$this->assertWPError( $error );
$this->assertSame( 'http_404', $error->get_error_code() );
}

/**
* @ticket 38231
* @dataProvider data_download_url_should_respect_filename_from_content_disposition_header
Expand Down
Loading