From 258ae0b03b848e24948122a6f6e90b813f02221d Mon Sep 17 00:00:00 2001 From: RJ <27843274+rjchow@users.noreply.github.com> Date: Tue, 3 Sep 2024 13:42:03 +0800 Subject: [PATCH] add: remote logger request uri sanitisation (JS) (#51046) * add: remote logger request uri sanitisation * md lint * Update packages/js/remote-logging/README.md Co-authored-by: Paul Sealock * pr feedback --------- Co-authored-by: Paul Sealock --- packages/js/remote-logging/README.md | 24 ++++++++++ .../changelog/add-query-params-sanitisation | 4 ++ .../js/remote-logging/src/remote-logger.ts | 44 ++++++++++++++++++- packages/js/remote-logging/src/test/index.ts | 23 ++++++++++ ...add-more-params-to-remote-logger-whitelist | 4 ++ .../src/Internal/Logging/RemoteLogger.php | 13 +++++- 6 files changed, 109 insertions(+), 3 deletions(-) create mode 100644 packages/js/remote-logging/changelog/add-query-params-sanitisation create mode 100644 plugins/woocommerce/changelog/add-more-params-to-remote-logger-whitelist diff --git a/packages/js/remote-logging/README.md b/packages/js/remote-logging/README.md index 49dac108054..c98a43a9ed8 100644 --- a/packages/js/remote-logging/README.md +++ b/packages/js/remote-logging/README.md @@ -172,3 +172,27 @@ addFilter( (endpoint) => 'https://my-custom-endpoint.com/js-error-log' ); ``` + +### `woocommerce_remote_logging_request_uri_whitelist` + +Modifies the list of whitelisted query parameters that won't be masked in the logged request URI + +**Parameters:** + +- `whitelist` (string[]): The default whitelist. + +**Return value:** (string[]) The modified whitelist. + +**Usage example:** + +```js +import { addFilter } from '@wordpress/hooks'; + +addFilter( + 'woocommerce_remote_logging_request_uri_whitelist', + 'my-plugin', + ( whitelist ) => { + return [ ...whitelist, 'exampleParam' ] + } +); +``` diff --git a/packages/js/remote-logging/changelog/add-query-params-sanitisation b/packages/js/remote-logging/changelog/add-query-params-sanitisation new file mode 100644 index 00000000000..59d27ec04e8 --- /dev/null +++ b/packages/js/remote-logging/changelog/add-query-params-sanitisation @@ -0,0 +1,4 @@ +Significance: minor +Type: update + +Add query params sanitisation diff --git a/packages/js/remote-logging/src/remote-logger.ts b/packages/js/remote-logging/src/remote-logger.ts index 06832a995d8..c35024a7110 100644 --- a/packages/js/remote-logging/src/remote-logger.ts +++ b/packages/js/remote-logging/src/remote-logger.ts @@ -33,6 +33,44 @@ export const REMOTE_LOGGING_LOG_ENDPOINT_FILTER = export const REMOTE_LOGGING_JS_ERROR_ENDPOINT_FILTER = 'woocommerce_remote_logging_js_error_endpoint'; +export const REMOTE_LOGGING_REQUEST_URI_PARAMS_WHITELIST_FILTER = + 'woocommerce_remote_logging_request_uri_whitelist'; + +export const REMPOTE_LOGGING_REQUEST_URI_PARAMS_DEFAULT_WHITELIST = [ + 'path', + 'page', + 'step', + 'task', + 'tab', + 'section', + 'status', + 'post_type', + 'taxonomy', + 'action', +]; + +export const sanitiseRequestUriParams = ( search: string ) => { + const params = new URLSearchParams( search ); + + /** + * This filter modifies the list of whitelisted query parameters that won't be masked + * in the logged request URI + * + * @filter woocommerce_remote_logging_request_uri_whitelist + * @param {string[]} whitelist The default whitelist + */ + const whitelist = applyFilters( + REMOTE_LOGGING_REQUEST_URI_PARAMS_WHITELIST_FILTER, + REMPOTE_LOGGING_REQUEST_URI_PARAMS_DEFAULT_WHITELIST + ) as typeof REMPOTE_LOGGING_REQUEST_URI_PARAMS_DEFAULT_WHITELIST; + for ( const [ key ] of params ) { + if ( ! whitelist.includes( key ) ) { + params.set( key, 'xxxxxx' ); + } + } + return params.toString(); +}; + const REMOTE_LOGGING_LAST_ERROR_SENT_KEY = 'wc_remote_logging_last_error_sent_time'; @@ -106,7 +144,8 @@ export class RemoteLogger { properties: { ...extraData?.properties, request_uri: - window.location.pathname + window.location.search, + window.location.pathname + + sanitiseRequestUriParams( window.location.search ), }, } ), trace: this.getFormattedStackFrame( @@ -213,7 +252,8 @@ export class RemoteLogger { tags: [ 'js-unhandled-error' ], properties: { request_uri: - window.location.pathname + window.location.search, + window.location.pathname + + sanitiseRequestUriParams( window.location.search ), }, } ), trace: this.getFormattedStackFrame( trace ), diff --git a/packages/js/remote-logging/src/test/index.ts b/packages/js/remote-logging/src/test/index.ts index 66a06a42f50..617467f3561 100644 --- a/packages/js/remote-logging/src/test/index.ts +++ b/packages/js/remote-logging/src/test/index.ts @@ -13,6 +13,8 @@ import { REMOTE_LOGGING_ERROR_DATA_FILTER, REMOTE_LOGGING_LOG_ENDPOINT_FILTER, REMOTE_LOGGING_JS_ERROR_ENDPOINT_FILTER, + sanitiseRequestUriParams, + REMOTE_LOGGING_REQUEST_URI_PARAMS_WHITELIST_FILTER, } from '../remote-logger'; import { fetchMock } from './__mocks__/fetch'; @@ -380,3 +382,24 @@ describe( 'captureException', () => { expect( fetchMock ).not.toHaveBeenCalled(); } ); } ); + +describe( 'sanitiseRequestUriParams', () => { + afterEach(() => { + removeFilter(REMOTE_LOGGING_REQUEST_URI_PARAMS_WHITELIST_FILTER, 'test' ); + }) + it( 'should replace non-whitelisted params with xxxxxx', () => { + expect(sanitiseRequestUriParams('?path=home&user=admin&token=abc123')).toEqual('path=home&user=xxxxxx&token=xxxxxx') + }) + it( 'should not replace whitelisted params with xxxxxx', () => { + expect(sanitiseRequestUriParams('?path=home')).toEqual('path=home') + }) + it( 'should not do anything if empty string is passed in', () => { + expect(sanitiseRequestUriParams('')).toEqual('') + }) + it( 'should apply filters correctly', () => { + addFilter( REMOTE_LOGGING_REQUEST_URI_PARAMS_WHITELIST_FILTER, 'test', (defaultWhitelist) => { + return [ ... defaultWhitelist, 'foo' ]; + }) + expect(sanitiseRequestUriParams('?path=home&foo=bar&user=admin&token=abc123')).toEqual('path=home&foo=bar&user=xxxxxx&token=xxxxxx') + }) +}) \ No newline at end of file diff --git a/plugins/woocommerce/changelog/add-more-params-to-remote-logger-whitelist b/plugins/woocommerce/changelog/add-more-params-to-remote-logger-whitelist new file mode 100644 index 00000000000..381b5ccebd0 --- /dev/null +++ b/plugins/woocommerce/changelog/add-more-params-to-remote-logger-whitelist @@ -0,0 +1,4 @@ +Significance: minor +Type: update + +Added more paths to remote logger query param whitelist diff --git a/plugins/woocommerce/src/Internal/Logging/RemoteLogger.php b/plugins/woocommerce/src/Internal/Logging/RemoteLogger.php index 6daa928b46e..3043cd110fe 100644 --- a/plugins/woocommerce/src/Internal/Logging/RemoteLogger.php +++ b/plugins/woocommerce/src/Internal/Logging/RemoteLogger.php @@ -438,7 +438,18 @@ class RemoteLogger extends \WC_Log_Handler { * @return string The sanitized request URI. */ private function sanitize_request_uri( $request_uri ) { - $default_whitelist = array( 'path', 'page', 'step', 'task', 'tab' ); + $default_whitelist = array( + 'path', + 'page', + 'step', + 'task', + 'tab', + 'section', + 'status', + 'post_type', + 'taxonomy', + 'action', + ); /** * Filter to allow other plugins to whitelist request_uri query parameter values for unmasked remote logging.