Add: Save time theme.json escaping #28061
Conversation
jorgefilipecosta
requested review from
oandregal,
spacedmonkey and
TimothyBJacobs
as code owners
|
Size Change: -20.8 kB (-2%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
lib/global-styles.php
Outdated
| */ | ||
| function gutenberg_global_styles_kses_init() { | ||
| gutenberg_global_styles_kses_remove_filters(); | ||
| if ( ! current_user_can( 'unfiltered_html' ) || true ) { |
There was a problem hiding this comment.
Hi @Soean, that was done on purpose to make it easier to test the PR, without needing a multisite where admin doesn't have an unfiltered HTML capability, or without using a plugin that removes the capability. Before merging the PR the "|| true" should be removed.
There was a problem hiding this comment.
Perhaps we can remove the true condition from this code and add it to the testing instructions? We need to test both that users can and can't store that data depending on their capabilities.
|
Hey, I've taken a first look at this. I want to review further the part of this PR that deals with hooking into the Here's my feedback about the other two things this PR does:
|
Hi @nosolosw, it happens on every preset. When we apply a background color for global the style generated is: You are right it is a different issue, would you prefer to separate that part of this PR in a dependent PR? We would need to merge that PR first. |
Hi @nosolosw, I guess we could remove the process_key function and the process_key calls? |
There was a problem hiding this comment.
Was able to read the other part of the code and this seems fine. I wasn't able to test all the use cases this addresses, though (don't know how to test the import case, for example). I think this is important to merge and test widely in the coming days, so I'm approving.
I'd like to see these follow-ups (the smaller the PR the easier/quicker to review/land):
-
Add tests.
-
The presets listed here need to be taken from the schema, so adding a new preset doesn't require touching multiple files (so people will forget).
-
We need to merge process_key & remove_insecure_properties.
I've also got a couple of questions about how we use isGlobalStylesUserThemeJSON:
-
We seem to use this flag to signal both that the data is a CPT coming from the user & that the data has been sanitized. Do you think it makes sense to have separate flags instead?
isUserDataandisSanitizedfor example. -
On the other hand, I haven't seen that we check that a post is sanitized before using it, so I'm not sure we need this and this. What do you think?
|
I've started to create follow-ups: #28171 |
|
Hi @nosolosw,
This flag is really important and we should decide well on its name as we can not easily change it. It is stored in the database and we need to be back-compatible with it. In fact, I wonder if as part of theme.json versioning it also makes sense to save a version on the database, so we are back-compatible with old user data?
This flag indicates that it is a user theme.json. If it is a user theme.json it is sanitized or not depending on the user capabilities and conditions/plugins being present on the save moment. I don't think a isSanitized flag is useful at all.
We need to check if the flag is preset or not. On HTML content there is no need to check because the filters always run (depending just on user capabilities). We can not run our filters unconditionally as otherwise, we may change JSON data from another plugin for example. If the flag is not preset the content can not be presumed to be safe, so before using we need to verify if the flag is there. |
|
Follow-up #28188 @jorgefilipecosta would you think you are able to add tests for this in a new PR? I can take care of the other follow-ups in some PRs I'm working on. |
|
Hi @nosolosw, |
This commit adds global styles user content escaping. In addition, it ports the logic on the Gutenberg plugin implemented on WordPress/gutenberg#28061 to the core. The logic tries to follow what was done for standard post content. See #54336. Props oandregal. git-svn-id: https://develop.svn.wordpress.org/trunk@52052 602fd350-edb4-49c9-b593-d223f7449a82
This commit adds global styles user content escaping. In addition, it ports the logic on the Gutenberg plugin implemented on WordPress/gutenberg#28061 to the core. The logic tries to follow what was done for standard post content. See #54336. Props oandregal. git-svn-id: https://develop.svn.wordpress.org/trunk@52052 602fd350-edb4-49c9-b593-d223f7449a82
This commit adds global styles user content escaping. In addition, it ports the logic on the Gutenberg plugin implemented on WordPress/gutenberg#28061 to the core. The logic tries to follow what was done for standard post content. See #54336. Props oandregal. Built from https://develop.svn.wordpress.org/trunk@52052 git-svn-id: http://core.svn.wordpress.org/trunk@51644 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This commit adds global styles user content escaping. In addition, it ports the logic on the Gutenberg plugin implemented on WordPress/gutenberg#28061 to the core. The logic tries to follow what was done for standard post content. See #54336. Props oandregal. Built from https://develop.svn.wordpress.org/trunk@52052 git-svn-id: https://core.svn.wordpress.org/trunk@51644 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This PR adds save time escaping of the user theme.json.
It tries to follow what was done for HTML post content escaping in src/wp-includes/kses.php.
Like KSES we rely on the content_save_pre filter to escape the content of the post.
Here comes the first challenge, code executing inside of that filter does not have access to the post type being escaped. The filter just receives the content and should output escaped content. So we need some way to given a post content string, identify if it is a user theme.json content.
The way we are doing this is by including a flag isGlobalStylesUserThemeJSON in the user theme.json, which marks if the content of the post is a user theme.json.
Our code totally ignores the user JSON if the flag is not present as in that case the content can not be considered safe.
To filter what CSS is possible we rely on safecss_filter_attr. If a rule is removed by safecss_filter_attr we discard it from the user theme.json.
How has this been tested?
Try to inject unsafe rules, e.g: SVG backgrounds in the gradients, and verify these rules are removed.
To make it easier to test, this PR is escaping styles submitted by an admin "! current_user_can( 'unfiltered_html' ) || true " the "|| true " should be removed before the merge.