Skip to content
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

Version 2.2.6 is incompatible with PhantomJS 2.1.1 #502

Closed
KieranCairney opened this issue Jan 20, 2021 · 7 comments
Closed

Version 2.2.6 is incompatible with PhantomJS 2.1.1 #502

KieranCairney opened this issue Jan 20, 2021 · 7 comments

Comments

@KieranCairney
Copy link

After upgrading to version 2.2.6, I've observed that under our PhantomJS unit test runner, the majority of my unit tests utilizing dompurify now fail.

The discrepancy between PhantomJS and using a real browser or chromeheadless here starts with window.Element. Logging window.Element in PhantomJS outputs an Object as opposed to a function when running in the DOM/chromeheadless. This eventually leads to a cascading failure where getParentNode (https://github.com/cure53/DOMPurify/blob/2.2.6/src/purify.js#L114) evaluates to null and so forth.

window.Element in PhantomJS 2.1.1 logs the following when logging in CLI during the test:
PhantomJS 2.1.1 (Mac OS X 0.0.0) INFO: Object{ALLOW_KEYBOARD_INPUT: 1, prototype: Object{webkitGetRegionFlowRanges: function webkitGetRegionFlowRanges() { ... }, hasAttributes: function hasAttributes() { ... }, querySelectorAll: function querySelectorAll() { ... }, getClientRects: function getClientRects() { ... }, scrollByPages: function scrollByPages() { ... }, remove: function remove() { ... }, scrollIntoViewIfNeeded: function scrollIntoViewIfNeeded() { ... }, setAttribute: function setAttribute() { ... }, webkitRequestFullscreen: function webkitRequestFullscreen() { ... }, getElementsByTagName: function getElementsByTagName() { ... }, getAttribute: function getAttribute() { ... }, querySelector: function querySelector() { ... }, webkitMatchesSelector: function webkitMatchesSelector() { ... }, hasAttribute: function hasAttribute() { ... }, getAttributeNode: function getAttributeNode() { ... }, getAttributeNS: function getAttributeNS() { ... }, ALLOW_KEYBOARD_INPUT: 1, getElementsByClassName: function getElementsByClassName() { ... }, removeAttributeNS: function removeAttributeNS() { ... }, setAttributeNode: function setAttributeNode() { ... }, setAttributeNS: function setAttributeNS() { ... }, hasAttributeNS: function hasAttributeNS() { ... }, blur: function blur() { ... }, scrollByLines: function scrollByLines() { ... }, removeAttribute: function removeAttribute() { ... }, setAttributeNodeNS: function setAttributeNodeNS() { ... }, removeAttributeNode: function removeAttributeNode() { ... }, getElementsByTagNameNS: function getElementsByTagNameNS() { ... }, getAttributeNodeNS: function getAttributeNodeNS() { ... }, focus: function focus() { ... }, scrollIntoView: function scrollIntoView() { ... }, getBoundingClientRect: function getBoundingClientRect() { ... }, webkitRequestFullScreen: function webkitRequestFullScreen() { ... }}}

Up until 2.2.4, there were no compatibility problems with PhantomJS. Is this something that was a coincidence and there is no plan to support going forward, or will this be treated as a bug? Thanks!

@cure53
Copy link
Owner

cure53 commented Jan 20, 2021

Hmmm, not sure if we wanna address this. @securitum-mb what do you think?

@cure53
Copy link
Owner

cure53 commented Jan 20, 2021

My gut feeling says, this is a bug in PhantomJS as they seem to return the wrong type.
Given that the project was discontinued quite a while ago, I see little use in fixing around their bugs.

@securityMB
Copy link

@cure53 I am not really sure how to fix it :/ I had a look and it seems that you can't get property descriptors of Node.prototype properties like parentNode:

phantomjs> Object.getOwnPropertyDescriptor(Node.prototype, 'parentNode')
undefined

Which means that we cannot do the clobbering protection correctly in this case.

I don't think this should be fixed in DOMPurify; but for the sake of tests, @KieranCairney, you can just simulate the property descriptors, something like:

const orig = Object.getOwnPropertyDescriptor;
Object.getOwnPropertyDescriptor = function(obj, prop) {
    if (prop === 'cloneNode')
        return {value: function() {return this.cloneNode()}}
    if (prop === 'nextSibling')
        return {value: function() {return this.nextSibling}}
    if (prop === 'childNodes')
        return {value: function() {return this.childNodes}}
    if (prop === 'parentNode')
        return {value: function() {return this.parentNode}}

    return orig(obj, prop);
}

@cure53
Copy link
Owner

cure53 commented Jan 20, 2021

Thanks @securitum-mb !

@KieranCairney
Copy link
Author

KieranCairney commented Jan 20, 2021

Thanks all for the quick replies, I also tested and confirmed the workaround does in fact solve my use case.
My team and I are working towards 100% transitioning off of PhantomJS to chromeheadless in this related project, however in the short to medium term, we were hoping for a workaround like this to still address security vulnerabilities for the time being, so thank you!

@cure53 cure53 closed this as completed Jan 21, 2021
@jridgewell
Copy link

jridgewell commented Jan 25, 2021

We're seeing this popup for Safari 9 and earlier. The descriptor for Node.prototype.parentNode is { get: undefined, set: undefined, enumerable: true, configurable: false } (and value is non-existent).

@cure53
Copy link
Owner

cure53 commented Jan 26, 2021

We don't support Safari 9, see README. Oldest Safari version we support is Safari 10, sorry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants