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

Bithound.metalsmith stylus.2.0.0 #821

Closed
wants to merge 3 commits into from

Conversation

NubleX
Copy link

@NubleX NubleX commented Jul 8, 2016

Back-up security.

@ghost
Copy link

ghost commented Jul 8, 2016

could you provide a little more information as to what exact security flaw is fixed here? also, it would be nice if you could fix the merge conflict 👍

@NubleX
Copy link
Author

NubleX commented Jul 8, 2016

I'm thinking long-term here since I'm building an OS to operate the central nerve system and artificial limbs.

@ghost
Copy link

ghost commented Jul 9, 2016

but... how does this relate to this website?

i do see the security flaw though. goes back to this. fair enough, now we only need to fix this merge conflict

@ghost
Copy link

ghost commented Jul 9, 2016

/cc @stevemao @lpinca @fhemberger do we want to introduce snyk into this project?

@lpinca
Copy link
Member

lpinca commented Jul 9, 2016

I have no strong opinions, -0.

@fhemberger
Copy link
Contributor

I think it would be a good idea, but I guess it won't be added to the org (just like greenkeeper was removed). I think @phillipj might know the reason.

@@ -49,16 +51,17 @@
"metalsmith-metadata": "0.0.2",
"metalsmith-permalinks": "0.4.0",
"metalsmith-prism": "2.1.1",
"metalsmith-stylus": "1.0.0",
"metalsmith-stylus": "^2.0.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these should be in another pr

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 for "Update dependencies" PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

metalsmith-stylus is already at version 2.0.0 on master.
Some other dependencies updated here are also already updated on master.

@phillipj
Copy link
Member

phillipj commented Jul 13, 2016

I wasn't aware of snyk, at first glance it seems like a good thing to have. Although I would prefer if it would be less obtrusive, especially thinking of the .snyk file.

I think it would be a good idea, but I guess it won't be added to the org (just like greenkeeper was removed). I think @phillipj might know the reason.

You're correct about not being able to grant the snyk GH application access. We could get around that issue by running it via Travis. I did some tests locally and seems like running $ snyk test does the job.

image

How do we want to integrate this in our current flow? E.g. should we break the build or just view snyk issues as (spammy) warnings? Or maybe just add this badge to README.md https://snyk.io/test/github/nodejs/nodejs.org/badge.svg (yes, that badge is for nodejs.org).

And seeing the current snyk report, we're not able to fix all issues as some dependencies seem to have been abandoned. Might need to replace a couple of deps and possibly change some of our code.

@fhemberger
Copy link
Contributor

@phillipj snyk protect applies patches locally for those issues found, as long as no official fix/update is provided.

@phillipj
Copy link
Member

@fhemberger sounds like blackmagic. What does that mean in detail? Got any examples of that by any chance?

@fhemberger
Copy link
Contributor

@phillipj
Copy link
Member

Aah, so snyk installs its own version of e.g. minimatch in prepublish - that's the blackmagic I was looking for.

Again snyk seems too intrusive and possibly misleading for my taste, but I won't hold it back if most of you like it, so -0 from me.

@NubleX
Copy link
Author

NubleX commented Jul 13, 2016

i got 80% stability mark on my node [😣]


From: Phillip Johnsen [email protected]
Sent: Wednesday, July 13, 2016 12:58:10 PM
To: nodejs/nodejs.org
Cc: The Russian; Author
Subject: Re: [nodejs/nodejs.org] Bithound.metalsmith stylus.2.0.0 (#821)

I wasn't aware of snyk, at first glance it seems like a good thing to have. Although I would prefer if it would be less obtrusive, especially thinking of the .snyk file.

I think it would be a good idea, but I guess it won't be added to the org (just like greenkeeper was removed). I think @phillipjhttps://github.com/phillipj might know the reason.

You're correct about not being able to grant the snyk GH application access. We could get around that issue by running it via Travis. I did some tests locally and seems like running $ snyk test does the job.

[image]https://cloud.githubusercontent.com/assets/1231635/16800474/ce0f5862-48f5-11e6-8001-dbd8cd92b1cf.png

How do we want to integrate this in our current flow? E.g. should we break the build or just view snyk issues as (spammy) warnings? Or maybe just add this badge to README.md [https://snyk.io/test/github/nodejs/nodejs.org/badge.svg] https://camo.githubusercontent.com/c4f1062c354546dbe64f397e5dc607e329d2325c/68747470733a2f2f736e796b2e696f2f746573742f6769746875622f6e6f64656a732f6e6f64656a732e6f72672f62616467652e737667 (yes, that badge is for nodejs.org).

And seeing the current snyk reporthttps://snyk.io/test/github/nodejs/nodejs.org, we're not able to fix all issues as some dependencies seem to have been abandoned. Might need to replace a couple of deps and possibly change some of our code.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com//pull/821#issuecomment-232323192, or mute the threadhttps://github.com/notifications/unsubscribe/ARgrO_0pdh1L-W51xOVPI1AuIjdieigQks5qVMTCgaJpZM4JIXcf.

@fhemberger
Copy link
Contributor

@phillipj Not exactly. It installs the official package from npm, then patches the vulnerability (if patching is possibe).

E.g: Dependency x@latest still uses package [email protected], which contains a vulnerability. It is reported to the author and gets patched, [email protected]. Still, the author of x takes their time to update the dependency (or isn't even aware of the update). So snyk tries to backport the patch to [email protected] (if possible) for the time being. It's the same "black magic" Linux package maintainers have been doing for years. 😃

I've been trying myself to reach out to npm package maintainers to address dependency vulnerability issues, it's very hard work and PRs are being ignored some times for ages even if you make it clear, that a dependency is vulnerable and a security issue. Especially if the package is n-levels deep in the dependency chain. So snyk tries to mitigate this problem.

@fhemberger
Copy link
Contributor

I've added a clean version of this PR.

@NubleX
Copy link
Author

NubleX commented Jul 31, 2016

nice one :)


From: Frederic Hemberger [email protected]
Sent: Tuesday, July 26, 2016 8:35:38 PM
To: nodejs/nodejs.org
Cc: The Russian; Author
Subject: Re: [nodejs/nodejs.org] Bithound.metalsmith stylus.2.0.0 (#821)

I've added a clean version of this PR.

You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com//pull/821#issuecomment-235363139, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ARgrOw9A66H_oxidPQAztlCKYnKNexwjks5qZlN6gaJpZM4JIXcf.

@fhemberger
Copy link
Contributor

Closing this for #841.

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.

7 participants