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

Fix: Unexpected block validation error with unescaped ampersands. #13406

Closed

Conversation

fastlinemedia
Copy link

Description

This is a first pass at fixing issue #12448.

The issue appears to be that block.attributes is passed escaped to isValidBlockContent using parseWithAttributeSchema while innerHTML is not escaped. This results in the valid check returning false even though nothing has actually changed.

How has this been tested?

Please see issue #12448 for details on testing.

@talldan
Copy link
Contributor

talldan commented Jan 22, 2019

Hi @fastlinemedia - Thanks for looking into this and working on a fix.

I think that from a user experience point of view, the one issue I have with this approach is that the user's content ends up being silently changed (even if it might be for better). That particularly happens in the case where the user might have invalid markup (like the case described in #13158). Any thoughts on how that might be addressed? (edit: I realise that was already the case before this PR, so ignore this comment!)

@gziolo gziolo added the [Feature] Block API API that allows to express the block paradigm. label Jan 22, 2019
@gziolo gziolo requested a review from aduth January 22, 2019 09:24
@@ -461,7 +461,11 @@ export function createBlockWithFallback( blockNode ) {
// provided source value with the serialized output before there are any modifications to
// the block. When both match, the block is marked as valid.
if ( ! isFallbackBlock ) {
block.isValid = isValidBlockContent( blockType, block.attributes, innerHTML );
const parsedHTML = parseWithAttributeSchema( innerHTML, {
Copy link
Member

Choose a reason for hiding this comment

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

This will degrade performance significantly for longer posts, given the implementation of parseWithAttributeSchema.

@aduth
Copy link
Member

aduth commented Jan 25, 2019

If I understand the issue correctly, it seems like something which can be built in to the validation logic itself, since there's already a "normalization" procedure there to handle these sorts of situations where two types of texts should be treated as equivalent.

/**
* Array of functions which receive a text string on which to apply normalizing
* behavior for consideration in text token equivalence, carefully ordered from
* least-to-most expensive operations.
*
* @type {Array}
*/
const TEXT_NORMALIZATIONS = [
identity,
getTextWithCollapsedWhitespace,
];

@aduth
Copy link
Member

aduth commented Jan 25, 2019

Previously: #11771

@aduth
Copy link
Member

aduth commented Jan 25, 2019

See alternative fix: #13512

@fastlinemedia
Copy link
Author

Thanks @aduth and @talldan! That all makes sense. I really appreciate the help with this one. I'll close in favor of #13512.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants