feat: fixed in key in binaries output #317

karniwl · 2018-12-30T13:09:10Z

Ready for review
Follows CONTRIBUTING rules
Reviewed by Snyk internal team

What does this PR do?

Adds a Fixed in version key in binaries/output, using npm semver rules.
Refactors formatting issues logic.

How should this be manually tested?

Run snyk test --docker node:5.10.1 for example to see the fixed versions of found vulnerabilities.

Screenshots

leongold · 2018-12-30T15:03:24Z

src/cli/commands/test.js

+function createFixedInText(versionRangeList, pkgVersion) {
+  let fixedVersion = '';
+  if (!pkgVersion && versionRangeList &&
+      versionRangeList.length && /^<\S+$/.test(versionRangeList[0])) {


This if clause is a bit difficult to read, how do you feel about extracting it to a function?

I think it's a bit of an overkill to extract it to a function as the meaning (if you know it of course... :) ) is pretty straightforward. I'll change it up and extract to a variable instead and let me know what you think

leongold · 2018-12-30T15:07:52Z

src/cli/commands/test.js

@@ -314,7 +320,7 @@ function displayResult(res, options) {
      remediationInfo: vuln.metadata.type !== 'license'
        ? createRemediationText(vuln, packageManager)
        : '',
-      fixedIn: options.docker ? createFixedInText(vuln) : '',
+      fixedIn: options.docker ? createFixedInText(vuln.list[0].semver.vulnerable, version) : '',


I understand there's special meaning to the first index of the vuln.list, perhaps we could store it in a variable? (same for versionRangeList[0] and versionList[1])

src/cli/commands/test.js

leongold · 2018-12-30T19:15:36Z

src/cli/commands/test.js

+  if (!pkgVersion) {
+    if (versionRangeList &&
+        versionRangeList.length &&
+        (lesserThan.test(versionRangeList[0]))) {


Thanks for the changes! Can we also store versionRangeList[0] in a variable? I don't understand the significance of the first index (I guess it's ordered and we need the first item?) :)

there isn't a special meaning, it's not ordered, we actually have only one item in the list in that case. I added a comment explaining it

I don't think this warrants a comment, variable fits nice (alleviates us from the need to use the index twice and documents there's just a single item in the list)

orkamara · 2018-12-31T11:17:37Z

src/cli/commands/test.js

+    }
+  } else {
+    for (const versionRange of versionRangeList) {
+      if (semver.valid(pkgVersion) && semver.satisfies(pkgVersion, versionRange)) {


Suggested change

if (semver.valid(pkgVersion) && semver.satisfies(pkgVersion, versionRange)) {

if (!semver.valid(pkgVersion) || !semver.satisfies(pkgVersion, versionRange)) {

continue;

}

I think this is much less readable than the current condition, we want to find the version range that matches our package version, imo it makes more sense to show what we are looking for, not what we're NOT looking for

I guess it's a style opinion - you're choice :)

src/cli/commands/test.js

orkamara · 2018-12-31T11:22:41Z

src/cli/commands/test.js

-      vulnOutput.fixedIn +
-      vulnOutput.extraInfo
-    );
+  var nonBinariesSortedGroupedVulns = sortedGroupedVulns.filter(function (vuln) {


Suggested change

var nonBinariesSortedGroupedVulns = sortedGroupedVulns.filter(function (vuln) {

var sortedGroupedVulns = sortedGroupedVulns.filter(function (vuln) {

Let's not change the all code to be docker related :)

that's the name I first gave and it caused a bug that wasted too much time because that's a variable name we already declared and changing it deletes all binaries vulns from it and then when filtering by upstream package manager we get an empty result...
I can change the order of the lines but that's too error-prone imo, I'll think of another solution but you're welcome to offer one as well.

The problem I'm thinking about is a developer that has no idea about what's binary is, so nonBinariesSortedGroupedVulns might be a bit confusing for him

src/cli/commands/test.js

test/acceptance/cli.acceptance.test.ts

src/cli/commands/test.js

… logic

snyksec · 2019-01-01T10:39:03Z

🎉 This PR is included in version 1.120.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

karniwl self-assigned this Dec 30, 2018

karniwl requested review from darmalovan and orkamara December 30, 2018 13:09

karniwl force-pushed the feat/support-docker-binaries branch from b9f155e to c2c43c8 Compare December 30, 2018 13:19

leongold reviewed Dec 30, 2018

View reviewed changes

karniwl force-pushed the feat/support-docker-binaries branch from 93bcb73 to 04338d3 Compare December 30, 2018 16:20

orkamara reviewed Dec 30, 2018

View reviewed changes

src/cli/commands/test.js Outdated Show resolved Hide resolved

src/cli/commands/test.js Outdated Show resolved Hide resolved

src/cli/commands/test.js Show resolved Hide resolved

leongold reviewed Dec 30, 2018

View reviewed changes

orkamara reviewed Dec 31, 2018

View reviewed changes

karniwl force-pushed the feat/support-docker-binaries branch from 8f089b5 to 1984ea9 Compare December 31, 2018 14:28

karniwl commented Jan 1, 2019

View reviewed changes

src/cli/commands/test.js Show resolved Hide resolved

orkamara approved these changes Jan 1, 2019

View reviewed changes

feat: display fixed in version key & refactor binaries issues display…

80daaf5

… logic

karniwl force-pushed the feat/support-docker-binaries branch from 1984ea9 to 80daaf5 Compare January 1, 2019 10:23

karniwl merged commit bb31229 into master Jan 1, 2019

karniwl deleted the feat/support-docker-binaries branch January 1, 2019 10:30

snyksec added the released label Jan 1, 2019

mmmd03 mentioned this pull request Jun 21, 2023

[Snyk] Fix for 1 vulnerabilities mmmd03/snyk#23

Open

froscheimer mentioned this pull request Jun 21, 2023

[Snyk] Fix for 1 vulnerabilities froscheimer/snyk#305

Open

romathur mentioned this pull request Jun 21, 2023

[Snyk] Fix for 1 vulnerabilities romathur/snyk#180

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: fixed in key in binaries output #317

feat: fixed in key in binaries output #317

karniwl commented Dec 30, 2018 •

edited

Loading

leongold Dec 30, 2018

karniwl Dec 30, 2018

leongold Dec 30, 2018

leongold Dec 30, 2018

karniwl Dec 31, 2018

leongold Dec 31, 2018

orkamara Dec 31, 2018

karniwl Dec 31, 2018

orkamara Jan 1, 2019

orkamara Dec 31, 2018

orkamara Dec 31, 2018

karniwl Dec 31, 2018

orkamara Jan 1, 2019

snyksec commented Jan 1, 2019

-      if (semver.valid(pkgVersion) && semver.satisfies(pkgVersion, versionRange)) {
+       if (!semver.valid(pkgVersion) || !semver.satisfies(pkgVersion, versionRange)) {
+       continue;
+       }

	var nonBinariesSortedGroupedVulns = sortedGroupedVulns.filter(function (vuln) {
	var sortedGroupedVulns = sortedGroupedVulns.filter(function (vuln) {

feat: fixed in key in binaries output #317

feat: fixed in key in binaries output #317

Conversation

karniwl commented Dec 30, 2018 • edited Loading

What does this PR do?

How should this be manually tested?

Screenshots

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snyksec commented Jan 1, 2019

karniwl commented Dec 30, 2018 •

edited

Loading