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-mustache: Output user errors when elements/attributes are sanitized #20285
amp-mustache: Output user errors when elements/attributes are sanitized #20285
Conversation
/to @alabiaga |
user().error(TAG, `Removed unsafe attribute: ${name}="${value}"`); | ||
} else if (r.element) { | ||
const {nodeName} = r.element; | ||
if (nodeName !== 'REMOVE') { // <remove> is added by DOMPurify. |
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.
why not just combine nodeName check in the initial element check.
if (r.attribute) {
const {name, value} = r.attribute;
user().error(TAG, `Removed unsafe attribute: ${name}="${value}"`);
} else if (r.element && r.element.nodeName !== 'REMOVE') {
...
}
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.
This reads more like prose to me:
// If an attribute is removed...
// Otherwise, if an element is removed...
// If the element is <remove>...
// Skip Edge, which throws "Permission denied" errors when inspecting | ||
// element properties in the testing iframe (Edge 17, Windows 10). | ||
describe.configure().skipEdge().run('amp-bind', function() { | ||
// Give more than default 2000ms timeout for local testing. |
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.
we should we keep this comment and move it to where you declared TIMEOUT.
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.
We're not referencing mochaTimeout
anymore, so there's no need to refer to "default timeout".
…ed (ampproject#20285) * Output user errors for elements/attrs sanitized by DOMPurify. * Make sanitizer error format consistent. * Add documentation with sanitization caveat. * Increase timeout of test-amp-bind to 15s.
Fixes #17897.