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
amp-script: Allow removal of attributes #23245
amp-script: Allow removal of attributes #23245
Conversation
/to @jridgewell |
Friendly ping. |
src/purifier.js
Outdated
@@ -390,17 +390,22 @@ export function validateAttributeChange(purifier, node, attr, value) { | |||
if (whitelist) { | |||
const {attribute, values} = whitelist; | |||
if (attribute === attr) { | |||
if (value == null || !values.includes(value)) { | |||
if (value === null || !values.includes(value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know the value can't be undefined
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, the TS interface uses string | null
. Maybe it's better to be defensive than readable here though.
* Allow removal of attributes. * Flipped a condition. * == instead of ===.
* Allow removal of attributes. * Flipped a condition. * == instead of ===.
Partial for #23156.
DOMPurify.isValidAttribute
expects non-null values.[sandbox=allow-forms]
.WHITELISTED_ATTRS
invalidateAttributeChange()
since I didn't realize those are already covered by the DOMPurify config.