Steve-Dee-Designs
/
woocommerce
mirror of https://github.com/woocommerce/woocommerce.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

add: sanitise query params in remote logging (PHP) (#51013)

This commit is contained in:
RJ 2024-09-02 12:38:22 +08:00 committed by GitHub
parent 3def18623e
commit 36ede651db
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 628 additions and 477 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
plugins/woocommerce/changelog/fix-remote-logger-sanitise-query-params Normal file
View File

@ -0,0 +1,4 @@
Significance: minor
Type: enhancement
Add query params masking to remote logger

50
plugins/woocommerce/src/Internal/Logging/RemoteLogger.php
View File

@ -70,7 +70,7 @@ class RemoteLogger extends \WC_Log_Handler {
'wc_version' => WC()->version,
'php_version' => phpversion(),
'wp_version' => get_bloginfo( 'version' ),
'request_uri' => filter_input( INPUT_SERVER, 'REQUEST_URI', FILTER_SANITIZE_URL ),
'request_uri' => $this->sanitize_request_uri( filter_input( INPUT_SERVER, 'REQUEST_URI', FILTER_SANITIZE_URL ) ),
),
);
@ -431,4 +431,52 @@ class RemoteLogger extends \WC_Log_Handler {
protected function is_dev_or_local_environment() {
return in_array( wp_get_environment_type(), array( 'development', 'local' ), true );
}
/**
* Sanitize the request URI to only allow certain query parameters.
*
* @param string $request_uri The request URI to sanitize.
* @return string The sanitized request URI.
*/
private function sanitize_request_uri( $request_uri ) {
$default_whitelist = array( 'path', 'page', 'step', 'task', 'tab' );
/**
* Filter to allow other plugins to whitelist request_uri query parameter values for unmasked remote logging.
*
* @since 9.4.0
*
* @param string $default_whitelist The default whitelist of query parameters.
*/
$whitelist = apply_filters( 'woocommerce_remote_logger_request_uri_whitelist', $default_whitelist );
$parsed_url = wp_parse_url( $request_uri );
if ( ! isset( $parsed_url['query'] ) ) {
return $request_uri;
}
parse_str( $parsed_url['query'], $query_params );
foreach ( $query_params as $key => &$value ) {
if ( ! in_array( $key, $whitelist, true ) ) {
$value = 'xxxxxx';
}
}
$parsed_url['query'] = http_build_query( $query_params );
return $this->build_url( $parsed_url );
}
/**
* Build a URL from its parsed components.
*
* @param array $parsed_url The parsed URL components.
* @return string The built URL.
*/
private function build_url( $parsed_url ) {
$path = $parsed_url['path'] ?? '';
$query = isset( $parsed_url['query'] ) ? "?{$parsed_url['query']}" : '';
$fragment = isset( $parsed_url['fragment'] ) ? "#{$parsed_url['fragment']}" : '';
return "$path$query$fragment";
}
}

105
plugins/woocommerce/tests/php/src/Internal/Logging/RemoteLoggerTest.php
View File

@ -1,7 +1,11 @@
<?php
declare( strict_types = 1 );
namespace Automattic\WooCommerce\Tests\Internal\Logging;
// phpcs:disable Universal.Namespaces.DisallowCurlyBraceSyntax.Forbidden -- need to override filter_input
// phpcs:disable Universal.Files.SeparateFunctionsFromOO.Mixed -- same
// phpcs:disable Universal.Namespaces.OneDeclarationPerFile.MultipleFound -- same
namespace Automattic\WooCommerce\Tests\Internal\Logging {
use Automattic\WooCommerce\Internal\Logging\RemoteLogger;
use WC_Rate_Limiter;
@ -56,6 +60,7 @@ class RemoteLoggerTest extends \WC_Unit_Test_Case {
'pre_http_request',
'woocommerce_remote_logger_formatted_log_data',
'pre_site_transient_update_plugins',
'woocommerce_remote_logger_request_uri_whitelist',
);
foreach ( $filters as $filter ) {
remove_all_filters( $filter );
@ -124,7 +129,6 @@ class RemoteLoggerTest extends \WC_Unit_Test_Case {
);
}
/**
* @testdox should_current_version_be_logged method behaves correctly
* @dataProvider should_current_version_be_logged_provider
@ -192,6 +196,7 @@ class RemoteLoggerTest extends \WC_Unit_Test_Case {
$this->assertNull( $result, 'The result should be null when no update is available.' );
}
/**
* @testdox get_formatted_log method returns expected array structure
* @dataProvider get_formatted_log_provider
@ -252,6 +257,79 @@ class RemoteLoggerTest extends \WC_Unit_Test_Case {
);
}
/**
* @testdox get_formatted_log method correctly sanitizes request URI
*/
public function test_get_formatted_log_sanitizes_request_uri() {
global $mock_filter_input, $mock_return;
$mock_filter_input = true;
$mock_return = '/shop?path=home&user=admin&token=abc123';
$formatted_log = $this->sut->get_formatted_log( 'error', 'Test message', array() );
$mock_filter_input = false;
$this->assertArrayHasKey( 'properties', $formatted_log );
$this->assertArrayHasKey( 'request_uri', $formatted_log['properties'] );
$this->assertNotNull( $formatted_log['properties']['request_uri'], 'Request URI should not be null' );
$this->assertStringContainsString( 'path=home', $formatted_log['properties']['request_uri'] );
$this->assertStringContainsString( 'user=xxxxxx', $formatted_log['properties']['request_uri'] );
$this->assertStringContainsString( 'token=xxxxxx', $formatted_log['properties']['request_uri'] );
}
/**
* @testdox sanitize_request_uri method respects whitelist filter
*/
public function test_sanitize_request_uri_respects_whitelist_filter() {
add_filter(
'woocommerce_remote_logger_request_uri_whitelist',
function ( $whitelist ) {
$whitelist[] = 'custom_param';
return $whitelist;
}
);
$request_uri = '/shop?path=home&custom_param=value&token=abc123';
$sanitized_uri = $this->invoke_private_method( $this->sut, 'sanitize_request_uri', array( $request_uri ) );
$this->assertStringContainsString( 'path=home', $sanitized_uri );
$this->assertStringContainsString( 'custom_param=value', $sanitized_uri );
$this->assertStringContainsString( 'token=xxxxxx', $sanitized_uri );
}
/**
* @testdox sanitize_request_uri method correctly sanitizes request URIs
*/
public function test_sanitize_request_uri() {
$reflection = new \ReflectionClass( $this->sut );
$method = $reflection->getMethod( 'sanitize_request_uri' );
$method->setAccessible( true );
// Test with whitelisted parameters.
$request_uri = '/shop?path=home&page=2&step=1&task=checkout';
$sanitized_uri = $method->invokeArgs( $this->sut, array( $request_uri ) );
$this->assertStringContainsString( 'path=home', $sanitized_uri );
$this->assertStringContainsString( 'page=2', $sanitized_uri );
$this->assertStringContainsString( 'step=1', $sanitized_uri );
$this->assertStringContainsString( 'task=checkout', $sanitized_uri );
// Test with non-whitelisted parameters.
$request_uri = '/shop?path=home&user=admin&token=abc123';
$sanitized_uri = $method->invokeArgs( $this->sut, array( $request_uri ) );
$this->assertStringContainsString( 'path=home', $sanitized_uri );
$this->assertStringContainsString( 'user=xxxxxx', $sanitized_uri );
$this->assertStringContainsString( 'token=xxxxxx', $sanitized_uri );
// Test with mixed parameters.
$request_uri = '/shop?path=home&page=2&user=admin&step=1&token=abc123';
$sanitized_uri = $method->invokeArgs( $this->sut, array( $request_uri ) );
$this->assertStringContainsString( 'path=home', $sanitized_uri );
$this->assertStringContainsString( 'page=2', $sanitized_uri );
$this->assertStringContainsString( 'step=1', $sanitized_uri );
$this->assertStringContainsString( 'user=xxxxxx', $sanitized_uri );
$this->assertStringContainsString( 'token=xxxxxx', $sanitized_uri );
}
/**
* @testdox should_handle method behaves correctly under different conditions
* @dataProvider should_handle_provider
@ -480,7 +558,6 @@ class RemoteLoggerTest extends \WC_Unit_Test_Case {
$this->setup_mock_plugin_updates( $enabled ? WC()->version : '9.0.0' );
}
/**
* Set up mock plugin updates.
*
@ -545,3 +622,25 @@ class RemoteLoggerWithEnvironmentOverride extends RemoteLogger {
}
}
//phpcs:enable Generic.Files.OneObjectStructurePerFile.MultipleFound, Squiz.Classes.ClassFileName.NoMatch, Suin.Classes.PSR4.IncorrectClassName
}
/**
* Mocks for global functions used in RemoteLogger.php
*/
namespace Automattic\WooCommerce\Internal\Logging {
/**
* The filter_input function will return NULL if we change the $_SERVER variables at runtime, so we
* need to override it in RemoteLogger's namespace when we want it to return a specific value for testing.
*
* @return mixed
*/
function filter_input() {
global $mock_filter_input, $mock_return;
if ( true === $mock_filter_input ) {
return $mock_return;
} else {
return call_user_func_array( '\filter_input', func_get_args() );
}
}
}