fix: ensure nested legacycli invocations forward errors

[CatalinSnyk]

Pull Request Submission Checklist

• Follows CONTRIBUTING guidelines
• Includes detailed description of changes
• Contains risk assessment (Low | Medium | High)
• Highlights breaking API changes (if applicable)
• Links to automated tests covering new functionality
• Includes manual testing instructions (if necessary)
• Updates relevant GitBook documentation (PR link: ___)

What does this PR do?

Following the work from CLI-646 that added the capability of capturing errors from TS CLI invocations, this adds PR aims to ensure nested invocations of the TS CLI are forwarded properly to debug logs, analytics and output.

One current gap left from CLI-646 is the fact that if the --sarif or --json flags are set, we don't send this errors to the IPC. We can retrieve error messages from the JSON payload, but in the case of SARIF, there is no error message present.
Extracting the error message from the JSON payload is just about parsing and accessing the "error" field. I added a fallback case and in case parsing fails, we default back to the normal Error.message field.

Here's an example of an error caught when the --json flag is set:

Error{
    message: "{\"ok\":false,\"error\":\"This is the actual error message\",\"path":\"./\"}"
    ...otherFields
}

Where should the reviewer start?

• main.ts - Removed JSON from the list of IPC exceptions, and moved the output case above the rest, just to simplify the conditions a bit and since the TS CLI will always output the errors in JSON format, while also sending them to the IPC.
• ipc.ts - Added a new argument to the sendError function: isJson is set when the --json flag is set, this will try extracting the error message from the stringified JSON payload from the messsage field of the caught error. Also included the IAC JSON output case where they return an array of error objects.
• cliv2.go - Changes are related to the output of the error coming from the IPC: if the JSON flag is set and the invocation uses STDIO, the error already was displayed by the TS CLI. But if STDIO is not used, than the error can be presented back to the user. Also added test cases around the decision table between STDIO and JSON config values.
• main.go - The output workflow configuration values were not set correctly in some cases because the flags were not added as persistent. The other thing added here was generating the right error message for the JSON output; if the error is Error Catalog, the proper message is the detail field.

How should this be manually tested?

Using the snyk-macos-arm64 sbom --format cyclonedx1.4+json --file=./foldernotthere command, the expected error message would surface the fact that the specified file is not present, instead of the "An error occurred while running the underlying analysis needed to generate the SBOM." message. Other commands that make use of TS CLI workflow invocations should also surface the errors captured though the IPC.

Related

• CLI-696

[github-actions]

	Warnings
⚠️	Since the CLI is unifying on a standard and improved tooling, we're starting to migrate old-style imports and exports to ES6 ones. A file you've modified is using either module.exports or require(). If you can, please update them to ES6 import syntax and export syntax. Files found: src/cli/ipc.ts src/cli/main.ts src/lib/iac/test/v2/output.ts test/setup-jest.ts
⚠️	There are multiple commits on your branch, please squash them locally before merging!

Generated by 🚫 dangerJS against 10f2590

[thisislawatts]

issue: Use imports here rather than commonjs require format.

[PeterSchafer]

question: @CatalinSnyk what about this comment?

[CatalinSnyk]

This would require a version bump for the dependency, and it was used in a lot of places around the code and tests, some commands also had it scoped under a different version than the main package.json; so I think it should not be included in this PR.

[thisislawatts]

Question: What is the incorrect behaviour you were seeing which required this change? I am not sure I understand how it is related to the change you are looking to make here?

[CatalinSnyk]

The configuration values were not being populated properly (eg: even if the --json flag was in the args, the configuration flag for the JSON output would have the default value of false).
This would result in incorrect output for errors sent thought IPC: we would get the normal human readable output even if JSON formatted output was expected.

[PeterSchafer]

This can probably be removed

[CatalinSnyk]

Note: this is still needed to properly populate the configuration, since we need it for the cliv2/internal/cliv2/cliv2.go:

useSTDIO := c.globalConfig.GetBool(configuration.WORKFLOW_USE_STDIO)
jsonEnabled := c.globalConfig.GetBool(output_workflow.OUTPUT_CONFIG_KEY_JSON)
hasBeenDisplayed := false
if useSTDIO && jsonEnabled {
    hasBeenDisplayed = true
}

If you remove it and test against snyk-macos-arm64 test --print-graph --file=./nothere --json or snyk-macos-arm64 iac test ./nothere ./notthere --json, but there would be json output coming from the TS CLI and human readable output from Golang.

[thisislawatts]

I removed these changes locally @CatalinSnyk, but have not yet been able recreate the behaviour you described.

-       // add output flags as persistent flags
-       _ = rootCommand.ParseFlags(os.Args)
-       globalConfiguration.AddFlagSet(rootCommand.LocalFlags())

~/snyk/cli/binary-releases/snyk-macos iac test ./notthere --json

Output

{
  "ok": false,
  "code": 1010,
  "error": "Could not find any valid IaC files",
  "path": "./notthere"
}

[CatalinSnyk]

Are you sure you had the newest changes pulled? With that exact lines removed, I just clean built it locally and i get the following:

{
  "ok": false,
  "code": 1010,
  "error": "Could not find any valid IaC files",
  "path": "./nothere"
}

 ERROR   Unspecified Error (SNYK-CLI-0000)
Info:    Could not find any valid IaC files
Docs:    https://docs.snyk.io/scan-with-snyk/error-catalog#snyk-cli-0000```

[CatalinSnyk]

This should be the only remaining changes to main.

[PeterSchafer]

Suggestion: This should probably live in the cliv2 package, just considering the commend and the imports

[CatalinSnyk]

If I do move it, I would also need to take the error chain iterator with it. Or maybe it's worth to relocate this error handling package somewhere out of the main package so it can be used around easier?

[j-luong]

suggestion: will need to remove formatting using stripAnsi here:

Suggested change

	new CLI.GeneralIACFailureError(formatFailuresList(allTestFailures)),
	new CLI.GeneralIACFailureError(stripAnsi(formatFailuresList(allTestFailures))),

[PeterSchafer]

I assume that FormattedCustomError is a customer error, so we might be able to do the assignment and stripping of formatting more centrally, similar to the CustomError.