From 9fc944cca7d6308cbf5429d3414c2ba1fb6e38f4 Mon Sep 17 00:00:00 2001 From: Dylan Decrulle Date: Wed, 18 Dec 2024 15:03:41 +0100 Subject: [PATCH 1/5] feat: disable policy feature when 403 or error when fetching and parsing bucket policy --- web/src/core/adapters/s3Client/s3Client.ts | 92 +++++++++++++------ web/src/core/ports/S3Client.ts | 8 +- .../core/usecases/fileExplorer/selectors.ts | 9 +- web/src/core/usecases/fileExplorer/state.ts | 7 +- web/src/core/usecases/fileExplorer/thunks.ts | 17 +++- .../ui/pages/myFiles/Explorer/Explorer.tsx | 9 +- .../ExplorerItems/ExplorerItem.stories.tsx | 2 + .../Explorer/ExplorerItems/ExplorerItem.tsx | 19 ++-- .../ExplorerItems/ExplorerItems.stories.tsx | 2 + .../Explorer/ExplorerItems/ExplorerItems.tsx | 9 +- .../ListExplorerItems.stories.tsx | 1 + .../ListExplorer/ListExplorerItems.tsx | 10 +- web/src/ui/pages/myFiles/MyFiles.tsx | 3 + 13 files changed, 136 insertions(+), 52 deletions(-) diff --git a/web/src/core/adapters/s3Client/s3Client.ts b/web/src/core/adapters/s3Client/s3Client.ts index 16ecec2f3..a0d4a62dd 100644 --- a/web/src/core/adapters/s3Client/s3Client.ts +++ b/web/src/core/adapters/s3Client/s3Client.ts @@ -331,27 +331,26 @@ export function createS3Client( const { awsS3Client } = await getAwsS3Client(); - const bucketPolicyAndAllowedPrefix = await (async () => { - const { GetBucketPolicyCommand } = await import("@aws-sdk/client-s3"); - - let sendResp: import("@aws-sdk/client-s3").GetBucketPolicyCommandOutput; + const resolveBucketPolicy = async () => { + const { GetBucketPolicyCommand, S3ServiceException } = await import( + "@aws-sdk/client-s3" + ); try { - sendResp = await awsS3Client.send( - new GetBucketPolicyCommand({ - Bucket: bucketName - }) + const sendResp = await awsS3Client.send( + new GetBucketPolicyCommand({ Bucket: bucketName }) ); - } catch { - console.log("The error is ok, there is no bucket policy"); - return undefined; - } - if (sendResp.Policy === undefined) { - return undefined; - } + // If the response does not have a policy, return a valid structure + if (!sendResp.Policy) { + console.info("Bucket policy is not defined, but it's okay."); + return { + isBucketPolicyAvailable: true, + bucketPolicy: undefined, + allowedPrefix: [] + }; + } - try { // Validate and parse the policy const parsedPolicy = s3BucketPolicySchema.parse( JSON.parse(sendResp.Policy) @@ -373,18 +372,55 @@ export function createS3Client( resource.replace(`arn:aws:s3:::${bucketName}/`, "") ); - return { bucketPolicy: parsedPolicy, allowedPrefix }; - } catch (e) { - console.warn("The best effort attempt failed to parse the policy", e); - return undefined; - } - })(); + return { + isBucketPolicyAvailable: true, + bucketPolicy: parsedPolicy, + allowedPrefix + }; + } catch (error: unknown) { + if (error instanceof S3ServiceException) { + switch (error.$metadata?.httpStatusCode) { + case 404: + console.info( + "Bucket policy does not exist (404), it's ok." + ); + return { + isBucketPolicyAvailable: true, + bucketPolicy: undefined, + allowedPrefix: [] + }; + case 403: + console.info("Access denied to bucket policy (403)."); + return { + isBucketPolicyAvailable: false, + bucketPolicy: undefined, + allowedPrefix: [] + }; + default: + console.error("An S3 error occurred:", error.message); + return { + isBucketPolicyAvailable: false, + bucketPolicy: undefined, + allowedPrefix: [] + }; + } + } - const { allowedPrefix, bucketPolicy } = bucketPolicyAndAllowedPrefix ?? { - allowedPrefix: [], - bucketPolicy: undefined + console.error( + "An unexpected error occurred when fetching bucket policy", + error + ); + return { + isBucketPolicyAvailable: false, + bucketPolicy: undefined, + allowedPrefix: [] + }; + } }; + const { isBucketPolicyAvailable, allowedPrefix, bucketPolicy } = + await resolveBucketPolicy(); + const Contents: import("@aws-sdk/client-s3")._Object[] = []; const CommonPrefixes: import("@aws-sdk/client-s3").CommonPrefix[] = []; @@ -439,7 +475,11 @@ export function createS3Client( } ); - return { objects: [...directories, ...files], bucketPolicy }; + return { + objects: [...directories, ...files], + bucketPolicy, + isBucketPolicyAvailable + }; }, setPathAccessPolicy: async ({ currentBucketPolicy, policy, path }) => { const { getAwsS3Client } = await prApi; diff --git a/web/src/core/ports/S3Client.ts b/web/src/core/ports/S3Client.ts index 50eaef238..c1c68e09b 100644 --- a/web/src/core/ports/S3Client.ts +++ b/web/src/core/ports/S3Client.ts @@ -33,9 +33,11 @@ export type S3Client = { /** * In charge of creating bucket if doesn't exist. */ - listObjects: (params: { - path: string; - }) => Promise<{ objects: S3Object[]; bucketPolicy: S3BucketPolicy | undefined }>; + listObjects: (params: { path: string }) => Promise<{ + objects: S3Object[]; + bucketPolicy: S3BucketPolicy | undefined; + isBucketPolicyAvailable: boolean; + }>; setPathAccessPolicy: (params: { path: string; diff --git a/web/src/core/usecases/fileExplorer/selectors.ts b/web/src/core/usecases/fileExplorer/selectors.ts index ae7880b9e..e83b0f4f3 100644 --- a/web/src/core/usecases/fileExplorer/selectors.ts +++ b/web/src/core/usecases/fileExplorer/selectors.ts @@ -55,6 +55,7 @@ const commandLogsEntries = createSelector( export type CurrentWorkingDirectoryView = { directoryPath: string; items: CurrentWorkingDirectoryView.Item[]; + isBucketPolicyFeatureEnabled: boolean; }; export namespace CurrentWorkingDirectoryView { @@ -93,12 +94,13 @@ const currentWorkingDirectoryView = createSelector( createSelector(state, state => state.objects), createSelector(state, state => state.ongoingOperations), createSelector(state, state => state.s3FilesBeingUploaded), - + createSelector(state, state => state.isBucketPolicyAvailable), ( directoryPath, objects, ongoingOperations, - s3FilesBeingUploaded + s3FilesBeingUploaded, + isBucketPolicyAvailable ): CurrentWorkingDirectoryView | null => { if (directoryPath === undefined) { return null; @@ -184,7 +186,8 @@ const currentWorkingDirectoryView = createSelector( return { directoryPath, - items + items, + isBucketPolicyFeatureEnabled: isBucketPolicyAvailable }; } ); diff --git a/web/src/core/usecases/fileExplorer/state.ts b/web/src/core/usecases/fileExplorer/state.ts index 439f1d86b..1af9a81e9 100644 --- a/web/src/core/usecases/fileExplorer/state.ts +++ b/web/src/core/usecases/fileExplorer/state.ts @@ -30,6 +30,7 @@ export type State = { resp: string | undefined; }[]; bucketPolicy: S3BucketPolicy; + isBucketPolicyAvailable: boolean; share: | { fileBasename: string; @@ -57,6 +58,7 @@ export const { reducer, actions } = createUsecaseActions({ Version: "2012-10-17", Statement: [] }, + isBucketPolicyAvailable: true, share: undefined }), reducers: { @@ -127,10 +129,12 @@ export const { reducer, actions } = createUsecaseActions({ directoryPath: string; objects: S3Object[]; bucketPolicy: S3BucketPolicy | undefined; + isBucketPolicyAvailable: boolean; }; } ) => { - const { directoryPath, objects, bucketPolicy } = payload; + const { directoryPath, objects, bucketPolicy, isBucketPolicyAvailable } = + payload; state.directoryPath = directoryPath; state.objects = objects; @@ -138,6 +142,7 @@ export const { reducer, actions } = createUsecaseActions({ if (bucketPolicy) { state.bucketPolicy = bucketPolicy; } + state.isBucketPolicyAvailable = isBucketPolicyAvailable; // Properly restore state when navigating back to // a directory with ongoing operations. state.ongoingOperations diff --git a/web/src/core/usecases/fileExplorer/thunks.ts b/web/src/core/usecases/fileExplorer/thunks.ts index 4596a3fb0..161d4daa8 100644 --- a/web/src/core/usecases/fileExplorer/thunks.ts +++ b/web/src/core/usecases/fileExplorer/thunks.ts @@ -147,9 +147,10 @@ const privateThunks = { return r.s3Client; }); - const { objects, bucketPolicy } = await s3Client.listObjects({ - path: directoryPath - }); + const { objects, bucketPolicy, isBucketPolicyAvailable } = + await s3Client.listObjects({ + path: directoryPath + }); if (ctx.completionStatus !== undefined) { dispatch(actions.commandLogCancelled({ cmdId })); @@ -173,7 +174,8 @@ const privateThunks = { actions.navigationCompleted({ directoryPath, objects, - bucketPolicy + bucketPolicy, + isBucketPolicyAvailable }) ); } @@ -279,7 +281,12 @@ export const thunks = { const state = getState()[name]; - const { directoryPath, objects } = state; + const { directoryPath, objects, isBucketPolicyAvailable } = state; + + if (!isBucketPolicyAvailable) { + console.info("Bucket policy is not available"); + return; + } const object = objects.find(o => o.basename === basename && o.kind === kind); diff --git a/web/src/ui/pages/myFiles/Explorer/Explorer.tsx b/web/src/ui/pages/myFiles/Explorer/Explorer.tsx index 69635a72b..930c35a27 100644 --- a/web/src/ui/pages/myFiles/Explorer/Explorer.tsx +++ b/web/src/ui/pages/myFiles/Explorer/Explorer.tsx @@ -62,6 +62,7 @@ export type ExplorerProps = { commandLogsEntries: CommandBarProps.Entry[] | undefined; evtAction: NonPostableEvt<"TRIGGER COPY PATH">; items: Item[]; + isBucketPolicyFeatureEnabled: boolean; onNavigate: (params: { directoryPath: string }) => void; changePolicy: (params: { policy: Item["policy"]; @@ -108,7 +109,7 @@ export const Explorer = memo((props: ExplorerProps) => { pathMinDepth, onViewModeChange, viewMode, - + isBucketPolicyFeatureEnabled, shareView, onShareFileOpen, onShareFileClose, @@ -433,6 +434,9 @@ export const Explorer = memo((props: ExplorerProps) => { onDeleteItem={itemsOnDeleteItem} onShare={onShareDialogOpen} evtAction={evtExplorerItemsAction} + isBucketPolicyFeatureEnabled={ + isBucketPolicyFeatureEnabled + } /> ); case "list": @@ -450,6 +454,9 @@ export const Explorer = memo((props: ExplorerProps) => { onDeleteItems={itemsOnDeleteItems} onShare={onShareDialogOpen} evtAction={evtExplorerItemsAction} + isBucketPolicyFeatureEnabled={ + isBucketPolicyFeatureEnabled + } /> ); } diff --git a/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItem.stories.tsx b/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItem.stories.tsx index 0191264bd..be83d21e8 100644 --- a/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItem.stories.tsx +++ b/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItem.stories.tsx @@ -14,6 +14,7 @@ type Story = StoryObj; export const FileSelected: Story = { args: { + isBucketPolicyFeatureEnabled: true, kind: "file", basename: "example-file.txt", size: 100000000, @@ -31,6 +32,7 @@ export const FileSelected: Story = { export const DirectoryUnselected: Story = { args: { kind: "directory", + isBucketPolicyFeatureEnabled: true, basename: "example-directory", size: undefined, policy: "public", diff --git a/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItem.tsx b/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItem.tsx index 9c11bee80..9489bc933 100644 --- a/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItem.tsx +++ b/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItem.tsx @@ -27,7 +27,7 @@ export type ExplorerItemProps = { /** File size in bytes */ size: number | undefined; - + isBucketPolicyFeatureEnabled: boolean; policy: Item["policy"]; onPolicyChange: () => void; onDoubleClick: () => void; @@ -45,7 +45,8 @@ export const ExplorerItem = memo((props: ExplorerItemProps) => { onDoubleClick, onPolicyChange, onClick, - isPolicyChanging + isPolicyChanging, + isBucketPolicyFeatureEnabled } = props; const prettySize = size ? fileSizePrettyPrint({ bytes: size }) : null; @@ -96,12 +97,14 @@ export const ExplorerItem = memo((props: ExplorerItemProps) => { })()} hasShadow={true} /> - + {isBucketPolicyFeatureEnabled && ( + + )}
diff --git a/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItems.stories.tsx b/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItems.stories.tsx index 3983f4127..daefd70e9 100644 --- a/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItems.stories.tsx +++ b/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItems.stories.tsx @@ -67,6 +67,7 @@ export const Default: Story = { args: { isNavigating: false, items: itemsSample, + isBucketPolicyFeatureEnabled: true, onNavigate: action("Navigate to directory"), onOpenFile: action("Open file"), onDeleteItem: action("Delete item"), @@ -84,6 +85,7 @@ export const EmptyDirectory: Story = { args: { isNavigating: false, items: [], + isBucketPolicyFeatureEnabled: true, onNavigate: action("Navigate to directory"), onOpenFile: action("Open file"), onDeleteItem: action("Delete item"), diff --git a/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItems.tsx b/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItems.tsx index ef0312a90..0f676ce68 100644 --- a/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItems.tsx +++ b/web/src/ui/pages/myFiles/Explorer/ExplorerItems/ExplorerItems.tsx @@ -17,6 +17,7 @@ export type ExplorerItemsProps = { isNavigating: boolean; items: Item[]; + isBucketPolicyFeatureEnabled: boolean; onNavigate: (params: { basename: string }) => void; onOpenFile: (params: { basename: string }) => void; @@ -43,14 +44,15 @@ export const ExplorerItems = memo((props: ExplorerItemsProps) => { className, items, isNavigating, + isBucketPolicyFeatureEnabled, onNavigate, onOpenFile, onSelectedItemKindValueChange, - evtAction, onPolicyChange, onCopyPath, onDeleteItem, - onShare + onShare, + evtAction } = props; const isEmpty = items.length === 0; @@ -158,6 +160,9 @@ export const ExplorerItems = memo((props: ExplorerItemsProps) => { isSelected={selectedItem.basename === basename} size={size} policy={policy} + isBucketPolicyFeatureEnabled={ + isBucketPolicyFeatureEnabled + } onPolicyChange={handlePolicyChange(item)} onClick={handleItemClick(item)} onDoubleClick={handleItemDoubleClick(item)} diff --git a/web/src/ui/pages/myFiles/Explorer/ListExplorer/ListExplorerItems.stories.tsx b/web/src/ui/pages/myFiles/Explorer/ListExplorer/ListExplorerItems.stories.tsx index 0d5a20e71..eb280986c 100644 --- a/web/src/ui/pages/myFiles/Explorer/ListExplorer/ListExplorerItems.stories.tsx +++ b/web/src/ui/pages/myFiles/Explorer/ListExplorer/ListExplorerItems.stories.tsx @@ -67,6 +67,7 @@ export const Default: Story = { args: { isNavigating: false, items: itemsSample, + isBucketPolicyFeatureEnabled: true, onNavigate: action("Navigate to directory"), onOpenFile: action("Open file"), onDeleteItems: action("Delete items"), diff --git a/web/src/ui/pages/myFiles/Explorer/ListExplorer/ListExplorerItems.tsx b/web/src/ui/pages/myFiles/Explorer/ListExplorer/ListExplorerItems.tsx index 675ab37c8..3d4b8fe8f 100644 --- a/web/src/ui/pages/myFiles/Explorer/ListExplorer/ListExplorerItems.tsx +++ b/web/src/ui/pages/myFiles/Explorer/ListExplorer/ListExplorerItems.tsx @@ -29,7 +29,7 @@ export type ListExplorerItemsProps = { isNavigating: boolean; items: Item[]; - + isBucketPolicyFeatureEnabled: boolean; onNavigate: (params: { basename: string }) => void; onOpenFile: (params: { basename: string }) => void; /** Assert initial value is none */ @@ -64,14 +64,15 @@ export const ListExplorerItems = memo((props: ListExplorerItemsProps) => { className, isNavigating, items, + isBucketPolicyFeatureEnabled, onNavigate, - evtAction, onCopyPath, onDeleteItems, onOpenFile, onPolicyChange, onSelectedItemKindValueChange, - onShare + onShare, + evtAction } = props; const apiRef = useGridApiRef(); @@ -313,6 +314,9 @@ export const ListExplorerItems = memo((props: ListExplorerItemsProps) => { initialState={{ pagination: { paginationModel: { pageSize: 25, page: 0 } + }, + columns: { + columnVisibilityModel: { policy: isBucketPolicyFeatureEnabled } } }} isRowSelectable={(params: GridRowParams) => diff --git a/web/src/ui/pages/myFiles/MyFiles.tsx b/web/src/ui/pages/myFiles/MyFiles.tsx index c20d6a38f..f2338f14f 100644 --- a/web/src/ui/pages/myFiles/MyFiles.tsx +++ b/web/src/ui/pages/myFiles/MyFiles.tsx @@ -196,6 +196,9 @@ function MyFiles(props: Props) { commandLogsEntries={commandLogsEntries} evtAction={evtExplorerAction} items={currentWorkingDirectoryView.items} + isBucketPolicyFeatureEnabled={ + currentWorkingDirectoryView.isBucketPolicyFeatureEnabled + } changePolicy={fileExplorer.changePolicy} onNavigate={fileExplorer.changeCurrentDirectory} onRefresh={onRefresh} From 256653b2dca31f54538786109ec684ac8d6b6a0f Mon Sep 17 00:00:00 2001 From: Dylan Decrulle Date: Thu, 19 Dec 2024 09:48:05 +0100 Subject: [PATCH 2/5] remove useless comment --- web/src/core/adapters/s3Client/s3Client.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/web/src/core/adapters/s3Client/s3Client.ts b/web/src/core/adapters/s3Client/s3Client.ts index a0d4a62dd..e85054f9e 100644 --- a/web/src/core/adapters/s3Client/s3Client.ts +++ b/web/src/core/adapters/s3Client/s3Client.ts @@ -341,7 +341,6 @@ export function createS3Client( new GetBucketPolicyCommand({ Bucket: bucketName }) ); - // If the response does not have a policy, return a valid structure if (!sendResp.Policy) { console.info("Bucket policy is not defined, but it's okay."); return { From 7899a0d64029ae7dc57fecffabd9fb113a72b5e9 Mon Sep 17 00:00:00 2001 From: Dylan Decrulle Date: Fri, 10 Jan 2025 10:37:09 +0100 Subject: [PATCH 3/5] review --- web/src/core/adapters/s3Client/s3Client.ts | 74 +++++++++++----------- 1 file changed, 36 insertions(+), 38 deletions(-) diff --git a/web/src/core/adapters/s3Client/s3Client.ts b/web/src/core/adapters/s3Client/s3Client.ts index e85054f9e..34e89b496 100644 --- a/web/src/core/adapters/s3Client/s3Client.ts +++ b/web/src/core/adapters/s3Client/s3Client.ts @@ -336,47 +336,12 @@ export function createS3Client( "@aws-sdk/client-s3" ); + let sendResp: import("@aws-sdk/client-s3").GetBucketPolicyCommandOutput; try { - const sendResp = await awsS3Client.send( + sendResp = await awsS3Client.send( new GetBucketPolicyCommand({ Bucket: bucketName }) ); - - if (!sendResp.Policy) { - console.info("Bucket policy is not defined, but it's okay."); - return { - isBucketPolicyAvailable: true, - bucketPolicy: undefined, - allowedPrefix: [] - }; - } - - // Validate and parse the policy - const parsedPolicy = s3BucketPolicySchema.parse( - JSON.parse(sendResp.Policy) - ); - - // Extract allowed prefixes based on the policy statements - const allowedPrefix = parsedPolicy.Statement.filter( - statement => - statement.Effect === "Allow" && - (statement.Action.includes("s3:GetObject") || - statement.Action.includes("s3:*")) - ) - .flatMap(statement => - Array.isArray(statement.Resource) - ? statement.Resource - : [statement.Resource] - ) - .map(resource => - resource.replace(`arn:aws:s3:::${bucketName}/`, "") - ); - - return { - isBucketPolicyAvailable: true, - bucketPolicy: parsedPolicy, - allowedPrefix - }; - } catch (error: unknown) { + } catch (error) { if (error instanceof S3ServiceException) { switch (error.$metadata?.httpStatusCode) { case 404: @@ -415,6 +380,39 @@ export function createS3Client( allowedPrefix: [] }; } + if (!sendResp.Policy) { + console.info("Bucket policy is not defined, but it's okay."); + return { + isBucketPolicyAvailable: true, + bucketPolicy: undefined, + allowedPrefix: [] + }; + } + + // Validate and parse the policy + const parsedPolicy = s3BucketPolicySchema.parse( + JSON.parse(sendResp.Policy) + ); + + // Extract allowed prefixes based on the policy statements + const allowedPrefix = parsedPolicy.Statement.filter( + statement => + statement.Effect === "Allow" && + (statement.Action.includes("s3:GetObject") || + statement.Action.includes("s3:*")) + ) + .flatMap(statement => + Array.isArray(statement.Resource) + ? statement.Resource + : [statement.Resource] + ) + .map(resource => resource.replace(`arn:aws:s3:::${bucketName}/`, "")); + + return { + isBucketPolicyAvailable: true, + bucketPolicy: parsedPolicy, + allowedPrefix + }; }; const { isBucketPolicyAvailable, allowedPrefix, bucketPolicy } = From d46cbc886d0a011cbbf6f834f83b62865fb1c35e Mon Sep 17 00:00:00 2001 From: Dylan Decrulle Date: Fri, 10 Jan 2025 10:42:49 +0100 Subject: [PATCH 4/5] scope error when parsing --- web/src/core/adapters/s3Client/s3Client.ts | 53 +++++++++++++--------- 1 file changed, 32 insertions(+), 21 deletions(-) diff --git a/web/src/core/adapters/s3Client/s3Client.ts b/web/src/core/adapters/s3Client/s3Client.ts index 34e89b496..b8e44c5d7 100644 --- a/web/src/core/adapters/s3Client/s3Client.ts +++ b/web/src/core/adapters/s3Client/s3Client.ts @@ -389,30 +389,41 @@ export function createS3Client( }; } - // Validate and parse the policy - const parsedPolicy = s3BucketPolicySchema.parse( - JSON.parse(sendResp.Policy) - ); + try { + // Validate and parse the policy + const parsedPolicy = s3BucketPolicySchema.parse( + JSON.parse(sendResp.Policy) + ); - // Extract allowed prefixes based on the policy statements - const allowedPrefix = parsedPolicy.Statement.filter( - statement => - statement.Effect === "Allow" && - (statement.Action.includes("s3:GetObject") || - statement.Action.includes("s3:*")) - ) - .flatMap(statement => - Array.isArray(statement.Resource) - ? statement.Resource - : [statement.Resource] + // Extract allowed prefixes based on the policy statements + const allowedPrefix = parsedPolicy.Statement.filter( + statement => + statement.Effect === "Allow" && + (statement.Action.includes("s3:GetObject") || + statement.Action.includes("s3:*")) ) - .map(resource => resource.replace(`arn:aws:s3:::${bucketName}/`, "")); + .flatMap(statement => + Array.isArray(statement.Resource) + ? statement.Resource + : [statement.Resource] + ) + .map(resource => + resource.replace(`arn:aws:s3:::${bucketName}/`, "") + ); - return { - isBucketPolicyAvailable: true, - bucketPolicy: parsedPolicy, - allowedPrefix - }; + return { + isBucketPolicyAvailable: true, + bucketPolicy: parsedPolicy, + allowedPrefix + }; + } catch (error) { + console.error("Failed to parse bucket policy:", error); + return { + isBucketPolicyAvailable: false, + bucketPolicy: undefined, + allowedPrefix: [] + }; + } }; const { isBucketPolicyAvailable, allowedPrefix, bucketPolicy } = From 9eb7699076c8ccfb9be928083e36544854cc0bf1 Mon Sep 17 00:00:00 2001 From: Dylan Decrulle Date: Fri, 10 Jan 2025 12:08:45 +0100 Subject: [PATCH 5/5] peer --- web/eslint.config.js | 3 +- web/src/core/adapters/s3Client/s3Client.ts | 121 +++++++++--------- .../adapters/s3Client/utils/policySchema.ts | 49 +++++-- web/src/core/ports/S3Client.ts | 4 +- 4 files changed, 101 insertions(+), 76 deletions(-) diff --git a/web/eslint.config.js b/web/eslint.config.js index 050aeccc3..063eb6bfa 100644 --- a/web/eslint.config.js +++ b/web/eslint.config.js @@ -42,7 +42,8 @@ export default tseslint.config( "no-empty": "off", "@typescript-eslint/no-unused-vars": "off", "@typescript-eslint/ban-ts-comment": "off", - "@typescript-eslint/ban-types": "off" + "@typescript-eslint/ban-types": "off", + "@typescript-eslint/no-unused-expressions": "off" } }, ...storybook.configs["flat/recommended"], diff --git a/web/src/core/adapters/s3Client/s3Client.ts b/web/src/core/adapters/s3Client/s3Client.ts index b8e44c5d7..78695a468 100644 --- a/web/src/core/adapters/s3Client/s3Client.ts +++ b/web/src/core/adapters/s3Client/s3Client.ts @@ -10,7 +10,7 @@ import { bucketNameAndObjectNameFromS3Path } from "./utils/bucketNameAndObjectNa import { exclude } from "tsafe/exclude"; import { fnv1aHashToHex } from "core/tools/fnv1aHashToHex"; import { checkIfS3KeyIsPublic } from "core/tools/checkIfS3KeyIsPublic"; -import { s3BucketPolicySchema } from "./utils/policySchema"; +import { zS3BucketPolicy } from "./utils/policySchema"; import { addObjectNameToListBucketCondition, addResourceArnInGetObjectStatement, @@ -342,46 +342,41 @@ export function createS3Client( new GetBucketPolicyCommand({ Bucket: bucketName }) ); } catch (error) { - if (error instanceof S3ServiceException) { - switch (error.$metadata?.httpStatusCode) { - case 404: - console.info( - "Bucket policy does not exist (404), it's ok." - ); - return { - isBucketPolicyAvailable: true, - bucketPolicy: undefined, - allowedPrefix: [] - }; - case 403: - console.info("Access denied to bucket policy (403)."); - return { - isBucketPolicyAvailable: false, - bucketPolicy: undefined, - allowedPrefix: [] - }; - default: - console.error("An S3 error occurred:", error.message); - return { - isBucketPolicyAvailable: false, - bucketPolicy: undefined, - allowedPrefix: [] - }; - } + if (!(error instanceof S3ServiceException)) { + console.error( + "An unknown error occurred when fetching bucket policy", + error + ); + return { + isBucketPolicyAvailable: false, + bucketPolicy: undefined, + allowedPrefix: [] + }; } - console.error( - "An unexpected error occurred when fetching bucket policy", - error - ); + switch (error.$metadata?.httpStatusCode) { + case 404: + console.info("Bucket policy does not exist (404), it's ok."); + return { + isBucketPolicyAvailable: true, + bucketPolicy: undefined, + allowedPrefix: [] + }; + case 403: + console.info("Access denied to bucket policy (403)."); + break; + default: + console.error("An S3 error occurred:", error.message); + break; + } return { isBucketPolicyAvailable: false, bucketPolicy: undefined, allowedPrefix: [] }; } + if (!sendResp.Policy) { - console.info("Bucket policy is not defined, but it's okay."); return { isBucketPolicyAvailable: true, bucketPolicy: undefined, @@ -389,41 +384,49 @@ export function createS3Client( }; } - try { - // Validate and parse the policy - const parsedPolicy = s3BucketPolicySchema.parse( - JSON.parse(sendResp.Policy) - ); + const s3BucketPolicy = (() => { + const s3BucketPolicy = JSON.parse(sendResp.Policy); - // Extract allowed prefixes based on the policy statements - const allowedPrefix = parsedPolicy.Statement.filter( - statement => - statement.Effect === "Allow" && - (statement.Action.includes("s3:GetObject") || - statement.Action.includes("s3:*")) - ) - .flatMap(statement => - Array.isArray(statement.Resource) - ? statement.Resource - : [statement.Resource] - ) - .map(resource => - resource.replace(`arn:aws:s3:::${bucketName}/`, "") - ); + try { + // Validate and parse the policy + zS3BucketPolicy.parse(s3BucketPolicy); + } catch (error) { + console.error("Bucket policy isn't of the expected shape", error); + return undefined; + } - return { - isBucketPolicyAvailable: true, - bucketPolicy: parsedPolicy, - allowedPrefix - }; - } catch (error) { - console.error("Failed to parse bucket policy:", error); + assert(is(s3BucketPolicy)); + + return s3BucketPolicy; + })(); + + if (s3BucketPolicy === undefined) { return { isBucketPolicyAvailable: false, bucketPolicy: undefined, allowedPrefix: [] }; } + + // Extract allowed prefixes based on the policy statements + const allowedPrefix = s3BucketPolicy.Statement.filter( + statement => + statement.Effect === "Allow" && + (statement.Action.includes("s3:GetObject") || + statement.Action.includes("s3:*")) + ) + .flatMap(statement => + Array.isArray(statement.Resource) + ? statement.Resource + : [statement.Resource] + ) + .map(resource => resource.replace(`arn:aws:s3:::${bucketName}/`, "")); + + return { + isBucketPolicyAvailable: true, + s3BucketPolicy, + allowedPrefix + }; }; const { isBucketPolicyAvailable, allowedPrefix, bucketPolicy } = diff --git a/web/src/core/adapters/s3Client/utils/policySchema.ts b/web/src/core/adapters/s3Client/utils/policySchema.ts index f3b081b3f..5bef1b1f9 100644 --- a/web/src/core/adapters/s3Client/utils/policySchema.ts +++ b/web/src/core/adapters/s3Client/utils/policySchema.ts @@ -1,22 +1,43 @@ -import { S3BucketPolicy } from "core/ports/S3Client"; +import type { S3BucketPolicy } from "core/ports/S3Client"; import { assert, Equals } from "tsafe"; import { z } from "zod"; +import { id } from "tsafe/id"; -const s3ActionSchema = z.custom<`s3:${string}`>( +const zS3Action = z.custom<`s3:${string}`>( val => typeof val === "string" && val.startsWith("s3:") ); -const s3PolicyStatementSchema = z.object({ - Effect: z.enum(["Allow", "Deny"]), - Principal: z.union([z.string(), z.object({ AWS: z.string().array() })]), - Action: z.union([s3ActionSchema, s3ActionSchema.array()]), - Resource: z.string().array(), - Condition: z.record(z.any()).optional() -}); +type S3PolicyStatement = S3BucketPolicy["Statement"][number]; -export const s3BucketPolicySchema = z.object({ - Version: z.literal("2012-10-17"), - Statement: z.array(s3PolicyStatementSchema) -}); +const zS3PolicyStatement = (() => { + type TargetType = S3PolicyStatement; -assert, S3BucketPolicy>>(true); + const zTargetType = z.object({ + Effect: z.enum(["Allow", "Deny"]), + Principal: z.union([z.string(), z.object({ AWS: z.string().array() })]), + Action: z.union([zS3Action, zS3Action.array()]), + Resource: z.string().array(), + Condition: z.record(z.any()).optional() + }); + + type InferredType = z.infer; + + assert>; + + return id>(zTargetType); +})(); + +export const zS3BucketPolicy = (() => { + type TargetType = S3BucketPolicy; + + const zTargetType = z.object({ + Version: z.literal("2012-10-17"), + Statement: z.array(zS3PolicyStatement) + }); + + type InferredType = z.infer; + + assert>; + + return id>(zTargetType); +})(); diff --git a/web/src/core/ports/S3Client.ts b/web/src/core/ports/S3Client.ts index c1c68e09b..3129a2532 100644 --- a/web/src/core/ports/S3Client.ts +++ b/web/src/core/ports/S3Client.ts @@ -71,11 +71,11 @@ type s3Action = `s3:${string}`; export type S3BucketPolicy = { Version: "2012-10-17"; - Statement: Array<{ + Statement: { Effect: "Allow" | "Deny"; Principal: string | { AWS: string[] }; Action: s3Action | s3Action[]; Resource: string[]; Condition?: Record; - }>; + }[]; };