fix(storage): axios handler error handling fix (aws-amplify#9587)

packages/storage/__tests__/providers/axios-http-handler.test.ts

@@ -3,11 +3,10 @@ import {
 	AxiosHttpHandler,
 	reactNativeRequestTransformer,
 } from '../../src/providers/axios-http-handler';
-import { HttpRequest } from '@aws-sdk/protocol-http';
+import { HttpRequest, HttpResponse } from '@aws-sdk/protocol-http';
 import { Platform, Logger } from '@aws-amplify/core';
 
 jest.mock('axios');
-jest.useFakeTimers();
 
 let request: HttpRequest;
 
@@ -25,10 +24,15 @@ describe('AxiosHttpHandler', () => {
 			port: 3000,
 			query: {},
 			headers: {},
-			clone: () => (null as unknown) as HttpRequest,
+			clone: () => null as unknown as HttpRequest,
 		};
 	});
 
+	afterEach(() => {
+		jest.clearAllMocks();
+		jest.resetAllMocks();
+	});
+
 	describe('.handle', () => {
 		it('should remove unsafe header host', async () => {
 			const handler = new AxiosHttpHandler();
@@ -146,6 +150,7 @@ describe('AxiosHttpHandler', () => {
 		});
 
 		it('should timeout after requestTimeout', async () => {
+			jest.useFakeTimers();
 			const handler = new AxiosHttpHandler({ requestTimeout: 1000 });
 			const req = handler.handle(request, options);
 			expect(setTimeout).toHaveBeenCalledTimes(1);
@@ -162,8 +167,11 @@ describe('AxiosHttpHandler', () => {
 				.mockImplementation(() => Promise.reject(new Error('err')));
 			const loggerSpy = jest.spyOn(Logger.prototype, '_log');
 			const handler = new AxiosHttpHandler();
-			await handler.handle(request, options);
-			expect(loggerSpy).toHaveBeenCalledWith('ERROR', 'err');
+			try {
+				await handler.handle(request, options);
+			} catch (_error) {
+				expect(loggerSpy).toHaveBeenCalledWith('ERROR', 'err');
+			}
 		});
 
 		it('cancel request should throw error', async () => {
@@ -177,6 +185,43 @@ describe('AxiosHttpHandler', () => {
 				'err'
 			);
 		});
+
+		it('unexpected error without a response object should be re-thrown', async () => {
+			axios.request = jest
+				.fn()
+				.mockImplementationOnce(() =>
+					Promise.reject(new Error('Unexpected error!'))
+				);
+			const handler = new AxiosHttpHandler();
+			await expect(handler.handle(request, options)).rejects.toThrowError(
+				'Unexpected error!'
+			);
+		});
+
+		it('error with response object should be converted to a HttpResponse', async () => {
+			const expectedHeaders = {
+				foo: 'bar',
+			};
+			const expectedStatusCode = 400;
+			const expectedBody = 'body';
+			axios.request = jest.fn().mockImplementationOnce(() =>
+				Promise.reject({
+					response: {
+						status: expectedStatusCode,
+						headers: expectedHeaders,
+						data: expectedBody,
+					},
+				})
+			);
+			const handler = new AxiosHttpHandler();
+			const result = await handler.handle(request, options);
+			expect(result.response).toBeInstanceOf(HttpResponse);
+			expect(result.response).toEqual({
+				statusCode: expectedStatusCode,
+				headers: expectedHeaders,
+				body: expectedBody,
+			});
+		});
 	});
 
 	describe('React Native Request Transformer', () => {

packages/storage/src/providers/axios-http-handler.ts

@@ -29,10 +29,24 @@ const logger = new Logger('axios-http-handler');
 export const SEND_UPLOAD_PROGRESS_EVENT = 'sendUploadProgress';
 export const SEND_DOWNLOAD_PROGRESS_EVENT = 'sendDownloadProgress';
 
+export type ErrorWithResponse = {
+	response: { status: number } & { [key: string]: any };
+};
+
 function isBlob(body: any): body is Blob {
 	return typeof Blob !== 'undefined' && body instanceof Blob;
 }
 
+function hasErrorResponse(error: any): error is ErrorWithResponse {
+	return (
+		typeof error !== 'undefined' &&
+		Object.prototype.hasOwnProperty.call(error, 'response') &&
+		typeof error.response !== 'undefined' &&
+		Object.prototype.hasOwnProperty.call(error.response, 'status') &&
+		typeof error.response.status === 'number'
+	);
+}
+
 const normalizeHeaders = (
 	headers: Record<string, string>,
 	normalizedName: string
@@ -186,15 +200,19 @@ export class AxiosHttpHandler implements HttpHandler {
 						logger.error(error.message);
 					}
 					// for axios' cancel error, we should re-throw it back so it's not considered an s3client error
-					// if we return empty, or an abitrary error HttpResponse, it will be hard to debug down the line
-					if (axios.isCancel(error)) {
+					// if we return empty, or an abitrary error HttpResponse, it will be hard to debug down the line.
+					//
+					// for errors that does not have a 'response' object, it's very likely that it is an unexpected error for
+					// example a disconnect. Without it we cannot meaningfully reconstruct a HttpResponse, and the AWS SDK might
+					// consider the request successful by mistake. In this case we should also re-throw the error.
+					if (axios.isCancel(error) || !hasErrorResponse(error)) {
 						throw error;
 					}
 					// otherwise, we should re-construct an HttpResponse from the error, so that it can be passed down to other
 					// aws sdk middleware (e.g retry, clock skew correction, error message serializing)
 					return {
 						response: new HttpResponse({
-							statusCode: error.response?.status,
+							statusCode: error.response.status,
 							body: error.response?.data,
 							headers: error.response?.headers,
 						}),