Fixed: prevent data corruption when options are saved during an AJAX request.#287
Merged
Fixed: prevent data corruption when options are saved during an AJAX request.#287
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds PLAUSIBLE_CI guards to short‑circuit CI paths, standardizes AJAX array access and JSON decoding, removes AJAX-driven settings merge from Helpers::get_settings, introduces integration tests for Ajax save_options, and adjusts Assets enqueue timing and several test helpers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/HelpersTest.php (1)
17-21:⚠️ Potential issue | 🟠 MajorFix TestableHelpers mock to properly implement the Client type check.
The
testGetJsUrlandtestGetJsPathtests fail becauseTestableHelpers::get_client()returns an anonymous class that doesn't extendClient. WhenHelpers::get_filename()evaluates theinstanceof Clientcheck on line 51 ofsrc/Helpers.php, it fails and returns an empty string, resulting in malformed URLs likehttps://plausible.io/js/.jsinstead ofhttps://plausible.io/js/pa-test-tracker-id.js.Override
get_filename()inTestableHelpersto return the test tracker ID directly, similar to the pattern used in other tests likeAdmin/ModuleTest.php.
🧹 Nitpick comments (3)
src/Ajax.php (1)
285-289: CI guard causes inconsistent behavior on empty options.When
PLAUSIBLE_CIis defined and$optionsis empty, the function returns early (line 286) without callingwp_send_json_error(). However, this means in CI the function returnsvoidsilently, while in production it sends a 400 error response. This asymmetry could mask issues in tests.Consider returning a value or throwing an exception to make test assertions more explicit.
tests/integration/AjaxTest.php (2)
35-54: Test lacks cleanup and proper assertions.
Missing
$_POSTcleanup: Tests modify$_POSTbut don't clean up intearDown(), potentially polluting subsequent tests.Empty catch block: With
PLAUSIBLE_CIdefined,save_options()returnsvoidinstead of callingdie(), so no exception is thrown. The try-catch is unnecessary.No assertion on return behavior: Consider asserting that
Messages::set_success()was called or checking transients.Suggested improvements
+ public function tearDown(): void { + unset( $_POST['_nonce'], $_POST['options'] ); + parent::tearDown(); + } public function testSaveOptionsSuccess() { $options = [ [ 'name' => 'domain_name', 'value' => 'example.com' ], [ 'name' => 'proxy_enabled', 'value' => 'on' ], ]; $_POST['_nonce'] = wp_create_nonce( 'plausible_analytics_toggle_option' ); $_POST['options'] = wp_json_encode( $options ); - // We use catch because wp_send_json_success calls die() - try { - $this->ajax->save_options(); - } catch ( \Exception $e ) { - // Catching any unexpected exceptions - } + $this->ajax->save_options(); $settings = Helpers::get_settings(); $this->assertEquals( 'example.com', $settings['domain_name'] ); $this->assertEquals( 'on', $settings['proxy_enabled'] ); }
83-97: Invalid JSON test could be more explicit.The test verifies settings weren't updated to
'invalid-json', but this assertion is weak since'invalid-json'wouldn't be a valid value anyway. Consider:
- Asserting that the original/default
domain_namevalue is preserved- Verifying that
Messages::set_error()was called with the expected messageMore robust assertion
public function testSaveOptionsInvalidJson() { + $original_settings = Helpers::get_settings(); + $original_domain = $original_settings['domain_name']; + $_POST['_nonce'] = wp_create_nonce( 'plausible_analytics_toggle_option' ); $_POST['options'] = 'invalid-json'; - // wp_send_json_error will be called, which we expect. - // In a real WP environment it would exit. - try { - $this->ajax->save_options(); - } catch ( \Exception $e ) { - } + $this->ajax->save_options(); // Verify that settings were NOT updated to something weird. $settings = Helpers::get_settings(); - $this->assertNotEquals( 'invalid-json', $settings['domain_name'] ); + $this->assertEquals( $original_domain, $settings['domain_name'] ); }
Collaborator
Author
|
@coderabbitai Could you expand on this comment? "Fix TestableHelpers mock to properly implement the Client type check." |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 12245 🏁 Script executed: Length of output: 4733
The ProblemIn public static function get_filename() {
$client = static::get_client();
if ( $client instanceof Client ) {
return $client->get_tracker_id();
}
return '';
}However, in protected static function get_client() {
return new class {
public function get_tracker_id() {
return 'pa-test-tracker-id';
}
};
}The IssueWhen tests call
This means your tests might not be actually testing what they appear to test, or they're passing for the wrong reasons. The SolutionThe mock should return an object that passes the type check. Here are two approaches: Option 1: Extend Client (recommended) protected static function get_client() {
return new class('mock-token') extends Client {
public function get_tracker_id() {
return 'pa-test-tracker-id';
}
};
}Option 2: Create a dedicated mock class class MockClient extends Client {
public function __construct() {
// Skip parent constructor
}
public function get_tracker_id() {
return 'pa-test-tracker-id';
}
}Would you like me to check if this is causing any test failures in the current implementation? |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/TestableHelpers.php (1)
18-23:⚠️ Potential issue | 🟡 MinorOverride
__construct()to avoid test side effects from the parent Client class.The anonymous class inherits Client's constructor, which performs I/O operations (
Helpers::get_hosted_domain_url()) and instantiatesDefaultApiandGuzzleClient. To keep the test helper lightweight and isolated, add a no-op constructor override:Suggested fix
return new class extends Client { + public function __construct() { + // Override to prevent parent initialization side effects in tests + } public function get_tracker_id() { return 'pa-test-tracker-id'; } };
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@tests/integration/AssetsTest.php`:
- Around line 18-46: The finally block in the AssetsTest removes actions but
doesn't restore them, causing hook leakage; update the finally block that
currently calls remove_filter( 'plausible_analytics_settings', [ $this,
'enableAdministratorTracking' ] ) and wp_set_current_user( null ) to also re-add
the removed hooks by calling add_action( 'wp_enqueue_scripts', 'maybe_enqueue' )
and add_action( 'wp_enqueue_scripts', 'maybe_enqueue', 11 ); alternatively
implement a tearDown() method on AssetsTest that restores these actions (and any
other altered hooks) so removeAction/remove_action calls from the test are
always undone.
Comment on lines
+18
to
+46
| try { | ||
| add_filter( 'plausible_analytics_settings', [ $this, 'enableAdministratorTracking' ] ); | ||
|
|
||
| $this->removeAction( 'wp_enqueue_scripts', 'maybe_enqueue' ); | ||
| $this->removeAction( 'wp_enqueue_scripts', 'maybe_enqueue', 11 ); | ||
| $class = $this->getMockBuilder( Assets::class ) | ||
| ->disableOriginalConstructor() | ||
| ->onlyMethods( [ 'get_js_url' ] ) | ||
| ->getMock(); | ||
|
|
||
| $class->method( 'get_js_url' ) | ||
| ->willReturn( 'https://plausible.test/js/plausible.js' ); | ||
| $this->removeAction( 'wp_enqueue_scripts', 'maybe_enqueue' ); | ||
| $this->removeAction( 'wp_enqueue_scripts', 'maybe_enqueue', 11 ); | ||
|
|
||
| ob_start(); | ||
| $class->method( 'get_js_url' ) | ||
| ->willReturn( 'https://plausible.test/js/plausible.js' ); | ||
|
|
||
| $class->maybe_enqueue_main_script(); | ||
| wp_set_current_user( 1 ); | ||
| $user = wp_get_current_user(); | ||
| $user->add_role( 'administrator' ); | ||
|
|
||
| do_action( 'wp_head' ); | ||
| $class->maybe_enqueue_main_script(); | ||
|
|
||
| $output = ob_get_clean(); | ||
| global $wp_scripts; | ||
| $data = $wp_scripts->get_data( 'plausible-analytics', 'after' ); | ||
|
|
||
| $this->assertStringContainsString( 'window.plausible', $output ); | ||
| $this->assertStringContainsString( 'plausible.init', $output ); | ||
| $this->assertStringContainsString( 'window.plausible', implode( '', $data ) ); | ||
| $this->assertStringContainsString( 'plausible.init', implode( '', $data ) ); | ||
| } finally { | ||
| remove_filter( 'plausible_analytics_settings', [ $this, 'enableAdministratorTracking' ] ); | ||
| wp_set_current_user( null ); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "TestCase.php" -type fRepository: plausible/wordpress
Length of output: 83
🏁 Script executed:
rg -n "function (setUp|tearDown)" tests/ -A 10Repository: plausible/wordpress
Length of output: 1267
🏁 Script executed:
rg -n "remove_all_filters|remove_all_actions|wp_filter" tests/ -C 2Repository: plausible/wordpress
Length of output: 866
🏁 Script executed:
cat tests/TestCase.phpRepository: plausible/wordpress
Length of output: 5644
🏁 Script executed:
cat tests/integration/AssetsTest.phpRepository: plausible/wordpress
Length of output: 5969
🏁 Script executed:
rg -n "class AssetsTest|function setUp|function tearDown" tests/integration/AssetsTest.php -A 5Repository: plausible/wordpress
Length of output: 211
🏁 Script executed:
web_search
What does Yoast WPTestUtils BrainMonkey TestCase do with WordPress hooks between tests tearDownRepository: plausible/wordpress
Length of output: 156
🏁 Script executed:
rg -n "brain_monkey|reset|tearDown" tests/ --type php -iRepository: plausible/wordpress
Length of output: 188
🏁 Script executed:
rg -n "BrainMonkey|wp_filter|wp_actions" . -type php --max-count=20Repository: plausible/wordpress
Length of output: 90
🏁 Script executed:
find . -name "composer.json" -o -name "composer.lock" | head -5Repository: plausible/wordpress
Length of output: 148
🏁 Script executed:
cat composer.jsonRepository: plausible/wordpress
Length of output: 2531
🏁 Script executed:
find vendor -name "TestCase.php" -path "*WPTestUtils*" 2>/dev/null | head -1Repository: plausible/wordpress
Length of output: 45
🏁 Script executed:
find vendor -name "*.php" -path "*wp-test-utils*" | grep -i testcase | head -3Repository: plausible/wordpress
Length of output: 104
🏁 Script executed:
rg -n "remove_all_filters|remove_all_actions|reset_all" vendor/ --max-count=10 2>/dev/null || echo "Search complete"Repository: plausible/wordpress
Length of output: 78
Restore removed actions to prevent hook state leakage across tests.
The removeAction calls on lines 26-27 should be restored in the finally block. Without restoration, subsequent tests may miss these hooks. Add the following to the finally block:
Add to finally block after line 45:
// Restore removed actions
add_action( 'wp_enqueue_scripts', 'maybe_enqueue' );
add_action( 'wp_enqueue_scripts', 'maybe_enqueue', 11 );Alternatively, implement a tearDown() method in AssetsTest that restores all removed actions for all test methods.
🤖 Prompt for AI Agents
In `@tests/integration/AssetsTest.php` around lines 18 - 46, The finally block in
the AssetsTest removes actions but doesn't restore them, causing hook leakage;
update the finally block that currently calls remove_filter(
'plausible_analytics_settings', [ $this, 'enableAdministratorTracking' ] ) and
wp_set_current_user( null ) to also re-add the removed hooks by calling
add_action( 'wp_enqueue_scripts', 'maybe_enqueue' ) and add_action(
'wp_enqueue_scripts', 'maybe_enqueue', 11 ); alternatively implement a
tearDown() method on AssetsTest that restores these actions (and any other
altered hooks) so removeAction/remove_action calls from the test are always
undone.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Ajax.php (1)
284-365:⚠️ Potential issue | 🟡 MinorInitialize list values and preserve falsy inputs when collapsing array options.
In the array normalization block,$options[0]->valuecan remain unset (notice risk later), and values like"0"are currently dropped. Initialize the array and use a non-empty-string check.🛠️ Suggested fix
$options = []; $array_name = preg_replace( '/\[[0-9]+]/', '', $input_array_elements[0]->name ); $options[0] = (object) []; $options[0]->name = $array_name; + $options[0]->value = []; foreach ( $input_array_elements as $input_array_element ) { - if ( $input_array_element->value ) { + if ( isset( $input_array_element->value ) && $input_array_element->value !== '' ) { $options[0]->value[] = $input_array_element->value; } }
🧹 Nitpick comments (1)
src/Ajax.php (1)
103-135: Use WordPress's standard protocol whitelist instead of user-controlled schemes.
Passing[ $parsed['scheme'] ]as the allowed protocols effectively whitelists any scheme present in the user input—including dangerous ones likejavascript:. This bypasses the security protections thatesc_url_raw()is designed to provide. Usewp_allowed_protocols()to leverage WordPress's curated list of safe schemes.♻️ Suggested adjustment
- if ( isset( $parsed['scheme'] ) ) { - return esc_url_raw( wp_unslash( $var ), [ $parsed['scheme'] ] ); - } + if ( isset( $parsed['scheme'] ) ) { + return esc_url_raw( wp_unslash( $var ), wp_allowed_protocols() ); + }
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/Admin/Module.php`:
- Around line 202-203: The ternary initializing $settings_proxy can access
$settings['proxy_enabled'] when $settings is not an array; update the expression
to match the safe pattern used in maybe_install_module and for $old_proxy by
making it: if is_array($settings) and isset($settings['proxy_enabled']) use
$settings['proxy_enabled'], otherwise fallback to an empty string—replace the
current mixed fallback with this consistent safe check for $settings_proxy.
🧹 Nitpick comments (1)
src/Admin/Module.php (1)
268-271: Consider checking the constant's value, not just its existence.The current check
defined( 'PLAUSIBLE_CI' )will bypass the test if the constant is defined with any value, includingfalseor0. If this is intentional (any definition indicates CI), the code is fine. If you want to allow disabling the bypass by settingPLAUSIBLE_CIto a falsy value, consider:Optional: Check constant value
- if ( defined( 'PLAUSIBLE_CI' ) ) { + if ( defined( 'PLAUSIBLE_CI' ) && PLAUSIBLE_CI ) {
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Fixed: patched minor XSS risk in clean()
…invalid_api_token
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 by CodeRabbit
Tests
Bug Fixes
New Features
Chores