-
Notifications
You must be signed in to change notification settings - Fork 33
Fix prototype pollution in removeAttributeNS #55
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
Fix prototype pollution in removeAttributeNS #55
Conversation
dom-element.js
Outdated
| DOMElement.prototype.removeAttributeNS = | ||
| function _Element_removeAttributeNS(namespace, name) { | ||
| var forbiddenKeys = ['__proto__', 'constructor', 'prototype']; | ||
| if (forbiddenKeys.includes(name)) { |
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.
How would name being any of these keys lead to a prototype pollution?
name is only used in delete attributes[name] below.
I don't think you can pollute the prototype using delete and you can't delete the prototype or constructor either.
dom-element.js
Outdated
| var attributes = this._attributes[namespace]; | ||
| if (attributes) { | ||
| delete attributes[name] |
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.
// Safely access and delete the attribute
const attributes = this._attributes?.[namespace];
if (attributes && Object.prototype.hasOwnProperty.call(attributes, name)) {
delete attributes[name];
}
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.
You're right, this makes more sense. I've updated the code.
|
Will there be a new release in npm to rectify this issue? I'm getting this being flagged out when I conduct a OWASP scan. Looking forward to your inputs. |
|
Is it possible to merge this and make a release. As it's failing audits for many packages including video.js |
|
@danrossi the library maintainer indicated it will not be released #54 (comment) |
This should prevent the prototype pollution vulnerability raised in #54