fix: improve reliability around call graph generation #1276

muscar · 2020-07-17T11:23:24Z

Ready for review
Follows CONTRIBUTING rules
Reviewed by Snyk internal team

What does this PR do?

This is meant to improve the experience around reachable vulns by:

giving control to the CLI on how long the call graph generator can run;
running the user command (e.g. monitor, test) even if the call graph generation failed;
allowing for better error messages is the call graph generation fails.

Merge after snyk/snyk-cli-interface#36 and snyk/snyk-mvn-plugin#73, and after updating the dependencies to the appropriate versions.

What are the relevant tickets?

https://snyksec.atlassian.net/browse/FLOW-336

Screenshots

Running with a timeout:

Running with a timeout and passing -d:

src/lib/monitor/index.ts

darmalovan · 2020-07-19T08:23:11Z

src/lib/types.ts

@@ -91,6 +92,7 @@ export interface MonitorOptions {
 // Used with the Docker plugin only. Allows application scanning.
 'app-vulns'?: boolean;
 reachableVulns?: boolean;
+ callGraphBuilderTimeout?: number;


darmalovan · 2020-07-19T08:24:05Z

@muscar nice! how should the usage of the timeout from the user perspective should look like?

src/lib/types.ts

src/lib/monitor/index.ts

src/lib/snyk-test/run-test.ts

darmalovan · 2020-07-20T14:11:40Z

@muscar can you add screenshot on the pr description?

help/help.txt

darmalovan · 2020-07-20T15:36:40Z

src/lib/monitor/index.ts

@@ -213,7 +218,17 @@ async function monitorDepTree(
 depTree = dropEmptyDeps(depTree);

 let callGraphPayload;
- if (scannedProject.callGraph) {
+ if (options.reachableVulns && scannedProject.callGraph?.innerError) {
+ const err = scannedProject.callGraph as CallGraphError;


I think the as as CallGraphError is not needed now, but maybe help in reading?

Can you elaborate on the different kinds of errors? If it's a timeout error, a permissions error, or something else. Each one will have a different action the user can take before contacting us

@orsagie We don't have enough context in the CLI right now to elaborate--because we only have a generic CallGraphError with a generic string message, and an inner error. Since string messages can change, I don't think inspecting those to glean more info is a good idea, because it leads to brittle code.

I do agree with your point that it's good to provide users with a more detailed reason for the failure so they can provide that info to support.

I see two ways of achieving this:

Change the error message to tell them to re-run the command with -d, and provide the output in their support query; or

Change the error message in the maven plugin to something less generic, and more error specific.

I would personally prefer the first approach as the code in the plugin doesn't have to change as new error cases appear, and we'll be getting almost the same info anyway. What do you think?

Just noticed I didn't comment in the right place, this comment's place is after the request to registry has been made, and regards how we handle those responses.

This error handling is specifically for after a call to the plugins, and in that case I agree with point 1. I think the error from maven plugin already asks to run with -d, can you double check? might not need to change anything here.

The maven plugin doesn't print the -d advice, but I will open a PR to make it do so.

darmalovan · 2020-07-20T15:52:54Z

help/help.txt

@@ -90,6 +90,13 @@ Maven options:
 --scan-all-unmanaged
 Auto detects maven jars and wars in given directory.
 Individual testing can be done with --file=<jar-file-name>
+ --reachable-vulns Try to analyze the application in order to find which of


I see other flags have a new line after the flag

Also, can add like others
(test and monitor command only)

Can you write that is only available for paid users?

I am a little uneasy about adding a flag for paid users only. In the past we have checked against feature flags, but eventually released to all. Maybe instead display an error saying something like This feature is not available for this org, please contact us if you wish to use it?

This is static text - rather not to share this info in the help text?

Then do it in both places. They might not be targeting the correct org?

I thought the plan is to roll this to free users at some point, right? And we already have a feature flag which we check for, and show a message. We could change the feature flag message slightly to say that this is a paid feature only (for now)?

cc/ @orsagie @darmalovan

darmalovan · 2020-07-20T15:54:07Z

help/help.txt

+ --reachable-vulns Try to analyze the application in order to find which of
+ the vulnerabe functions and packages are actually called
+ by client code.
+ --reachable-vulns-timeout The amount of time (in seconds) to wait for the


--reachable-vulns-timeout=<number> ? (and new line)

Is there a case where users want different timeouts for different cases? Consider allowing to set a longer default in the config? (maybe not for now, but in future)

@orsagie I don't know. I think this is highly project specific, so the users are in the best position to tweak it as needed. We provide a default just because we don't want it to block their flows for too long.

darmalovan · 2020-07-20T15:55:43Z

src/cli/args.ts

@@ -218,10 +221,15 @@ export function args(rawArgv: string[]): Args {
 }
 }

+ if (argv.reachableVulnsTimeout === undefined) {
+ argv.reachableVulnsTimeout = REACHABLE_VULNS_TIMEOUT.toString();


Why toString btw?

@darmalovan Because WebStorm (and typescript) complains that the value can't be a number.

src/cli/args.ts

CLAassistant · 2020-07-21T09:27:32Z

All committers have signed the CLA.

help/help.txt

orsagie

small typo, but otherwise the changes LGTM now.

src/lib/monitor/index.ts

github-actions · 2020-07-21T16:07:52Z

Expected release notes (by @muscar)

fixes:
improve reliability around call graph generation (44fc1e8)

I hereby acknowledge these release notes are 🥙 AWESOME 🥙

snyksec · 2020-07-22T07:46:48Z

🎉 This PR is included in version 1.364.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

muscar requested a review from darmalovan July 17, 2020 11:23

darmalovan reviewed Jul 19, 2020

View reviewed changes

src/lib/monitor/index.ts Outdated Show resolved Hide resolved

darmalovan reviewed Jul 19, 2020

View reviewed changes

muscar force-pushed the fix/flow-336-call-graph-generation-reliability-improvements branch from 7fb307a to 34f53d4 Compare July 20, 2020 13:53

muscar marked this pull request as ready for review July 20, 2020 13:53

muscar requested review from a team as code owners July 20, 2020 13:53

ghost requested review from MegaBean and orsagie July 20, 2020 13:53

muscar force-pushed the fix/flow-336-call-graph-generation-reliability-improvements branch from 34f53d4 to 081577a Compare July 20, 2020 13:56