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.3.11 not backward compatible #712

Closed
Mirco469 opened this issue Aug 23, 2022 · 9 comments
Closed

Version 2.3.11 not backward compatible #712

Mirco469 opened this issue Aug 23, 2022 · 9 comments

Comments

@Mirco469
Copy link

Bug

Hello, i'd like to point out a problem with the latest version

Background & Context

I had set in my package.json the following dependency to this library: "dompurify": "^2.3.3", this meant that with the last npm install it installed version 2.3.11

Bug

With the latest version 2.3.11 my project compilation threw an error stating:
error TS2554: Expected 2 arguments, but got 1.
return this.sanitizer.bypassSecurityTrustHtml(DOMPurify.sanitize(html));
~~~~~~~~~~~~~~
node_modules/dompurify/dist/purify.cjs.d.ts:89:45
function sanitize(dirty: string | Node, cfg: any): any;

If this is wanted, you should not use a minor version for not backward compatibily code.

Thank you for your work! Mirco.

@cure53
Copy link
Owner

cure53 commented Aug 23, 2022

Hm, interesting. I am a bit confused as to where the error is coming from.

The only really new thing that 2.3.11 ships are the auto-generated types. Before this, we had none we shipped ourselves, and I was not aware that this might cause errors being thrown in existing environments.

Do you have any clue what the issue and how to solve it?

@brentkeller
Copy link

From our usage, and I suspect others, we're not passing the cfg parameter to sanitize. The types aren't showing it as optional so it's starting to throw errors in Typescript.

I'm not that familiar with what this package supports, but if it is possible to add a default value for the cfg parameter that might be enough so the types mark it as optional. If default values can't be used then maybe reverting and doing a non-backwards compatible version bump might be the answer.

@aryanisml
Copy link

Yes the same issue we also facing today after running with version 2.3.11 which updated few hours ago.
Currently down graded the version 2.3.10 and running application.
Please use the suggestion of @brentkeller

@cure53
Copy link
Owner

cure53 commented Aug 23, 2022

Gotcha, we'll have a look, thanks!

@cure53
Copy link
Owner

cure53 commented Aug 23, 2022

Since I don't have a setup I can test with here, would this do the trick?

  1. Change src/purify.js#1319, define cfg via default parameter:
  DOMPurify.sanitize = function (dirty, cfg = {}) { // = {} was added
    let body;
    let importedNode;
    let currentNode;
    let oldNode;
    let returnNode;
  1. This will generate different files for the dist folder:
  DOMPurify.sanitize = function (dirty) {
    var cfg = arguments.length > 1 && arguments[1] !== undefined ? arguments[1] : {};
    var body;
    var importedNode;
    var currentNode;
    var oldNode;
    var returnNode;
  1. This will give us different typings, which should fix the issue
    /**
     * Sanitize
     * Public method providing core sanitation functionality
     *
     * @param {String|Node} dirty string or DOM node
     * @param {Object} configuration object
     */
    sanitize(dirty: string | Node, ...args: any[]): any;
    /**

Does that fix the problem and/or is there any more elegant way?

cure53 added a commit that referenced this issue Aug 23, 2022
fix: marked cfg for sanitize() as optional / default param
@aryanisml
Copy link

Checking with current project.

@aryanisml
Copy link

Test

Checking with current project.

Tested with production code its working. Thanks for quick help

@cure53
Copy link
Owner

cure53 commented Aug 23, 2022

Amazing, thanks, will release 2.3.12 now :)

@cure53
Copy link
Owner

cure53 commented Aug 23, 2022

Done, this should be fixed, thanks for the quick heads-up & help!

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