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

Make DOMPurify work in Node.js #29

Closed
2 of 5 tasks
fhemberger opened this issue Jul 6, 2014 · 36 comments
Closed
2 of 5 tasks

Make DOMPurify work in Node.js #29

fhemberger opened this issue Jul 6, 2014 · 36 comments

Comments

@fhemberger
Copy link
Contributor

Regarding issue #26 and #27, I originally held back Common JS style exports and publishing on npm on purpose, as DOMPurify doesn't run on a pure Node.js environment (it does client side with Browserify).

I'm still looking for a way to get it to work on Node.js as well. jsdom lacks DOM Level 2 Traversal methods like createNodeIterator at the moment, which DOMPurify uses internally.

What's currently missing:

  • document.implementation.createHTMLDocument
    (polyfilled with return jsdom('<html><body></body></html>');)
  • `window.NodeFilter (extracted the properties needed for DOMPurify from the spec)
  • document.createNodeIterator(root, whatToShow, filter, entityReferenceExpansion)
  • NodeIterator.nextNode() implementation
  • Setter method for document.body.outerHTML
@fhemberger
Copy link
Contributor Author

I'm currently trying to implement the NodeIterator myself.

@cure53
Copy link
Owner

cure53 commented Oct 4, 2014

Is there any news on this one?

@fhemberger
Copy link
Contributor Author

Nope, sorry. Didn't have time during the last months to work on the NodeIterator. It's still on my list.

@dashed
Copy link

dashed commented Oct 22, 2014

Sighs.

Was wondering why I was getting this error.

/path/to/node_modules/dompurify/purify.js:405
        if(typeof document.implementation.createHTMLDocument === 'undefined') 
                  ^
ReferenceError: document is not defined
...

Please don't host DOMPurify on npm if it's not going to work on node.js env; it's not a package manager for libraries that conveniently support CommonJS.

@fhemberger
Copy link
Contributor Author

npm is not a package manager exclusively for Node.js modules (Please read about the npm 2.0 release).
Also, DOMpurify works great with Browserify, that's why it's on npm.

@fhemberger
Copy link
Contributor Author

Today, I've sent a PR to the jsdom project to include add document.implementation.createHTMLDocument() to living standard.

You were right @cure53, dealing with DOM implementations is really a mess. ;)

@dashed
Copy link

dashed commented Oct 25, 2014

Hmmm. Are you planning on adding jsdom as a dependency?

Would it be possible to use cheerio as an alternative?

@fhemberger
Copy link
Contributor Author

Cheerio is a jQuery-like HTML parser, not a DOM implementation (which we need, e.g. for createHTMLDocument, NodeFilter, NodeIterator, etc.).

@cure53
Copy link
Owner

cure53 commented Oct 26, 2014

@fhemberger Nice!

Would that mean that w ecan make DOMPurify happen in combination with jsdom? Or do we need to start creating a feature table of missing APIs and work our way through them?

@fhemberger
Copy link
Contributor Author

NodeFilter and NodeIterator are still missing in jsdom. NodeFilter is just a bunch of constants, NodeIterator is a tiny bit more complex. ;) Both should pass W3C's implementation tests as well.

Then we should be able to use DOMPurify together with jsdom for Node.js.

@dashed
Copy link

dashed commented Oct 27, 2014

I have a feeling this can be done without jsdom. DOMPurify caught my attention because of its performance characteristics; and jsdom may slow things down a lot.

@fhemberger
Copy link
Contributor Author

Well, if you know of a different DOM implementation and you can get DOMPurify working with it, we'd happily accept a pull request. At the moment, jsdom is the only thing that comes to my mind …

@cure53
Copy link
Owner

cure53 commented Oct 27, 2014

@dashed I fully agree with @fhemberger, if there was a way to do it w/o jsdom: awesome :)

Using DOMPurify with node.js opens many many new doors.

@Shipow
Copy link

Shipow commented Nov 12, 2014

Would be really awesome to have it run in full node.js env. Looking forward to see that happening! Good luck guys!

@fhemberger
Copy link
Contributor Author

I'm already pushing updates to jsdom, I'll have a bit more time in December so I hope we can close this issue by the end of the year. ;)

@wtfuii
Copy link

wtfuii commented Feb 11, 2015

+1 for this feature..

@Joris-van-der-Wel
Copy link
Contributor

jsdom 5.1.0 supports NodeIterator: jsdom/jsdom#1092
There are still other issues though, such as firstElementChild

@fhemberger
Copy link
Contributor Author

Thanks, I read the announcement (but hadn't notice the version has already been released).
I hope to find the time soon to look into this topic again. Sorry this takes so long, but fiddling around with a DOM implementation in Node is really messy. ;)

@Joris-van-der-Wel
Copy link
Contributor

jsdom 5.2.0 has many fixes to make dompurify work. Most html snippets can now be "purified" properly.
Although I have to load dompurify as a script using jsdom.env because dompurify uses a lot of globals. It would be great if I could instantiate dompurify by passing it a Window.

There are still a few test cases failing though, which would require more work in jsdom. One example is that jsdom currently throws an exception if an attribute is set with an invalid name: foo.innerHTML = "<img id/=' >". The spec says that resolving parser errors is optional, but it is of course not how the browsers behave. The result is that dompurify is not able to clean up such snippets.

@Joris-van-der-Wel
Copy link
Contributor

Above has been fixed. There are now 13 failing test cases.
Additional stuff to fix in jsdom:

  • A few test cases with svg tags fail, the output looks safe, but it is not what the test cases expect
  • stub or implement adoptNode
  • stub or implement activeElement
  • Do not throw if the style attribute contains invalid CSS: https://github.com/chad3814/CSSStyleDeclaration/issues/30
  • Implement insertAdjacentHTML (required if you want to use KEEP_CONTENT)

@fhemberger
Copy link
Contributor Author

Thanks @Joris-van-der-Wel for digging into this, looks like we're getting closer.
Unfortunately, I'm pretty occupied at the moment, so I'm not able to dive into the jsdom code right now to implement the missing features. 😞

@cure53
Copy link
Owner

cure53 commented May 18, 2015

A quick question: I think we are close to the next release. Should we go ahead or wait for you guys? Not sure how far this ticket is in total, thus asking :)

@fhemberger
Copy link
Contributor Author

No, don't wait for me. Just go ahead and release it. Node support would be a major version increment anyway.

@Joris-van-der-Wel
Copy link
Contributor

As for releasing:
I do not think that any XSS attempts will get through if you use jsdom with these unresolved issues. The effects of these issues are:

  • adoptNode, activeElement and maybe others that are not in the test cases may get clobbered if you only filter using jsdom. These are not causing XSS leaks.
  • Invalid CSS in style tags will throw. For example if DOMPurify is used in a node http server, it would give clients a 500 error instead of the sanitized html.
  • The KEEP_CONTENT option does not work at all

The only thing I am not sure about yet are the 2 cases (87 and 173) that fail with svg content. I have not looked at those extensively. Perhaps all those need are an additional "expected" value in the test case itself.

Here is the output of the test cases: https://joris-van-der-wel.github.io/DOMPurify-04d7218-on-jsdom-5.4.0.html and here is the script I used to generate this https://joris-van-der-wel.github.io/DOMPurify-test-jsdom.js

@Joris-van-der-Wel
Copy link
Contributor

So if none of those issues can cause a security issue, I would say, release it

@dhardtke
Copy link

So will 0.6.4 support NodeJS?

@Joris-van-der-Wel
Copy link
Contributor

It supports iojs. (nodejs will be supported in the near future because iojs and nodejs are merging).

Try this:

npm install cure53/DOMPurify jsdom
var document = require('jsdom').jsdom();
var dompurify = require('dompurify')(document.defaultView);
console.log(dompurify.sanitize(`
        hell <script>alert("hi");</script>
        <div onclick="alert(123);">
                o
        </div>
        world
        <img id="createElement">
`));

@fhemberger
Copy link
Contributor Author

I'm so grateful for your help so we finally got this out. Thanks!

@fhemberger
Copy link
Contributor Author

I haven't had the time for testing it, but can we close this issue now?

@Joris-van-der-Wel
Copy link
Contributor

Well, enabling KEEP_CONTENT has no effect now. Beyond that, I can imagine wanting to add a test runner in DOMPurify for jsdom (also see #61).

And the readme will need updating

@fhemberger
Copy link
Contributor Author

Updated the README just now: 6b0c682.
I'm closing this issue now and create a new issue for KEEP_CONTENT.

@CaptainN
Copy link

In case anyone else comes looking - to make current jsdom work (Feb 2019):

import { JSDOM } from 'jsdom'
import DOMPurify from 'dompurify'
const { window } = new JSDOM('<!DOCTYPE html>')
const domPurify = DOMPurify(window)
console.log(domPurify.sanitize(`
        hell <script>alert("hi");</script>
        <div onclick="alert(123);">
                o
        </div>
        world
        <img id="createElement">
`));

@kkomelin
Copy link

@CaptainN Thanks for your solution.
I've gone further and created an isomorphic wrapper which allows using DOMPurify on both frontend and backend seamlessly https://github.com/kkomelin/isomorphic-dompurify
I would appreciate your feedback on the wrapper.

@EddyVinck
Copy link

EddyVinck commented Apr 6, 2020

I just stumbled upon this problem. I found a lightweight alternative for a simple DOMPurify use-case that works in Node: https://github.com/leizongmin/js-xss

import xss from 'xss';

export function sanitizeText(string) {
  // only include whitelisted tags, remove the others
  return xss(string, { whiteList: ['b', 'i', 'strong'], stripIgnoreTag: true });
}

It has to be lightweight because I'm using it with server-side rendered React, which runs in Node first but then runs in the browser.

@kkomelin
Copy link

kkomelin commented Apr 6, 2020

@EddyVinck There are a few xss-filtering solutions on NPM, you're free to use any of them.

What led me to DOMPurify is this presentation Building Secure React Applications by Philippe De Ryck. Since you're working with React, it may interest you too.

@etiennejcharles
Copy link

Well seems like a port was created 2 months ago
https://github.com/kkomelin/isomorphic-dompurify#readme

MonkeyDo added a commit to akashgp09/bookbrainz-site that referenced this issue May 3, 2021
BrunoBernardino added a commit to padloc/padloc that referenced this issue Jun 22, 2022
* Sanitize email HTML to prevent XSS

Fixes #457

* Replace dompurify with isomorphic-dompurify as per cure53/DOMPurify#29

* Switch to dompurify and make it work server-side on our own.
timur987 added a commit to Automattic/wp-calypso that referenced this issue Aug 15, 2023
Using DOMPurify as in a guide produces an error because JEST do not have
access to window object, so I applied the solution linked below:
cure53/DOMPurify#29 (comment)
timur987 added a commit to Automattic/wp-calypso that referenced this issue Aug 16, 2023
Using DOMPurify as in a guide produces an error because JEST do not have
access to window object, so I applied the solution linked below:
cure53/DOMPurify#29 (comment)
timur987 added a commit to Automattic/wp-calypso that referenced this issue Aug 18, 2023
* Allow anchors in banner's description

* Fetch JITMs on the "All posts" page

* Fix a failed client-side test

Using DOMPurify as in a guide produces an error because JEST do not have
access to window object, so I applied the solution linked below:
cure53/DOMPurify#29 (comment)

* Fix tests by removing JSDOM usage

Recently I've used JSDOM to imitate window object presence
to overcome JEST tests falling since JEST uses node environment.
Eventually it turned into a webpack breakage.
The solution is in mocking DOMPurify.

* Add missed dependency

* Change JITM's message path

Recently I used an incorrect message path and did not notice
it because previously I mocked response from public-api.wordpress.com.

* Remove isJetpack condition for Blaze's JITM

since the current site might be connected to Jetpack.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests