From aa2c1ade62ec591f83764f614abcb86c194464fb Mon Sep 17 00:00:00 2001 From: Jamie Epp Date: Tue, 26 Nov 2024 09:46:01 -0800 Subject: [PATCH] feat(storage-browser): validate schema of data received from read APIs --- .../handlers/__tests__/download.spec.ts | 48 ++++++++++++- .../integrity/checkRequiredKeys.spec.ts | 71 +++++++++++++++++++ .../__tests__/listLocationItems.spec.ts | 28 ++++++++ .../handlers/__tests__/listLocations.spec.ts | 34 +++++++++ .../actions/handlers/download.ts | 2 + .../handlers/integrity/checkRequiredKeys.ts | 17 +++++ .../actions/handlers/integrity/index.ts | 1 + .../actions/handlers/listLocationItems.ts | 14 ++++ .../actions/handlers/listLocations.ts | 14 ++++ 9 files changed, 226 insertions(+), 3 deletions(-) create mode 100644 packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/integrity/checkRequiredKeys.spec.ts create mode 100644 packages/react-storage/src/components/StorageBrowser/actions/handlers/integrity/checkRequiredKeys.ts create mode 100644 packages/react-storage/src/components/StorageBrowser/actions/handlers/integrity/index.ts diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/download.spec.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/download.spec.ts index cf602feaf34..3f663d4b130 100644 --- a/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/download.spec.ts +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/download.spec.ts @@ -1,9 +1,8 @@ +/* eslint-disable @typescript-eslint/no-unsafe-argument */ import * as StorageModule from '../../../storage-internal'; import { downloadHandler, DownloadHandlerInput } from '../download'; -const downloadSpy = jest.spyOn(StorageModule, 'getUrl'); - const baseInput: DownloadHandlerInput = { config: { accountId: 'accountId', @@ -23,6 +22,24 @@ const baseInput: DownloadHandlerInput = { }; describe('downloadHandler', () => { + const mockElement: HTMLAnchorElement = { + click: jest.fn(), + } as unknown as HTMLAnchorElement; + + beforeEach(() => { + jest.clearAllMocks(); + jest + .spyOn(StorageModule, 'getUrl') + .mockResolvedValue({ url: new URL('https://mock-url/') } as any); + jest.spyOn(document, 'createElement').mockReturnValue(mockElement); + jest + .spyOn(document.body, 'appendChild') + .mockImplementation((element) => element); + jest + .spyOn(document.body, 'removeChild') + .mockImplementation((element) => element); + }); + it('calls `getUrl` with the expected values', () => { downloadHandler(baseInput); @@ -41,6 +58,31 @@ describe('downloadHandler', () => { }, }; - expect(downloadSpy).toHaveBeenCalledWith(expected); + expect(StorageModule.getUrl).toHaveBeenCalledWith(expected); + }); + + it('should download if getUrl provides a url', async () => { + const result = await downloadHandler(baseInput).result; + + expect(document.createElement).toHaveBeenCalledWith('a'); + expect(mockElement.href).toBe('https://mock-url/'); + expect(mockElement.download).toBe('prefix/file-name'); + expect(mockElement.target).toBe('_blank'); + expect(document.body.appendChild).toHaveBeenCalledWith(mockElement); + expect(mockElement.click).toHaveBeenCalled(); + expect(document.body.removeChild).toHaveBeenCalledWith(mockElement); + + expect(result).toEqual({ status: 'COMPLETE' }); + }); + + it('should fail if getUrl does not return a url', async () => { + jest.spyOn(StorageModule, 'getUrl').mockResolvedValue({} as any); + + const result = await downloadHandler(baseInput).result; + expect(result).toEqual({ + message: + 'Required keys missing for StorageGetUrlOutput: url.\nObject: {}', + status: 'FAILED', + }); }); }); diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/integrity/checkRequiredKeys.spec.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/integrity/checkRequiredKeys.spec.ts new file mode 100644 index 00000000000..eb590918073 --- /dev/null +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/integrity/checkRequiredKeys.spec.ts @@ -0,0 +1,71 @@ +import { checkRequiredKeys } from '../../integrity'; + +interface TestObject { + key1?: string | null; + key2?: string | null; + key3?: string | null; + extraKey?: string | null; +} + +describe('checkRequiredKeys', () => { + it('should not throw when all required keys are present', () => { + const object: TestObject = { + key1: 'value1', + key2: 'value2', + }; + + expect( + checkRequiredKeys(object, 'test-object', ['key1', 'key2']) + ).toBeUndefined(); + }); + + it('should not throw when required keys array is empty', () => { + const object: TestObject = { key1: 'value1', key2: 'value2' }; + + expect(checkRequiredKeys(object, 'test-object', [])).toBeUndefined(); + }); + + it('should not throw when the object has extra keys', () => { + const object: TestObject = { + key1: 'value1', + key2: 'value2', + extraKey: 'extra', + }; + + expect( + checkRequiredKeys(object, 'test-object', ['key1', 'key2']) + ).toBeUndefined(); + }); + + it('should throw when a required key is missing', () => { + const object: TestObject = { key1: 'value1', key3: 'value3' }; + + expect(() => + checkRequiredKeys(object, 'test-object', ['key1', 'key2']) + ).toThrow(); + }); + + it('should throw when object is empty and required keys are specified', () => { + const object: TestObject = {}; + + expect(() => + checkRequiredKeys(object, 'test-object', ['key1', 'key2']) + ).toThrow(); + }); + + it('should throw on null values correctly', () => { + const object: TestObject = { key1: null, key2: 'value2' }; + + expect(() => + checkRequiredKeys(object, 'test-object', ['key1', 'key2']) + ).toThrow(); + }); + + it('should throw on undefined values correctly', () => { + const object: TestObject = { key1: undefined, key2: 'value2' }; + + expect(() => + checkRequiredKeys(object, 'test-object', ['key1', 'key2']) + ).toThrow(); + }); +}); diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/listLocationItems.spec.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/listLocationItems.spec.ts index 84f51da6e81..2c5f5acde4d 100644 --- a/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/listLocationItems.spec.ts +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/listLocationItems.spec.ts @@ -1,3 +1,4 @@ +/* eslint-disable @typescript-eslint/no-unsafe-argument */ import * as StorageModule from '../../../storage-internal'; import { @@ -104,6 +105,33 @@ describe('listLocationItemsHandler', () => { expect(listItems.items).toHaveLength(input.options.pageSize); expect(listSpy).toHaveBeenCalledTimes(2); }); + + it('should throw if list does not provide items', async () => { + listSpy.mockResolvedValueOnce({} as any); + await expect(listLocationItemsHandler(baseInput)).rejects.toThrow( + 'Required keys missing for ListOutput: items.' + ); + }); + + ['path', 'lastModified', 'size'].forEach((requiredKey) => { + it(`should throw if any item is missing ${requiredKey}`, async () => { + listSpy.mockResolvedValueOnce({ + items: [ + { path: `${prefix}-1`, lastModified: new Date(), size: 0 }, + { + path: `${prefix}-2`, + lastModified: new Date(), + size: 0, + [requiredKey]: undefined, + }, + ], + }); + + await expect(listLocationItemsHandler(baseInput)).rejects.toThrow( + `Required keys missing for ListOutputItem #1: ${requiredKey}.` + ); + }); + }); }); describe('parseResult', () => { diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/listLocations.spec.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/listLocations.spec.ts index 60fd72da488..f31e3c3a2ae 100644 --- a/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/listLocations.spec.ts +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/__tests__/listLocations.spec.ts @@ -1,3 +1,4 @@ +/* eslint-disable @typescript-eslint/no-unsafe-argument */ import { listCallerAccessGrants, ListLocationsOutput, @@ -127,4 +128,37 @@ describe('listLocationsHandler', () => { 'Storage Browser: Must provide accountId to `listCallerAccessGrants`.' ); }); + + it('should throw if list does not provide locations', async () => { + input.config.accountId = accountId; + mockListCallerAccessGrants.mockResolvedValueOnce({} as any); + await expect(listLocationsHandler(input)).rejects.toThrow( + 'Required keys missing for ListLocationsOutput: locations.' + ); + }); + + ['scope', 'type', 'permission'].forEach((requiredKey) => { + input.config.accountId = accountId; + it(`should throw if any location is missing ${requiredKey}`, async () => { + mockListCallerAccessGrants.mockResolvedValueOnce({ + locations: [ + { + scope: 's3://bucket/prefix1/*', + permission: 'READWRITE', + type: 'PREFIX', + }, + { + scope: 's3://bucket/prefix1/*', + permission: 'READWRITE', + type: 'PREFIX', + [requiredKey]: undefined, + }, + ], + }); + + await expect(listLocationsHandler(input)).rejects.toThrow( + `Required keys missing for AccessGrantLocation #1: ${requiredKey}.` + ); + }); + }); }); diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/download.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/download.ts index 63cb0f7e3ac..42fb1b8bd87 100644 --- a/packages/react-storage/src/components/StorageBrowser/actions/handlers/download.ts +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/download.ts @@ -1,4 +1,5 @@ import { getUrl } from '../../storage-internal'; +import { checkRequiredKeys } from './integrity'; import { FileDataItem, TaskHandler, @@ -50,6 +51,7 @@ export const downloadHandler: DownloadHandler = ({ expectedBucketOwner: accountId, }, }).then((result) => { + checkRequiredKeys(result, 'StorageGetUrlOutput', ['url']); return result; }); diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/integrity/checkRequiredKeys.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/integrity/checkRequiredKeys.ts new file mode 100644 index 00000000000..b2cc7446efb --- /dev/null +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/integrity/checkRequiredKeys.ts @@ -0,0 +1,17 @@ +export const checkRequiredKeys = ( + objectToCheck: T, + objectName: string, + requiredKeys: Extract[] +): void => { + const missingValues = requiredKeys.filter( + (key) => objectToCheck[key] === undefined || objectToCheck[key] === null + ); + + if (missingValues.length > 0) { + throw Error( + `Required keys missing for ${objectName}: ${[...missingValues].join( + ', ' + )}.\nObject: ${JSON.stringify(objectToCheck)}` + ); + } +}; diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/integrity/index.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/integrity/index.ts new file mode 100644 index 00000000000..1e9abc95f35 --- /dev/null +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/integrity/index.ts @@ -0,0 +1 @@ +export * from './checkRequiredKeys'; diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/listLocationItems.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/listLocationItems.ts index 2fe3c8f38dd..80b5f83525b 100644 --- a/packages/react-storage/src/components/StorageBrowser/actions/handlers/listLocationItems.ts +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/listLocationItems.ts @@ -4,6 +4,7 @@ import { ListPaginateInput, ListOutput, } from '../../storage-internal'; +import { checkRequiredKeys } from './integrity'; import { ListHandler, ListHandlerInput, @@ -90,6 +91,17 @@ export const parseResult = ( prefix ); +const validateResult = (output: ListOutput) => { + checkRequiredKeys(output, `ListOutput`, ['items']); + output.items.forEach((item, i) => + checkRequiredKeys(item, `ListOutputItem #${i}`, [ + 'path', + 'lastModified', + 'size', + ]) + ); +}; + export const listLocationItemsHandler: ListLocationItemsHandler = async ( input ) => { @@ -143,6 +155,8 @@ export const listLocationItemsHandler: ListLocationItemsHandler = async ( }; const output = await list(listInput); + validateResult(output); + nextNextToken = output.nextToken; const items = parseResult(output, prefix); diff --git a/packages/react-storage/src/components/StorageBrowser/actions/handlers/listLocations.ts b/packages/react-storage/src/components/StorageBrowser/actions/handlers/listLocations.ts index b85ba3c3460..d0723f43594 100644 --- a/packages/react-storage/src/components/StorageBrowser/actions/handlers/listLocations.ts +++ b/packages/react-storage/src/components/StorageBrowser/actions/handlers/listLocations.ts @@ -1,8 +1,10 @@ import { listCallerAccessGrants, LocationCredentialsProvider, + ListLocationsOutput as ListCallerAccessGrantsOutput, } from '../../storage-internal'; import { assertAccountId } from '../../validators'; +import { checkRequiredKeys } from './integrity'; import { ListHandlerOptions, @@ -53,6 +55,17 @@ export interface ListLocationsHandlerOutput { export interface ListLocationsHandler extends ListHandler {} +const validateResult = (output: ListCallerAccessGrantsOutput) => { + checkRequiredKeys(output, 'ListLocationsOutput', ['locations']); + output.locations.forEach((location, i) => + checkRequiredKeys(location, `AccessGrantLocation #${i}`, [ + 'scope', + 'type', + 'permission', + ]) + ); +}; + export const listLocationsHandler: ListLocationsHandler = async (input) => { const { config, options } = input; const { accountId, credentials, customEndpoint, region } = config; @@ -74,6 +87,7 @@ export const listLocationsHandler: ListLocationsHandler = async (input) => { pageSize: remainingPageSize, region, }); + validateResult(output); const parsedOutput = getFilteredLocations(output.locations, exclude);