From 10fe89fb18fa96a686071966a84a5691e856eef9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=A9=AC=E7=99=BB=E5=B1=B1?= Date: Tue, 28 Jul 2026 17:49:30 +0800 Subject: [PATCH] fix: handle readonly bucket states safely --- app/(dashboard)/browser/content.tsx | 13 ++-- app/(dashboard)/browser/page.tsx | 20 ++---- components/buckets/list.tsx | 19 ++---- components/data-table/data-table.tsx | 12 ++++ components/object/list.tsx | 75 +++++++++++------------ lib/bucket-access.ts | 16 +++++ lib/bucket-policy-status.ts | 23 +++++++ tests/lib/bucket-access.test.ts | 17 +++++ tests/lib/bucket-policy-status.test.ts | 27 ++++++++ tests/lib/data-table-error-source.test.js | 12 ++++ tests/lib/object-delete-safety.test.js | 3 +- tests/lib/object-list-source.test.js | 6 +- 12 files changed, 162 insertions(+), 81 deletions(-) create mode 100644 lib/bucket-access.ts create mode 100644 lib/bucket-policy-status.ts create mode 100644 tests/lib/bucket-access.test.ts create mode 100644 tests/lib/bucket-policy-status.test.ts create mode 100644 tests/lib/data-table-error-source.test.js diff --git a/app/(dashboard)/browser/content.tsx b/app/(dashboard)/browser/content.tsx index 1eaeb0ea..109b4f9b 100644 --- a/app/(dashboard)/browser/content.tsx +++ b/app/(dashboard)/browser/content.tsx @@ -18,6 +18,7 @@ import { useTasks } from "@/contexts/task-context" import { ObjectPreviewModal } from "@/components/object/preview-modal" import { useObject } from "@/hooks/use-object" import { usePermissions } from "@/hooks/use-permissions" +import { isMissingBucketError } from "@/lib/bucket-access" interface BrowserContentProps { bucketName: string @@ -51,15 +52,9 @@ export function BrowserContent({ bucketName, keyPath = "", preview = false, prev headBucket(bucketName) .then(() => {}) .catch((error: unknown) => { - const err = error as { $metadata?: { httpStatusCode?: number }; Code?: string; message?: string } - const status = err?.$metadata?.httpStatusCode - const code = (err?.Code ?? (error as Error)?.message ?? "").toLowerCase() - const isAccessDenied = - status === 403 || - code === "accessdenied" || - code === "forbidden" || - (typeof code === "string" && (code.includes("access denied") || code.includes("forbidden"))) - message.error(isAccessDenied ? t("Access Denied") : t("Bucket not found")) + if (!isMissingBucketError(error)) return + + message.error(t("Bucket not found")) const params = new URLSearchParams(searchParams.toString()) params.delete("bucket") params.delete("prefix") diff --git a/app/(dashboard)/browser/page.tsx b/app/(dashboard)/browser/page.tsx index e38ae19b..f4eddc4d 100644 --- a/app/(dashboard)/browser/page.tsx +++ b/app/(dashboard)/browser/page.tsx @@ -21,6 +21,7 @@ import { useMessage } from "@/lib/feedback/message" import { formatDateTime, formatInteger, niceBytes } from "@/lib/functions" import { normalizeDateToIso } from "@/lib/safe-date" import { getAccountBucketUsage } from "@/lib/account-bucket-usage" +import { loadBucketPolicyStatuses } from "@/lib/bucket-policy-status" import { BrowserContent } from "./content" import type { ColumnDef } from "@tanstack/react-table" @@ -58,26 +59,13 @@ function BrowserBucketsPage() { } try { - const results = await Promise.all( - bucketNames.map(async (name) => { - try { - const resp = (await getBucketPolicyStatus(name)) as { - PolicyStatus?: { IsPublic?: boolean } - } - return { name, isPublic: resp?.PolicyStatus?.IsPublic === true } - } catch { - return { name, isPublic: false } - } - }), - ) + const policyMap = await loadBucketPolicyStatuses(bucketNames, getBucketPolicyStatus) if (fetchId !== fetchIdRef.current) return - const policyMap = Object.fromEntries(results.map((r) => [r.name, r.isPublic])) - setData((prev) => prev.map((row) => ({ ...row, - IsPublic: policyMap[row.Name] ?? false, + IsPublic: policyMap[row.Name], })), ) } catch { @@ -226,7 +214,7 @@ function BrowserBucketsPage() { {t("Private")} ) } - return policyLoading ? : "--" + return policyLoading ? : "-" }, }) diff --git a/components/buckets/list.tsx b/components/buckets/list.tsx index e389b845..1fb05855 100644 --- a/components/buckets/list.tsx +++ b/components/buckets/list.tsx @@ -14,6 +14,7 @@ import { Spinner } from "@/components/ui/spinner" import { formatDateTime, formatInteger, niceBytes } from "@/lib/functions" import { normalizeDateToIso } from "@/lib/safe-date" import { getAccountBucketUsage } from "@/lib/account-bucket-usage" +import { loadBucketPolicyStatuses } from "@/lib/bucket-policy-status" import type { ColumnDef } from "@tanstack/react-table" export interface BucketListRow { @@ -50,25 +51,13 @@ export function BucketList({ title, emptyDescription, getBucketHref }: BucketLis } try { - const results = await Promise.all( - bucketNames.map(async (name) => { - try { - const resp = (await getBucketPolicyStatus(name)) as { - PolicyStatus?: { IsPublic?: boolean } - } - return { name, isPublic: resp?.PolicyStatus?.IsPublic === true } - } catch { - return { name, isPublic: false } - } - }), - ) + const policyMap = await loadBucketPolicyStatuses(bucketNames, getBucketPolicyStatus) if (fetchId !== fetchIdRef.current) return - const policyMap = Object.fromEntries(results.map((r) => [r.name, r.isPublic])) setData((prev) => prev.map((row) => ({ ...row, - IsPublic: policyMap[row.Name] ?? false, + IsPublic: policyMap[row.Name], })), ) } finally { @@ -202,7 +191,7 @@ export function BucketList({ title, emptyDescription, getBucketHref }: BucketLis {t("Private")} ) } - return policyLoading ? : "--" + return policyLoading ? : "-" }, }, ], diff --git a/components/data-table/data-table.tsx b/components/data-table/data-table.tsx index 8935a486..d564498a 100644 --- a/components/data-table/data-table.tsx +++ b/components/data-table/data-table.tsx @@ -12,6 +12,8 @@ import { cn } from "@/lib/utils" interface DataTableProps { table: Table isLoading?: boolean + errorTitle?: string + errorDescription?: string emptyTitle?: string emptyDescription?: string caption?: string @@ -67,6 +69,8 @@ function getAriaSort(column: Column): React.AriaAttribute export function DataTable({ table, isLoading = false, + errorTitle, + errorDescription, emptyTitle = "No data", emptyDescription = "There is nothing to display yet.", caption, @@ -132,6 +136,14 @@ export function DataTable({ + ) : errorTitle ? ( + + +
+ +
+ + ) : hasRows ? ( table.getRowModel().rows.map((row) => ( (null) const [data, setData] = React.useState([]) const [nextToken, setNextToken] = React.useState() const [showScrollShortcuts, setShowScrollShortcuts] = React.useState(false) const [bucketVersioningState, setBucketVersioningState] = React.useState("unknown") - const [versioningError, setVersioningError] = React.useState("") - const [versioningReload, setVersioningReload] = React.useState(0) const [deleteDialogOpen, setDeleteDialogOpen] = React.useState(false) const [deleteDialogKeys, setDeleteDialogKeys] = React.useState([]) const [deleteAllVersions, setDeleteAllVersions] = React.useState(false) @@ -203,6 +217,7 @@ export function ObjectList({ const requestScope = activeScopeRef.current loadingRef.current = true setLoading(true) + if (!shouldAppend) setListError(null) try { const response = await listObject(bucket, prefix || undefined, resolvedPageSize, token, { includeDeleted: showDeleted, @@ -226,6 +241,7 @@ export function ObjectList({ } setNextToken(r.NextContinuationToken) + setListError(null) const prefixItems: ObjectRow[] = (r.CommonPrefixes ?? []).map((item) => ({ Key: item.Prefix ?? "", @@ -261,6 +277,7 @@ export function ObjectList({ setNextToken(undefined) if (!shouldAppend) { setData([]) + setListError(isAccessDeniedError(error) ? "accessDenied" : "loadFailed") } } } finally { @@ -327,7 +344,6 @@ export function ObjectList({ const loadBucketVersioningStatus = async () => { setBucketVersioningState("unknown") - setVersioningError("") try { const resp = await getBucketVersioning(bucket) @@ -338,7 +354,6 @@ export function ObjectList({ console.error("Failed to load bucket versioning status:", error) if (!cancelled) { setBucketVersioningState("unknown") - setVersioningError(t("Failed to get data")) } } } @@ -348,7 +363,7 @@ export function ObjectList({ return () => { cancelled = true } - }, [bucket, getBucketVersioning, t, versioningReload]) + }, [bucket, getBucketVersioning]) const displayKey = React.useCallback( (key: string) => { @@ -531,7 +546,6 @@ export function ObjectList({ size="sm" onClick={() => openDeleteDialog([row.original.Key])} disabled={bucketVersioningState === "unknown"} - aria-describedby={bucketVersioningState === "unknown" ? "object-versioning-status" : undefined} > {t("Delete")} @@ -781,7 +795,6 @@ export function ObjectList({ className="border-destructive text-destructive" onClick={handleBatchDelete} disabled={bucketVersioningState === "unknown"} - aria-describedby={bucketVersioningState === "unknown" ? "object-versioning-status" : undefined} > {t("Delete Selected")} @@ -824,44 +837,30 @@ export function ObjectList({ - {bucketVersioningState === "unknown" ? ( -
- - {versioningError || t("Loading…")} - - {versioningError ? ( - - ) : null} -
- ) : null} - -
- - {t("Loaded {count} objects", { - count: data.length, - })} - - {t("Filtering and sorting apply to loaded objects")} -
+ {!listError ? ( +
+ + {t("Loaded {count} objects", { + count: data.length, + })} + + {t("Filtering and sorting apply to loaded objects")} +
+ ) : null} {nextToken ? (
diff --git a/lib/bucket-access.ts b/lib/bucket-access.ts new file mode 100644 index 00000000..bfd799f8 --- /dev/null +++ b/lib/bucket-access.ts @@ -0,0 +1,16 @@ +interface S3ServiceError { + $metadata?: { + httpStatusCode?: number + } + Code?: string + name?: string + message?: string +} + +export function isMissingBucketError(error: unknown): boolean { + const serviceError = error as S3ServiceError + if (serviceError?.$metadata?.httpStatusCode === 404) return true + + const code = (serviceError?.Code ?? serviceError?.name ?? "").toLowerCase() + return code === "nosuchbucket" || code === "notfound" +} diff --git a/lib/bucket-policy-status.ts b/lib/bucket-policy-status.ts new file mode 100644 index 00000000..4f705565 --- /dev/null +++ b/lib/bucket-policy-status.ts @@ -0,0 +1,23 @@ +export interface BucketPolicyStatusResponse { + PolicyStatus?: { + IsPublic?: boolean + } +} + +export async function loadBucketPolicyStatuses( + bucketNames: string[], + getBucketPolicyStatus: (bucketName: string) => Promise, +): Promise> { + const results = await Promise.all( + bucketNames.map(async (name) => { + try { + const response = (await getBucketPolicyStatus(name)) as BucketPolicyStatusResponse + return [name, response.PolicyStatus?.IsPublic] as const + } catch { + return [name, undefined] as const + } + }), + ) + + return Object.fromEntries(results) +} diff --git a/tests/lib/bucket-access.test.ts b/tests/lib/bucket-access.test.ts new file mode 100644 index 00000000..61e91e81 --- /dev/null +++ b/tests/lib/bucket-access.test.ts @@ -0,0 +1,17 @@ +import test from "node:test" +import assert from "node:assert/strict" + +import { isMissingBucketError } from "../../lib/bucket-access" + +test("isMissingBucketError redirects only for a definitive missing bucket response", () => { + assert.equal(isMissingBucketError({ $metadata: { httpStatusCode: 404 } }), true) + assert.equal(isMissingBucketError({ Code: "NoSuchBucket" }), true) +}) + +test("isMissingBucketError does not treat access denied as a missing bucket", () => { + assert.equal(isMissingBucketError({ $metadata: { httpStatusCode: 403 }, Code: "AccessDenied" }), false) +}) + +test("isMissingBucketError keeps the current bucket when availability is uncertain", () => { + assert.equal(isMissingBucketError(new Error("Network request failed")), false) +}) diff --git a/tests/lib/bucket-policy-status.test.ts b/tests/lib/bucket-policy-status.test.ts new file mode 100644 index 00000000..b02189f7 --- /dev/null +++ b/tests/lib/bucket-policy-status.test.ts @@ -0,0 +1,27 @@ +import test from "node:test" +import assert from "node:assert/strict" + +import { loadBucketPolicyStatuses } from "../../lib/bucket-policy-status" + +test("loadBucketPolicyStatuses preserves public and private responses", async () => { + const statuses = await loadBucketPolicyStatuses(["public", "private"], async (bucketName) => ({ + PolicyStatus: { IsPublic: bucketName === "public" }, + })) + + assert.deepEqual(statuses, { + public: true, + private: false, + }) +}) + +test("loadBucketPolicyStatuses leaves access-denied responses unknown", async () => { + const statuses = await loadBucketPolicyStatuses(["allowed", "denied"], async (bucketName) => { + if (bucketName === "denied") throw new Error("AccessDenied") + return { PolicyStatus: { IsPublic: false } } + }) + + assert.deepEqual(statuses, { + allowed: false, + denied: undefined, + }) +}) diff --git a/tests/lib/data-table-error-source.test.js b/tests/lib/data-table-error-source.test.js new file mode 100644 index 00000000..39a6ef76 --- /dev/null +++ b/tests/lib/data-table-error-source.test.js @@ -0,0 +1,12 @@ +import test from "node:test" +import assert from "node:assert/strict" +import fs from "node:fs" + +test("DataTable renders request failures as an announced error state before empty content", () => { + const source = fs.readFileSync("components/data-table/data-table.tsx", "utf8") + + assert.match(source, /errorTitle\?: string/) + assert.match(source, /\) : errorTitle \? \(/) + assert.match(source, /
/) + assert.match(source, / { test("object deletion stays blocked until versioning state is known", () => { assert.match(objectListSource, /setBucketVersioningState\("unknown"\)/) - assert.match(objectListSource, /versioningError/) assert.match(objectListSource, /bucketVersioningState === "unknown"/) - assert.match(objectListSource, /role=\{versioningError \? "alert" : "status"\}/) + assert.doesNotMatch(objectListSource, /object-versioning-status/) assert.doesNotMatch(objectListSource, /catch\s*\{[\s\S]{0,120}setBucketVersioningState\("disabled"\)/) }) diff --git a/tests/lib/object-list-source.test.js b/tests/lib/object-list-source.test.js index 886a3371..7c117d47 100644 --- a/tests/lib/object-list-source.test.js +++ b/tests/lib/object-list-source.test.js @@ -10,12 +10,16 @@ test("object list normalizes LastModified through the safe date helper", () => { assert.equal(source.includes('item.LastModified ? item.LastModified.toISOString() : ""'), false) }) -test("object list falls back to an empty table instead of crashing the page on fetch errors", () => { +test("object list distinguishes access errors from a confirmed empty bucket", () => { const source = fs.readFileSync("components/object/list.tsx", "utf8") assert.equal(source.includes('console.error("Failed to fetch objects:", error)'), true) assert.equal(source.includes('message.error((error as Error)?.message ?? t("Failed to load objects"))'), true) assert.equal(source.includes("setData([])"), true) + assert.equal(source.includes('setListError(isAccessDeniedError(error) ? "accessDenied" : "loadFailed")'), true) + assert.equal(source.includes('listError === "accessDenied"'), true) + assert.equal(source.includes('t("Access Denied")'), true) + assert.equal(source.includes("{!listError ? ("), true) }) test("object list lazy loads additional object batches instead of showing a paginator", () => {