WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#30409 closed defect (bug) (fixed)

Twenty Fifteen: HTML characters in the color scheme CSS are escaped.

Reported by: iamtakashi Owned by: iandstewart
Milestone: 4.1 Priority: normal
Severity: blocker Version: 4.1
Component: Bundled Theme Keywords: has-patch commit
Focuses: Cc:
PR Number:

Description

HTML characters in the color scheme CSS in the head is escaped for example:

input[type="button"]
.pingback .comment-body > a,

This causes that some of color scheme specific styles are broken.

https://cldup.com/5gvoMDhWRo.png

Attachments (9)

30409.diff (590 bytes) - added by iamtakashi 5 years ago.
Remove Sanitization Callback from Color Scheme CSS setting.
30409.patch (19.8 KB) - added by ocean90 5 years ago.
30409.2.diff (10.0 KB) - added by georgestephanis 5 years ago.
30409.3.diff (13.9 KB) - added by ocean90 5 years ago.
@todo preview
30409.4.diff (17.1 KB) - added by ocean90 5 years ago.
30409.5.diff (20.7 KB) - added by ocean90 5 years ago.
30409.4.1.diff (18.2 KB) - added by georgestephanis 5 years ago.
30409.5.1.diff (21.9 KB) - added by georgestephanis 5 years ago.
30409.4-fixed-sidebar-color.diff (18.1 KB) - added by ocean90 5 years ago.
iamtakashi: sidebar_textcolor should be 100% - $color_scheme[4] and secondary_sidebar_textcolor` should be the one with 70%.

Download all attachments as: .zip

Change History (31)

#1 @iandstewart
5 years ago

  • Milestone changed from Awaiting Review to 4.1
  • Severity changed from normal to blocker

This ticket was mentioned in Slack in #core-themes by iamtakashi. View the logs.


5 years ago

@iamtakashi
5 years ago

Remove Sanitization Callback from Color Scheme CSS setting.

#3 @iamtakashi
5 years ago

  • Keywords has-patch added; needs-patch removed

#4 @iandstewart
5 years ago

  • Owner set to iandstewart
  • Resolution set to fixed
  • Status changed from new to closed

In 30398:

Twenty Fifteen: escaping here breaks color schemes.

Props celloexpressions, iamtakashi, fixes #30409.

#5 follow-up: @nacin
5 years ago

esc_html() is not a sanitization function. Please never do what [30398] un-did. :-)

#6 in reply to: ↑ 5 @iamtakashi
5 years ago

Replying to nacin:

esc_html() is not a sanitization function. Please never do what [30398] un-did. :-)

Do we need to use something else? CSS shouldn't need escaping though is this good idea to use wp_filter_nohtml_kses? I'm asking this because a theme check reports the setting missing a sanitisation.

#7 @lancewillett
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Core doesn't have sanitize_css so ...

@nacin I know you added a smiley but "Please never do" isn't helpful without an alternative suggestion.

This ticket was mentioned in Slack in #core-themes by lancewillett. View the logs.


5 years ago

@ocean90
5 years ago

#9 @lancewillett
5 years ago

Discussing today in Slack, next step is to work with 30409.patch approach — remove the theme_mod and instead generate the CSS from a shared JS/PHP template. See also notes at https://core.trac.wordpress.org/ticket/29988#comment:18

#10 @georgestephanis
5 years ago

https://core.trac.wordpress.org/attachment/ticket/30409/30409.2.diff is what I'm thinking -- store the css in a function that can interpolate either JS tokens for wp.template to use, or colors, for outputting directly to the front-end.

Last edited 5 years ago by georgestephanis (previous) (diff)

@ocean90
5 years ago

@todo preview

@ocean90
5 years ago

@ocean90
5 years ago

#11 @ocean90
5 years ago

  • Keywords needs-testing added

30409.4.diff and 30409.5.diff are based on 30409.2.diff.

In .4 the CSS generation is still done in the control and then send to the Previewer. Sending an event to the preview is easy (see #27666/[29048]), capturing the event not because the preview is still private. The patch will change this, otherwise we have to do something like trunk/src/wp-includes/js/customize-preview-widgets.js@29051#L107.

In .5 I moved the CSS generation to the preview. Since the script depends on Iris we have to move Iris out of is_admin().

#12 @georgestephanis
5 years ago

Just uploaded 30409.4.1.diff and 30409.5.1.diff -- based off the prior respective patches, they just move the derivative color logic into the twentyfifteen_get_color_scheme_css function, so only the six color scheme colors need to be passed in, rather than calculating before passing. If they are passed, though, it will override any internal calculations.

Had a light discussion with @ocean90 in slack here as to this relocation of the calculations: https://wordpress.slack.com/archives/core-themes/p1418478625000200

Last edited 5 years ago by georgestephanis (previous) (diff)

This ticket was mentioned in Slack in #core by johnbillion. View the logs.


5 years ago

This ticket was mentioned in Slack in #core-themes by lancewillett. View the logs.


5 years ago

#15 @lancewillett
5 years ago

Should we use sanitize_hex_color on the hex values that aren't the rdba values?

Testing 30409.4.1.diff passes, color schemes save and correct CSS is served inline as expected.

@ocean90
5 years ago

iamtakashi: sidebar_textcolor should be 100% - $color_scheme[4] and secondary_sidebar_textcolor` should be the one with 70%.

#16 @nacin
5 years ago

OK, let's get the latest patch in. Exception:

  • The change to src/wp-includes/js/customize-preview.js should be done in its own commit, preferably its own quick ticket.

Things I still don't like, and would consider adjustments to:

  • The editable colors being 0, 1, and 4, rather than 0, 1, and 2. This is far too magical and I have no idea what it means.
  • Using numeric arrays for color definitions when it's really important for others to understand what is what, but associative arrays internally when it doesn't matter as much.
  • Lots of really complicated stuff in a default theme. Lots of opportunities to simplify.

And while I am here, things that are definitely fixable:

  • Far, far too much unnecessary use of esc_html(), especially on translated strings.
  • Lots of things are pluggable that shouldn't be.

#17 @nacin
5 years ago

  • Keywords commit added; needs-testing removed

#18 @ocean90
5 years ago

In 30891:

Customizer: Export Preview instance to wp.customize.preview.

see #30409, #30726.

#19 @ocean90
5 years ago

In 30893:

Twenty Fifteen: Don't save unfiltered CSS in a setting.

Introduce twentyfifteen_get_color_scheme_css( $colors ) which will be used for Underscore/Backbone and the PHP side. Let twentyfifteen_color_scheme_css() handle the color calculation on PHP side.

see #30409.

#20 @johnbillion
5 years ago

In 30923:

Customizer: Export Preview instance to wp.customize.preview.

Merges [30891] to the 4.1 branch.

see #30409, #30726.

#21 @johnbillion
5 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

In 30927:

Twenty Fifteen: Don't save unfiltered CSS in a setting.

Introduce twentyfifteen_get_color_scheme_css( $colors ) which will be used for Underscore/Backbone and the PHP side. Let twentyfifteen_color_scheme_css() handle the color calculation on PHP side.

Merges [30893] to the 4.1 branch.

Fixes #30409.
Props ocean90

This ticket was mentioned in Slack in #core by ocean90. View the logs.


4 years ago

Note: See TracTickets for help on using tickets.