Merge pull request #179 from snyk/fix/improve-error-handling-code-report

fix: improve report errors [ZEN-541]

src/http.ts

@@ -39,13 +39,18 @@ function isSubsetErrorCode<T>(code: any, messages: { [c: number]: string }): cod
   return false;
 }
 
-function generateError<E>(errorCode: number, messages: { [c: number]: string }, apiName: string): ResultError<E> {
+function generateError<E>(
+  errorCode: number,
+  messages: { [c: number]: string },
+  apiName: string,
+  error?: string,
+): ResultError<E> {
   if (!isSubsetErrorCode<E>(errorCode, messages)) {
     throw { errorCode, messages, apiName };
   }
 
   const statusCode = errorCode;
-  const statusText = messages[errorCode];
+  const statusText = error ?? messages[errorCode];
 
   return {
     type: 'error',
@@ -379,6 +384,22 @@ export async function getAnalysis(
   return generateError<GetAnalysisErrorCodes>(res.errorCode, GET_ANALYSIS_ERROR_MESSAGES, 'getAnalysis');
 }
 
+export type ReportErrorCodes =
+  | GenericErrorTypes
+  | ErrorCodes.unauthorizedUser
+  | ErrorCodes.unauthorizedBundleAccess
+  | ErrorCodes.badRequest
+  | ErrorCodes.notFound;
+
+const REPORT_ERROR_MESSAGES: { [P in ReportErrorCodes]: string } = {
+  ...GENERIC_ERROR_MESSAGES,
+  [ErrorCodes.unauthorizedUser]: DEFAULT_ERROR_MESSAGES[ErrorCodes.unauthorizedUser],
+  [ErrorCodes.unauthorizedBundleAccess]: DEFAULT_ERROR_MESSAGES[ErrorCodes.unauthorizedBundleAccess],
+  [ErrorCodes.notFound]: DEFAULT_ERROR_MESSAGES[ErrorCodes.notFound],
+  [ErrorCodes.badRequest]: DEFAULT_ERROR_MESSAGES[ErrorCodes.badRequest],
+  [ErrorCodes.serverError]: 'Getting report failed',
+};
+
 export interface UploadReportOptions extends GetAnalysisOptions {
   report: ReportOptions;
 }
@@ -394,7 +415,7 @@ export type UploadReportResponseDto = ReportResult | AnalysisFailedResponse | An
 
 export async function initReport(
   options: UploadReportOptions,
-): Promise<Result<InitUploadResponseDto, GetAnalysisErrorCodes>> {
+): Promise<Result<InitUploadResponseDto, ReportErrorCodes>> {
   const config: Payload = {
     headers: {
       ...commonHttpHeaders(options),
@@ -420,12 +441,10 @@ export async function initReport(
 
   const res = await makeRequest<InitUploadResponseDto>(config);
   if (res.success) return { type: 'success', value: res.body };
-  return generateError<GetAnalysisErrorCodes>(res.errorCode, GET_ANALYSIS_ERROR_MESSAGES, 'initReport');
+  return generateError<ReportErrorCodes>(res.errorCode, REPORT_ERROR_MESSAGES, 'initReport');
 }
 
-export async function getReport(
-  options: GetReportOptions,
-): Promise<Result<UploadReportResponseDto, GetAnalysisErrorCodes>> {
+export async function getReport(options: GetReportOptions): Promise<Result<UploadReportResponseDto, ReportErrorCodes>> {
   const config: Payload = {
     headers: {
       ...commonHttpHeaders(options),
@@ -436,7 +455,7 @@ export async function getReport(
 
   const res = await makeRequest<UploadReportResponseDto>(config);
   if (res.success) return { type: 'success', value: res.body };
-  return generateError<GetAnalysisErrorCodes>(res.errorCode, GET_ANALYSIS_ERROR_MESSAGES, 'getReport');
+  return generateError<ReportErrorCodes>(res.errorCode, REPORT_ERROR_MESSAGES, 'getReport', res.error?.message);
 }
 
 export function getVerifyCallbackUrl(authHost: string): string {

src/needle.ts

@@ -107,6 +107,11 @@ export async function makeRequest<T = void>(
       emitter.apiRequestLog(`Requested url --> ${url} | error --> ${err}`);
     }
 
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment, @typescript-eslint/no-unsafe-member-access
+    const errorMessage = response?.body?.error;
+    if (errorMessage) {
+      error = error ?? new Error(errorMessage);
+    }
     errorCode = errorCode ?? ErrorCodes.serviceUnavailable;
 
     // Try to avoid breaking requests due to temporary network errors

src/report.ts

@@ -18,11 +18,17 @@ const sleep = (duration: number) => new Promise(resolve => setTimeout(resolve, d
 async function pollReport(
   options: UploadReportOptions,
 ): Promise<Result<AnalysisFailedResponse | ReportResult, GetAnalysisErrorCodes>> {
-  // Return early if project name is not provided.
   const projectName = options.report?.projectName?.trim();
+  const projectNameMaxLength = 64;
   if (!projectName || projectName.length === 0) {
     throw new Error('"project-name" must be provided for "report"');
   }
+  if (projectName.length > projectNameMaxLength) {
+    throw new Error(`"project-name" must not exceed ${projectNameMaxLength} characters`);
+  }
+  if (/[^A-Za-z0-9-_/]/g.test(projectName)) {
+    throw new Error(`"project-name" must not contain spaces or special characters except [/-_]`);
+  }
 
   emitter.analyseProgress({
     status: AnalysisStatus.waiting,

tests/report.spec.ts

@@ -2,11 +2,13 @@ import path from 'path';
 import { baseURL, sessionToken, source } from './constants/base';
 import { reportBundle } from '../src/report';
 import * as http from '../src/http';
+import * as needle from '../src/needle';
 import { sampleProjectPath, initReportReturn, getReportReturn } from './constants/sample';
 
 const mockInitReport = jest.spyOn(http, 'initReport');
 mockInitReport.mockReturnValue(Promise.resolve({ type: 'success', value: initReportReturn }));
-jest.spyOn(http, 'getReport').mockReturnValue(Promise.resolve({ type: 'success', value: getReportReturn }));
+const mockGetReport = jest.spyOn(http, 'getReport');
+mockGetReport.mockReturnValue(Promise.resolve({ type: 'success', value: getReportReturn }));
 
 describe('Functional test for report', () => {
   const paths: string[] = [path.join(sampleProjectPath)];
@@ -51,12 +53,76 @@ describe('Functional test for report', () => {
       projectName: undefined,
     };
 
-    expect(async () => {
-      await reportBundle({
+    await expect(
+      reportBundle({
         bundleHash: 'dummy-bundle',
         ...baseConfig,
         report: reportConfig,
-      });
-    }).rejects.toThrowError('"project-name" must be provided for "report"');
+      }),
+    ).rejects.toHaveProperty('message', '"project-name" must be provided for "report"');
+  });
+
+  it('should fail report if the given project name exceeds the maximum length', async () => {
+    const longProjectName = 'a'.repeat(65);
+    const reportConfig = {
+      enabled: true,
+      projectName: longProjectName,
+    };
+
+    await expect(
+      reportBundle({
+        bundleHash: 'dummy-bundle',
+        ...baseConfig,
+        report: reportConfig,
+      }),
+    ).rejects.toHaveProperty('message', `"project-name" must not exceed 64 characters`);
+  });
+
+  it('should fail report if the given project name includes invalid characters', async () => {
+    const invalidProjectName = '*&^%$';
+    const reportConfig = {
+      enabled: true,
+      projectName: invalidProjectName,
+    };
+
+    await expect(
+      reportBundle({
+        bundleHash: 'dummy-bundle',
+        ...baseConfig,
+        report: reportConfig,
+      }),
+    ).rejects.toHaveProperty('message', `"project-name" must not contain spaces or special characters except [/-_]`);
+  });
+
+  it('getReport should return error with received error message rather than generic error message', async () => {
+    const statusCode = 400;
+    const expectedErrorMessage = 'Dummy received error message';
+
+    mockGetReport.mockRestore();
+    jest
+      .spyOn(needle, 'makeRequest')
+      .mockReturnValue(
+        Promise.resolve({ success: false, errorCode: statusCode, error: new Error(expectedErrorMessage) }),
+      );
+
+    const reportConfig = {
+      enabled: true,
+      projectName: 'test-project',
+      targetName: 'test-target',
+      targetRef: 'test-ref',
+      remoteRepoUrl: 'https://github.com/owner/repo',
+    };
+
+    await expect(
+      reportBundle({
+        bundleHash: 'dummy-bundle',
+        ...baseConfig,
+        report: reportConfig,
+      }),
+    ).rejects.toMatchObject({
+      apiName: 'getReport',
+      statusCode: statusCode,
+      statusText: expectedErrorMessage,
+    });
   });
 });