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

HTML API: Guard against non-string attribute values. #55368

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from

Conversation

dmsnell
Copy link
Member

@dmsnell dmsnell commented Oct 15, 2023

What?

For existing uses of the HTML API, guards against treating possibly-non-string attribute values as strings. This existing use exposes a risk of crashing.

Why?

Crashing is bad 😄
Also this is part of ongoing efforts to establish proper example usage of the HTML API throughout all of Core.

How?

Because there are three ways for an attribute in HTML to exist, the HTML API reports three kinds of values for get_attribute() calls:

  • null means that no attribute exists of the given name
  • true means that the attribute exists but there is no value, e.g. <input checked>.
  • a string value means that the attribute exists and has a value, e.g. <img src="test">.

When operating on the value returned by get_attribute() then it's important to ensure that it's a string value before treating it as one. A call to empty() is not enough because a boolean attribute, being true, does not return false for empty().

In this patch, blocks that read and then use attribute values as strings have been updated in order to guard against cases where the attribute might not be the string the code expects.

Testing Instructions

There should be no functional or visual changes in this patch except where previously a site may have encountered unexpected HTML attribute values.

E.g. For the cover block, this is how it renders with no style attribute on the tag.
Screenshot 2023-10-14 at 6 52 30 PM

By manually adding style without a value, it's possible to trigger the bug.
Screenshot 2023-10-14 at 6 52 43 PM

This leads to an unwanted style attribute in the page render, because true was coerced to a string.
Screenshot 2023-10-14 at 6 53 07 PM

With this patch applied these cases should all disappear.

Because there are three ways for an attribute in HTML to exist, the HTML API
reports three kinds of values for `get_attribute()` calls:
 - `null` means that no attribute exists of the given name
 - `true` means that the attribute exists but there is no value, e.g. '<input checked>'.
 - a string value means that the attribute exists and has a value, e.g. '<img src="test">'.

When operating on the value returned by `get_attribute()` then it's important to
ensure that it's a string value before treating it as one. A call to `empty()` is not
enough because a boolean attribute, being `true`, does not return `false` for `empty()`.

In this patch blocks that read and then use attribute values as strings have been updated
in order to guard against cases where the attribute might not be the string the code
expects.
@dmsnell dmsnell added [Type] Bug An existing feature does not function as intended [Package] Blocks /packages/blocks [Feature] HTML API An API for updating HTML attributes in markup labels Oct 15, 2023
@github-actions
Copy link

This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress.

If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged.

If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack.

Thank you! ❤️

View changed files
❔ lib/experimental/interactivity-api/directive-processing.php
❔ lib/experimental/interactivity-api/directives/wp-bind.php
❔ lib/experimental/interactivity-api/directives/wp-class.php
❔ lib/experimental/interactivity-api/directives/wp-context.php
❔ lib/experimental/interactivity-api/directives/wp-style.php
❔ lib/experimental/interactivity-api/directives/wp-text.php

@github-actions
Copy link

Flaky tests detected in 79a58a1.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6521295210
📝 Reported issues:

@luisherranz
Copy link
Member

Is this still a work in progress?

While reviewing it, I noticed that I didn't fix this problem completely:

I'll do another PR to address that.

Once that one is merged, you can rebase this into trunk and solve the conflicts (which should be straightforward).

@dmsnell dmsnell marked this pull request as ready for review October 18, 2023 20:04
@dmsnell
Copy link
Member Author

dmsnell commented Oct 18, 2023

@luisherranz I don't remember intentionally setting it to draft. thank you. it needs review, especially since I'm not fully aware of all the nuances of the code I'm changing here.

@luisherranz
Copy link
Member

Ok, thanks Dennis. If it's ok, I'm going to wait until we merge this PR, and then resolve the conflicts here and review it 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] HTML API An API for updating HTML attributes in markup [Package] Blocks /packages/blocks [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants