Skip to content
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

Jetpack Tweaks: Decode child selectors > in Custom CSS #746

Merged
merged 2 commits into from
Apr 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
add_action( 'admin_notices', __NAMESPACE__ . '\notify_import_rules_stripped' );
add_action( 'csstidy_optimize_subvalue', __NAMESPACE__ . '\sanitize_csstidy_subvalues' );
add_action( 'safecss_parse_pre', __NAMESPACE__ . '\update_csstidy_safelist', 0 );
add_filter( 'wp_get_custom_css', __NAMESPACE__ . '\filter_custom_css' );

/**
* Sanitize CSS saved through the Core/Jetpack editor inside the Customizer
Expand Down Expand Up @@ -222,3 +223,16 @@ function update_csstidy_safelist() {
$GLOBALS['csstidy']['all_properties'] = array_merge( $GLOBALS['csstidy']['all_properties'], $properties_for_csstidy );
}
}

/**
* Replace encoded > in CSS with a > so that CSS rules are valid.
*
* Remove this once the root issue is fixed in https://github.com/Automattic/jetpack/issues/21603.
*
* @param string $css CSS pulled in from the Custom CSS post type.
*
* @return string
*/
function filter_custom_css( $css ) {
return str_replace( '>', '>', $css );
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any other characters this could happen to? Would it make sense to run something like html_entity_decode() here instead of a simple string replace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was hesitant to do that at first because I'm not totally sure why it's encoded, if there would be a security implication to decoding everything. Looking at the different selectors available, no others are encoded, but it would be possible to add anything to an attribute selector, which could get encoded (like a[href*='a=1&b=2], would become a[href*='a=1&b=2]). I'm not sure if supporting that edge case is worth any security issue (but… maybe there is no issue here?)

I updated the test to check against almost all the selectors available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm going to merge this as-is, if other selectors become a problem we can make the switch then.

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
<?php

namespace WordCamp\Tests;

use WP_UnitTestCase;

defined( 'WPINC' ) || die();

/**
* Class Test_Jetpack_CSS_Sanitization
*
* @group mu-plugins
* @group jetpack-tweaks
* @group jetpack-css
*
* @package WordCamp\Tests
*/
class Test_Jetpack_CSS_Sanitization extends WP_UnitTestCase {

/**
* Test that no selector characters are encoded.
*/
public function test_selectors_not_encoded() {
$input = <<<CSS
.class > p,
p ~ span,
h2 + p,
col || td,
.pseudo:visited,
.pseudo::before,
[title],
a[href="https://example.org"],
a[title*='an example'],
span[data-emoji~=🐈‍⬛]
span[attr$="한글"] {
color: lightcoral;
}
CSS;

$post = wp_update_custom_css_post( $input );
$this->assertNotWPError( $post );

$output = wp_get_custom_css();
$this->assertEquals( $input, $output );
}

/**
* Test that HTML code is correcly stripped.
*/
public function test_html_not_allowed() {
$input = <<<CSS
/* HTML-ish code should be stripped */
<class> p {
color: lightcoral;
}
CSS;

$post = wp_update_custom_css_post( $input );
$this->assertNotWPError( $post );

$output = wp_get_custom_css();
$this->assertNotEquals( $input, $output );
}
}