Skip to content

Commit 101a21f

Browse files
sirrealsirreal
committed
Customize: Allow arbitrary CSS in global styles custom CSS.
Relax Global Styles custom CSS filters to allow arbitrary CSS. Escape HTML characters `<>&` in Global Styles data to prevent it from being mangled by post content filters. The data is JSON encoded and stored in `post_content`. Filters operating on `post_content` expect it to contain HTML. Some KSES filters would otherwise remove essential CSS features like the `<custom-ident>` CSS data type because they appear to be HTML tags. [61418] changed STYLE tag generation to use the HTML API for improved safety. Developed in #10641. Props jonsurrell, dmsnell, westonruter, ramonopoly, oandregal, jorgefilipecosta, sabernhardt, soyebsalar01. See #64418. git-svn-id: https://develop.svn.wordpress.org/trunk@61486 602fd350-edb4-49c9-b593-d223f7449a82
1 parent 37bee55 commit 101a21f

File tree

3 files changed

+207
-9
lines changed

3 files changed

+207
-9
lines changed

‎src/wp-includes/kses.php‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2386,7 +2386,13 @@ function wp_filter_global_styles_post( $data ) {
23862386
$data_to_encode = WP_Theme_JSON::remove_insecure_properties( $decoded_data, 'custom' );
23872387

23882388
$data_to_encode['isGlobalStylesUserThemeJSON'] = true;
2389-
return wp_slash( wp_json_encode( $data_to_encode ) );
2389+
/**
2390+
* JSON encode the data stored in post content.
2391+
* Escape characters that are likely to be mangled by HTML filters: "<>&".
2392+
*
2393+
* This matches the escaping in {@see WP_REST_Global_Styles_Controller::prepare_item_for_database()}.
2394+
*/
2395+
return wp_slash( wp_json_encode( $data_to_encode, JSON_UNESCAPED_SLASHES | JSON_HEX_TAG | JSON_HEX_AMP ) );
23902396
}
23912397
return $data;
23922398
}

‎src/wp-includes/rest-api/endpoints/class-wp-rest-global-styles-controller.php‎

Lines changed: 79 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -275,7 +275,14 @@ protected function prepare_item_for_database( $request ) {
275275
}
276276
$config['isGlobalStylesUserThemeJSON'] = true;
277277
$config['version'] = WP_Theme_JSON::LATEST_SCHEMA;
278-
$changes->post_content = wp_json_encode( $config );
278+
/**
279+
* JSON encode the data stored in post content.
280+
* Escape characters that are likely to be mangled by HTML filters: "<>&".
281+
*
282+
* This data is later re-encoded by {@see wp_filter_global_styles_post()}.
283+
* The escaping is also applied here as a precaution.
284+
*/
285+
$changes->post_content = wp_json_encode( $config, JSON_UNESCAPED_SLASHES | JSON_HEX_TAG | JSON_HEX_AMP );
279286
}
280287

281288
// Post title.
@@ -659,22 +666,87 @@ public function get_theme_items( $request ) {
659666
/**
660667
* Validate style.css as valid CSS.
661668
*
662-
* Currently just checks for invalid markup.
669+
* Currently just checks that CSS will not break an HTML STYLE tag.
663670
*
664671
* @since 6.2.0
665672
* @since 6.4.0 Changed method visibility to protected.
673+
* @since 7.0.0 Only restricts contents which risk prematurely closing the STYLE element,
674+
* either through a STYLE end tag or a prefix of one which might become a
675+
* full end tag when combined with the contents of other styles.
666676
*
667677
* @param string $css CSS to validate.
668678
* @return true|WP_Error True if the input was validated, otherwise WP_Error.
669679
*/
670680
protected function validate_custom_css( $css ) {
671-
if ( preg_match( '#</?\w+#', $css ) ) {
672-
return new WP_Error(
673-
'rest_custom_css_illegal_markup',
674-
__( 'Markup is not allowed in CSS.' ),
675-
array( 'status' => 400 )
681+
$length = strlen( $css );
682+
for (
683+
$at = strcspn( $css, '<' );
684+
$at < $length;
685+
$at += strcspn( $css, '<', ++$at )
686+
) {
687+
$remaining_strlen = $length - $at;
688+
/**
689+
* Custom CSS text is expected to render inside an HTML STYLE element.
690+
* A STYLE closing tag must not appear within the CSS text because it
691+
* would close the element prematurely.
692+
*
693+
* The text must also *not* end with a partial closing tag (e.g., `<`,
694+
* `</`, … `</style`) because subsequent styles which are concatenated
695+
* could complete it, forming a valid `</style>` tag.
696+
*
697+
* Example:
698+
*
699+
* $style_a = 'p { font-weight: bold; </sty';
700+
* $style_b = 'le> gotcha!';
701+
* $combined = "{$style_a}{$style_b}";
702+
*
703+
* $style_a = 'p { font-weight: bold; </style';
704+
* $style_b = 'p > b { color: red; }';
705+
* $combined = "{$style_a}\n{$style_b}";
706+
*
707+
* Note how in the second example, both of the style contents are benign
708+
* when analyzed on their own. The first style was likely the result of
709+
* improper truncation, while the second is perfectly sound. It was only
710+
* through concatenation that these two scripts combined to form content
711+
* that would have broken out of the containing STYLE element, thus
712+
* corrupting the page and potentially introducing security issues.
713+
*
714+
* @see https://html.spec.whatwg.org/multipage/parsing.html#rawtext-end-tag-name-state
715+
*/
716+
$possible_style_close_tag = 0 === substr_compare(
717+
$css,
718+
'</style',
719+
$at,
720+
min( 7, $remaining_strlen ),
721+
true
676722
);
723+
if ( $possible_style_close_tag ) {
724+
if ( $remaining_strlen < 8 ) {
725+
return new WP_Error(
726+
'rest_custom_css_illegal_markup',
727+
sprintf(
728+
/* translators: %s is the CSS that was provided. */
729+
__( 'The CSS must not end in "%s".' ),
730+
esc_html( substr( $css, $at ) )
731+
),
732+
array( 'status' => 400 )
733+
);
734+
}
735+
736+
if ( 1 === strspn( $css, " \t\f\r\n/>", $at + 7, 1 ) ) {
737+
return new WP_Error(
738+
'rest_custom_css_illegal_markup',
739+
sprintf(
740+
/* translators: %s is the CSS that was provided. */
741+
__( 'The CSS must not contain "%s".' ),
742+
esc_html( substr( $css, $at, 8 ) )
743+
),
744+
array( 'status' => 400 )
745+
);
746+
}
747+
}
677748
}
749+
678750
return true;
679751
}
680752
}

‎tests/phpunit/tests/rest-api/rest-global-styles-controller.php‎

Lines changed: 121 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -650,6 +650,7 @@ public function test_update_item_valid_styles_css() {
650650
/**
651651
* @covers WP_REST_Global_Styles_Controller::update_item
652652
* @ticket 57536
653+
* @ticket 64418
653654
*/
654655
public function test_update_item_invalid_styles_css() {
655656
wp_set_current_user( self::$admin_id );
@@ -659,7 +660,7 @@ public function test_update_item_invalid_styles_css() {
659660
$request = new WP_REST_Request( 'PUT', '/wp/v2/global-styles/' . self::$global_styles_id );
660661
$request->set_body_params(
661662
array(
662-
'styles' => array( 'css' => '<p>test</p> body { color: red; }' ),
663+
'styles' => array( 'css' => '</style>' ),
663664
)
664665
);
665666
$response = rest_get_server()->dispatch( $request );
@@ -826,4 +827,123 @@ public function test_global_styles_route_args_schema() {
826827
$this->assertArrayHasKey( 'type', $route_data[0]['args']['id'] );
827828
$this->assertSame( 'integer', $route_data[0]['args']['id']['type'] );
828829
}
830+
831+
/**
832+
* @covers WP_REST_Global_Styles_Controller::update_item
833+
* @ticket 64418
834+
*/
835+
public function test_update_allows_valid_css_with_more_syntax() {
836+
wp_set_current_user( self::$admin_id );
837+
if ( is_multisite() ) {
838+
grant_super_admin( self::$admin_id );
839+
}
840+
$request = new WP_REST_Request( 'PUT', '/wp/v2/global-styles/' . self::$global_styles_id );
841+
$css = <<<'CSS'
842+
@property --animate {
843+
syntax: "<custom-ident>";
844+
inherits: true;
845+
initial-value: false;
846+
}
847+
h1::before { content: "fun & games"; }
848+
CSS;
849+
$request->set_body_params(
850+
array(
851+
'styles' => array( 'css' => $css ),
852+
)
853+
);
854+
855+
$response = rest_get_server()->dispatch( $request );
856+
$data = $response->get_data();
857+
$this->assertSame( $css, $data['styles']['css'] );
858+
859+
// Compare expected API output to WP internal values.
860+
$request = new WP_REST_Request( 'GET', '/wp/v2/global-styles/' . self::$global_styles_id );
861+
$response = rest_get_server()->dispatch( $request );
862+
$this->assertSame( $css, $response->get_data()['styles']['css'] );
863+
}
864+
865+
/**
866+
* @covers WP_REST_Global_Styles_Controller::validate_custom_css
867+
* @ticket 64418
868+
*
869+
* @dataProvider data_custom_css_allowed
870+
*/
871+
public function test_validate_custom_css_allowed( string $custom_css ) {
872+
$controller = new WP_REST_Global_Styles_Controller();
873+
$validate = Closure::bind(
874+
function ( $css ) {
875+
return $this->validate_custom_css( $css );
876+
},
877+
$controller,
878+
$controller
879+
);
880+
881+
$this->assertTrue( $validate( $custom_css ) );
882+
}
883+
884+
/**
885+
* Data provider.
886+
*
887+
* @return array<string, string[]>
888+
*/
889+
public static function data_custom_css_allowed(): array {
890+
return array(
891+
'@property declaration' => array(
892+
'@property --prop { syntax: "<custom-ident>"; inherits: true; initial-value: false; }',
893+
),
894+
'Different close tag' => array( '</stylesheet>' ),
895+
'Not a style close tag' => array( '/*</style*/' ),
896+
'Not a style close tag 2' => array( '/*</style_' ),
897+
'Empty' => array( '' ),
898+
'Short content' => array( '/**/' ),
899+
);
900+
}
901+
902+
/**
903+
* @covers WP_REST_Global_Styles_Controller::validate_custom_css
904+
* @ticket 64418
905+
*
906+
* @dataProvider data_custom_css_disallowed
907+
*/
908+
public function test_validate_custom_css( string $custom_css, string $expected_error_message ) {
909+
$controller = new WP_REST_Global_Styles_Controller();
910+
$validate = Closure::bind(
911+
function ( $css ) {
912+
return $this->validate_custom_css( $css );
913+
},
914+
$controller,
915+
$controller
916+
);
917+
918+
$result = $validate( $custom_css );
919+
$this->assertWPError( $result );
920+
$this->assertSame( $expected_error_message, $result->get_error_message() );
921+
}
922+
923+
/**
924+
* Data provider.
925+
*
926+
* @return array<string, string[]>
927+
*/
928+
public static function data_custom_css_disallowed(): array {
929+
return array(
930+
'style close tag' => array( 'css…</style>…css', 'The CSS must not contain "&lt;/style&gt;".' ),
931+
'style close tag upper case' => array( '</STYLE>', 'The CSS must not contain "&lt;/STYLE&gt;".' ),
932+
'style close tag mixed case' => array( '</sTyLe>', 'The CSS must not contain "&lt;/sTyLe&gt;".' ),
933+
'style close tag in comment' => array( '/*</style>*/', 'The CSS must not contain "&lt;/style&gt;".' ),
934+
'style close tag (/)' => array( '</style/', 'The CSS must not contain "&lt;/style/".' ),
935+
'style close tag (\t)' => array( "</style\t", "The CSS must not contain \"&lt;/style\t\"." ),
936+
'style close tag (\f)' => array( "</style\f", "The CSS must not contain \"&lt;/style\f\"." ),
937+
'style close tag (\r)' => array( "</style\r", "The CSS must not contain \"&lt;/style\r\"." ),
938+
'style close tag (\n)' => array( "</style\n", "The CSS must not contain \"&lt;/style\n\"." ),
939+
'style close tag (" ")' => array( '</style ', 'The CSS must not contain "&lt;/style ".' ),
940+
'truncated "<"' => array( '<', 'The CSS must not end in "&lt;".' ),
941+
'truncated "</"' => array( '</', 'The CSS must not end in "&lt;/".' ),
942+
'truncated "</s"' => array( '</s', 'The CSS must not end in "&lt;/s".' ),
943+
'truncated "</ST"' => array( '</ST', 'The CSS must not end in "&lt;/ST".' ),
944+
'truncated "</sty"' => array( '</sty', 'The CSS must not end in "&lt;/sty".' ),
945+
'truncated "</STYL"' => array( '</STYL', 'The CSS must not end in "&lt;/STYL".' ),
946+
'truncated "</stYle"' => array( '</stYle', 'The CSS must not end in "&lt;/stYle".' ),
947+
);
948+
}
829949
}

0 commit comments

Comments
 (0)