DOMPurify icon indicating copy to clipboard operation
DOMPurify copied to clipboard

Support for Node and TypeScript

Open vhscom opened this issue 3 years ago • 12 comments

Feature

New developers are struggling to use DOMPurify with modern development frameworks. Eventually they stumble on https://github.com/cure53/DOMPurify/issues/400 but I'd like to channel 2015 and ask for baked in Node support. And type definitions out of the box like XSS would be good to add as well.

vhscom avatar May 27 '22 18:05 vhscom

Heya, I am not sure what to do with this ticket. I am not a Node person, don't know what's hot-or-not right now in this realm etc. - this library is pretty much written for the browser and happens to also work with jsdom.

But.

If we get a juicy PR or alike that actually implements what you need, happy to review and go with it. But, based on the above, I really have nothing actionable. Hope that males sense.

cure53 avatar May 31 '22 17:05 cure53

I understand. I'm more browser myself. The two actionable items are:

  1. Support isomorphic rendering in SvelteKit to address the Node.
  2. Integrate the type definitions from @types/dompurify or, perhaps, adorn the public API with JSDoc type hints.

The isomorphic rendering is a step towards ultimately just running in a Web Worker after a Node-based app compilation step. If this library can be made to run in SvelteKit and deploy to Cloudflare Pages, that's two birds with one stone.

For types, a definition file exists but it requires a separate install. If someone creates a PR to add TypeScript type definitions, or possibly just JSDoc type hints, the out of the box developer experience would be much improved.

Here's an example Foss app where such a juicy 🧃 Pull could be tested: https://github.com/vhscom/metaform

Unfortunately, my laptop is on the fritz. But this is a popular lib so I know it would make a good alternative to js-xss if these two items can be tackled.

vhscom avatar Jun 01 '22 12:06 vhscom

@kkomelin Would you possibly be up for integrating your SSR changes into the main library?

vhscom avatar Jun 01 '22 12:06 vhscom

Cool :smile: I would be fine with integrating those if it can be done without affecting current use-cases (my feeling is it won't so all good).

cure53 avatar Jun 01 '22 12:06 cure53

Hi guys,

@vhscom Thanks for your suggestion. It's a good idea.

@cure53 That's great that you're open for such integration. Thanks.

I've created an issue to discuss this idea with my co-maintainer and everyone interested https://github.com/kkomelin/isomorphic-dompurify/issues/124

kkomelin avatar Jun 02 '22 20:06 kkomelin

Hey all, any progress here? :)

cure53 avatar Aug 03 '22 13:08 cure53

Hi @cure53,

I have not yet received any response from my co-maintainer but we may not need his confirm actually.

As for the types, I can suggest using TypeScript compiler which can generate typings by JS files.

npx -p typescript tsc src/**/*.js --declaration --allowJs --emitDeclarationOnly --outDir types

Creating .d.ts Files from .js files

kkomelin avatar Aug 03 '22 14:08 kkomelin

As for combining dompurify and isomorphic-dompurify, it should not be hard code-wise but it can introduce new issues for dompurify project like this one https://github.com/kkomelin/isomorphic-dompurify/issues/54

kkomelin avatar Aug 03 '22 14:08 kkomelin

@kkomelin I might need some help here to understand the general issue. Or issues. So, basically we have two problems here, correct?

Problem one is that we don't have baked in Node support. That, we could fix by bringing isomorphic-dompurify and DOMPurify together, so people don't need to maintain two dependencies, not just one. Alternatively, we could update the dusty README and simply point people to your project, correct?

Problem two is that we don't natively ship any type definitions, so folks have problems like this one, correct? https://github.com/cure53/DOMPurify/issues/705 This we could fix by generating the types ourselves, as you described, correct?

cure53 avatar Aug 09 '22 14:08 cure53

@kkomelin And, if I understood Problem two properly, would we not want to generate type definitions for the dist files rather than for the src files? Just wondering - I am not too experienced with TypeScript and related development frameworks.

npx -p typescript tsc src/**/*.js --declaration --allowJs --emitDeclarationOnly --outDir types

cure53 avatar Aug 09 '22 14:08 cure53

@cure53 Thanks for working on this issue and suggesting some solutions.

Problem one is that we don't have baked in Node support. That, we could fix by bringing isomorphic-dompurify and DOMPurify together, so people don't need to maintain two dependencies, not just one. Alternatively, we could update the dusty README and simply point people to your project, correct?

I think you got it right.

Problem two is that we don't natively ship any type definitions, so folks have problems like this one, correct? https://github.com/cure53/DOMPurify/issues/705 This we could fix by generating the types ourselves, as you described, correct?

I don't think generating types would be enough to solve #705 but it'd definitely improve the dompurify project in general.

@kkomelin And, if I understood Problem two properly, would we not want to generate type definitions for the dist files rather than for the src files? Just wondering - I am not too experienced with TypeScript and related development frameworks.

It's a matter of experimentation because I usually write TypeScript code right away and don't generate typings by JS files afterwards.

kkomelin avatar Aug 09 '22 19:08 kkomelin

Thanks @kkomelin

Just a heads-up, we updated the README. Maybe this helps solving some of the issues?

https://github.com/cure53/DOMPurify#running-dompurify-on-the-server

cure53 avatar Aug 10 '22 13:08 cure53

Thank you very much @cure53 for updating the readme.

As for whether it solves all the problems or not, we need to ask @vhscom

kkomelin avatar Aug 13 '22 05:08 kkomelin

Quick update, we now also generate type definitions ourselves, which appears to solve this issue:

https://github.com/cure53/DOMPurify/issues/705#issuecomment-1214166741

Does this also solve the issue described here?

cure53 avatar Aug 13 '22 14:08 cure53

That's great news. Thanks @cure53!

@vhscom Could you please confirm if the solutions introduced by @cure53 address the issues you initially reported?

kkomelin avatar Aug 13 '22 15:08 kkomelin

Closing this for now as I feel we are done here for good :D Please let me know in case anything is missing / was overlooked.

cure53 avatar Aug 23 '22 09:08 cure53

Thank you very much @cure53 for your effort.

kkomelin avatar Aug 23 '22 12:08 kkomelin