Tag Processor: throw when supplied unacceptible attribute names. #44431
Conversation
dmsnell
force-pushed
the
update/tag-processor-validate-attribute-names
branch
2 times, most recently
from
d1fc120 to
8c7344d
Compare
The `WP_HTML_Tag_Processor` allows setting new HTML attributes with a given name and value. Previously this has allowed any string input for the attribute name, but we have to be careful not to print output that might break the HTML we're modifying. In this patch we're adding a check against the given attribute name and rejecting invalid or unacceptible names. WordPress here is more restrictive than HTML5.
dmsnell
force-pushed
the
update/tag-processor-validate-attribute-names
branch
from
8c7344d to
c284114
Compare
|
|
|
I needed to manually set |
| throw new Exception( 'Invalid attribute name' ); | ||
| } | ||
|
|
||
| return; |
There was a problem hiding this comment.
return new WP_Error perhaps?
There was a problem hiding this comment.
I don't think we should return an object as devs won't check the return value anyway.
The special case in the debug mode looks like a reasonable approach. I'm curious whether there is any standardized way of logging those issues.
Maybe we could call _doing_it_wrong. We use that in several places when registering blocks or similar stuff for the block editor, for example:
There was a problem hiding this comment.
this is the big point about this patch that I'm unsure of. my only goal is to quickly bail if someone submits the wrong attribute, as I don't think it's likely people will send user-input attribute names here. if they do do that then I don't want it to crash the full render.
I'll ask around and see if anyone else has experience with this kind of failure.
There was a problem hiding this comment.
my only goal is to quickly bail if someone submits the wrong attribute
Yes, think this is the best approach here. If a plugin allows user input for attribute names (very unlikely as usually these are not random), it will have to make sure the input is valid, or the attribute will be skipped.
There was a problem hiding this comment.
I'm curious whether there is any standardized way of logging those issues.
Think there isn't one, unfortunately. Has been discussed few times afaik but a decision on how to implement it was not reached. Thinking it is time for this to be added to core. Perhaps another constant: WP_DEV_MODE or similar, then be much bolder about throwing errors and exceptions (with backtrace?) and writing in logs.
Using WP_DEBUG seems proper here imho. doing_it_wrong doesn't seem as good because it is targeted more at developers that try to use some function/method improperly (when there are better ways).
There was a problem hiding this comment.
@azaozz, great feedback! Thank you so much for clarifying where given options fit best.
The tests verifying behaviors dependent on whether `WP_DEBUG` is set need to run in an isolated environment so that the constant's definition doesn't bleed into other tests. The isolated runner is very slow in comparison to the non-isolated runner so only those tests dependent on the constant are moved over.
|
I added some positive test cases in ff9da88. |
Part of #44410
What
The
WP_HTML_Tag_Processorallows setting new HTML attributes with a given name and value. Previously this has allowed any string input for the attribute name, but we have to be careful not to print output that might break the HTML we're modifying.In this patch we're adding a check against the given attribute name and rejecting invalid or unacceptible names. WordPress here is more restrictive than HTML5.
Why?
We don't want to allow setting attributes whose names could break the HTML syntax or open new vulnerabilities in code that parses post content.
How?
Validate the given attribute name with a PCRE pattern that looks for unalloyed characters.
Testing Instructions
Please audit the code and review the new unit tests.
Review with an eye towards security.
cc: @azaozz