Changeset 51626

Timestamp:

08/17/2021 12:14:20 AM (5 years ago)

Author:

SergeyBiryukov

Message:

Code Modernization: Check the return type of parse_url() in download_url().

As per the PHP manual:

If the component parameter is omitted, an associative array is returned.
If the component parameter is specified, parse_url() returns a string (or an int, in the case of PHP_URL_PORT) instead of an array. If the requested component doesn't exist within the given URL, null will be returned.

Reference: ​PHP Manual: parse_url(): Return Values

This commit adds three unit tests for download_url():

• The first test is "girl-scouting" to make sure that the code up to the point where the error is expected is tested.
• The second test exposed a PHP 8.1 basename(): Passing null to parameter #1 ($path) of type string is deprecated error due to the call to parse_url() returning null when the component requested does not exist in the passed URL.
• The output of the call to parse_url() stored in the $url_path variable is used in more places in the function logic. The third test exposes a second PHP 8.1 deprecation notice, this time for substr(): Passing null to parameter #1 ($string) of type string is deprecated.

This commit also removes duplicate parse_url() calls. Neither $url nor $url_filename are changed between when they are first received/defined and when they are re-used, so there is no need to repeat the function calls.

Follow-up to [51606], [51622].

Props jrf, hellofromTonya, SergeyBiryukov.
See #53635.

Location:

trunk

Files:

• src/wp-admin/includes/file.php (modified) (3 diffs)
• tests/phpunit/tests/admin/includesFile.php (modified) (1 diff)

trunk/src/wp-admin/includes/file.php

@@ -1127 +1127 @@
         }
 
-        $url_filename = basename( parse_url( $url, PHP_URL_PATH ) );
+        $url_path     = parse_url( $url, PHP_URL_PATH );
+        $url_filename = '';
+        if ( is_string( $url_path ) && '' !== $url_path ) {
+                $url_filename = basename( $url_path );
+        }
 
         $tmpfname = wp_tempnam( $url_filename );
@@ -1213 +1217 @@
 
                         $signature_url = false;
-                        $url_path      = parse_url( $url, PHP_URL_PATH );
-
-                        if ( '.zip' === substr( $url_path, -4 ) || '.tar.gz' === substr( $url_path, -7 ) ) {
+
+                        if ( is_string( $url_path ) && ( '.zip' === substr( $url_path, -4 ) || '.tar.gz' === substr( $url_path, -7 ) ) ) {
                                 $signature_url = str_replace( $url_path, $url_path . '.sig', $url );
                         }
@@ -1244 +1247 @@
 
                 // Perform the checks.
-                $signature_verification = verify_file_signature( $tmpfname, $signature, basename( parse_url( $url, PHP_URL_PATH ) ) );
+                $signature_verification = verify_file_signature( $tmpfname, $signature, $url_filename );
         }
 

trunk/tests/phpunit/tests/admin/includesFile.php

@@ -78 +78 @@
                 return 5;
         }
+
+        /**
+         * Verify that a WP_Error object is returned when invalid input is passed as the `$url` parameter.
+         *
+         * @covers ::download_url
+         * @dataProvider data_download_url_empty_url
+         *
+         * @param mixed $url Input URL.
+         */
+        public function test_download_url_empty_url( $url ) {
+                $error = download_url( $url );
+                $this->assertWPError( $error );
+                $this->assertSame( 'http_no_url', $error->get_error_code() );
+                $this->assertSame( 'Invalid URL Provided.', $error->get_error_message() );
+        }
+
+        /**
+         * Data provider.
+         *
+         * @return array
+         */
+        public function data_download_url_empty_url() {
+                return array(
+                        'null'         => array( null ),
+                        'false'        => array( false ),
+                        'integer 0'    => array( 0 ),
+                        'empty string' => array( '' ),
+                        'string 0'     => array( '0' ),
+                );
+        }
+
+        /**
+         * Test that PHP 8.1 "passing null to non-nullable" deprecation notice
+         * is not thrown when the `$url` does not have a path component.
+         *
+         * @ticket 53635
+         * @covers ::download_url
+         */
+        public function test_download_url_no_warning_for_url_without_path() {
+                $result = download_url( 'https://example.com' );
+
+                $this->assertIsString( $result );
+                $this->assertNotEmpty( $result ); // File path will be generated, but will never be empty.
+        }
+
+        /**
+         * Test that PHP 8.1 "passing null to non-nullable" deprecation notice
+         * is not thrown when the `$url` does not have a path component,
+         * and signature verification via a local file is requested.
+         *
+         * @ticket 53635
+         * @covers ::download_url
+         */
+        public function test_download_url_no_warning_for_url_without_path_with_signature_verification() {
+                add_filter(
+                        'wp_signature_hosts',
+                        static function( $urls ) {
+                                $urls[] = 'example.com';
+                                return $urls;
+                        }
+                );
+                $error = download_url( 'https://example.com', 300, true );
+
+                /*
+                 * Note: This test is not testing the signature verification itself.
+                 * There is no signature available for the domain used in the test,
+                 * which is why an error is expected and that's fine.
+                 * The point of the test is to verify that the call to `verify_file_signature()`
+                 * is actually reached and that no PHP deprecation notice is thrown
+                 * before this point.
+                 */
+                $this->assertWPError( $error );
+                $this->assertSame( 'signature_verification_no_signature', $error->get_error_code() );
+        }
 }