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.

Sign up for GitHub

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

fix: extra error handling and debugging [IAC-3138] #5580

Merged
merged 1 commit into from
Nov 15, 2024
Merged

fix: extra error handling and debugging [IAC-3138] #5580

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
20 changes: 20 additions & 0 deletions src/cli/commands/test/iac/scan.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as cloneDeep from 'lodash.clonedeep';
import * as assign from 'lodash.assign';
import * as debugLib from 'debug';

import {
IacFileInDirectory,
Expand Down Expand Up @@ -30,6 +31,8 @@ import { getRepositoryRootForPath } from '../../../../lib/iac/git';
import { getInfo } from '../../../../lib/project-metadata/target-builders/git';
import { buildMeta, GitRepository, GitRepositoryFinder } from './meta';

const debug = debugLib('snyk-iac');

export async function scan(
iacOrgSettings: IacOrgSettings,
options: any,
Expand Down Expand Up @@ -109,6 +112,7 @@ export async function scan(
iacScanFailures = [...iacScanFailures, ...(failures || [])];
iacIgnoredIssuesCount += ignoreCount;
} catch (error) {
debug(`Scan error for path ${path}, details below`);
res = formatTestError(error);
}

Expand Down Expand Up @@ -155,12 +159,17 @@ export async function scan(
function formatTestError(error) {
let errorResponse;
if (error instanceof Error) {
debug(`Error: ${error.name} ${error.message}`);
debug(`Stack trace: ${error.stack}`);
errorResponse = error;
} else if (Array.isArray(error)) {
return error.map(formatTestError);
} else if (typeof error !== 'object') {
debug(`Error value: ${error}`);
errorResponse = new Error(error);
} else {
// we should not get here, but if we do, we want to log the thrown object
debug('Unexpected error object:', safeStringify(error));
try {
errorResponse = JSON.parse(error.message);
} catch (unused) {
Expand All @@ -170,6 +179,17 @@ function formatTestError(error) {
return errorResponse;
}

function safeStringify(obj: unknown): string {
try {
return JSON.stringify(obj);
} catch (e) {
if (e instanceof Error) {
return `Error stringifying object: ${e.message}`;
}
return `Error stringifying object`;
}
}

class CurrentWorkingDirectoryTraversalError extends CustomError {
public filename: string;
public projectRoot: string;
Expand Down
23 changes: 20 additions & 3 deletions src/lib/snyk-test/iac-test-result.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import pick = require('lodash.pick');
import { CustomError } from '../errors';
import { BasicResultData, SEVERITY, TestDepGraphMeta } from './legacy';
import * as debugLib from 'debug';

const debug = debugLib('snyk-iac');

export interface AnnotatedIacIssue {
id: string;
Expand Down Expand Up @@ -58,10 +61,24 @@ const IAC_ISSUES_KEY = 'infrastructureAsCodeIssues';
export function mapIacTestResult(
iacTest: IacTestResponse,
): MappedIacTestResponse | IacTestError {
if (iacTest instanceof CustomError) {
if (iacTest instanceof Error) {
Copy link
Contributor Author

@sergiu-snyk sergiu-snyk Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This is the main fix. The customer log attached to the Jira ticket indicates that an Error is returned in some cases from the policy engine (it should not) and, because the check was done only for CustomError, any Error was allowed to go on the happy path being treated as an actual result and crashing with undefined on prop checking.

This is a fix for a regression introduced here: 4d08086#diff-634d0df2f6eab15512d93b5a3ab1ada33e7146aecd9771844e6f4d503ef3f066L200

return mapIacTestError(iacTest);
}

if (!iacTest.result) {
// This is an unexpected scenario, we should always have a result object,
// but if we don't, we should handle it gracefully.
debug(`invalid scan result: ${iacTest}`);
const errorMessage = iacTest.path
? `Invalid result for ${iacTest.path}`
: 'Invalid result';
return mapIacTestError(
new CustomError(
`${errorMessage}. Please run the command again with the \`-d\` flag and contact [email protected] with the contents of the output`,
),
);
}
Copy link
Contributor Author

@sergiu-snyk sergiu-snyk Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This condition, where a result is not set, should never happen anymore if all errors are caught by the fix above. But if it still does, we log a user friendly message instead of letting it crash trying to access props for an inexistent result.


const infrastructureAsCodeIssues =
iacTest?.result?.cloudConfigResults.map(mapIacIssue) || [];
const {
Expand All @@ -76,10 +93,10 @@ export function mapIacTestResult(
};
}

export function mapIacTestError(error: CustomError) {
export function mapIacTestError(error: Error) {
return {
ok: false,
code: error.code,
code: error instanceof CustomError ? error.code : undefined,
error: error.message,
path: (error as any).path,
};
Expand Down
Loading