Fix JSDOM transative vulnerability

[avra-m3]

We noticed a transitive vulnerability in our automated npm audit checks.

It would be good if we could get this fixed. Ran tests and it looked fine, not sure what uses jsdom to be able to test any behavior has changed directly.

If you can point me at the features that depend on jsdom happy to test them directly and add any required test cases.

Cheers.

For reference

node_modules/json-schema
  jsprim  0.3.0 - 2.0.1
  Depends on vulnerable versions of json-schema
  node_modules/jsprim
    http-signature  1.0.0 - 1.3.5
    Depends on vulnerable versions of jsprim
    node_modules/http-signature
      request  >=2.66.0
      Depends on vulnerable versions of http-signature
      node_modules/request
        jsdom  9.10.0 - 16.5.3
        Depends on vulnerable versions of request
        node_modules/fabric/node_modules/jsdom
          fabric  >=2.4.0
          Depends on vulnerable versions of jsdom
          node_modules/fabric

[andtii]

Yes we are doing a security review of all our deps and this is the last one having issues. Please get this in

[asturur]

Yeah i can get this in, if it does not cut any meaningfull node version.
It cuts node8 we don't care.
Are you using fabricJS in the browser only or on the server?
Are you aware that JSDOM won't get included in the bundle if mocked out correctly?

[asturur]

Please remove the minor version bumb and i ll merge ( need to look into why tests are failing )

[asturur]

this
https://github.com/fabricjs/fabric.js/pull/7510/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R85
would become >= 10

[avra-m3]

Alright, on it now :), sorry for the delay.

[avra-m3]

Updated PR

[asturur]

@ShaMan123 and @melchiar i was thinking to merge this ( vulnerabilities need to be fixed even if they are not triggerable in an immediate way ).
Bumping this requires to release fabric 5.0, at this point i would declare the only breaking change node version.
I m unsure if is better to bump up a little be more and cut at 14?
node 10 last patch was april this year, node 12 will be end of live april next year. Seems dull suggesting people they can still use it.

[melchiar]

I agree, better to bump the version up higher. There may be more vulnerabilities found with 10 or 12 that would then require we do yet another major version release.

[ShaMan123]

+1
Let's take fabric to the future

[asturur]

Argh need to investigate tests.

[avra-m3]

Hey @asturur, happy to investigate 🙂

[avra-m3]

I can't see anything immediately obvious in the JSDom changelogs.

It appears the image loading behaviour of parseFromString has changed in jsdom at some point, and now just hangs attempting to retrieve the image.

I'll keep looking into a solution but not sure there is one.

[asturur]

there are 2 issues right now.
a setTimeout change that break the test, but that is our fault probably, a better test can be written.
The other is the handling of non available images

[avra-m3]

there are 2 issues right now. a setTimeout change that break the test, but that is our fault probably, a better test can be written. The other is the handling of non available images

This setTimeout issue, I assume you mean the test having a timeout in general? Or is there another broken test I didn't see.

I think we can change the behaviour in jsdom to fix the missing image hanging, but I don't have time right now to look into it. I likely won't get time until christmas. .

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[asturur]

Hi @avra-m3 sorry for the long time.
I'm merging this so you get credit for the work done, thank you.
But i m also immediately updating to node 14 minimum and jsdom 19.
There is no need to stay behind on those.