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

Build deps have security issues #1369

Closed
5 tasks
marcoscaceres opened this issue Nov 30, 2016 · 28 comments
Closed
5 tasks

Build deps have security issues #1369

marcoscaceres opened this issue Nov 30, 2016 · 28 comments

Comments

@marcoscaceres
Copy link
Contributor

marcoscaceres commented Nov 30, 2016

Snyk is reporting a bunch of medium to high security issues with dependencies with this version of highlightjs. It might just be a matter of updating some dependencies.

  • High severity vuln found in minimatch@0.3.0, introduced via highlight.js@9.8.0
  • Low severity vuln found in hawk@1.1.1, introduced via highlight.js@9.8.0
  • Medium severity vuln found in request@2.40.0, introduced via highlight.js@9.8.0
  • Low severity vuln found in cli@0.6.6, introduced via highlight.js@9.8.0
  • High severity vuln found in handlebars@2.0.0, introduced via highlight.js@9.8.0
@isagalaev
Copy link
Member

Thanks, we'll look into it at some point!

Just a note though, these are all build dependency, we don't depend on anything at runtime, so users are safe :-)

@marcoscaceres marcoscaceres changed the title Security issues with deps Build deps have security issues Nov 30, 2016
@marcoscaceres
Copy link
Contributor Author

@isagalaev, thanks! Updated the title so not to scare anyone.

@isagalaev
Copy link
Member

Oh, that's thoughtful of you, thanks!

@Sannis
Copy link
Collaborator

Sannis commented Dec 2, 2016

@marcoscaceres, how are you get this report? I've tried to check hljs here and get "No known vulnerabilities found".

@marcoscaceres
Copy link
Contributor Author

@Sannis, if I recall correctly, I got it from cloning:

https://github.com/w3c/respec/

And then running snyk wizard.

@marcoscaceres
Copy link
Contributor Author

@Sannis you might need to "npm install" first, but not sure... might just work without.

@sourrust
Copy link
Member

sourrust commented Dec 3, 2016

Going through all the dependencies listed, it looks like most are from the build library we are using gear.js -- which is pretty much a dead project.

  • minimatch@0.3.0 is from gear-lib -> glob
  • hawk@1.1.1 is from gear-lib -> less -> request
  • request@2.40.0 is from gear-lib -> less
  • cli@0.6.6 is from gear-lib -> jshint
  • handlebars@2.0.0 is from gear-lib

I've been thinking about rewriting the build system because I was pretty much the only one that knew how to modify it and the documentation for gear.js isn't that great. Now that we have security issues it might be the extra push to actually get started on this.

I don't want to bother with other build systems that have been coming out because our build process is complex enough that the style these build library have don't really compliment our build process and I will end up working against them in the long run.

I'll layout my ideas more cleanly in a seperate issue before actually starting the build script rewrite so @isagalaev, @Sannis, and myself can actually have a better comprehension of our new build script. I'll claim responsible of this issue, so thank you @marcoscaceres for reporting this.

@sourrust sourrust self-assigned this Dec 3, 2016
@isagalaev
Copy link
Member

Jeremy thanks for weighing in on that! One immediate note from me regarding this:

our build process is complex enough

It's also a good opportunity to re-think if we need it to be this complex these days. The Internet is different, and old assumption may not hold up anymore. I don't have anything concrete right now, but I want to signal that nothing is set in stone regarding it.

@marcoscaceres
Copy link
Contributor Author

@sourrust @isagalaev, this is really exciting to hear! please cc me on that bug. I have a few suggestions to simplify things - and would like to show you our current set up (as it shows the issues with the current build system when used with another project).

@logicplace
Copy link
Contributor

To comment on the build process being simplified, being able to use the system as-is in addition to building a package would be pretty ideal for debugging/development, I think. Right now, since I want to be able to debug things easily (also since it's GitHub pages...), I edited the lang files I needed to register inline and just include everything via script/src or jQuery loading. It works, it just feels unnecessary. I'm sure there's a good design that would allow for both models. :]

@ydnar
Copy link

ydnar commented Feb 25, 2019

This raised its head again today, with several upstream vulnerabilities.

@slinkardbrandon
Copy link

I have a project template I generate with (so I can’t have a package-lock file). This started killing my CI today.

@marcoscaceres
Copy link
Contributor Author

We could really use some help with this. We need to get rid of gear, but it’s a fair amount of work.

@saschanaz
Copy link
Contributor

How about forking gear to update its dependencies?

@marcoscaceres
Copy link
Contributor Author

Worth a shot. The project is dead, so it’s not like it risks getting out of sync.

@marcoscaceres
Copy link
Contributor Author

Seems that the project has literally been deleted from Github... it used to be hosted by Yahoo.

@saschanaz
Copy link
Contributor

The main developer forked the project and it's now here: https://github.com/twobit/gear

@marcoscaceres
Copy link
Contributor Author

Nice find @saschanaz! I've forked it here: https://github.com/highlightjs/gear

Do you think you have time to poke at it? Might just be a matter of updating some of the really bad ones.

@zackdotcomputer
Copy link

👋 just saying hi, one of the many people who followed their npm audit trail to this issue today.

A quick question, though... if the gear and gear-lib dependencies are only used for the build tools and not by anything in the actual "work product" of the library, is there a reason they couldn't just be moved to devDependencies while a longer-term fix is put in place? That way they would be able to stop flowing downstream to people depending on this package? I am new here, though, so totally would believe if the answer is a flat "no" because I'm missing something about how dependers are using the files in the tools/ folder.

@marcoscaceres
Copy link
Contributor Author

@zackzachariah, it's a good suggestion but ./tools/build.js is actually a core part of the library as that's used to build bundles locally. The library doesn't ship with any bundles, and lots of people build bundles locally (remember this library predates webpack and friends).

@zackdotcomputer
Copy link

👍 Ok, makes complete sense. I'll let you focus on your work then and just keep lurking so I know when to ping the various packages in the dependency chain between us to update.

@marcoscaceres
Copy link
Contributor Author

Thanks @zackzachariah. Appreciate any comments/suggestions.

@saschanaz
Copy link
Contributor

Just cloning it... (||||-----------)

@saschanaz
Copy link
Contributor

@marcoscaceres Would you also fork https://github.com/twobit/gear-lib ? The majority of security issues are from that package.

@marcoscaceres
Copy link
Contributor Author

cloning ... (||||-----------)

@marcoscaceres
Copy link
Contributor Author

https://github.com/highlightjs/gear-lib

@isagalaev
Copy link
Member

@marcoscaceres

./tools/build.js is actually a core part of the library as that's used to build bundles locally.

Wait, that couldn't be right. When we build an npm build it doesn't include any javascript besides the core library and bundled languages (and those have literally no dependencies). So I think it's not only possible, but should be fairly trivial to move all of the node.js dependencies into devDependencies.

Building bundles locally definitely sounds to me as a developer activity, as it assumes a source checkout rather then npm install.

@marcoscaceres
Copy link
Contributor Author

Building bundles locally definitely sounds to me as a developer activity, as it assumes a source checkout rather then npm install.

On reflection, I agree. I’ve sent a PR to move all the dependencies be dev dependencies. Waiting on review approval.

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

9 participants