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

devDependency for jsdom is very outdated #461

Closed
peernohell opened this issue Aug 26, 2020 · 5 comments · Fixed by #464
Closed

devDependency for jsdom is very outdated #461

peernohell opened this issue Aug 26, 2020 · 5 comments · Fixed by #464

Comments

@peernohell
Copy link
Contributor

DOMPurify uses jsdom version 8.x.x as a devDependency, however jsdom is currently only supporting version 16 and above.
See https://raw.githubusercontent.com/jsdom/jsdom/master/.github/ISSUE_TEMPLATE.md

Background & Context

There some bugs in older version of jsdom, that can crash purify execution.

Test Case

Very simple domPurify test case to trigger the exception:

const createDOMPurify = require('dompurify');
const { JSDOM } = require('jsdom');

const window = (new JSDOM('')).window;
const DOMPurify = createDOMPurify(window);

let src = '<img src="https://test.com/test.png">';
console.log(DOMPurify.sanitize(src));

Bug

This issue #375 is closed because it's a bug in jsdom, but as purify is not up to date the bug is still present in purify.

Input

<img src="https://test.com/test.png">

Given output

xxx/node_modules/jsdom/lib/jsdom/browser/resources/per-document-resource-loader.js:18
    const request = this._resourceLoader.fetch(url, {
                                         ^

TypeError: Cannot read property 'fetch' of null
    at PerDocumentResourceLoader.fetch (xxx/node_modules/jsdom/lib/jsdom/browser/resources/per-document-resource-loader.js:18:42)
    at HTMLImageElementImpl._attrModified (xxx/node_modules/jsdom/lib/jsdom/living/nodes/HTMLImageElement-impl.js:39:36)
    at Object.exports.appendAttribute (xxx/node_modules/jsdom/lib/jsdom/living/attributes.js:62:11)
    at Object.exports.setAttributeValue (xxx/node_modules/jsdom/lib/jsdom/living/attributes.js:218:13)
    at JSDOMParse5Adapter.adoptAttributes (xxx/node_modules/jsdom/lib/jsdom/browser/parser/html.js:152:18)
    at JSDOMParse5Adapter.createElement (xxx/node_modules/jsdom/lib/jsdom/browser/parser/html.js:73:10)
    at Parser._appendElement (xxx/node_modules/parse5/lib/parser/index.js:553:42)
    at areaStartTagInBody (xxx/node_modules/parse5/lib/parser/index.js:1519:7)
    at Object.startTagInBody [as START_TAG_TOKEN] (xxx/node_modules/parse5/lib/parser/index.js:1725:17)
    at Parser._processToken (xxx/node_modules/parse5/lib/parser/index.js:657:55)

This request have been already asked here #449 but it didn't point that an existing bug exist and require this upgrade. I understand that it has a big impact on test but it's make purify unusable as any text like that will not be parsable by purify and will failed.

@cure53
Copy link
Owner

cure53 commented Aug 26, 2020

Heya, thanks for filing this. I am unsure if we have the time to address this anytime soon. cc @tdeekens

@peernohell
Copy link
Contributor Author

I don't know that much purify but if I can be of any help don't hesitate to guide me :)

@cure53
Copy link
Owner

cure53 commented Aug 26, 2020

Thanks for the offer :) The problem is, that we would have to renovate the test suite to make sure it is compatible with later versions of jsdom. I am personally not involved with anything jsdom (a.k.a. have no clue where to get started in this case) and primarily take care of the core library.

If you have any ideas where to start patching, I'd be super happy to look into that together.

@cure53
Copy link
Owner

cure53 commented Aug 31, 2020

Closing this for now

@peernohell
Copy link
Contributor Author

I just update test and they are all passing now with new jsdom version.
Check PR #464

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

Successfully merging a pull request may close this issue.

2 participants