fix(log): honor whitespace and array-shaped IP values in exclude rule

[faisalahammad]

Summary

Exclude rules with an IP address filter (and all other fields set to Any) silently no-op'd when the stored rule value had whitespace around commas or was shaped as an array. Log::record_matches_rules() only split the value with explode( ',', ... ) and used a strict in_array against it, so "127.0.0.1, 8.8.8.8" never matched a real 8.8.8.8 and the record was still written.

Fixes #1824

Changes

classes/class-log.php

Before:

if ( 'ip_address' === $exclude_key ) {
    $ip_addresses = explode( ',', $exclude_value );

    if ( in_array( $record['ip_address'], $ip_addresses, true ) ) {
        ++$matches_found;
    }
}

After:

if ( 'ip_address' === $exclude_key ) {
    // Stored value shape varies: the admin form posts one
    // comma-joined string per row, while a direct API or test
    // caller can hand us an array of IPs. Normalize first, then
    // trim and drop empties so a stored "1.1.1.1, 8.8.8.8" or
    // ["1.1.1.1", " 8.8.8.8"] both match a real client IP.
    $ip_addresses = is_array( $exclude_value )
        ? $exclude_value
        : explode( ',', (string) $exclude_value );

    $ip_addresses = array_filter(
        array_map( 'trim', $ip_addresses ),
        function ( $value ) {
            return '' !== $value;
        }
    );

    if ( ! empty( $record['ip_address'] ) && in_array( $record['ip_address'], $ip_addresses, true ) ) {
        ++$matches_found;
    }
}

Why: the admin Exclude form joins multi-IP values with a comma (src/js/admin-exclude.js), so the stored value is a single string. Without trimming, "127.0.0.1, 8.8.8.8" becomes ['127.0.0.1', ' 8.8.8.8'] after explode and the strict in_array never matches. A direct API or test caller can also hand us an array, and explode( ',', array ) raises a TypeError on PHP 8+.

Repro before the fix

$log->record_matches_rules(
    array( 'ip_address' => '8.8.8.8' ),
    array( 'ip_address' => '127.0.0.1, 8.8.8.8' )
);
// false (expected: true)

Testing

Test 1: IP-only exclude rule excludes a record

1. In wp-admin, open Stream -> Settings -> Exclude.
2. Add a rule: leave Author or Role, Context, and Action as Any. Set IP Address to 127.0.0.1. Save.
3. Visit wp-admin/profile.php and click Update Profile.
4. Open Stream -> Records.
   Result: no new "Profile updated" record is present (with the fix).

Test 2: multi-IP rule with whitespace

1. Same as Test 1, but set IP Address to 127.0.0.1, 8.8.8.8 (with or without space after the comma).
2. Make a request from 8.8.8.8.
   Result: the request is excluded.

Automated tests run:

• composer test-one -- --filter='Test_Log::test_ip_only_exclude_rule_excludes_record' (single + multisite, green)
• composer test-one -- --filter='Test_Log::test_ip_address_rule_matches_with_whitespace' (green)
• composer lint-php and composer lint-tests (green)
• CodeRabbit review: 0 findings.

[shadyvb]

Looks good! With one nitpick about the unnecessary anonymous function usage with array_filter().

[PatelUtkarsh]

@faisalahammad Thanks you for your contribution!

The screenshot in #1824 shows a rule value that has no whitespace and no comma, thus the mechanism in this PR cannot apply to that report. I also reproduced a different cause on macOS that gives the same symptom. See the blocker comment on the test file.

Note: PR comments are created with help of LLM and manually reviewed before posting it.

[PatelUtkarsh]

Blocker: this test does not show the defect in #1824, and I do not think that whitespace is the cause of that report.

Evidence 1: the screenshot in the issue

The screenshot shows one rule row. Author or Role, Context, and Action all show the placeholder text. The IP Address cell has exactly one select2 tag: × 127.0.0.1.

One tag. No comma. Thus the stored value is a bare 127.0.0.1, and it has no whitespace to remove and no second address. The correction in this PR operates only on a value that has a comma or a space. Thus it cannot change the result for this reporter.

Evidence 2: the test passes without the correction

I changed classes/class-log.php back to the origin/develop version. Then I ran this test with no changes. The test failed only at line 313, which is the whitespace assertion at the end. The first two assertions, which use the same conditions as the report, both passed on develop without the correction in this PR.

Evidence 3: I reproduced a different cause on macOS

The reporter uses macOS. On macOS, the name localhost gives the IPv6 address first:

$ dscacheutil -q host -a name localhost
  ipv6_address: ::1        <-- first
  ip_address: 127.0.0.1

Thus, when the web server accepts IPv6 and the user opens http://localhost, the value of REMOTE_ADDR is ::1. I made a test with a small PHP server that shows REMOTE_ADDR:

Server on localhost (accepts IPv6):
  http://localhost:8899/   => REMOTE_ADDR=::1

Server on 0.0.0.0 (IPv4 only):
  http://localhost:8899/   => REMOTE_ADDR=127.0.0.1
  http://127.0.0.1:8899/   => REMOTE_ADDR=127.0.0.1

The value depends on the address that the server accepts, not on the text that the user writes. Thus a user can open localhost, see a correct rule of 127.0.0.1, and get ::1 in the record.

A probe test with a rule of 127.0.0.1 gives these results:

REMOTE_ADDR=127.0.0.1          excluded=true
REMOTE_ADDR=::1                excluded=false
REMOTE_ADDR=::ffff:127.0.0.1   excluded=false
REMOTE_ADDR=172.18.0.1         excluded=false

This agrees with the report: the rule does nothing, and no error message occurs.

Note that this is not a defect in the comparison. ::1 and 127.0.0.1 are different addresses, thus a strict comparison is correct. The defect is that Stream gives the user no feedback. The user writes an address that looks correct, Stream records a different address, but the UI does not show this difference.

How to reproduce this without the reporter

1. Start WordPress on a server that accepts IPv6. The default Apache on macOS does this.
2. Open the site with http://localhost, not with http://127.0.0.1.
3. Add an exclude rule. Set the IP address to 127.0.0.1. Set all the other fields to Any. Save.
4. Open wp-admin/profile.php. Select Update Profile.
5. Open Stream, then Records. The record is present. The IP column shows ::1.
6. Change the rule to ::1. Do step 4 again. The record is absent.

Step 6 is the proof. If the rule operates with ::1 but not with 127.0.0.1, this is the cause.

A second, different defect that I found

This one is real, but it is probably not the defect of this reporter. classes/class-plugin.php:221 resolves the client IP address with FILTER_VALIDATE_IP. Some hosts copy the X-Forwarded-For value into REMOTE_ADDR with no change, thus REMOTE_ADDR holds more than one address:

'203.0.113.9'               => '203.0.113.9'
'203.0.113.9, 70.41.3.18'   => false
'203.0.113.9:51234'         => false

A chain gives false, not the first address, because of Filter_Input::filter() at classes/class-filter-input.php:114-117. Then Log::log():136 writes an empty IP column for every record, and Log::is_record_excluded():182 puts false into the record. Thus no IP rule can ever agree. This gives the same symptom, but only on a site that has a proxy.

What to do

1. Remove Fixes #1824 from this PR. Keep this PR for the whitespace defect, which is real but is a different defect.
2. Do the six steps above. If step 6 gives the result that I describe, make a new issue for the true cause.

The whitespace correction is good. My objection is only to the statement that it closes #1824.

[PatelUtkarsh]

Warning: the ! empty( $record['ip_address'] ) condition has no effect on the result.

When the code gets to this line, two things are already true. The condition ! isset( $record[ $exclude_key ] ) at line 240 passed. And line 254 removed all the empty items from $ip_addresses. Thus an empty record IP address, or a record IP address of '0', cannot agree with an item in the list. The result is the same with the condition and without it.

But an empty record IP address does occur in operation. When REMOTE_ADDR holds a proxy chain, classes/class-plugin.php:221 gives false, and that value goes into the record. See my blocker comment on the test file.

Thus, if you added this condition for that reason, the condition is in the wrong place. It hides the problem here, but the correction belongs at the source in classes/class-plugin.php:221.

If the condition is only defensive, tell that in a comment. At this time, test_ip_address_rule_matches_with_whitespace (lines 213-221 of the test file) makes an assertion about this condition. Thus a subsequent reader will think that the condition is necessary.

[PatelUtkarsh]

Warning: I could not find operational code that gives an array to this parameter. Thus this branch is not necessary.

I examined each path that supplies $exclude_value:

• abilities/class-ability-create-exclusion-rule.php:138 makes a (string) cast. Line 147 does a check with FILTER_VALIDATE_IP. Thus the ability always stores a scalar value.
• Settings::render_field() (classes/class-settings.php:1100) reads a scalar value.
• src/js/admin-exclude.js:378-383 joins the selected values into one string with commas.
• Log::is_record_excluded():197 gives the stored value to the function with no change.

Only the test in this PR gives an array to this function. Thus the test is the only proof that the branch is necessary, but the test is also a part of this PR.

Do one of these two things. Show operational code that gives an array. Or remove this branch and the two array assertions in the test (lines 240-262). The (string) cast is sufficient for the PHP 8 TypeError that the description mentions.

[PatelUtkarsh]

Nit: the comment has five lines, but the code has four lines.

The comment also tells about a "direct API or test caller". I could not find such a caller. See my other comment about the array branch.

Two lines are sufficient: the admin form joins the values with commas, thus the code must remove the spaces before the strict comparison.

[PatelUtkarsh]

Nit: this test method makes assertions about four different behaviors, but the name tells about only one behavior. The four behaviors are: removal of spaces, an empty record IP, removal of empty items, and an array value.

If the test fails, the name of the test will point to the incorrect behavior. Divide this test into more than one test. Or give it a more general name, for example test_ip_address_rule_normalization.