Multisite site registration table doesn't support IPv6

[dd32]

The wp_registration_log table stores a log of created Multisite sites, this includes an IP address.

It appears that the table (and code) only expects a IPv4 address, not allowing for IPv6.

Attached PR is untested and only validated on a visual review.

Trac ticket: https://core.trac.wordpress.org/ticket/63986#ticket

[wildworks]

Since the 6.9 RC1 release is coming soon, I will punt this ticket to 7.0.

[juanmaguitar]

From today's Bug Scrub:

@dd32 There's feedback pending to be addressed on ​https://github.com/WordPress/wordpress-develop/pull/9911

Do you think will be ready on time for WP 7.0 or shall we punt it for a Future Release?

[dd32]

I'm not able to shepperd this into 7.0, bumping to 7.1 unless someone else wants to take it on.

Since the description notes the change is "untested and only validated on a visual review", here's a regression test for the core behavior — an IPv6 REMOTE_ADDR being stored intact by wpmu_log_new_registrations(). It passes on this branch and fails on trunk, where the old /[^0-9., ]/ mask strips the colons and hex digits (e.g. 2001:0db8:…:7334 becomes 200108853…07334).

Goes in tests/phpunit/tests/multisite/wpmuLogNewRegistrations.php:

/**
         * Ensures an IPv6 remote address is preserved when logging a registration.
         *
         * @ticket 63986
         */
        public function test_wpmu_log_new_registrations_preserves_ipv6_address() {
                global $wpdb;

                $ipv6                   = '2001:0db8:85a3:0000:0000:8a2e:0370:7334';
                $original_remote_addr   = isset( $_SERVER['REMOTE_ADDR'] ) ? $_SERVER['REMOTE_ADDR'] : null;
                $_SERVER['REMOTE_ADDR'] = $ipv6;

                try {
                        wpmu_log_new_registrations( 1, 1 );

                        $logged_ip = $wpdb->get_var( "SELECT IP FROM {$wpdb->registration_log} WHERE blog_id = 1 ORDER BY ID DESC LIMIT 1" );
                } finally {
                        if ( null === $original_remote_addr ) {
                                unset( $_SERVER['REMOTE_ADDR'] );
                        } else {
                                $_SERVER['REMOTE_ADDR'] = $original_remote_addr;
                        }
                }

                $this->assertSame(
                        $ipv6,
                        $logged_ip,
                        'The IPv6 remote address was not stored intact in the registration log.'
                );
        }

Run with -c tests/phpunit/multisite.xml. One note while here: the sanitizing regex only allows lowercase hex (a-f), so an uppercase-hex IPv6 literal would still be partly stripped — worth considering a-fA-F if that's a concern. Happy to push the test as a commit.

[motylanogha]

Unit test: IPv6 REMOTE_ADDR preserved by wpmu_log_new_registrations(). Passes against PR #9911, fails on trunk.

[motylanogha]

Added a regression test asserting an IPv6 REMOTE_ADDR is stored intact by wpmu_log_new_registrations(). It passes with the patch in PR #9911 and fails on trunk, where the old /[^{0-9., ]/ mask strips the colons and hex digits. Run with -c tests/phpunit/multisite.xml. Patch attached (63986-tests.diff); same test posted on the PR. Side note: the new mask only allows lowercase hex (a-f), so an uppercase-hex IPv6 literal would still be partly stripped — worth considering a-fA-F.}

[wildworks]

This ticket has seen little activity and has been punted several times, so I will change it to a future release.

[motylanogha]

Following up on the uppercase-hex note from comment:7, which is still open in PR #9911. Recording the detail here so it is not lost now that the ticket is on Future Release.

The mask in the PR is:

'IP' => preg_replace( '/[^a-f0-9:.]/', '', wp_unslash( $_SERVER['REMOTE_ADDR'] ) ),

Uppercase hex is valid in an IPv6 literal (RFC 4291; RFC 5952 only recommends lowercase for text output). Such an address is not rejected by this mask, it is silently rewritten into a different, still well-formed address:

2001:DB8::1                   ->  2001:8::1
FE80::1                       ->  80::1
2001:DB8:85A3::8A2E:370:7334  ->  2001:8:853::82:370:7334

The row then holds an IP that was never seen, which is a worse failure mode than the truncation this ticket started from. Two characters fix it:

-                               'IP'              => preg_replace( '/[^a-f0-9:.]/', '', wp_unslash( $_SERVER['REMOTE_ADDR'] ) ),
+                               'IP'              => preg_replace( '/[^a-fA-F0-9:.]/', '', wp_unslash( $_SERVER['REMOTE_ADDR'] ) ),

In practice most SAPIs populate REMOTE_ADDR from inet_ntop(), which emits lowercase, so this will not be hit often. It is still a sanitizing step that corrupts valid input rather than rejecting it.

Regression test for it, same shape as the one already in 63986-tests.diff. It fails against PR #9911 as it stands and passes with the widened mask:

        /**
         * Ensures an uppercase IPv6 remote address is not mangled by the sanitizing mask.
         *
         * @ticket 63986
         */
        public function test_wpmu_log_new_registrations_preserves_uppercase_ipv6_address() {
                global $wpdb;

                $ipv6                   = '2001:DB8:85A3::8A2E:370:7334';
                $original_remote_addr   = isset( $_SERVER['REMOTE_ADDR'] ) ? $_SERVER['REMOTE_ADDR'] : null;
                $_SERVER['REMOTE_ADDR'] = $ipv6;

                try {
                        wpmu_log_new_registrations( 1, 1 );

                        $logged_ip = $wpdb->get_var( "SELECT IP FROM {$wpdb->registration_log} WHERE blog_id = 1 ORDER BY ID DESC LIMIT 1" );
                } finally {
                        if ( null === $original_remote_addr ) {
                                unset( $_SERVER['REMOTE_ADDR'] );
                        } else {
                                $_SERVER['REMOTE_ADDR'] = $original_remote_addr;
                        }
                }

                $this->assertSame(
                        $ipv6,
                        $logged_ip,
                        'An uppercase IPv6 remote address was mangled by the IP sanitizing mask.'
                );
        }

Run with -c tests/phpunit/multisite.xml, alongside the existing lowercase test.