Opened 4 years ago
Closed 4 years ago
#52894 closed defect (bug) (fixed)
The wp_sanitize_script_attributes function added in version 5.7 does not escape attributes in some cases.
Reported by: |
|
Owned by: |
|
---|---|---|---|
Milestone: | 5.7.1 | Priority: | normal |
Severity: | critical | Version: | 5.7 |
Component: | Script Loader | Keywords: | has-patch commit fixed-major dev-feedback |
Focuses: | Cc: |
Description
The wp_sanitize_script_attributes function added in version 5.7 is not escaped if the array value is true.
Steps To Reproduce:
1.Add the following code to your theme's functions.php.
echo wp_get_script_tag( ['>console.log("hello")</script><script' => true ] );
2.Display the admin page.
3.The following script tag is output and executed.
<script >console.log("hello")</script><script></script>
Problematic source code:
line 2356 in the wp-includes/script-loader.php file.
$attributes_string .= $html5_script_support ? sprintf( ' %1$s="%2$s"', esc_attr( $attribute_name ), esc_attr( $attribute_name ) ) : ' ' . $attribute_name;
I think I need to escape as follows:
$attributes_string .= $html5_script_support ? sprintf( ' %1$s="%2$s"', esc_attr( $attribute_name ), esc_attr( $attribute_name ) ) : ' ' . esc_attr( $attribute_name );
Attachments (1)
Change History (9)
#3
@
4 years ago
With the patch applied, I added the following code to the theme's functions.php.
echo wp_get_script_tag( ['>console.log("hello");console.log(\'world\')</script><script' => true ] );
The output script tag looks like this, and the quotes appear to be converted.
<script >console.log("hello");console.log('world')</script><script></script>
#4
@
4 years ago
Fine, but that is still escaping, not sanitizing. Some of those characters are not valid for attributes.
#5
@
4 years ago
- Keywords commit added
Strictly speaking, @joyously is correct. While browsers can handle entities, the /
in the example above confuses browsers and causes the attribute to be split in to two (tested in Firefox on macOS):
>console.log("hello");console.log('world')< script><script
From the security perspective of keeping the visitor safe, it's doing the job so I think it's good to go in. As sanitization is a semi defined umbrella term for some combination of validation, escaping and filtering I think the name is fine for now.
#6
@
4 years ago
- Owner set to peterwilsoncc
- Resolution set to fixed
- Status changed from new to closed
In 50575:
#7
@
4 years ago
- Keywords fixed-major dev-feedback added
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for committing to the 5.7 branch.
Although it's not required until RC1, for my own piece of mind it would be great if a second committer could sign-off on the backport with the dev-reviewed
keyword. Ta.
This new function isn't sanitizing; it is only escaping. I think it's misnamed.
When the example is output, it has the angle brackets changed to entities, but the quotes are still nested, so it's not good.
If it were to sanitize, it would strip_tags and convert quotes.