Skip to content

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

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
faisalahammad wants to merge 2 commits into
base: develop
Choose a base branch
from
+144 −3
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
508925a
fix(log): honor whitespace and array-shaped IP values in exclude rule
faisalahammad Jul 19, 2026
55e4c75
fix(log): simplify ip_address exclude normalization
faisalahammad Aug 23, 2026
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
15 changes: 12 additions & 3 deletions classes/class-log.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,9 +242,18 @@ public function record_matches_rules( $record, $exclude_rules ) {
}

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

if ( in_array( $record['ip_address'], $ip_addresses, true ) ) {
// 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.
Comment on lines +245 to +249

@PatelUtkarsh PatelUtkarsh Aug 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

$ip_addresses = is_array( $exclude_value )
? $exclude_value
: explode( ',', (string) $exclude_value );
Comment on lines +250 to +252

@PatelUtkarsh PatelUtkarsh Aug 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


$ip_addresses = array_filter( array_map( 'trim', $ip_addresses ) );

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

@PatelUtkarsh PatelUtkarsh Aug 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

++$matches_found;
}
} elseif ( $record[ $exclude_key ] === $exclude_value ) {
Expand Down
132 changes: 132 additions & 0 deletions tests/phpunit/test-class-log.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,4 +181,136 @@ public function test_can_match_record_id_address() {
)
);
}

public function test_ip_address_rule_matches_with_whitespace() {

@PatelUtkarsh PatelUtkarsh Aug 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Whitespace around commas (admin form join + user paste) must not
// silently fail the IP match. See issue #1824.
$this->assertTrue(
$this->plugin->log->record_matches_rules(
array(
'ip_address' => '8.8.8.8',
),
array(
'ip_address' => '1.1.1.1, 8.8.8.8',
),
'Trailing space after comma does not break the match'
)
);

$this->assertTrue(
$this->plugin->log->record_matches_rules(
array(
'ip_address' => '8.8.8.8',
),
array(
'ip_address' => '8.8.8.8 ',
),
'Trailing space on a single-IP rule still matches'
)
);

$this->assertFalse(
$this->plugin->log->record_matches_rules(
array(
'ip_address' => '',
),
array(
'ip_address' => '1.1.1.1',
),
'Empty record IP never matches an IP-only rule'
)
);

// Empty tokens between commas (e.g. user-pasted "1.1.1.1, ,") must be
// dropped, not treated as a valid match against the empty record IP.
$this->assertTrue(
$this->plugin->log->record_matches_rules(
array(
'ip_address' => '1.1.1.1',
),
array(
'ip_address' => '1.1.1.1, ,',
),
'Empty comma-separated tokens are dropped before matching'
)
);

// Stored value may already be an array (direct API callers, tests).
// Both shapes must match.
$this->assertTrue(
$this->plugin->log->record_matches_rules(
array(
'ip_address' => '8.8.8.8',
),
array(
'ip_address' => array( '127.0.0.1', '8.8.8.8' ),
),
'Array-shaped IP rule matches the second entry'
)
);

$this->assertTrue(
$this->plugin->log->record_matches_rules(
array(
'ip_address' => '8.8.8.8',
),
array(
'ip_address' => array( ' 8.8.8.8 ' ),
),
'Array-shaped IP rule trims whitespace per entry'
)
);
}

public function test_ip_only_exclude_rule_excludes_record() {
// End-to-end coverage for the bug in #1824. Shape mirrors the
// parallel-array rule format produced by both the wp-admin Exclude
// list and the stream/create-exclusion-rule ability.
$this->plugin->settings->options['exclude_rules'] = array(
'exclude_row' => array( 0 => '' ),
'author_or_role' => array( 0 => '' ),
'connector' => array( 0 => '' ),
'context' => array( 0 => '' ),
'action' => array( 0 => '' ),
'ip_address' => array( 0 => '127.0.0.1' ),
);

$user = $this->factory->user->create_and_get();
$user->add_role( 'administrator' );

$this->assertTrue(
$this->plugin->log->is_record_excluded(
'users',
'profile',
'updated',
$user,
'127.0.0.1'
),
'IP-only rule excludes a record from the matching IP'
);

$this->assertFalse(
$this->plugin->log->is_record_excluded(
'users',
'profile',
'updated',
$user,
'8.8.8.8'
),
'IP-only rule does not exclude a record from a different IP'
);

// Whitespace in the stored IP value must still match.
$this->plugin->settings->options['exclude_rules']['ip_address'][0] = '127.0.0.1, 8.8.8.8';
$this->assertTrue(
$this->plugin->log->is_record_excluded(
'users',
'profile',
'updated',
$user,
'8.8.8.8'
),
'Comma-joined IP list with whitespace matches the second entry'
);
}
Comment on lines +265 to +315

@PatelUtkarsh PatelUtkarsh Aug 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}