Pattern directory: test the directive marker instead of reading the markup - #777
Conversation
…arkup `content_has_block_directives()` tokenized the content, and the sanitised copy of it, looking for a tag with a `data-wp-*` attribute. That is narrower than the rule it enforces: a pattern may not carry the marker at all. `WP_HTML_Tag_Processor` does not descend into an element whose contents are text, so a tag inside one is invisible to it. Scanning the `wp_kses_post()` form as well covers that only where KSES removes the element that did the hiding. It removes `<script>` and `<style>`, which is why the existing cases are caught, and it keeps `<title>` and `<textarea>`, which hold their contents as text just the same -- so for those, neither pass ever reads the tag and the check returns false. Test the marker directly. It is what the helper already did as a fast-path negative, it cannot be hidden by an element boundary, and it drops the sanitise-and-tokenize pass. The widening this brings is deliberate: content that merely mentions `data-wp-` in prose is now refused too. Checked against the fields the validator reads (`post_content`, `post_title`, `post_excerpt`) on 600 published patterns from the public API and the 1,858 in the seed exports -- none carries the marker. The 40 of 600 that do carry it hold it in the rendered output core emits for blocks like `core/details`, which this never reads. Tests: rows for `<title>`, `<textarea>` and the marker in plain prose, in both the submission and translated-content providers; the first two fail before this change. The comments on the existing rows described tokenizer behaviour that no longer decides the outcome, and the two added in #773 described it incorrectly -- `<svg>` is not a raw-text element and the tag processor reads straight into it; the nested `<script>` was doing the hiding. Rows kept as regression coverage, explanations corrected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
KokkieH
left a comment
There was a problem hiding this comment.
Approach looks right to me, and tests pass locally as well as in CI. Approving. One question, one suggestion, two nits, none blocking.
1. The widening reaches further than prose (question)
The likelier real-world matches are URLs, filenames and class names, not prose: /big-data-wp-plugin/, data-wp-hero.jpg, class="acme-data-wp-card". These are now refused in content, title, excerpt and any string block attribute. The attribute case is new, since attribute_has_directive() used to tokenize. The submitter would get "Patterns cannot contain interactivity directives", which is confusing for an image filename.
Zero hits across 2,458 patterns says this is rare, and failing closed is the right direction, so I'm fine shipping as is. If we want to soften it, /(?<![a-z0-9_-])data-wp-/i spares the embedded cases (a bare /data-wp-hero.jpg would still match). Otherwise a test row and a docblock sentence covering URLs and classes would put the actual reach on record.
2. Check the stored form too (suggestion)
is_translated_content_allowed() tests both the assembled and the stored bytes. The submission path now tests only the submitted ones. reject_unstable_blocks() already computes $stored, so running the same test there is nearly free and keeps the two paths consistent. Fine as a follow-up.
Nits
- The description says all three new rows went into both providers, but
pattern-translated-content-test.phponly gets the<title>row. - The docblock and test comments mostly explain the removed approach, which reads better in the commit message. I'd trim the docblock to the rule plus one sentence on why a substring test was chosen. The corrected
<svg>comments are a nice fix.
Follow-up to #773, which @obenland suggested I just fix.
Why
content_has_block_directives()decides whether content carries an Interactivity API directive by tokenizing it, and thewp_kses_post()copy of it, looking for a tag with adata-wp-*attribute. That is narrower than the rule it enforces, which is that a pattern may not carry the marker at all.WP_HTML_Tag_Processordoes not descend into an element whose contents are text, so a tag inside one is invisible to it. Scanning the sanitised form as well covers that only where KSES removes the element that did the hiding. It removes<script>and<style>, which is why the cases already in the providers are caught. It keeps<title>and<textarea>, which hold their contents as text in just the same way — so for those, neither pass ever reads the tag:So the check's coverage depends on the KSES allowed-list, which nothing in the code records, and a shift in either that list or the tokenizer's handling would quietly narrow it.
What changed
Test the marker. It is what the helper already did as its fast-path negative; making it the decision is complete by construction, since no element boundary can hide a substring, and it drops the sanitise-and-tokenize pass.
This widens the rule, deliberately. Content that merely mentions
data-wp-in prose is now refused as well. I checked that against the fields the validator actually reads —post_content,post_title,post_excerpt:api.wordpress.org/patterns/1.0.wp-env/data/exportsseedWorth knowing if you go looking: 40 of those 600 patterns do contain
data-wp-, all of it in the rendered HTML core emits for blocks likecore/details. That is thecontentfield in the API response, notpattern_content, and this check never reads it. A row pinning the prose case is in the provider so the trade-off is visible rather than implied — happy to drop it if you would rather leave prose alone.Tests. Rows for
<title>,<textarea>and the bare marker in both the submission provider and the translated-content one. The first two fail before this change.Comments. The explanations on the existing rows described tokenizer behaviour that no longer decides the outcome. The two added in #773 also described it incorrectly:
<svg>is not a raw-text element and the tag processor reads straight into it (as it does into<desc>and<annotation-xml>) — the nested<script>was doing the hiding, which is the row directly above them. The rows are worth keeping as regression coverage; the explanations are corrected.Testing
rest_pattern_interactivity_directivecontain the literal marker, so all stay caught; no row indata_valid_content()ordata_allowed_content()contains it, so none regresses. No existing row depends on a marker that only appears after JSON-decoding an attribute — andattribute_has_directive(), which walks decoded attribute values, still covers that case.mailto:button, a query loop, acore/detailsaccordion, anddata-foo/data-wp/data-wpx-yattributes) stay accepted.I could not run PHPUnit or
phpcslocally —npm installfails on this machine (@wordpress/primitives@4.50.0) andcomposer installcan't authenticate to GitHub, sovendor/is stale and phpcs won't load its rulesets. Leaning on CI for both; worth a second look from someone with a working checkout.🤖 Generated with Claude Code