Skip to content

WooCommerce Analytics: Improve get_current_url() and URL sanitization #44679

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

Merged
merged 7 commits into from
Aug 11, 2025
+34 −27
Merged

WooCommerce Analytics: Improve get_current_url() and URL sanitization #44679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
01fc346
[WooCommerce Analytics] Refactor session cookie handling and improve …
chihsuan Aug 7, 2025
56e6f5a
Add changelog
chihsuan Aug 7, 2025
c39a8f5
[WooCommerce Analytics] Refactor session cookie handling and improve …
chihsuan Aug 7, 2025
4853b2e
[WooCommerce Analytics] Refactor landing page handling and improve se…
chihsuan Aug 7, 2025
8dc316f
Update common props
chihsuan Aug 8, 2025
07d2cf1
[WooCommerce Analytics] Refactor session engagement logic
chihsuan Aug 8, 2025
3dd77b8
[WooCommerce Analytics] Enhance session data handling for landing pag…
chihsuan Aug 8, 2025
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
3 changes: 3 additions & 0 deletions projects/packages/woocommerce-analytics/changelog/enhance-js-url-sanitize
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Significance: patch
Type: fixed
Comment: enhance sanitize logic, no change log need since not released yet
58 changes: 31 additions & 27 deletions projects/packages/woocommerce-analytics/src/class-woo-analytics-trait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,22 +375,18 @@ private function maybe_record_engagement( $properties = array() ) {
return;
}

$event_js = $this->process_event_properties( 'woocommerceanalytics_session_engagement', $properties );
$add_engagement_to_cookie_js = "
const cookies = document.cookie.split('; ').reduce((acc, cookie) => {
const [name, value] = cookie.split('=');
acc[name] = value;
return acc;
}, {});

if (cookies.woocommerceanalytics_session) {
let sessionData = JSON.parse(decodeURIComponent(cookies.woocommerceanalytics_session));
sessionData.is_engaged = true;
document.cookie = `woocommerceanalytics_session=\${JSON.stringify(sessionData)}; expires=\${sessionData.expires}; path=/; secure; samesite=strict`;
}
";

wc_enqueue_js( $add_engagement_to_cookie_js );
$event_js = $this->process_event_properties( 'woocommerceanalytics_session_engagement', $properties );

$session_data = $this->get_session_cookie();
if ( empty( $session_data ) ) {
return;
}
$session_data['is_engaged'] = true;
$session_data['landing_page'] = rawurlencode( $session_data['landing_page'] );
$encoded_session_data = wp_json_encode( $session_data );
$cookie_js = "document.cookie = 'woocommerceanalytics_session={$encoded_session_data}; expires={$session_data['expires']}; path=/; secure; samesite=strict';";
wc_enqueue_js( $cookie_js );

wc_enqueue_js( "_wca.push({$event_js});" );
$this->engaged_session = true;
}
Expand All @@ -408,16 +404,16 @@ public function maybe_start_session() {
$this->is_new_session = true;

$session_expiration = $this->get_session_expiration_time();
$event_js = $this->process_event_properties( 'woocommerceanalytics_session_started' );
$cookie_js = "
const sessionData = JSON.stringify({
session_id: '$this->session_id',
landing_page: encodeURIComponent('$this->landing_page'),
Copy link
Member Author

@chihsuan chihsuan Aug 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If landing_page contains a malicious URL /?test='+window.alert('XSS')+', it would get interpolated directly into the JavaScript:

const sessionData = JSON.stringify({
        session_id: '9a2eabba-0ed7-49f1-b1d4-0d009e487c79',
        landing_page: encodeURIComponent('https://example.com/?test='+window.alert('XSS')+''),
        expires: 'Fri, 08 Aug 2025 08:53:34 GMT',
});

Now, we encode the entire object via PHP to avoid any potential attacks.

expires: '$session_expiration',
});
document.cookie = `woocommerceanalytics_session=\${sessionData}; expires=$session_expiration; path=/; secure; samesite=strict`;
";
$session_data = array(
'session_id' => $this->session_id,
'landing_page' => rawurlencode( $this->landing_page ),
'expires' => $session_expiration,
);
$encoded_data = wp_json_encode( $session_data );
$cookie_js = "document.cookie = 'woocommerceanalytics_session={$encoded_data}; expires={$session_expiration}; path=/; secure; samesite=strict';";
Copy link
Member Author

@chihsuan chihsuan Aug 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I considered using PHP to set the cookie, but I realized that it might not be the best approach for our use case since we may want to delay setting the cookie until getting user consent for tracking in the future implementation.

Additionally, PHP cookie can only be set before any output is sent to the browser. When I tried to set the cookie using PHP, I encountered an error message saying “Cannot send session cookie - headers already sent.”.

wc_enqueue_js( $cookie_js ); // save the session cookie for further events in the session

$event_js = $this->process_event_properties( 'woocommerceanalytics_session_started' );
wc_enqueue_js( "_wca.push({$event_js});" ); // trigger session started event
}
}
Expand Down Expand Up @@ -819,7 +815,15 @@ public function get_landing_page() {
* @return array
*/
public function get_session_cookie() {
return json_decode( sanitize_text_field( wp_unslash( $_COOKIE['woocommerceanalytics_session'] ?? '' ) ), true ) ?? array();
// phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized -- JSON is decoded and validated below. We don't need to sanitize the cookie value because we're not outputting it but decoding it as JSON. Sanitization might break the JSON.
$raw_cookie = isset( $_COOKIE['woocommerceanalytics_session'] ) ? wp_unslash( $_COOKIE['woocommerceanalytics_session'] ) : '';

if ( ! $raw_cookie ) {
return array();
}

$decoded = json_decode( $raw_cookie, true );
return is_array( $decoded ) ? $decoded : array();
}

/**
Expand All @@ -828,7 +832,7 @@ public function get_session_cookie() {
* @return string
*/
public function get_current_url() {
return sanitize_url( wp_unslash( ( empty( $_SERVER['HTTPS'] ) ? 'http' : 'https' ) . "://$_SERVER[HTTP_HOST]$_SERVER[REQUEST_URI]" ) ); // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotValidatedNotSanitized -- actually escaped with sanitize_url.
return home_url( add_query_arg( null, null ) );
Copy link
Member Author

@chihsuan chihsuan Aug 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I replaced usage of the $_SERVER variable with home_url() to retrieve the current URL, as home_url() should generally be more dependable.

Copy link
Member Author

@chihsuan chihsuan Aug 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The sanitize_url does not provide protection against XSS attacks, but it can be useful when storing URLs in a database. In this scenario, however, we are not storing the URL in a database but sending it to an external server.

echo sanitize_url( wp_unslash( ( "https://example.com/?test='+window.alert('XSS')+'' ) );

https://example.com/?test='+window.alert('XSS')+ // nothing is escaped

}

/**
Expand Down
Loading