-
Notifications
You must be signed in to change notification settings - Fork 825
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.
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
WooCommerce Analytics: Improve get_current_url()
and URL sanitization
#44679
Conversation
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
$cookie_js = " | ||
const sessionData = JSON.stringify({ | ||
session_id: '$this->session_id', | ||
landing_page: encodeURIComponent('$this->landing_page'), |
There was a problem hiding this comment.
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.
dd4ea3d
to
e0a7d50
Compare
'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';"; |
There was a problem hiding this comment.
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.”.
…URL sanitization * Refactor session cookie creation to use wp_json_encode for better data handling. * Enhance get_current_url method to improve URL sanitization and handling of request components.
…data encoding * Simplify session data creation by directly using an array and encoding it with rawurlencode. * Update get_session_cookie method to handle cookie decoding more robustly without unnecessary sanitization. * Ensure proper handling of session data retrieval from cookies.
…ssion data encoding * Update landing page retrieval to use the method directly instead of a variable. * Change session data encoding to use wp_json_encode for consistency and improved handling. * Simplify cookie decoding by removing unnecessary rawurlencode.
ef7aa48
to
4853b2e
Compare
@@ -828,7 +835,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 ) ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
* Simplify session engagement handling by directly modifying session data and using wp_json_encode for cookie creation. * Remove unnecessary cookie decoding logic and streamline JavaScript enqueueing for engagement tracking.
…e tracking * Add landing page encoding to session data before cookie creation. * Ensure session engagement flag is set correctly for improved tracking.
get_current_url()
and URL sanitization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
207cd57
into
release/woocommerce-analytics-0.5.0
Fixes WOOA7S-344
Proposed changes:
Implements security improvements to address potential XSS vulnerabilities in WooCommerce Analytics session cookie handling and URL sanitization identified during code review.
Note: The session management is a new feature that is not yet in production.
The Problem:
During security analysis, theoretical XSS vulnerabilities were identified where user-controlled data (
session_id
,landing_page
,session_expiration
) was directly interpolated into JavaScript strings without proper escaping. However, during extensive testing with various payloads, browsers automatically URL-encode special characters (e.g.,'
becomes%27
), which prevents most practical XSS attacks. Despite multiple attack attempts, no successful XSS execution was achieved through these vectors.The Solution (Defense in Depth):
Although the vulnerabilities appear largely mitigated by browser behavior, defensive security improvements were implemented following security best practices:
Enhanced Session Cookie Security:
rawurlencode(wp_json_encode())
for proper data encodingrawurldecode()
and robust validationImproved URL Handling:
$_SERVER
variable usage with WordPress URL functions (home_url()
+add_query_arg()
)get_current_url()
method with WordPress-native URL handling:home_url( add_query_arg( null, null ) )
Security Improvements:
$_SERVER
access with secure WordPress URL functionsTesting instructions:
Security Testing Performed
XSS Attack Attempts:
/?test=');alert('XSS');//
'
→%27
,)
→%29
)Functional Testing
Session Cookie Functionality:
woocommerceanalytics_session
cookie contains properly encoded JSONsession_id
,landing_page
, andexpires
fieldsw.gif
request contains the expectedlanding_page
andsession_id
fields.woocommerceanalytics_session
cookie is updated with theis_engaged
field set to true.Other information:
Jetpack product discussion
Discussed with Woo Analytics team as part of security hardening review.
Does this pull request change what data or activity we track or use?
No, this PR only improves the security of existing data handling without changing what data is tracked or how it's used.