From 277fd78bcae806d31140b2f526bf54e2d3ff0704 Mon Sep 17 00:00:00 2001 From: jonathansadowski Date: Tue, 14 Jun 2022 12:31:27 -0500 Subject: [PATCH] Sync payment gateway titles fix (#33418) * Introduce a 'safe_text' field that allows a reduced subset of HTML tags. * Escape on input as well as output; generalize sanitization function. * Set reasonable default rules. * Guard against invalid callbacks (escaping/sanitizing) functions. * Update plugins/woocommerce/src/Internal/Utilities/HtmlSanitizer.php Co-authored-by: Peter Fabian * Allow alt (accessibility) for img tags; allow class attributes for img and span tags. * Allow class attr for p tags. * Use safe_text for payment gateway titles. * Make HtmlSanitizer available through Utils service provider. * Update settings code to pull HtmlSanitizer as a service and to use the new sanitize() method. * Remove `style` from list of allowed attributes. Allowing arbitrary CSS rules through style could undo the intent of this change, since that would allow a range of positioning and sizing changes to be effected. * Remove unusued import. * If no (KSES) rules are specifed, then strip all tags (this is a safer default strategy). * For better safety, only apply pre-processor callbacks; remove responsibility for trimming of strings. Applying callbacks to a string after it ahs passed through wp_kses() could (potentially) undo the work done by that function, and result in unexpected tags in the sanitizer's output. Co-authored-by: barryhughes <3594411+barryhughes@users.noreply.github.com> Co-authored-by: Peter Fabian --- .../abstracts/abstract-wc-payment-gateway.php | 4 +- .../abstracts/abstract-wc-settings-api.php | 20 +++++ .../gateways/bacs/class-wc-gateway-bacs.php | 2 +- .../cheque/class-wc-gateway-cheque.php | 2 +- .../gateways/cod/class-wc-gateway-cod.php | 2 +- .../paypal/includes/settings-paypal.php | 2 +- .../UtilsClassesServiceProvider.php | 3 + .../src/Internal/Utilities/HtmlSanitizer.php | 88 +++++++++++++++++++ .../Internal/Utilities/HtmlSanitizerTest.php | 81 +++++++++++++++++ 9 files changed, 199 insertions(+), 5 deletions(-) create mode 100644 plugins/woocommerce/src/Internal/Utilities/HtmlSanitizer.php create mode 100644 plugins/woocommerce/tests/php/src/Internal/Utilities/HtmlSanitizerTest.php diff --git a/plugins/woocommerce/includes/abstracts/abstract-wc-payment-gateway.php b/plugins/woocommerce/includes/abstracts/abstract-wc-payment-gateway.php index 065d8d5bedd..6e8f947297c 100644 --- a/plugins/woocommerce/includes/abstracts/abstract-wc-payment-gateway.php +++ b/plugins/woocommerce/includes/abstracts/abstract-wc-payment-gateway.php @@ -10,6 +10,7 @@ */ use Automattic\Jetpack\Constants; +use Automattic\WooCommerce\Internal\Utilities\HtmlSanitizer; if ( ! defined( 'ABSPATH' ) ) { exit; @@ -304,7 +305,8 @@ abstract class WC_Payment_Gateway extends WC_Settings_API { * @return string */ public function get_title() { - return apply_filters( 'woocommerce_gateway_title', $this->title, $this->id ); + $title = wc_get_container()->get( HtmlSanitizer::class )->sanitize( $this->title, HtmlSanitizer::LOW_HTML_BALANCED_TAGS_NO_LINKS ); + return apply_filters( 'woocommerce_gateway_title', $title, $this->id ); } /** diff --git a/plugins/woocommerce/includes/abstracts/abstract-wc-settings-api.php b/plugins/woocommerce/includes/abstracts/abstract-wc-settings-api.php index 6aa83bc744e..2debc123e6a 100644 --- a/plugins/woocommerce/includes/abstracts/abstract-wc-settings-api.php +++ b/plugins/woocommerce/includes/abstracts/abstract-wc-settings-api.php @@ -9,6 +9,8 @@ defined( 'ABSPATH' ) || exit; +use Automattic\WooCommerce\Internal\Utilities\HtmlSanitizer; + /** * WC_Settings_API class. */ @@ -852,6 +854,24 @@ abstract class WC_Settings_API { return wp_kses_post( trim( stripslashes( $value ) ) ); } + /** + * Sanitize 'Safe Text' fields. + * + * These fields are similar to regular text fields, but a much smaller set of HTML tags are allowed. By default, + * this means `
`, ``, `

` and `` tags. + * + * Note: this is a sanitization method, rather than a validation method (the name is due to some historic naming + * choices). + * + * @param string $key Field key (currently unused). + * @param string $value Posted Value. + * + * @return string + */ + public function validate_safe_text_field( string $key, string $value ): string { + return wc_get_container()->get( HtmlSanitizer::class )->sanitize( $value, HtmlSanitizer::LOW_HTML_BALANCED_TAGS_NO_LINKS ); + } + /** * Validate Price Field. * diff --git a/plugins/woocommerce/includes/gateways/bacs/class-wc-gateway-bacs.php b/plugins/woocommerce/includes/gateways/bacs/class-wc-gateway-bacs.php index 661e84483fc..3d98230c900 100644 --- a/plugins/woocommerce/includes/gateways/bacs/class-wc-gateway-bacs.php +++ b/plugins/woocommerce/includes/gateways/bacs/class-wc-gateway-bacs.php @@ -86,7 +86,7 @@ class WC_Gateway_BACS extends WC_Payment_Gateway { ), 'title' => array( 'title' => __( 'Title', 'woocommerce' ), - 'type' => 'text', + 'type' => 'safe_text', 'description' => __( 'This controls the title which the user sees during checkout.', 'woocommerce' ), 'default' => __( 'Direct bank transfer', 'woocommerce' ), 'desc_tip' => true, diff --git a/plugins/woocommerce/includes/gateways/cheque/class-wc-gateway-cheque.php b/plugins/woocommerce/includes/gateways/cheque/class-wc-gateway-cheque.php index 7f5d003b72e..7023a7fb6e8 100644 --- a/plugins/woocommerce/includes/gateways/cheque/class-wc-gateway-cheque.php +++ b/plugins/woocommerce/includes/gateways/cheque/class-wc-gateway-cheque.php @@ -62,7 +62,7 @@ class WC_Gateway_Cheque extends WC_Payment_Gateway { ), 'title' => array( 'title' => __( 'Title', 'woocommerce' ), - 'type' => 'text', + 'type' => 'safe_text', 'description' => __( 'This controls the title which the user sees during checkout.', 'woocommerce' ), 'default' => _x( 'Check payments', 'Check payment method', 'woocommerce' ), 'desc_tip' => true, diff --git a/plugins/woocommerce/includes/gateways/cod/class-wc-gateway-cod.php b/plugins/woocommerce/includes/gateways/cod/class-wc-gateway-cod.php index e4b3e65c908..2e87dd94361 100644 --- a/plugins/woocommerce/includes/gateways/cod/class-wc-gateway-cod.php +++ b/plugins/woocommerce/includes/gateways/cod/class-wc-gateway-cod.php @@ -75,7 +75,7 @@ class WC_Gateway_COD extends WC_Payment_Gateway { ), 'title' => array( 'title' => __( 'Title', 'woocommerce' ), - 'type' => 'text', + 'type' => 'safe_text', 'description' => __( 'Payment method description that the customer will see on your checkout.', 'woocommerce' ), 'default' => __( 'Cash on delivery', 'woocommerce' ), 'desc_tip' => true, diff --git a/plugins/woocommerce/includes/gateways/paypal/includes/settings-paypal.php b/plugins/woocommerce/includes/gateways/paypal/includes/settings-paypal.php index 53729429e8e..19b8291d15c 100644 --- a/plugins/woocommerce/includes/gateways/paypal/includes/settings-paypal.php +++ b/plugins/woocommerce/includes/gateways/paypal/includes/settings-paypal.php @@ -16,7 +16,7 @@ return array( ), 'title' => array( 'title' => __( 'Title', 'woocommerce' ), - 'type' => 'text', + 'type' => 'safe_text', 'description' => __( 'This controls the title which the user sees during checkout.', 'woocommerce' ), 'default' => __( 'PayPal', 'woocommerce' ), 'desc_tip' => true, diff --git a/plugins/woocommerce/src/Internal/DependencyManagement/ServiceProviders/UtilsClassesServiceProvider.php b/plugins/woocommerce/src/Internal/DependencyManagement/ServiceProviders/UtilsClassesServiceProvider.php index ac5d60709cc..1e214574357 100644 --- a/plugins/woocommerce/src/Internal/DependencyManagement/ServiceProviders/UtilsClassesServiceProvider.php +++ b/plugins/woocommerce/src/Internal/DependencyManagement/ServiceProviders/UtilsClassesServiceProvider.php @@ -7,6 +7,7 @@ namespace Automattic\WooCommerce\Internal\DependencyManagement\ServiceProviders; use Automattic\WooCommerce\Internal\DependencyManagement\AbstractServiceProvider; use Automattic\WooCommerce\Internal\Utilities\DatabaseUtil; +use Automattic\WooCommerce\Internal\Utilities\HtmlSanitizer; /** * Service provider for the non-static utils classes in the Automattic\WooCommerce\src namespace. @@ -20,6 +21,7 @@ class UtilsClassesServiceProvider extends AbstractServiceProvider { */ protected $provides = array( DatabaseUtil::class, + HtmlSanitizer::class, ); /** @@ -27,5 +29,6 @@ class UtilsClassesServiceProvider extends AbstractServiceProvider { */ public function register() { $this->share( DatabaseUtil::class ); + $this->share( HtmlSanitizer::class ); } } diff --git a/plugins/woocommerce/src/Internal/Utilities/HtmlSanitizer.php b/plugins/woocommerce/src/Internal/Utilities/HtmlSanitizer.php new file mode 100644 index 00000000000..f30015d3a23 --- /dev/null +++ b/plugins/woocommerce/src/Internal/Utilities/HtmlSanitizer.php @@ -0,0 +1,88 @@ + array( + 'stripslashes', + 'force_balance_tags', + ), + 'wp_kses_rules' => array( + 'br' => true, + 'img' => array( + 'alt' => true, + 'class' => true, + 'src' => true, + 'title' => true, + ), + 'p' => array( + 'class' => true, + ), + 'span' => array( + 'class' => true, + 'title' => true, + ), + ), + ); + + /** + * Sanitizes the HTML according to the provided rules. + * + * @see wp_kses() + * + * @param string $html HTML string to be sanitized. + * @param array $sanitizer_rules { + * Optional and defaults to self::TRIMMED_BALANCED_LOW_HTML_NO_LINKS. Otherwise, one or more of the following + * keys should be set. + * + * @type array $pre_processors Callbacks to run before invoking `wp_kses()`. + * @type array $wp_kses_rules Element names and attributes to allow, per `wp_kses()`. + * } + * + * @return string + */ + public function sanitize( string $html, array $sanitizer_rules = self::LOW_HTML_BALANCED_TAGS_NO_LINKS ): string { + if ( isset( $sanitizer_rules['pre_processors'] ) && is_array( $sanitizer_rules['pre_processors'] ) ) { + $html = $this->apply_string_callbacks( $sanitizer_rules['pre_processors'], $html ); + } + + // If no KSES rules are specified, assume all HTML should be stripped. + $kses_rules = isset( $sanitizer_rules['wp_kses_rules'] ) && is_array( $sanitizer_rules['wp_kses_rules'] ) + ? $sanitizer_rules['wp_kses_rules'] + : array(); + + return wp_kses( $html, $kses_rules ); + } + + /** + * Applies callbacks used to process the string before and after wp_kses(). + * + * If a callback is invalid we will short-circuit and return an empty string, on the grounds that it is better to + * output nothing than risky HTML. We also call the problem out via _doing_it_wrong() to highlight the problem (and + * increase the chances of this being caught during development). + * + * @param callable[] $callbacks The callbacks used to mutate the string. + * @param string $string The string being processed. + * + * @return string + */ + private function apply_string_callbacks( array $callbacks, string $string ): string { + foreach ( $callbacks as $callback ) { + if ( ! is_callable( $callback ) ) { + _doing_it_wrong( __CLASS__ . '::apply', esc_html__( 'String processors must be an array of valid callbacks.', 'woocommerce' ), esc_html( WC()->version ) ); + return ''; + } + + $string = (string) $callback( $string ); + } + + return $string; + } +} diff --git a/plugins/woocommerce/tests/php/src/Internal/Utilities/HtmlSanitizerTest.php b/plugins/woocommerce/tests/php/src/Internal/Utilities/HtmlSanitizerTest.php new file mode 100644 index 00000000000..276c443d32b --- /dev/null +++ b/plugins/woocommerce/tests/php/src/Internal/Utilities/HtmlSanitizerTest.php @@ -0,0 +1,81 @@ +sut = wc_get_container()->get( HtmlSanitizer::class ); + parent::set_up(); + } + + /** + * @testdox Test inputs and outputs for the HtmlSanitizer's pre-configured TRIMMED_BALANCED_LOW_HTML_NO_LINKS rule. + * @dataProvider expectations_for_trimmed_balanced_low_html_no_links_rule + * + * @param string $test_string The string to be sanitized. + * @param string $expected How we expect the string to look after sanitization. + * @param string $explanation Notes about why we expect this/what we're testing. + */ + public function test_trimmed_balanced_low_html_sanitizer( string $test_string, string $expected, string $explanation ) { + $this->assertEquals( $expected, $this->sut->sanitize( $test_string, HtmlSanitizer::LOW_HTML_BALANCED_TAGS_NO_LINKS ), $explanation ); + } + + /** + * Describes expectations for the HtmlSanitizer's pre-configured TRIMMED_BALANCED_LOW_HTML_NO_LINKS rule. + * + * @return string[][] + */ + public function expectations_for_trimmed_balanced_low_html_no_links_rule() { + return array( + array( 'Simple Text', 'Simple Text', 'Plain text without HTML tags passes through unchanged.' ), + array( ' Leading/trailing whitespace ', ' Leading/trailing whitespace ', 'Leading and trailing whitespace will be maintained.' ), + array( '

Paragraph

', '

Paragraph

', 'Paragraph tags are allowed' ), + array( '

Paragraph

', '

Paragraph

', 'Disallowed tags are removed, allowed tags remain.' ), + array( '

Purchase

', '

Purchase

', 'Unbalanced tags are removed.' ), + ); + } + + /** + * @testdox Tests that 'bad' string processor callbacks are handled correctly. + */ + public function test_handling_of_invalid_processor_callbacks() { + $this->setExpectedIncorrectUsage( HtmlSanitizer::class . '::apply' ); + + $output = $this->sut->sanitize( + 'Test string', + array( + 'pre_processors' => array( + 'strtoupper', + 'invalid_callback_1', + ), + 'post_processors' => array( + 'invalid_callback_2', + 'strrev', + ), + ) + ); + + $this->assertEquals( '', $output, 'When invalid callbacks are provided, an empty string will be returned.' ); + } + + /** + * @testdox An empty ruleset is equivalent to asking that all HTML elements be removed. + */ + public function test_no_kses_rules_specified() { + $this->assertEquals( 'foo', $this->sut->sanitize( '

foo

', array() ) ); + } +}