Fix: global dismiss leaks across rules sharing an object (PRO-1264) - #1861
Merged
pattonwebz merged 4 commits intoAug 4, 2026
Conversation
Adds a failing regression test proving a large-batch/global dismiss touches every row sharing the same object regardless of rule, when it should only affect rows matching both rule and object. Confirmed failing against current dismiss_issue() largeBatch query, which filters only by siteid + object.
dismiss_issue()'s largeBatch path selected and updated every row matching only siteid+object, so a global/large-batch dismiss on one rule violation also silently dismissed unrelated rule violations that happened to share the same object. Fetch the representative issue's rule alongside its object and filter the batch query by both, matching the scoping the pro plugin's global-ignore lookup already uses. Also fixes two existing largeBatch tests whose fixtures used different rules per row within a single "batch" object — that setup encoded the same assumption behind the bug, so those rows would no longer be recognized as belonging to the same batch under the corrected query.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughLarge-batch issue dismissal and reopen operations now scope matching rows by both ChangesLarge-batch operation scoping
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant REST_API
participant Representative_Issue_Query
participant Batch_Permission_Query
participant Issue_Rows
REST_API->>Representative_Issue_Query: Retrieve rule and object
REST_API->>Batch_Permission_Query: Match siteid, rule, and object
Batch_Permission_Query->>Issue_Rows: Select matching issue rows
REST_API->>Issue_Rows: Apply dismiss or reopen operation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
- dismiss_issue(): guard on the representative row itself (not just the object it contains) before proceeding, and reword the stale largeBatch comment that still described an "object-based update". - Regression test now asserts ignre_global, not just ignre, since this bug is specifically about global-dismiss scoping. - Add test_large_batch_reopen_only_affects_matching_rule mirroring the dismiss-side regression test for the reopen/undismiss direction. - Add test_large_batch_dismiss_succeeds_when_unrelated_rule_row_is_on_unauthorized_post proving the practical benefit of the narrower scope: a batch no longer 403s over an unrelated rule violation on a post the caller can't edit. Found by a multi-persona code review (correctness, security, adversarial, testing, maintainability, project-standards, agent-native, learnings) of PR #1861. Full suite (877 tests) and phpcs pass.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/phpunit/includes/classes/RestApiEndpointsTest.php (1)
1093-1124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that the unrelated-rule row stays untouched.
The test only checks the 200 status. It does not prove that the batch excluded the admin-owned row. Capture the second
insert_idand assert itsignreis still0. This makes the test fail if the rule filter is later removed but the permission check is relaxed.♻️ Proposed additional assertion
[ '%d', '%d', '%s', '%s', '%s', '%s', '%d', '%d', '%d', '%d' ] ); + $unrelated_rule_issue_id = $wpdb->insert_id; // phpcs:enable WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching @@ $this->assertSame( 200, $response->get_status(), 'Large batch dismiss should succeed even though an unrelated-rule issue on an unauthorized post shares the object.' ); + + // phpcs:ignore WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching -- Need fresh data for assertions. + $unrelated_ignre = $wpdb->get_var( + $wpdb->prepare( 'SELECT ignre FROM %i WHERE id = %d', $table_name, $unrelated_rule_issue_id ) + ); + $this->assertSame( '0', $unrelated_ignre, 'The unrelated-rule issue on the unauthorized post must stay open.' ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/phpunit/includes/classes/RestApiEndpointsTest.php` around lines 1093 - 1124, Update the test around the two database inserts and the large-batch dispatch to capture the second insert’s ID, then query that row afterward and assert its ignre value remains 0. Keep the existing status assertion and use the captured ID to verify the unrelated-rule row was not modified.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/phpunit/includes/classes/RestApiEndpointsTest.php`:
- Around line 1093-1124: Update the test around the two database inserts and the
large-batch dispatch to capture the second insert’s ID, then query that row
afterward and assert its ignre value remains 0. Keep the existing status
assertion and use the captured ID to verify the unrelated-rule row was not
modified.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: eaa4d416-062c-4607-b4a4-85054998d549
📒 Files selected for processing (2)
includes/classes/class-rest-api.phptests/phpunit/includes/classes/RestApiEndpointsTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- includes/classes/class-rest-api.php
test_large_batch_dismiss_succeeds_when_unrelated_rule_row_is_on_unauthorized_post only checked the 200 response status, which would also pass if the rule filter were removed and the permission check merely relaxed instead. Capture the admin-owned row's id and assert its ignre stays 0, so the test actually proves the batch excluded it rather than just succeeding.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
object.dismiss_issue()'s largeBatch path inclass-rest-api.phpselected and updated every row matchingsiteid+objectonly. The pro plugin's parallel fix (PR Backport: Release/1.17.0 #832 on the paired PRO-1239 branch) scoped its own query byrule+object, but never touched this one — the actual query that flipsignre/ignre_globalon live rows.rulealongsideobjectand filters the batch-selection query by both, mirroring the pro plugin's already-correct scoping.Test plan
test_large_batch_dismiss_only_affects_matching_rule— seeds two posts sharing rule+object and a third sharing only the object; confirms the third is left untouched after a global dismiss. Confirmed failing against the pre-fix query, passing after.test_large_batch_dismiss_authorized_on_all,test_large_batch_dismiss_authorized_on_some) whose fixtures used different rules per row within one "batch" — that setup encoded the same assumption behind the bug and needed to share a rule to still be considered one batch under the corrected query.RestApiEndpointsTestsuite passes (13/13).docker compose+scripts/setup-phpunit.shflow.Linear: PRO-1264 (related to PRO-1239)
🤖 Generated with Claude Code
Summary by CodeRabbit