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

Upgrade dependencies to fix security issues #70

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

prantlf
Copy link

@prantlf prantlf commented Jun 21, 2019

I upgraded all dependencies in package.json to their current versions.

The new version of SVGO returns Promises. I'll try to modify crass to do it too. It will be a bigger change and a breaking one.

                       === npm audit security report ===
┌──────────────────────────────────────────────────────────────────────────────┐
│                                Manual Review                                 │
│            Some vulnerabilities require your attention to resolve            │
│                                                                              │
│         Visit https://go.npm.me/audit-guide for additional guidance          │
└──────────────────────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ Moderate      │ Denial of Service                                            │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ js-yaml                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=3.13.0                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ crass                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ crass > svgo > js-yaml                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/788                             │
└───────────────┴──────────────────────────────────────────────────────────────┘
┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Code Injection                                               │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ js-yaml                                                      │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=3.13.1                                                     │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ crass                                                        │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ crass > svgo > js-yaml                                       │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://npmjs.com/advisories/813                             │
└───────────────┴──────────────────────────────────────────────────────────────┘

found 2 vulnerabilities (1 moderate, 1 high) in 2481 scanned packages
  2 vulnerabilities require manual review. See the full report for details.

@mattbasta
Copy link
Owner

Thanks for putting this together. It's been on my list for a while, but fairly low priority because of the low risk of exploit (I certainly hope nobody is using crass in a server-side runtime environment!).

I would like to eventually move to an async API. But doing that safely is tricky (lots of remembering to await). I think that change would need to be preceded by a port to TS, which is what almost all of my code is these days anyway.

@mattbasta
Copy link
Owner

I'll try to comb through this PR soon!

@prantlf
Copy link
Author

prantlf commented Jun 21, 2019

Yes, it's not going to be trivial. I'm afraid, that I posted this PR too early with too little work... Having the optimize method synchronous is very convenient for easier coding. It's also an interface used by all nodes. The broad usage of this method makes the change bigger, than I initially thought.

The only method, which really needs to be asynchronous is optimizeDataURI because of the usage of SVGO. I was playing with the idea of returning the promise only from there and when any of the optimize methods is called, deciding on what to do by testing the result with instanceof Promise. I'm not sure, if it'd make the work simpler.

And you're right, the risk is low. It's just that npm starting to provoke me by that audit report :-)

I tried a "hotfix" by forking the last synchronous svgo@0.7.2 to @prantlf/svgo and depending on it. It's a zero-effort change in crass to get rid of the security warning, but depending on an old package wasn't not the final solution. If you're interested, I could open a PR with that as a temporary "silencing" the npm audit.

@mattbasta
Copy link
Owner

I took a bit of time to do some work this morning. Namely, I've done the following:

  • Bumping the version to 1.0.0; it feels like I'd might as well do a major version bump if I'm going to change the API
  • Drop node <10
  • Since node <10 is gone, I dropped babel/browserify
  • Swapped mocha out for jest, which I much prefer these days anyway

To get svgo, the plan is to make pretty, optimize, and all the associated helper functions (that have side effects or use async code) async. Which actually doesn't seem so bad; most of it is going to be some find-and-replace and slapping await in a bunch of spots. I'll see about doing that soon.

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

Successfully merging this pull request may close these issues.

2 participants