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

Web Worker Support #242

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

wSedlacek
Copy link

@wSedlacek wSedlacek commented Dec 28, 2023

Current Behavior

When importing into a worker environment the following runtime error occurs

Uncaught ReferenceError: window is not defined
    at node_modules/.pnpm/[email protected][email protected]/node_modules/isomorphic-dompurify/browser.js (isomorphic-dompurify.js?v=650fd638:949:22)

Change in behavior

The runtime error does not occur and DOMPurify is accessible both as a export
and on self.DOMPurify/globalThis.DOMPurify in workers

Developer Notes

There are probably a dozen different ways to fix this issue, the one chosen was
picked for being the one with the fewest number of changes while being
backwards compatible all the way back to browser versions from 2012 (Firefox
back to 2004)

See: https://developer.mozilla.org/en-US/docs/Web/API/Window/self#examples

Other approaches considered

  • Using typeof window !== "unefined"
    • This method of determining if the window exists and if this code is
      executing in a worker was considered however after this check was done the
      split in exuection after this was detected would have either needed to use
      self or pollyfil window on self. Since self always works it seemed
      more straight forward to just always use it instead of window

Example:

const globalObject = typeof window !== "undefined" ? window : self;
module.exports = globalObject.DOMPurify || (globalObject.DOMPurify = require('dompurify').default || require('dompurify'));
  • Using globalThis
    • This method would be better if trying to target the same code on server and
      browser however it is a much newer feature and not available on all platforms
      (ie, React Native, etc.)

Example:

module.exports = globalThis.DOMPurify || (globalThis.DOMPurify = require('dompurify').default || require('dompurify'));
  • Using typeof window !== "undefined" but not assigning to self.DOMPurify
    • This would simply remove the accessing of window in worker environments
      effectively killing the polyfil nature of this in worker environments but
      still allowing the import

Example:

const dompurify = require("dompurify")

if (typeof window !== "undefined") {
  window.DOMPurify = window.DOMPurify || dompurify.default || dompurify;
}

module.exports = typeof window !== "undefined" ? window.DOMPurify : dompurify.default || dompurify;
  • Using optional chaning
    • This would have had the same outcome of not having DOMPurify accessible on
      self/globalThis as the previous method however it would rely on a feature
      only aviable in newer JavaScript environments.

Example:

const dompurify = require("dompurify")

if (typeof window !== "undefined") {
  window.DOMPurify = window.DOMPurify || dompurify.default || dompurify;
}

module.exports = window?.DOMPurify || dompurify.default || dompurify;
  • Using null coalescing
    • Again this feature is very new and not supported in every environment
const dompurify = require("dompurify")

if (typeof window !== "undefined") {
  window.DOMPurify ??= dompurify.default || dompurify;
}

module.exports = window?.DOMPurify : dompurify.default || dompurify;

fixes: #240

@kkomelin
Copy link
Owner

kkomelin commented Dec 30, 2023

Thank you very much @wSedlacek for such a thoughtful analysis of solutions and choosing the best one for your PR!
Please give me a couple of days to test the chosen solution.

@kkomelin kkomelin added the enhancement New feature or request label Dec 30, 2023
@kkomelin
Copy link
Owner

kkomelin commented Dec 30, 2023

@wSedlacek I'm having hard times importing isomorphic-dompurify from inside of a Web Worker script. I don't have much experience with web workers. Could you please instruct me how to do it?

I'm using Vite vanilla template and importing this way import DOMPurify from "isomorphic-dompurify";, but unfortunately it gives me DOMPurify.sanitize is not a function.

In my package.json:

 "dependencies": {
    "isomorphic-dompurify": "github:wSedlacek/isomorphic-dompurify#feat/worker-support"
  }

@wSedlacek
Copy link
Author

I’ll create an example today for testing and get back to you.

@wSedlacek wSedlacek marked this pull request as draft January 2, 2024 17:01
@wSedlacek
Copy link
Author

wSedlacek commented Jan 2, 2024

https://github.com/cure53/DOMPurify/blob/d7498e0546c0f67cf95437ae430ff2035582a689/src/purify.js#L90-L96

So dompurify itself is also checking the window and early exiting. I am going to mark this as a draft for a moment while I evaluate the options.

Update:
I was able to get it running with linkedom as noted in the comments of cure53/DOMPurify#577

import createDOMPurify from "isomorphic-dompurify";
import { parseHTML } from "linkedom";

const { window } = parseHTML("<!DOCTYPE html><body>");
const DOMPurify = createDOMPurify(window);

self.onmessage = (event) => {
  const purified = DOMPurify.sanitize(event.data);
  console.log(`On Worker Thread: ${purified}`); // Should be <img src="x”> but is <img src=x onerror=alert(1)//>
};

The only issue is that while it does run, it doesn’t actually santaize anything at all.
This all said, without the changing in this PR the example doesn’t run inside works at all (because it would crash when trying to use window.DOMPurify on import)
I think it is better to have the issues with web workers be entirity with dompurify instead of the introp layer.

@wSedlacek wSedlacek marked this pull request as ready for review January 2, 2024 17:28
@kkomelin
Copy link
Owner

kkomelin commented Jan 3, 2024

Thank you @wSedlacek for your work on this issue! Really great job!

However, I think we cannot release this change without dompurify itself adding support for Web Worker, so I suggest us keeping this issue open in case anything changes in the future. For your particular case, you may consider using alternatives for now.

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

Successfully merging this pull request may close these issues.

Web Worker Support
2 participants