-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Allow block attributes strings to terminate in \ character #71291
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
Conversation
|
Size Change: +16 B (0%) Total Size: 1.92 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e6d7683. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/17397854799
|
| // Replace escaped `\` characters with the unicode escape sequence. | ||
| .replaceAll( '\\\\', '\\u005c' ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dmsnell I'd love to get your opinion on this (and WordPress/wordpress-develop#9558). Initially, I was worried about block invalidation, but if the block was serialized (validly, in a way that can be deserialized) I think there's actually some flexibility because the attribute values are compared not their string representation.
Initially, I maintained the same escaping only addressing the problem of incorrect replacement of syntactic JSON " string delimiter. That approach yielded a very complex regular expression with negative lookbehind and placeholder replacement:
gutenberg/packages/blocks/src/api/serializer.js
Lines 301 to 307 in 6975f7f
| * - "\"" -> "\u0022" | |
| * - "\\" -> "\\" | |
| * - "\\\"" -> "\\\u0022" | |
| * | |
| * See https://developer.wordpress.org/reference/functions/wp_kses_stripslashes/ | |
| */ | |
| .replace( /(?<!\\)(\\\\)*\\"/g, '$1\\u0022' ) |
Instead, I decided to replace escaped backslashes first (\\ to \u005c) and later replace escaped doubled quotes (\" to \u0022). This can be achieved with simple string replacements and no regular expressions.
A simpler approach is possible in PHP to fix the \" replacement by using JSON_HEX_QUOT flag when encoding. However, the updated approach to replace escaped backslashes is easy to match. See WordPress/wordpress-develop#9558.
This is from a long time ago, but feedback is welcome if you have any @aduth (via #6619).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will give some thought to it, but it may not come before next week. I have a lot of catch-up from WCUS
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the block was serialized (validly, in a way that can be deserialized) I think there's actually some flexibility because the attribute values are compared not their string representation.
This should be accurate. There is even more flexibility than that too, because some block attributes are normalized before comparison for validation (e.g. CSS class names, extra HTML attributes, whitespace).
a1d81e0 to
e6d7683
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @ehti, @Alexius08. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Thank you, @sirreal! This is on my to-do list, and I will do some manual testing shortly. |
Mamaduka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.

What?
The JSON encoding with additional transformations causes some block attributes to be mis-encoded, resulting in invalid JSON and eliminating all the block attributes.
This is a partial fix, it also requires a fix to the core block serializer proposed in WordPress/wordpress-develop#9558. On its own, this fix prevents JSON from being mis-encoded on the client. However, the server continues to mis-encode JSON and lose the block attributes.
Coordinating the fixes correctly is a difficult challenge, however I think the fixes can be landed independently because if the issue persists in either the client or the server (Gutenberg or Core), the JSON will be mis-encoded and attributes stripped.
Block attribute string values (or keys, but this is much less common) that end in
\cause the transform to incorrectly replace a JSON string closing"with\u0022, breaking the JSON:{ "desired": "\\", "actual": "\\u0022 }Fixes #16508.
Requires WordPress/wordpress-develop#9558 (Trac ticket 63917)
Why?
\is valid string contents and should be allowed.How?
Change the block attribute encoding to first replace escaped
\characters with its unicode escape sequence, then perform subsequent replacements. This ensures that"after and escaped\character will not be incorrectly replaced. Additionally, this should make the JSON safe fromwp_kses_stripslashes()because the problematic sequence\"should never appear in the JSON.Change the escaping regular expression to ensure that the escaped"character (\") is not a false escape that is actually preceded by an escaped\character like\\".Testing Instructions
Create a new post at
/wp-admin/post-new.php.Add a "More" block (
/more).Replace the default text with the backslash character:
\.Save. Before this patch block would have invalid attributes. Checking the code editor reveals this:
With this patch, the block attributes are encoded as valid JSON and the
\custom text is preserved in the block.