Skip to content

Commit

Permalink
fix: correct remediation broken after last refactoring
Browse files Browse the repository at this point in the history
  • Loading branch information
Konstantin Yegupov committed May 10, 2019
1 parent a948fa4 commit 42c2341
Show file tree
Hide file tree
Showing 6 changed files with 316 additions and 33 deletions.
1 change: 1 addition & 0 deletions src/cli/commands/protect/wizard.js
Expand Up @@ -125,6 +125,7 @@ function processWizardFlow(options) {
});
})
.then(() => {
// We need to have modules information for remediation. See Payload.modules
options.traverseNodeModules = true;

return snyk.test(cwd, options).then((res) => {
Expand Down
23 changes: 18 additions & 5 deletions src/lib/snyk-test/legacy.ts
Expand Up @@ -29,9 +29,18 @@ interface AnnotatedIssue extends IssueData {
isUpgradable: boolean;
isPatchable: boolean;
nearestFixedInVersion?: string;

// These fields present for "node_module" based scans to allow remediation
bundled?: any;
shrinkwrap?: any;
__filename?: string;
parentDepType: string;

dockerfileInstruction?: any;
dockerBaseImage?: any;
}

interface LegacyVulnApiResult {
export interface LegacyVulnApiResult {
vulnerabilities: AnnotatedIssue[];
ok: boolean;
dependencyCount: number;
Expand All @@ -42,8 +51,11 @@ interface LegacyVulnApiResult {
packageManager: string;
ignoreSettings: object | null;
summary: string;
docker?: object;
docker?: {baseImage?: any};
severityThreshold?: string;

filesystemPolicy?: boolean;
uniqueCount?: any;
}

interface UpgradePathItem {
Expand Down Expand Up @@ -80,6 +92,7 @@ interface TestDepGraphResult {
};
docker: {
binariesVulns?: TestDepGraphResult;
baseImage?: any;
};
}

Expand All @@ -96,7 +109,7 @@ interface TestDepGraphMeta {
org: string;
}

interface TestDepGraphResponse {
export interface TestDepGraphResponse {
result: TestDepGraphResult;
meta: TestDepGraphMeta;
}
Expand Down Expand Up @@ -145,7 +158,7 @@ function convertTestDepGraphResultToLegacy(
name: pkgInfo.pkg.name,
version: pkgInfo.pkg.version as string,
nearestFixedInVersion: pkgIssue.fixInfo.nearestFixedInVersion,
});
}) as AnnotatedIssue; // TODO(kyegupov): get rid of type assertion

vulns.push(annotatedIssue);
}
Expand All @@ -168,7 +181,7 @@ function convertTestDepGraphResultToLegacy(
name: pkgInfo.pkg.name,
version: pkgInfo.pkg.version as string,
nearestFixedInVersion: pkgIssue.fixInfo.nearestFixedInVersion,
});
}) as any as AnnotatedIssue; // TODO(kyegupov): get rid of forced type assertion
vulns.push(annotatedIssue);
}
}
Expand Down
63 changes: 35 additions & 28 deletions src/lib/snyk-test/run-test.ts
Expand Up @@ -14,15 +14,30 @@ import request = require('../request');
import snyk = require('../');
import spinner = require('../spinner');
import common = require('./common');
import {DepTree} from '../types';
import gemfileLockToDependencies = require('../../lib/plugins/rubygems/gemfile-lock-to-dependencies');
import {convertTestDepGraphResultToLegacy, AnnotatedIssue} from './legacy';
import {convertTestDepGraphResultToLegacy, AnnotatedIssue, LegacyVulnApiResult, TestDepGraphResponse} from './legacy';
import {SingleDepRootResult, MultiDepRootsResult, isMultiResult, TestOptions} from '../types';

// tslint:disable-next-line:no-var-requires
const debug = require('debug')('snyk');

export = runTest;

interface DepTreeFromResolveDeps extends DepTree {
numDependencies: number;
pluck: any;
}

interface PayloadBody {
depGraph?: depGraphLib.DepGraph; // missing for legacy endpoint (options.vulnEndpoint)
policy: string;
targetFile?: string;
projectNameOverride?: string;
hasDevDependencies?: boolean;
docker?: any;
}

interface Payload {
method: string;
url: string;
Expand All @@ -31,23 +46,13 @@ interface Payload {
'x-is-ci': boolean;
authorization: string;
};
body?: {
depGraph?: depGraphLib.DepGraph, // missing for legacy endpoint (options.vulnEndpoint)
policy: string;
targetFile?: string;
projectNameOverride?: string;
hasDevDependencies?: boolean;
docker?: any;
};
body?: PayloadBody;
qs?: object | null;
modules?: {
numDependencies: number;
pluck: any;
};
modules?: DepTreeFromResolveDeps;
}

async function runTest(packageManager: string, root: string, options): Promise<any[]> {
const results: object[] = [];
async function runTest(packageManager: string, root: string, options): Promise<LegacyVulnApiResult[]> {
const results: LegacyVulnApiResult[] = [];
const spinnerLbl = 'Querying vulnerabilities database...';
try {
const payloads = await assemblePayloads(root, options);
Expand All @@ -62,10 +67,11 @@ async function runTest(packageManager: string, root: string, options): Promise<a
}

await spinner(spinnerLbl);
let res = await sendPayload(payload, hasDevDependencies);
// Type assertion might be a lie, but we are correcting that below
let res = await sendPayload(payload, hasDevDependencies) as LegacyVulnApiResult;
if (depGraph) {
res = convertTestDepGraphResultToLegacy(
res,
res as any as TestDepGraphResponse, // Double "as" required by Typescript for dodgy assertions
depGraph,
packageManager,
options.severityThreshold);
Expand Down Expand Up @@ -100,7 +106,7 @@ async function runTest(packageManager: string, root: string, options): Promise<a
analytics.add('vulns-pre-policy', res.vulnerabilities.length);
res.filesystemPolicy = !!payloadPolicy;
if (!options['ignore-policy']) {
res.policy = res.policy || payloadPolicy;
res.policy = res.policy || payloadPolicy as string;
const policy = await snyk.policy.loadFromText(res.policy);
res = policy.filter(res, root);
}
Expand All @@ -112,7 +118,7 @@ async function runTest(packageManager: string, root: string, options): Promise<a
if (dockerfilePackage) {
vuln.dockerfileInstruction = dockerfilePackage.instruction;
}
vuln.dockerBaseImage = res.docker.baseImage;
vuln.dockerBaseImage = res.docker!.baseImage;
return vuln;
});
}
Expand All @@ -130,7 +136,8 @@ async function runTest(packageManager: string, root: string, options): Promise<a
}
}

function sendPayload(payload: Payload, hasDevDependencies: boolean): Promise<any> {
function sendPayload(payload: Payload, hasDevDependencies: boolean):
Promise<LegacyVulnApiResult | TestDepGraphResponse> {
const filesystemPolicy = payload.body && !!payload.body.policy;
return new Promise((resolve, reject) => {
request(payload, (error, res, body) => {
Expand Down Expand Up @@ -283,7 +290,7 @@ async function assembleLocalPayloads(root, options): Promise<Payload[]> {
}
}

let body: any = {
let body: PayloadBody = {
targetFile: pkg.targetFile || options.file,
projectNameOverride: options.projectName,
policy: policy && policy.toString(),
Expand All @@ -304,13 +311,6 @@ async function assembleLocalPayloads(root, options): Promise<Payload[]> {
body.depGraph = depGraph;
}

if (['yarn', 'npm'].indexOf(options.packageManager) !== -1) {
const isLockFileBased = options.file.endsWith('package-lock.json') || options.file.endsWith('yarn.lock');
if (isLockFileBased && !options.traverseNodeModules) {
body.modules = pkg;
}
}

const payload: Payload = {
method: 'POST',
url: config.API + (options.vulnEndpoint || '/test-dep-graph'),
Expand All @@ -323,6 +323,13 @@ async function assembleLocalPayloads(root, options): Promise<Payload[]> {
body,
};

if (['yarn', 'npm'].indexOf(options.packageManager) !== -1) {
const isLockFileBased = options.file.endsWith('package-lock.json') || options.file.endsWith('yarn.lock');
if (!isLockFileBased || options.traverseNodeModules) {
payload.modules = pkg as DepTreeFromResolveDeps; // See the output of resolve-deps
}
}

payloads.push(payload);
}
return payloads;
Expand Down
1 change: 1 addition & 0 deletions test/acceptance/cli.acceptance.test.ts
Expand Up @@ -989,6 +989,7 @@ test('`test npm-out-of-sync` out of sync fails', async (t) => {
}
});

// TODO(kyegupov): drop this check, we're on node > 4
if (getRuntimeVersion() > 4) {
// yarn lockfile based testing is only supported for node 4+
test('`test yarn-out-of-sync` out of sync fails', async (t) => {
Expand Down
172 changes: 172 additions & 0 deletions test/acceptance/fixtures/npm-package/test-graph-result.json
@@ -0,0 +1,172 @@
{
"result": {
"affectedPkgs": {
"debug@2.2.0": {
"pkg": {
"name": "debug",
"version": "2.2.0"
},
"issues": {
"SNYK-JS-QS-10407": {
"issueId": "SNYK-JS-QS-10407",
"fixInfo": {
"isUpgradable": false,
"isPatchable": true,
"upgradePaths": [
{
"path": [
{
"name": "debug",
"version": "2.2.0"
}
]
}
]
}
}
}
}
},
"issuesData": {
"SNYK-JS-QS-10407": {
"CVSSv3": "CVSS:3.0/AV:N/AC:L/PR:N/UI:N/S:U/C:N/I:H/A:N",
"alternativeIds": [
"SNYK-JS-QS-10407"
],
"creationTime": "2017-02-14T11:44:54.163000Z",
"credit": [
"Snyk Security Research Team"
],
"cvssScore": 7.4,
"description": "## Overview\r\n[`qs`](https://www.npmjs.com/package/qs) is a querystring parser that supports nesting and arrays, with a depth limit.\r\n\r\nBy default `qs` protects against attacks that attempt to overwrite an object's existing prototype properties, such as `toString()`, `hasOwnProperty()`,etc.\r\n\r\nFrom [`qs` documentation](https://github.com/ljharb/qs):\r\n> By default parameters that would overwrite properties on the object prototype are ignored, if you wish to keep the data from those fields either use plainObjects as mentioned above, or set allowPrototypes to true which will allow user input to overwrite those properties. WARNING It is generally a bad idea to enable this option as it can cause problems when attempting to use the properties that have been overwritten. Always be careful with this option.\r\n\r\nOverwriting these properties can impact application logic, potentially allowing attackers to work around security controls, modify data, make the application unstable and more.\r\n\r\nIn versions of the package affected by this vulnerability, it is possible to circumvent this protection and overwrite prototype properties and functions by prefixing the name of the parameter with `[` or `]`. e.g. `qs.parse(\"]=toString\")` will return `{toString = true}`, as a result, calling `toString()` on the object will throw an exception.\r\n\r\n**Example:**\r\n```js\r\nqs.parse('toString=foo', { allowPrototypes: false })\r\n// {}\r\n\r\nqs.parse(\"]=toString\", { allowPrototypes: false })\r\n// {toString = true} <== prototype overwritten\r\n```\r\n\r\nFor more information, you can check out our [blog](https://snyk.io/blog/high-severity-vulnerability-qs/).\r\n\r\n## Disclosure Timeline\r\n- February 13th, 2017 - Reported the issue to package owner.\r\n- February 13th, 2017 - Issue acknowledged by package owner.\r\n- February 16th, 2017 - Partial fix released in versions `6.0.3`, `6.1.1`, `6.2.2`, `6.3.1`.\r\n- March 6th, 2017 - Final fix released in versions `6.4.0`,`6.3.2`, `6.2.3`, `6.1.2` and `6.0.4`\r\n\r\n## Remediation\r\nUpgrade `qs` to version `6.4.0` or higher.\r\n**Note:** The fix was backported to the following versions `6.3.2`, `6.2.3`, `6.1.2`, `6.0.4`.\r\n\r\n## References\r\n- [GitHub Commit](https://github.com/ljharb/qs/commit/beade029171b8cef9cee0d03ebe577e2dd84976d)\r\n- [Report of an insufficient fix](https://github.com/ljharb/qs/issues/200)",
"disclosureTime": "2017-02-13T00:00:00Z",
"functions": [],
"id": "npm:qs:20170213",
"identifiers": {
"ALTERNATIVE": [
"SNYK-JS-QS-10407"
],
"CVE": [
"CVE-2017-1000048"
],
"CWE": [
"CWE-20"
]
},
"language": "js",
"methods": [
{
"methodId": {
"className": "nan",
"filePath": "lib/parse.js",
"methodName": "parseObject"
},
"version": [
"<6.3.2 >=6.3.0",
"<6.2.3 >=6.2.0",
"<6.1.2 >=6.1.0",
"<6.0.4"
]
}
],
"modificationTime": "2018-10-10T14:56:55.142935Z",
"moduleName": "qs",
"packageManager": "npm",
"packageName": "qs",
"patches": [
{
"comments": [],
"id": "patch:npm:qs:20170213:7",
"modificationTime": "2018-09-04T11:57:08.692963Z",
"urls": [
"https://s3.amazonaws.com/snyk-rules-pre-repository/snapshots/master/patches/npm/qs/20170213/603_604.patch"
],
"version": "=6.0.3"
},
{
"comments": [],
"id": "patch:npm:qs:20170213:6",
"modificationTime": "2018-09-04T11:57:08.691571Z",
"urls": [
"https://s3.amazonaws.com/snyk-rules-pre-repository/snapshots/master/patches/npm/qs/20170213/602_604.patch"
],
"version": "=6.0.2"
},
{
"comments": [],
"id": "patch:npm:qs:20170213:5",
"modificationTime": "2018-09-04T11:57:08.690235Z",
"urls": [
"https://s3.amazonaws.com/snyk-rules-pre-repository/snapshots/master/patches/npm/qs/20170213/611_612.patch"
],
"version": "=6.1.1"
},
{
"comments": [],
"id": "patch:npm:qs:20170213:4",
"modificationTime": "2018-09-04T11:57:08.688957Z",
"urls": [
"https://s3.amazonaws.com/snyk-rules-pre-repository/snapshots/master/patches/npm/qs/20170213/610_612.patch"
],
"version": "=6.1.0"
},
{
"comments": [],
"id": "patch:npm:qs:20170213:3",
"modificationTime": "2018-09-04T11:57:08.687714Z",
"urls": [
"https://s3.amazonaws.com/snyk-rules-pre-repository/snapshots/master/patches/npm/qs/20170213/622_623.patch"
],
"version": "=6.2.2"
},
{
"comments": [],
"id": "patch:npm:qs:20170213:2",
"modificationTime": "2018-09-04T11:57:08.686294Z",
"urls": [
"https://s3.amazonaws.com/snyk-rules-pre-repository/snapshots/master/patches/npm/qs/20170213/621_623.patch"
],
"version": "=6.2.1"
},
{
"comments": [],
"id": "patch:npm:qs:20170213:1",
"modificationTime": "2018-09-04T11:57:08.684986Z",
"urls": [
"https://s3.amazonaws.com/snyk-rules-pre-repository/snapshots/master/patches/npm/qs/20170213/631_632.patch"
],
"version": "=6.3.1"
},
{
"comments": [],
"id": "patch:npm:qs:20170213:0",
"modificationTime": "2018-09-04T11:57:08.683816Z",
"urls": [
"https://s3.amazonaws.com/snyk-rules-pre-repository/snapshots/master/patches/npm/qs/20170213/630_632.patch"
],
"version": "=6.3.0"
}
],
"publicationTime": "2017-03-01T10:00:54Z",
"semver": {
"vulnerable": [
"<6.3.2 >=6.3.0 || <6.2.3 >=6.2.0 || <6.1.2 >=6.1.0 || <6.0.4"
]
},
"severity": "high",
"title": "Prototype Override Protection Bypass"
}
}
},
"meta": {
"isPublic": false,
"isLicensesEnabled": false,
"licensesPolicy": null,
"ignoreSettings": null,
"policy": "# Snyk (https://snyk.io) policy file, patches or ignores known vulnerabilities.\nversion: v1.13.1\nignore: {}\n# patches apply the minimum changes required to fix a vulnerability\npatch:\n 'npm:qs:20170213':\n - npm-package-with-git-url > qs:\n patched: '2018-11-04T12:47:13.696Z'\n",
"org": "darmalovan",
"summary": "1 vulnerable dependency path"

},
"filesystemPolicy": true
}

0 comments on commit 42c2341

Please sign in to comment.