fix: do not show invalidSeverityThreshold error for HTTP 400 responses

[gergo-papp]

• Follows CONTRIBUTING rules

What does this PR do?

Fixes a misleading error message.

In c2399ae we assumed that all 400 responses returned by our backends are due to an incorrect severity threshold (eg snyk test --severity-threshold=non-sense).

This is incorrect - the CLI calls into many different backend services and we can't assume anything about the reason of the failure.

Where should the reviewer start?

First understand how error messages are parsed/formatted here:

cli/src/lib/errors/legacy-errors.js

Lines 82 to 106 in 351c7bd

	// try to lookup the error string based either on the error code OR
	// the actual error.message (which can be "Unauthorized" for instance),
	// otherwise send the error message back
	message =
	error.userMessage ||
	codes[error.code || error.message] ||
	errors[error.code || error.message];
	if (message) {
	if (error instanceof FormattedCustomError) {
	message = error.formattedUserMessage;
	} else {
	message = message.replace(/(%s)/g, error.message).trim();
	message = chalk.bold.red(message);
	}
	} else if (error.code) {
	// means it's a code error
	message =
	'An unknown error occurred. Please run with `-d` and include full trace ' +
	'when reporting to Snyk';
	analytics.add('unknown-error-code', JSON.stringify(error));
	} else {
	// should be one of ours
	message = error.message;
	}
	}

Then it should be clear that this line doesn't really make sense:

cli/src/lib/errors/legacy-errors.js

Line 56 in 351c7bd

400: errors.invalidSeverityThreshold,

If we do not provide a value for 400 above, then we'll use a generic error message, which should be more appropiate.

How should this be manually tested?

1. Force the backend to return a 400 error
   • I did this by changing generateMonitorDependenciesRequest to return an invalid request
2. run snyk container monitor node:latest, the result is Invalid severity threshold, please use one of low | medium | high | critical
3. run this again, but on my branch, the result is: An unknown error occurred. Please run with -d and include full trace when reporting to Snyk
4. running again, but with -d gives: the following result (this is not usper nice, but is still better than giving misleading info)

An unknown error occurred. Please run with `-d` and include full trace when reporting to Snyk
gergo-papp@YFT40J7F32 cli % ./bin/snyk container monitor node:latest -d
...
MonitorError: Server returned unexpected error for the monitor request. Status code: 400
    at monitorDependencies (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/lib/ecosystems/monitor.ts:168:17)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)
    at selectAndExecuteMonitorStrategy (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/lib/ecosystems/monitor.ts:98:7)
    at Object.monitorEcosystem (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/lib/ecosystems/monitor.ts:81:36)
    at monitor (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/cli/commands/monitor/index.ts:144:27)
    at runCommand (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/cli/main.ts:51:25)
    at main (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/cli/main.ts:310:11)
    at /Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/cli/index.ts:13:3
    at Object.callHandlingUnexpectedErrors (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/lib/unexpected-error.ts:28:5)
  snyk Exit code: 2 +0ms
  snyk analytics {
...
  "command": "bad-command",
  "metadata": {
    "policies": 1,
    "policyLocations": [
      "node:latest"
    ],
    "error-message": "Server returned unexpected error for the monitor request. Status code: 400",
    "error": "MonitorError: Server returned unexpected error for the monitor request. Status code: 400\n    at monitorDependencies (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/lib/ecosystems/monitor.ts:168:17)\n    at processTicksAndRejections (node:internal/process/task_queues:95:5)\n    at selectAndExecuteMonitorStrategy (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/lib/ecosystems/monitor.ts:98:7)\n    at Object.monitorEcosystem (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/lib/ecosystems/monitor.ts:81:36)\n    at monitor (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/cli/commands/monitor/index.ts:144:27)\n    at runCommand (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/cli/main.ts:51:25)\n    at main (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/cli/main.ts:310:11)\n    at /Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/cli/index.ts:13:3\n    at Object.callHandlingUnexpectedErrors (/Users/gergo-papp/Development/GitHub/snyk/cli/dist/cli/webpack:/snyk/src/lib/unexpected-error.ts:28:5)",
    "error-code": 400,
    "command": "monitor"
  },

Any background context you want to provide?

Part of a customer ticket - the customer uses a proxy that returns a 400 error. Instead of showing the actual error we display a totally misleading message:

Invalid severity threshold, please use one of low | medium | high | critical

What are the relevant tickets?

• https://snyk.zendesk.com/agent/tickets/59541
• https://snyksec.atlassian.net/browse/SUP-2006

Screenshots

N/A

Additional questions

N/A

[gergo-papp]

Warnings
⚠️
You've modified files in src/ directory, but haven't updated anything in test folder. Is there something that could be tested?

Generated by 🚫 dangerJS against 849aac0

there are no existing tests for this and there's no big behavioral change

[shlomiSnyk]

there are no existing tests for this and there's no big behavioral change

I would consider adding a test here.. the fact that there are no existing tests doesn't mean we should continue with it.
Also.. i feel like adding a test\s with meaningful names would also contribute to make it clearer on why this change was introduced and specifically what's the scenario when 400 is returned.

[gergo-papp]

there are no existing tests for this and there's no big behavioral change

I would consider adding a test here.. the fact that there are no existing tests doesn't mean we should continue with it. Also.. i feel like adding a test\s with meaningful names would also contribute to make it clearer on why this change was introduced and specifically what's the scenario when 400 is returned.

I added new tests for the entire file

[bastiandoetsch]

question: so I may be missing something, but how are invalid severity thresholds now reported? And where can I find an Acceptance Tests for that?

[gergo-papp]

question: so I may be missing something, but how are invalid severity thresholds now reported? And where can I find an Acceptance Tests for that?

There's an explicit INVALID_SEVERITY_THRESHOLD/InvalidSeverityThreshold error. See this query for examples.

I've tested and running snyk test --severity-threshold=non-sense still return the existing error and here's an acceptance tests that covers it:

cli/test/tap/severity-threshold.test.ts

Line 47 in 351c7bd

test('"snyk test --severity-threshold=non-sense"', async (t) => {

I think the same error was added for 400 errors many years ago by mistake, so this should not influence the severity errors - but it will fix HTTP 400 bad requests.

See here for some additional manual tests:

[bastiandoetsch]

nitpick: that referenced test doesn't fail if the severity does NOT throw an error. Approving now, though.