Skip to content

Commit

Permalink
Merge pull request #3433 from snyk/feat/container-app-vulns-by-default
Browse files Browse the repository at this point in the history
feat: container app vulns by default
  • Loading branch information
tommyknows committed Jan 25, 2023
2 parents 47f32d7 + 6723568 commit 733d36f
Show file tree
Hide file tree
Showing 9 changed files with 88 additions and 62 deletions.
3 changes: 0 additions & 3 deletions src/cli/commands/monitor/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,6 @@ export default async function monitor(...args0: MethodArgs): Promise<any> {
if (options.docker && options['remote-repo-url']) {
throw new Error('`--remote-repo-url` is not supported for container scans');
}

// TODO remove 'app-vulns' options and warning message once
// https://github.com/snyk/cli/pull/3433 is merged
if (options.docker) {
// order is important here, we want:
// 1) exclude-app-vulns set -> no app vulns
Expand Down
2 changes: 0 additions & 2 deletions src/cli/commands/test/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,6 @@ export default async function test(
throw new MissingArgError();
}

// TODO remove 'app-vulns' options and warning message once
// https://github.com/snyk/cli/pull/3433 is merged
if (options.docker) {
// order is important here, we want:
// 1) exclude-app-vulns set -> no app vulns
Expand Down
75 changes: 49 additions & 26 deletions src/lib/formatters/test/format-test-results.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,11 +176,7 @@ export function getDisplayedOutput(
const fixAdvice = fixTip ? `\n\n${fixTip}` : '';

const dockerfileWarning = getDockerfileWarning(res.scanResult);
const dockerSuggestion = getDockerSuggestionText(
options,
config,
res?.docker?.baseImage,
);
const dockerSuggestion = getDockerSuggestionText(options, res);
const dockerDocsLink = getDockerRemediationDocsLink(dockerAdvice, config);

const vulns = res.vulnerabilities || [];
Expand Down Expand Up @@ -256,31 +252,58 @@ export function dockerUserCTA(options) {
return '';
}

function getDockerSuggestionText(options, config, baseImageRes): string {
if (!options.docker || options.isDockerUser) {
function getDockerSuggestionText(
options: Options & TestOptions,
result: TestResult,
): string {
if (
config?.disableSuggestions === 'true' ||
!options.docker ||
options.isDockerUser
) {
return '';
}

let dockerSuggestion = '';
if (config && config.disableSuggestions !== 'true') {
const optOutSuggestions =
'\n\nTo remove this message in the future, please run `snyk config set disableSuggestions=true`';
if (!options.file) {
if (!baseImageRes) {
dockerSuggestion +=
chalk.bold.white(
'\n\nSnyk wasn’t able to auto detect the base image, use `--file` option to get base image remediation advice.' +
`\nExample: $ snyk container test ${options.path} --file=path/to/Dockerfile`,
) + optOutSuggestions;
}
} else if (!options['exclude-base-image-vulns']) {
dockerSuggestion +=
chalk.bold.white(
'\n\nPro tip: use `--exclude-base-image-vulns` to exclude from display Docker base image vulnerabilities.',
) + optOutSuggestions;
}
const tips: string[] = [];
// exclude-base-image-vulns flag tip
if (options.file && !options['exclude-base-image-vulns']) {
tips.push(
'Pro tip: use `--exclude-base-image-vulns` to exclude from display Docker base image vulnerabilities.',
);
}

// dockerfile flag tip for base image
if (!options.file && !result?.docker?.baseImage) {
tips.push(
`Snyk wasn’t able to auto detect the base image, use \`--file\` option to get base image remediation advice.
Example: $ snyk container test ${options.path} --file=path/to/Dockerfile`,
);
}

// disable-app-vulns flag tip
if (options.docker && result.targetFile && result.uniqueCount > 0) {
tips.push(
'Snyk found some vulnerabilities in your image applications (Snyk searches for these vulnerabilities by default). See https://snyk.co/app-vulns for more information.',
);
}

if (tips.length === 0) {
return '';
}
return dockerSuggestion;

return (
'\n\n' +
tips
// not sure why the tip to disable tips wasn't marked as white, maybe we should change that too?
.map((tip) => chalk.bold.white(tip))
.concat(
// add tip to disable tips
`To remove ${
tips.length > 1 ? 'these messages' : 'this message'
} in the future, please run \`snyk config set disableSuggestions=true\``,
)
.join('\n\n')
);
}
function getDockerfileWarning(scanResult: ScanResult | undefined): string {
if (!scanResult) {
Expand Down
2 changes: 1 addition & 1 deletion test/acceptance/fake-server.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const featureFlagDefaults = (): Map<string, boolean> => {
return new Map([
['cliFailFast', false],
['iacIntegratedExperience', false],
['containerCliAppVulnsEnabled', false],
['containerCliAppVulnsEnabled', true],
]);
};

Expand Down
29 changes: 19 additions & 10 deletions test/jest/acceptance/snyk-test/app-vuln-container-project.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,16 +5,16 @@ import { runSnykCLI } from '../../util/runSnykCLI';
describe('container test projects behavior with --app-vulns, --file and --exclude-base-image-vulns flags', () => {
it('should find nothing when only vulns are in base image', async () => {
const { code, stdout } = await runSnykCLI(
`container test docker-archive:test/fixtures/container-projects/os-app-alpine-and-debug.tar --json --exclude-base-image-vulns`,
`container test docker-archive:test/fixtures/container-projects/os-app-alpine-and-debug.tar --exclude-app-vulns --json --exclude-base-image-vulns`,
);

const jsonOutput = JSON.parse(stdout);
expect(jsonOutput.ok).toEqual(true);
expect(code).toEqual(0);
}, 10000);
it('should find all vulns when using --app-vulns', async () => {
it('should find all vulns including app vulns', async () => {
const { code, stdout } = await runSnykCLI(
`container test docker-archive:test/fixtures/container-projects/os-packages-and-app-vulns.tar --json --experimental --app-vulns`,
`container test docker-archive:test/fixtures/container-projects/os-packages-and-app-vulns.tar --json --experimental`,
);
const jsonOutput = JSON.parse(stdout);

Expand Down Expand Up @@ -48,9 +48,18 @@ describe('container test projects behavior with --app-vulns, --file and --exclud
expect(jsonOutput.uniqueCount).toBeGreaterThan(0);
expect(code).toEqual(1);
}, 10000);
it('should find all vulns when using --app-vulns without experimental flag', async () => {

it('should show app vulns tip when available', async () => {
const { stdout } = await runSnykCLI(
`container test docker-archive:test/fixtures/container-projects/os-packages-and-app-vulns.tar`,
);

expect(stdout).toContain(`Testing docker-archive:test`);
}, 10000);

it('should find all vulns without experimental flag', async () => {
const { code, stdout } = await runSnykCLI(
`container test docker-archive:test/fixtures/container-projects/os-packages-and-app-vulns.tar --json --app-vulns`,
`container test docker-archive:test/fixtures/container-projects/os-packages-and-app-vulns.tar --json`,
);
const jsonOutput = JSON.parse(stdout);

Expand All @@ -68,7 +77,7 @@ describe('container test projects behavior with --app-vulns, --file and --exclud
);

const { code, stdout } = await runSnykCLI(
`container test docker-archive:test/fixtures/container-projects/os-packages-and-app-vulns.tar --json --file=${dockerfilePath} --exclude-base-image-vulns`,
`container test docker-archive:test/fixtures/container-projects/os-packages-and-app-vulns.tar --exclude-app-vulns --json --file=${dockerfilePath} --exclude-base-image-vulns`,
);
const jsonOutput = JSON.parse(stdout);

Expand All @@ -77,13 +86,13 @@ describe('container test projects behavior with --app-vulns, --file and --exclud
expect(code).toEqual(1);
}, 10000);

it('finds dockerfile instructions and app vulns when excluding base image vulns and using --app-vulns', async () => {
it('finds dockerfile instructions and app vulns when excluding base image vulns', async () => {
const dockerfilePath = path.normalize(
'test/fixtures/container-projects/Dockerfile-vulns',
);

const { code, stdout } = await runSnykCLI(
`container test docker-archive:test/fixtures/container-projects/os-packages-and-app-vulns.tar --json --app-vulns --file=${dockerfilePath} --exclude-base-image-vulns`,
`container test docker-archive:test/fixtures/container-projects/os-packages-and-app-vulns.tar --json --file=${dockerfilePath} --exclude-base-image-vulns`,
);
const jsonOutput = JSON.parse(stdout);

Expand All @@ -95,7 +104,7 @@ describe('container test projects behavior with --app-vulns, --file and --exclud
}, 10000);
});

describe('container test projects behavior with --app-vulns, --json flags', () => {
describe('container test projects behavior with --json flag', () => {
let server;
let env: Record<string, string>;

Expand Down Expand Up @@ -128,7 +137,7 @@ describe('container test projects behavior with --app-vulns, --json flags', () =

it('returns a json with the --experimental flags', async () => {
const { code, stdout } = await runSnykCLI(
`container test docker-archive:test/fixtures/container-projects/os-app-alpine-and-debug.tar --app-vulns --json --experimental`,
`container test docker-archive:test/fixtures/container-projects/os-app-alpine-and-debug.tar --json --experimental`,
{
env,
},
Expand Down
2 changes: 0 additions & 2 deletions test/jest/unit/ecosystems-monitor-docker.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,6 @@ describe('monitorEcosystem docker/container', () => {
{
path: '/srv',
docker: true,
'app-vulns': true,
org: 'my-org',
tags: 'keyone=valueone',
},
Expand All @@ -76,7 +75,6 @@ describe('monitorEcosystem docker/container', () => {
{
path: '/srv',
docker: true,
'app-vulns': true,
org: 'my-org',
} as Options,
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,9 @@ Warning: Unable to analyse Dockerfile provided through \`--file\`.
Pro tip: use \`--exclude-base-image-vulns\` to exclude from display Docker base image vulnerabilities.
To remove this message in the future, please run \`snyk config set disableSuggestions=true\`"
Snyk found some vulnerabilities in your image applications (Snyk searches for these vulnerabilities by default). See [URL]
To remove these messages in the future, please run \`snyk config set disableSuggestions=true\`"
`;

exports[`displayResult Docker test result with base image non resolvable warning 1`] = `
Expand Down Expand Up @@ -163,7 +165,9 @@ Warning: Unable to analyse Dockerfile provided through \`--file\`.
Pro tip: use \`--exclude-base-image-vulns\` to exclude from display Docker base image vulnerabilities.
To remove this message in the future, please run \`snyk config set disableSuggestions=true\`"
Snyk found some vulnerabilities in your image applications (Snyk searches for these vulnerabilities by default). See [URL]
To remove these messages in the future, please run \`snyk config set disableSuggestions=true\`"
`;

exports[`displayResult Docker test result with remediation advice 1`] = `
Expand Down
17 changes: 7 additions & 10 deletions test/tap/cli-monitor.acceptance.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1721,7 +1721,7 @@ if (!isWindows) {
[
{
docker: true,
'exclude-app-vulns': true,
'exclude-app-vulns': false,
org: 'explicit-org',
path: 'foo:latest',
},
Expand Down Expand Up @@ -1790,7 +1790,7 @@ if (!isWindows) {
[
{
docker: true,
'exclude-app-vulns': true,
'exclude-app-vulns': false,
file: 'Dockerfile',
org: 'explicit-org',
path: 'foo:latest',
Expand Down Expand Up @@ -1858,7 +1858,7 @@ if (!isWindows) {
[
{
docker: true,
'exclude-app-vulns': true,
'exclude-app-vulns': false,
org: 'explicit-org',
'policy-path': 'custom-location',
path: 'foo:latest',
Expand All @@ -1873,8 +1873,7 @@ if (!isWindows) {
const policyString = req.body.scanResult.policy;
t.deepEqual(policyString, expected, 'sends correct policy');
});

test('`monitor foo:latest --docker` with app vulns feature flag enabled', async (t) => {
test('`monitor foo:latest --docker` with exlude app vulns flag', async (t) => {
chdirWorkspaces('npm-package-policy');
const spyPlugin = stubDockerPluginResponse(
{
Expand All @@ -1894,26 +1893,24 @@ if (!isWindows) {
t,
);

server.setFeatureFlag('containerCliAppVulnsEnabled', true);
await cli.monitor('foo:latest', {
docker: true,
'exclude-app-vulns': true,
org: 'explicit-org',
});
t.same(
spyPlugin.getCall(0).args,
[
{
docker: true,
'exclude-app-vulns': false,
'exclude-app-vulns': true,
org: 'explicit-org',
path: 'foo:latest',
},
],
'calls docker plugin with expected arguments',
);
server.setFeatureFlag('containerCliAppVulnsEnabled', false);
});

test('`monitor foo:latest --docker --platform=linux/arm64`', async (t) => {
const platform = 'linux/arm64';
const spyPlugin = stubDockerPluginResponse(
Expand Down Expand Up @@ -1962,7 +1959,7 @@ if (!isWindows) {
[
{
docker: true,
'exclude-app-vulns': true,
'exclude-app-vulns': false,
path: 'foo:latest',
platform,
},
Expand Down
12 changes: 6 additions & 6 deletions test/tap/cli-test/cli-test.docker.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export const DockerTests: AcceptanceTests = {
[
{
docker: true,
'exclude-app-vulns': true,
'exclude-app-vulns': false,
org: 'explicit-org',
projectName: null,
packageManager: null,
Expand Down Expand Up @@ -142,7 +142,7 @@ export const DockerTests: AcceptanceTests = {
[
{
docker: true,
'exclude-app-vulns': true,
'exclude-app-vulns': false,
org: 'explicit-org',
projectName: null,
packageManager: null,
Expand Down Expand Up @@ -295,7 +295,7 @@ export const DockerTests: AcceptanceTests = {
{
file: 'Dockerfile',
docker: true,
'exclude-app-vulns': true,
'exclude-app-vulns': false,
org: 'explicit-org',
projectName: null,
packageManager: null,
Expand Down Expand Up @@ -410,7 +410,7 @@ export const DockerTests: AcceptanceTests = {
[
{
docker: true,
'exclude-app-vulns': true,
'exclude-app-vulns': false,
org: 'explicit-org',
projectName: null,
packageManager: null,
Expand Down Expand Up @@ -486,7 +486,7 @@ export const DockerTests: AcceptanceTests = {
[
{
docker: true,
'exclude-app-vulns': true,
'exclude-app-vulns': false,
org: 'explicit-org',
projectName: null,
packageManager: null,
Expand Down Expand Up @@ -568,7 +568,7 @@ export const DockerTests: AcceptanceTests = {
[
{
docker: true,
'exclude-app-vulns': true,
'exclude-app-vulns': false,
org: 'explicit-org',
projectName: null,
packageManager: null,
Expand Down

0 comments on commit 733d36f

Please sign in to comment.