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

Feat: snyk fix v1 (Python) in behind FF #1707

Merged
merged 9 commits into from
Mar 30, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .circleci/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ jobs:
- run:
name: Lerna Publish
command: |
lerna publish minor --yes --no-push --no-git-tag-version
lerna publish minor --yes --no-push --no-git-tag-version --exact
- run:
name: Install osslsigncode
command: sudo apt-get install -y osslsigncode
Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
"@snyk/cli-interface": "2.11.0",
"@snyk/code-client": "3.1.5",
"@snyk/dep-graph": "^1.27.1",
"@snyk/fix": "1.501.0",
"@snyk/gemfile": "1.2.0",
"@snyk/graphlib": "^2.1.9-patch.3",
"@snyk/inquirer": "^7.3.3-patch",
Expand Down Expand Up @@ -106,6 +107,7 @@
"micromatch": "4.0.2",
"needle": "2.6.0",
"open": "^7.0.3",
"ora": "5.3.0",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

snyk fix & @snyk/fix uses Ora, intention is for this to be the future and only spinner.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm trying to figure out why is nothing failing, if the @snyk/fix is not defined in package.json 🤔 I guess we need a new CI check to make sure that no npm linked dependencies are used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just added it now via npm i --save @snyk/fix is the version as expected?

"os-name": "^3.0.0",
"promise-queue": "^2.2.5",
"proxy-agent": "^3.1.1",
Expand Down
15 changes: 15 additions & 0 deletions src/cli/commands/fix/convert-legacy-test-result-to-new.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { DepGraphData } from '@snyk/dep-graph';
import { TestResult } from '../../../lib/ecosystems/types';
import { TestResult as LegacyTestResult } from '../../../lib/snyk-test/legacy';

export function convertLegacyTestResultToNew(
testResult: LegacyTestResult,
): TestResult {
return {
issuesData: {} as any, // TODO: add converter
issues: [], // TODO: add converter
remediation: testResult.remediation,
// TODO: grab this once Ecosystems flow starts sending back ScanResult
depGraphData: {} as DepGraphData,
};
}
25 changes: 25 additions & 0 deletions src/cli/commands/fix/convert-legacy-test-result-to-scan-result.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
import { ScanResult } from '../../../lib/ecosystems/types';
import { TestResult } from '../../../lib/snyk-test/legacy';

export function convertLegacyTestResultToScanResult(
testResult: TestResult,
): ScanResult {
if (!testResult.packageManager) {
throw new Error(
'Only results with packageManagers are supported for conversion',
);
}
return {
identity: {
type: testResult.packageManager,
// this is because not all plugins send it back today, but we should always have it
targetFile: testResult.targetFile || testResult.displayTargetFile,
},
name: testResult.projectName,
// TODO: grab this once Ecosystems flow starts sending back ScanResult
facts: [],
policy: testResult.policy,
// TODO: grab this once Ecosystems flow starts sending back ScanResult
target: {} as any,
};
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import * as fs from 'fs';
import * as pathLib from 'path';
import { convertLegacyTestResultToNew } from './convert-legacy-test-result-to-new';
import { convertLegacyTestResultToScanResult } from './convert-legacy-test-result-to-scan-result';
import { TestResult } from '../../../lib/snyk-test/legacy';

export function convertLegacyTestResultToFixEntities(
testResults: (TestResult | TestResult[]) | Error,
root: string,
): any {
if (testResults instanceof Error) {
return [];
}
const oldResults = Array.isArray(testResults) ? testResults : [testResults];
return oldResults.map((res) => ({
workspace: {
readFile: async (path: string) => {
return fs.readFileSync(pathLib.resolve(root, path), 'utf8');
},
writeFile: async (path: string, content: string) => {
return fs.writeFileSync(pathLib.resolve(root, path), content, 'utf8');
},
},
scanResult: convertLegacyTestResultToScanResult(res),
testResult: convertLegacyTestResultToNew(res),
}));
}
13 changes: 13 additions & 0 deletions src/cli/commands/fix/get-display-path.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import * as pathLib from 'path';

import { isLocalFolder } from '../../../lib/detect';

export function getDisplayPath(path: string): string {
if (!isLocalFolder(path)) {
return path;
}
if (path === process.cwd()) {
return '.';
}
return pathLib.relative(process.cwd(), path);
}
109 changes: 109 additions & 0 deletions src/cli/commands/fix/index.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,109 @@
export = fix;

import * as Debug from 'debug';
import * as snykFix from '@snyk/fix';
import * as ora from 'ora';

import { MethodArgs } from '../../args';
import * as snyk from '../../../lib';
import { TestResult } from '../../../lib/snyk-test/legacy';

import { convertLegacyTestResultToFixEntities } from './convert-legacy-tests-results-to-fix-entities';
import { formatTestError } from '../test/format-test-error';
import { processCommandArgs } from '../process-command-args';
import { validateCredentials } from '../test/validate-credentials';
import { validateTestOptions } from '../test/validate-test-options';
import { setDefaultTestOptions } from '../test/set-default-test-options';
import { validateFixCommandIsSupported } from './validate-fix-command-is-supported';
import { Options, TestOptions } from '../../../lib/types';
import { getDisplayPath } from './get-display-path';

const debug = Debug('snyk-fix');
const snykFixFeatureFlag = 'cliSnykFix';

interface FixOptions {
dryRun?: boolean;
quiet?: boolean;
}
async function fix(...args: MethodArgs): Promise<string> {
const { options: rawOptions, paths } = await processCommandArgs<FixOptions>(
...args,
);
const options = setDefaultTestOptions<FixOptions>(rawOptions);
debug(options);
await validateFixCommandIsSupported(options);
validateTestOptions(options);
validateCredentials(options);
const results: snykFix.EntityToFix[] = [];
results.push(...(await runSnykTestLegacy(options, paths)));

// fix
debug(
`Organization has ${snykFixFeatureFlag} feature flag enabled for experimental Snyk fix functionality`,
);
const { dryRun, quiet } = options;
const { fixSummary, meta } = await snykFix.fix(results, { dryRun, quiet });
if (meta.fixed === 0) {
throw new Error(fixSummary);
}
return fixSummary;
}

/* @deprecated
* TODO: once project envelope is default all code below will be deleted
* we should be calling test via new Ecosystems instead
*/
async function runSnykTestLegacy(
options: Options & TestOptions & FixOptions,
paths: string[],
): Promise<snykFix.EntityToFix[]> {
const results: snykFix.EntityToFix[] = [];
const stdOutSpinner = ora({
isSilent: options.quiet,
stream: process.stdout,
});
const stdErrSpinner = ora({
isSilent: options.quiet,
stream: process.stdout,
});
stdErrSpinner.start();
stdOutSpinner.start();

for (const path of paths) {
let displayPath = path;
try {
displayPath = getDisplayPath(path);
stdOutSpinner.info(`Running \`snyk test\` for ${displayPath}`);
// Create a copy of the options so a specific test can
// modify them i.e. add `options.file` etc. We'll need
// these options later.
const snykTestOptions = {
...options,
path,
projectName: options['project-name'],
};

const testResults: TestResult[] = [];

const testResultForPath: TestResult | TestResult[] = await snyk.test(
path,
{ ...snykTestOptions, quiet: true },
);
testResults.push(
...(Array.isArray(testResultForPath)
? testResultForPath
: [testResultForPath]),
);
const newRes = convertLegacyTestResultToFixEntities(testResults, path);
results.push(...newRes);
} catch (error) {
const testError = formatTestError(error);
const userMessage = `Test for ${displayPath} failed with error: ${testError.message}.\nRun \`snyk test ${displayPath} -d\` for more information.`;
stdErrSpinner.fail(userMessage);
debug(userMessage);
}
}
stdOutSpinner.stop();
stdErrSpinner.stop();
return results;
}
36 changes: 36 additions & 0 deletions src/cli/commands/fix/validate-fix-command-is-supported.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import * as Debug from 'debug';

import { getEcosystemForTest } from '../../../lib/ecosystems';

import { isFeatureFlagSupportedForOrg } from '../../../lib/feature-flags';
import { CommandNotSupportedError } from '../../../lib/errors/command-not-supported';
import { FeatureNotSupportedByEcosystemError } from '../../../lib/errors/not-supported-by-ecosystem';
import { Options, TestOptions } from '../../../lib/types';

const debug = Debug('snyk-fix');
const snykFixFeatureFlag = 'cliSnykFix';

export async function validateFixCommandIsSupported(
options: Options & TestOptions,
): Promise<boolean> {
if (options.docker) {
throw new FeatureNotSupportedByEcosystemError('snyk fix', 'docker');
}

const ecosystem = getEcosystemForTest(options);
if (ecosystem) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Am I missing something here? We will always throw FeatureNotSupportedByEcosystemError unless the ecosystem is null in which case we do support it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thats correct, snyk fix v1 doesn't support any ecosystems only old style test for now. So only if this something with a packageManager via old test flow

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. So a null ecosystem means that we are using the old test flow? Maybe a comment would help to make this clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

throw new FeatureNotSupportedByEcosystemError('snyk fix', ecosystem);
}

const snykFixSupported = await isFeatureFlagSupportedForOrg(
snykFixFeatureFlag,
options.org,
);

if (!snykFixSupported.ok) {
debug(snykFixSupported.userMessage);
throw new CommandNotSupportedError('snyk fix', options.org || undefined);
}

return true;
}
1 change: 1 addition & 0 deletions src/cli/commands/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ const commands = {
ignore: hotload('./ignore'),
modules: hotload('./modules'),
monitor: hotload('./monitor'),
fix: hotload('./fix'),
policy: hotload('./policy'),
protect: hotload('./protect'),
test: hotload('./test'),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export function generateSnykTestError(error) {
export function formatTestError(error) {
// Possible error cases:
// - the test found some vulns. `error.message` is a
// JSON-stringified
Expand Down
11 changes: 7 additions & 4 deletions src/cli/commands/test/index.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
export = test;

import * as Debug from 'debug';
import * as pathLib from 'path';
const cloneDeep = require('lodash.clonedeep');
const assign = require('lodash.assign');
import chalk from 'chalk';

import * as snyk from '../../../lib';
import { isCI } from '../../../lib/is-ci';
import * as Debug from 'debug';
import * as pathLib from 'path';
import {
IacFileInDirectory,
Options,
Expand Down Expand Up @@ -51,10 +52,10 @@ import {

import { test as iacTest } from './iac-test-shim';
import { validateCredentials } from './validate-credentials';
import { generateSnykTestError } from './generate-snyk-test-error';
import { validateTestOptions } from './validate-test-options';
import { setDefaultTestOptions } from './set-default-test-options';
import { processCommandArgs } from '../process-command-args';
import { formatTestError } from './format-test-error';

const debug = Debug('snyk-test');
const SEPARATOR = '\n-------------------------------------------------------\n';
Expand Down Expand Up @@ -108,7 +109,9 @@ async function test(...args: MethodArgs): Promise<TestCommandResult> {
res = await snyk.test(path, testOpts);
}
} catch (error) {
res = generateSnykTestError(error);
// not throwing here but instead returning error response
// for legacy flow reasons.
res = formatTestError(error);
}

// Not all test results are arrays in order to be backwards compatible
Expand Down
4 changes: 3 additions & 1 deletion src/cli/commands/test/set-default-test-options.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import * as config from '../../../lib/config';
import { Options, ShowVulnPaths, TestOptions } from '../../../lib/types';

export function setDefaultTestOptions(options: Options): Options & TestOptions {
export function setDefaultTestOptions<CommandOptions>(
options: Options & CommandOptions,
): Options & TestOptions & CommandOptions {
const svpSupplied = (options['show-vulnerable-paths'] || '')
.toString()
.toLowerCase();
Expand Down
2 changes: 2 additions & 0 deletions src/lib/ecosystems/types.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { DepGraphData } from '@snyk/dep-graph';
import { RemediationChanges } from '../snyk-test/legacy';
import { Options } from '../types';

export type Ecosystem = 'cpp' | 'docker' | 'code';
Expand Down Expand Up @@ -71,6 +72,7 @@ export interface TestResult {
issues: Issue[];
issuesData: IssuesData;
depGraphData: DepGraphData;
remediation?: RemediationChanges;
}

export interface EcosystemPlugin {
Expand Down
17 changes: 17 additions & 0 deletions src/lib/errors/command-not-supported.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import { CustomError } from './custom-error';

export class CommandNotSupportedError extends CustomError {
public readonly command: string;
public readonly org?: string;

constructor(command: string, org?: string) {
super(`${command} is not supported for org ${org}.`);
this.code = 422;
this.command = command;
this.org = org;

this.userMessage = `\`${command}\` is not supported ${
org ? `for org '${org}'` : ''
}`;
}
}
18 changes: 18 additions & 0 deletions src/lib/errors/not-supported-by-ecosystem.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
import { CustomError } from './custom-error';
import { SupportedPackageManagers } from '../package-managers';
import { Ecosystem } from '../ecosystems/types';

export class FeatureNotSupportedByEcosystemError extends CustomError {
public readonly feature: string;

constructor(
feature: string,
ecosystem: SupportedPackageManagers | Ecosystem,
) {
super(`Unsupported ecosystem ${ecosystem} for ${feature}.`);
this.code = 422;
this.feature = feature;

this.userMessage = `\`${feature}\` is not supported for ecosystem '${ecosystem}'`;
}
}
2 changes: 1 addition & 1 deletion src/lib/plugins/get-deps-from-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export async function getDepsFromPlugin(
root,
);

if (!options.json && userWarningMessage) {
if (!options.json && !options.quiet && userWarningMessage) {
Copy link
Contributor Author

@lili2311 lili2311 Mar 16, 2021

Choose a reason for hiding this comment

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

snyk fix calls snyk test, we want to control the snyk fix output to only be what the command spinner wants to display. So calling snyk fix in --quiet mode, however it looks like while we support the param we did not honour it.

console.warn(chalk.bold.red(userWarningMessage));
}
return inspectRes;
Expand Down
4 changes: 3 additions & 1 deletion src/lib/snyk-test/assemble-payloads.ts
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,9 @@ export async function assembleEcosystemPayloads(
path.relative('..', '.') + ' project dir');

spinner.clear<void>(spinnerLbl)();
await spinner(spinnerLbl);
if (!options.quiet) {
await spinner(spinnerLbl);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

snyk fix calls snyk test, we want to control the snhyk fix output to only be what the command spinner wants to display. So calling snyk fix in --quiet mode, however it looks like while we support the param we did not honour it.


try {
const plugin = getPlugin(ecosystem);
Expand Down
Loading