From bf13b71fce157e5aa6ba4daff7c83d5c47800233 Mon Sep 17 00:00:00 2001 From: F3l1x1vo Date: Tue, 2 Jun 2026 15:36:07 +0200 Subject: [PATCH 1/8] #185 move saved s3 connection from backend to frontend --- TECH_DEBT.md | 14 +--- .../storage/modals/PreviewModal.svelte | 52 ++++++++------ .../modals/preview/FallbackPreview.svelte | 9 ++- .../preview/FallbackPreview.svelte.spec.ts | 39 +++++----- .../storage/modals/upload/UploadModal.svelte | 24 +++++-- src/lib/server/storage/connection.ts | 62 ++++++++++++++++ src/lib/server/storage/service.test.ts | 72 ++++++++----------- src/lib/server/storage/service.ts | 56 +++++++-------- .../server/storage/user-connections.test.ts | 67 ----------------- src/lib/server/storage/user-connections.ts | 20 ------ src/lib/server/storage/utils.test.ts | 31 +++----- src/lib/server/storage/utils.ts | 17 +++-- src/lib/storage/connection-storage.ts | 17 ++++- src/lib/storage/download.ts | 53 +++++++++----- src/lib/storage/state.svelte.ts | 13 +++- src/lib/storage/upload.ts | 19 ++++- src/routes/(app)/storage/+layout.server.ts | 19 ++--- src/routes/(app)/storage/+layout.ts | 35 +++++++++ src/routes/(app)/storage/+page.server.ts | 25 ++----- .../[bucket]/[...prefix]/+page.server.ts | 23 +----- .../storage/[bucket]/[...prefix]/+page.ts | 46 ++++++++++++ .../[bucket]/[...prefix]/page.server.test.ts | 59 ++++----------- .../(app)/storage/api/buckets/+server.ts | 16 +++++ .../(app)/storage/api/delete/+server.ts | 12 ++-- .../(app)/storage/api/delete/server.test.ts | 12 +++- .../(app)/storage/api/download/+server.ts | 22 +++--- .../(app)/storage/api/download/server.test.ts | 10 ++- .../(app)/storage/api/objects/+server.ts | 30 ++++++++ .../(app)/storage/api/preview/+server.ts | 23 +++--- .../(app)/storage/api/preview/server.test.ts | 16 +++-- .../(app)/storage/api/upload/+server.ts | 14 ++-- .../(app)/storage/api/upload/server.test.ts | 21 ++++-- .../(app)/storage/layout.server.test.ts | 42 +---------- src/routes/(app)/storage/page.server.test.ts | 23 ++---- 34 files changed, 526 insertions(+), 487 deletions(-) create mode 100644 src/lib/server/storage/connection.ts delete mode 100644 src/lib/server/storage/user-connections.test.ts delete mode 100644 src/lib/server/storage/user-connections.ts create mode 100644 src/routes/(app)/storage/+layout.ts create mode 100644 src/routes/(app)/storage/[bucket]/[...prefix]/+page.ts create mode 100644 src/routes/(app)/storage/api/buckets/+server.ts create mode 100644 src/routes/(app)/storage/api/objects/+server.ts diff --git a/TECH_DEBT.md b/TECH_DEBT.md index 1f8fb6d4..63333d51 100644 --- a/TECH_DEBT.md +++ b/TECH_DEBT.md @@ -30,11 +30,11 @@ S3 connection credentials (access key ID and secret access key) are persisted in --- -### In-memory storage connection state +### Download endpoint buffers entire object in browser memory -**File:** `src/lib/server/storage/user-connections.ts` +**File:** `src/lib/storage/download.ts` -Per-user S3 connection configs (endpoint, region, credentials) are stored in a server-side `Map`. All connections are lost on server restart and cannot be shared across multiple replicas. Acceptable for the initial phase; mirrors the same pattern used by the Trino user-clients module. Long-term fix: persist encrypted connection configs server-side, tied to the authenticated session. +`downloadObject` fetches the full S3 object body via the `/storage/api/download` endpoint, buffers it as a `Blob` in browser memory, then triggers a programmatic anchor click. This is simpler than streaming directly to disk but means the entire object must fit in browser memory before the save dialog appears. Acceptable for the current object sizes; for very large files (multiple GiB) this will cause memory pressure. Long-term fix: use the [File System Access API](https://developer.mozilla.org/en-US/docs/Web/API/File_System_Access_API) `createWritable()` to stream bytes directly to disk without buffering, with a fallback to the current Blob approach for Firefox (which does not support `showSaveFilePicker`). --- @@ -128,14 +128,6 @@ The dev server accepts requests from any host. This enables DNS rebinding attack --- -### Upload endpoint uses in-memory S3 credentials - -**File:** `src/routes/(app)/storage/api/upload/+server.ts`, `src/lib/server/storage/user-connections.ts` - -The upload endpoint reads S3 credentials from the same per-user in-memory connection map used by download and preview. This introduces no additional security risk beyond what is already documented in the "In-memory storage connection state" entry above. Long-term fix: same as that entry — persist encrypted credentials server-side. - ---- - ### No server-side file size limit on uploads (v0) **File:** `src/routes/(app)/storage/api/upload/+server.ts` diff --git a/src/lib/components/storage/modals/PreviewModal.svelte b/src/lib/components/storage/modals/PreviewModal.svelte index 1b1fea02..89bdd4ac 100644 --- a/src/lib/components/storage/modals/PreviewModal.svelte +++ b/src/lib/components/storage/modals/PreviewModal.svelte @@ -13,7 +13,10 @@ import PdfPreview from './preview/PdfPreview.svelte'; import FallbackPreview from './preview/FallbackPreview.svelte'; import { keyToName, formatFileSize } from '$lib/storage/utils.js'; - import { resolve } from '$app/paths'; + import { downloadObject, DownloadError } from '$lib/storage/download.js'; + import { loadConnectionLocally, getConnectionHeader } from '$lib/storage/connection-storage.js'; + import { addToast } from '$lib/stores/toast.svelte.js'; + import { STORAGE_CONNECTION_HEADER } from '$lib/storage/connection-storage.js'; interface Props { open: boolean; @@ -87,9 +90,14 @@ preview = { kind: 'loading' }; revokeBlobUrls(); + const conn = loadConnectionLocally(); + const headers: HeadersInit = conn + ? { [STORAGE_CONNECTION_HEADER]: getConnectionHeader(conn) } + : {}; + try { const params = new URLSearchParams({ bucket: bkt, key }); - const res = await fetch(`/storage/api/preview?${params}`); + const res = await fetch(`/storage/api/preview?${params}`, { headers }); if (!res.ok) { const errBody = await res.json().catch(() => ({})); @@ -211,9 +219,22 @@ const filename = $derived(objectKey ? keyToName(objectKey) : ''); - function buildDownloadUrl(key: string, bkt: string): string { - const params = new URLSearchParams({ bucket: bkt, key }); - return `${resolve('/storage/api/download')}?${params}`; + async function triggerDownload() { + if (!objectKey) return; + const conn = loadConnectionLocally(); + if (!conn) { + addToast('error', m.storage_download_error_unknown()); + return; + } + try { + await downloadObject(bucket, objectKey, getConnectionHeader(conn)); + } catch (err) { + if (err instanceof DownloadError) { + addToast('error', err.message); + } else { + addToast('error', m.storage_download_error_unknown()); + } + } } function close() { @@ -340,8 +361,7 @@ {:else if preview.kind === 'fallback'} {/if} @@ -351,25 +371,15 @@ {#if preview.kind === 'text' || preview.kind === 'csv' || preview.kind === 'parquet' || preview.kind === 'image' || preview.kind === 'pdf'}
{#if (preview.kind === 'text' || preview.kind === 'csv') && preview.truncated} - + {:else if preview.kind === 'image' || preview.kind === 'pdf'} - + {/if}
diff --git a/src/lib/components/storage/modals/preview/FallbackPreview.svelte.spec.ts b/src/lib/components/storage/modals/preview/FallbackPreview.svelte.spec.ts index 726db3b1..2002f867 100644 --- a/src/lib/components/storage/modals/preview/FallbackPreview.svelte.spec.ts +++ b/src/lib/components/storage/modals/preview/FallbackPreview.svelte.spec.ts @@ -1,5 +1,5 @@ import { page } from 'vitest/browser'; -import { describe, expect, it } from 'vitest'; +import { describe, expect, it, vi } from 'vitest'; import { render } from 'vitest-browser-svelte'; import { faker } from '@faker-js/faker'; import FallbackPreview from './FallbackPreview.svelte'; @@ -7,32 +7,28 @@ import FallbackPreview from './FallbackPreview.svelte'; describe('FallbackPreview', () => { const baseProps = { contentType: 'application/octet-stream', - downloadUrl: '/download/file.bin', - name: 'file.bin' + onDownload: vi.fn() }; it('should show unsupported title when isBinary is false', async () => { render(FallbackPreview, { ...baseProps, isBinary: false }); - // Find the download link as a reliable anchor - const link = page.getByRole('link'); - await expect.element(link).toBeInTheDocument(); - await expect.element(link).toHaveAttribute('download', 'file.bin'); + const btn = page.getByRole('button'); + await expect.element(btn).toBeInTheDocument(); }); it('should show binary title when isBinary is true', async () => { render(FallbackPreview, { ...baseProps, isBinary: true }); - const link = page.getByRole('link'); - await expect.element(link).toBeInTheDocument(); + const btn = page.getByRole('button'); + await expect.element(btn).toBeInTheDocument(); }); - it('should render download link with correct attributes', async () => { + it('should render a download button', async () => { render(FallbackPreview, { ...baseProps }); - const link = page.getByRole('link'); - await expect.element(link).toHaveAttribute('href', '/download/file.bin'); - await expect.element(link).toHaveAttribute('download', 'file.bin'); + const btn = page.getByRole('button'); + await expect.element(btn).toBeInTheDocument(); }); it('should show contentType in mono text', async () => { @@ -49,19 +45,18 @@ describe('FallbackPreview', () => { await expect.element(page.getByText(mimeType)).toBeInTheDocument(); }); - it('should handle long filenames', async () => { - const name = `${faker.string.alphanumeric(100)}.dat`; - render(FallbackPreview, { ...baseProps, name }); + it('should call onDownload when button is clicked', async () => { + const onDownload = vi.fn(); + render(FallbackPreview, { ...baseProps, onDownload }); - const link = page.getByRole('link'); - await expect.element(link).toHaveAttribute('download', name); + await page.getByRole('button').click(); + expect(onDownload).toHaveBeenCalledOnce(); }); it('should handle special characters in names', async () => { - const name = 'file (copy) [2].bin'; - render(FallbackPreview, { ...baseProps, name }); + render(FallbackPreview, { ...baseProps }); - const link = page.getByRole('link'); - await expect.element(link).toHaveAttribute('download', name); + const btn = page.getByRole('button'); + await expect.element(btn).toBeInTheDocument(); }); }); diff --git a/src/lib/components/storage/modals/upload/UploadModal.svelte b/src/lib/components/storage/modals/upload/UploadModal.svelte index 57f1ac18..0fc49279 100644 --- a/src/lib/components/storage/modals/upload/UploadModal.svelte +++ b/src/lib/components/storage/modals/upload/UploadModal.svelte @@ -8,6 +8,7 @@ import Modal from '$lib/components/Modal.svelte'; import { checkObjectExists, uploadFile, UploadError } from '$lib/storage/upload.js'; import { formatFileSize } from '$lib/storage/utils.js'; + import { loadConnectionLocally, getConnectionHeader } from '$lib/storage/connection-storage.js'; import UploadDropzone from './UploadDropzone.svelte'; import UploadConflictEntry from './UploadConflictEntry.svelte'; import UploadEntryStatus from './UploadEntryStatus.svelte'; @@ -98,10 +99,13 @@ if (phase !== 'selected') return; phase = 'checking'; + const conn = loadConnectionLocally(); + const connHeader = conn ? getConnectionHeader(conn) : ''; + const results = await Promise.all( entries.map(async (e) => { try { - return { id: e.id, conflict: await checkObjectExists(bucket, e.targetKey) }; + return { id: e.id, conflict: await checkObjectExists(bucket, e.targetKey, connHeader) }; } catch { return { id: e.id, conflict: false }; } @@ -150,10 +154,18 @@ entries = entries.map((e) => e.id === entry.id ? { ...e, status: 'uploading' as const, progress: 0 } : e ); + const conn = loadConnectionLocally(); + const connHeader = conn ? getConnectionHeader(conn) : ''; try { - await uploadFile(bucket, key, entry.file, (pct) => { - entries = entries.map((e) => (e.id === entry.id ? { ...e, progress: pct } : e)); - }); + await uploadFile( + bucket, + key, + entry.file, + (pct) => { + entries = entries.map((e) => (e.id === entry.id ? { ...e, progress: pct } : e)); + }, + connHeader + ); entries = entries.map((e) => e.id === entry.id ? { ...e, status: 'done' as const, progress: 100 } : e ); @@ -215,8 +227,10 @@ entries = entries.map((e) => (e.id === id ? { ...e, renameState: 'checking' as const } : e)); const newKey = resolvedKey(entry); + const conn = loadConnectionLocally(); + const connHeader = conn ? getConnectionHeader(conn) : ''; try { - const exists = await checkObjectExists(bucket, newKey); + const exists = await checkObjectExists(bucket, newKey, connHeader); const nextState: RenameState = exists ? 'conflict' : 'ok'; entries = entries.map((e) => (e.id === id ? { ...e, renameState: nextState } : e)); } catch { diff --git a/src/lib/server/storage/connection.ts b/src/lib/server/storage/connection.ts new file mode 100644 index 00000000..21e17b4c --- /dev/null +++ b/src/lib/server/storage/connection.ts @@ -0,0 +1,62 @@ +import { error } from '@sveltejs/kit'; +import { StorageConnectionSchema } from '$lib/storage/schemas.js'; +import { STORAGE_CONNECTION_HEADER } from '$lib/storage/connection-storage.js'; +import type { S3ConnectionConfig } from './types.js'; +import { logger } from '$lib/server/logging'; + +export { STORAGE_CONNECTION_HEADER }; + +const log = logger.child({ module: 'storage-connection' }); + +/** + * Parse and validate a base64-encoded JSON connection payload from a request header value. + * Throws a 400 HTTP error if the payload is malformed or fails schema validation. + * Throws a 400 HTTP error if the connection type is not 's3'. + */ +function parseConnectionPayload(raw: string): S3ConnectionConfig { + let data: unknown; + try { + data = JSON.parse(atob(raw)); + } catch { + throw error(400, 'Invalid storage connection header'); + } + + const parsed = StorageConnectionSchema.safeParse(data); + if (!parsed.success) { + log.debug({ issues: parsed.error.issues }, 'storage connection header validation failed'); + throw error(400, 'Invalid storage connection configuration'); + } + + if (parsed.data.type !== 's3') { + throw error(400, 'Storage backend not supported'); + } + + return { + type: 's3', + endpoint: parsed.data.endpoint || undefined, + pathStyle: parsed.data.pathStyle, + region: parsed.data.region, + accessKeyId: parsed.data.accessKeyId || undefined, + secretAccessKey: parsed.data.secretAccessKey || undefined + }; +} + +/** + * Extract the storage connection config from the `X-Storage-Connection` request header. + * Returns `null` if the header is absent. Throws on malformed or invalid payloads. + */ +export function getConnectionFromHeader(request: Request): S3ConnectionConfig | null { + const header = request.headers.get(STORAGE_CONNECTION_HEADER); + if (!header) return null; + return parseConnectionPayload(header); +} + +/** + * Extract the storage connection config from the `X-Storage-Connection` request header. + * Throws a 401 HTTP error if the header is absent. + */ +export function requireConnection(request: Request): S3ConnectionConfig { + const conn = getConnectionFromHeader(request); + if (!conn) throw error(401, 'No storage connection configured'); + return conn; +} diff --git a/src/lib/server/storage/service.test.ts b/src/lib/server/storage/service.test.ts index 60f59388..fa4be91f 100644 --- a/src/lib/server/storage/service.test.ts +++ b/src/lib/server/storage/service.test.ts @@ -6,15 +6,6 @@ vi.mock('$lib/server/logging', () => ({ logger: { child: () => ({ info: vi.fn(), debug: vi.fn(), warn: vi.fn() }) } })); -const mockGetUserConnection = vi.fn(); -const mockSetUserConnection = vi.fn(); -const mockClearUserConnection = vi.fn(); -vi.mock('./user-connections.js', () => ({ - getUserConnection: (...args: unknown[]) => mockGetUserConnection(...args), - setUserConnection: (...args: unknown[]) => mockSetUserConnection(...args), - clearUserConnection: (...args: unknown[]) => mockClearUserConnection(...args) -})); - const mockSend = vi.fn(); vi.mock('./s3-client.js', () => ({ createS3Client: () => ({ send: (...args: unknown[]) => mockSend(...args) }) @@ -31,7 +22,7 @@ const mockProvider = { listAllKeys: vi.fn() }; vi.mock('./utils.js', () => ({ - getProviderForUser: () => mockProvider + getProvider: () => mockProvider })); // eslint-disable-next-line @typescript-eslint/no-unused-vars @@ -54,36 +45,32 @@ import { uploadObject, deleteObjects } from './service.js'; +import type { S3ConnectionConfig } from './types.js'; -describe('storage service', () => { - const userId = faker.string.uuid(); +const config: S3ConnectionConfig = { + type: 's3', + region: faker.location.countryCode() +}; +describe('storage service', () => { beforeEach(() => { vi.clearAllMocks(); }); describe('listBuckets', () => { - it('returns empty array when no connection', async () => { - mockGetUserConnection.mockReturnValue(null); - expect(await listBuckets(userId)).toEqual([]); - }); - it('returns bucket names', async () => { - mockGetUserConnection.mockReturnValue({ type: 's3', region: 'us-east-1' }); mockSend.mockResolvedValue({ Buckets: [{ Name: 'a' }, { Name: 'b' }] }); - expect(await listBuckets(userId)).toEqual(['a', 'b']); + expect(await listBuckets(config)).toEqual(['a', 'b']); }); it('filters out buckets with no name', async () => { - mockGetUserConnection.mockReturnValue({ type: 's3', region: 'us-east-1' }); mockSend.mockResolvedValue({ Buckets: [{ Name: 'a' }, { Name: undefined }, { Name: '' }] }); - expect(await listBuckets(userId)).toEqual(['a']); + expect(await listBuckets(config)).toEqual(['a']); }); it('handles null Buckets in response', async () => { - mockGetUserConnection.mockReturnValue({ type: 's3', region: 'us-east-1' }); mockSend.mockResolvedValue({ Buckets: null }); - expect(await listBuckets(userId)).toEqual([]); + expect(await listBuckets(config)).toEqual([]); }); }); @@ -91,7 +78,7 @@ describe('storage service', () => { it('proxies to provider', async () => { const page = { objects: [], prefixes: [], nextToken: null }; mockProvider.listObjects.mockResolvedValue(page); - const result = await listObjects(userId, 'bucket', 'prefix/', 100); + const result = await listObjects(config, 'bucket', 'prefix/', 100); expect(mockProvider.listObjects).toHaveBeenCalledWith('prefix/', 100, undefined); expect(result).toEqual(page); }); @@ -99,21 +86,21 @@ describe('storage service', () => { it('passes continuation token when provided', async () => { const page = { objects: [], prefixes: [], nextToken: null }; mockProvider.listObjects.mockResolvedValue(page); - await listObjects(userId, 'bucket', '', 10, 'tok'); + await listObjects(config, 'bucket', '', 10, 'tok'); expect(mockProvider.listObjects).toHaveBeenCalledWith('', 10, 'tok'); }); it('passes undefined when continuation token is null', async () => { const page = { objects: [], prefixes: [], nextToken: null }; mockProvider.listObjects.mockResolvedValue(page); - await listObjects(userId, 'bucket', '', 10, null); + await listObjects(config, 'bucket', '', 10, null); expect(mockProvider.listObjects).toHaveBeenCalledWith('', 10, undefined); }); it('throws S3ServiceException via mapS3ErrorToHttp', async () => { const err = makeS3Error('AccessDenied'); mockProvider.listObjects.mockRejectedValue(err); - await expect(listObjects(userId, 'bucket', '', 10)).rejects.toThrow(err); + await expect(listObjects(config, 'bucket', '', 10)).rejects.toThrow(err); expect(mockMapS3ErrorToHttp).toHaveBeenCalledWith(err, { bucket: 'bucket', operation: 'listObjects' @@ -123,7 +110,7 @@ describe('storage service', () => { it('rethrows non-S3 errors', async () => { const err = new Error('network'); mockProvider.listObjects.mockRejectedValue(err); - await expect(listObjects(userId, 'bucket', '', 10)).rejects.toThrow('network'); + await expect(listObjects(config, 'bucket', '', 10)).rejects.toThrow('network'); expect(mockMapS3ErrorToHttp).not.toHaveBeenCalled(); }); }); @@ -132,14 +119,14 @@ describe('storage service', () => { it('returns download from provider', async () => { const download = { stream: new ReadableStream(), contentType: 'text/plain' }; mockProvider.getObject.mockResolvedValue(download); - const result = await downloadObject(userId, 'bucket', 'key.txt'); + const result = await downloadObject(config, 'bucket', 'key.txt'); expect(result).toEqual(download); }); it('throws S3ServiceException via mapS3ErrorToHttp', async () => { const err = makeS3Error('NoSuchKey'); mockProvider.getObject.mockRejectedValue(err); - await expect(downloadObject(userId, 'bucket', 'key.txt')).rejects.toThrow(err); + await expect(downloadObject(config, 'bucket', 'key.txt')).rejects.toThrow(err); expect(mockMapS3ErrorToHttp).toHaveBeenCalledWith(err, { bucket: 'bucket', key: 'key.txt', @@ -150,7 +137,7 @@ describe('storage service', () => { it('rethrows non-S3 errors', async () => { const err = new Error('timeout'); mockProvider.getObject.mockRejectedValue(err); - await expect(downloadObject(userId, 'bucket', 'k')).rejects.toThrow('timeout'); + await expect(downloadObject(config, 'bucket', 'k')).rejects.toThrow('timeout'); expect(mockMapS3ErrorToHttp).not.toHaveBeenCalled(); }); }); @@ -159,13 +146,13 @@ describe('storage service', () => { it('proxies to provider', async () => { const meta = { contentType: 'text/plain', contentLength: 42 }; mockProvider.getMetadata.mockResolvedValue(meta); - expect(await getObjectMetadata(userId, 'bucket', 'k')).toEqual(meta); + expect(await getObjectMetadata(config, 'bucket', 'k')).toEqual(meta); }); it('throws S3ServiceException via mapS3ErrorToHttp', async () => { const err = makeS3Error('NoSuchKey'); mockProvider.getMetadata.mockRejectedValue(err); - await expect(getObjectMetadata(userId, 'bucket', 'k')).rejects.toThrow(err); + await expect(getObjectMetadata(config, 'bucket', 'k')).rejects.toThrow(err); expect(mockMapS3ErrorToHttp).toHaveBeenCalledWith(err, { bucket: 'bucket', key: 'k', @@ -176,7 +163,7 @@ describe('storage service', () => { it('rethrows non-S3 errors', async () => { const err = new Error('gone'); mockProvider.getMetadata.mockRejectedValue(err); - await expect(getObjectMetadata(userId, 'bucket', 'k')).rejects.toThrow('gone'); + await expect(getObjectMetadata(config, 'bucket', 'k')).rejects.toThrow('gone'); expect(mockMapS3ErrorToHttp).not.toHaveBeenCalled(); }); }); @@ -184,7 +171,7 @@ describe('storage service', () => { describe('uploadObject', () => { it('proxies to provider', async () => { mockProvider.putObject.mockResolvedValue(undefined); - await uploadObject(userId, 'bucket', 'k', Buffer.from('x'), 'text/plain', 1); + await uploadObject(config, 'bucket', 'k', Buffer.from('x'), 'text/plain', 1); expect(mockProvider.putObject).toHaveBeenCalledWith('k', expect.any(Buffer), 'text/plain', 1); }); @@ -192,7 +179,7 @@ describe('storage service', () => { const err = makeS3Error('AccessDenied'); mockProvider.putObject.mockRejectedValue(err); await expect( - uploadObject(userId, 'bucket', 'k', Buffer.from('x'), 'text/plain') + uploadObject(config, 'bucket', 'k', Buffer.from('x'), 'text/plain') ).rejects.toThrow(err); expect(mockMapS3ErrorToHttp).toHaveBeenCalledWith(err, { bucket: 'bucket', @@ -205,7 +192,7 @@ describe('storage service', () => { const err = new Error('disk full'); mockProvider.putObject.mockRejectedValue(err); await expect( - uploadObject(userId, 'bucket', 'k', Buffer.from('x'), 'text/plain') + uploadObject(config, 'bucket', 'k', Buffer.from('x'), 'text/plain') ).rejects.toThrow('disk full'); expect(mockMapS3ErrorToHttp).not.toHaveBeenCalled(); }); @@ -215,7 +202,7 @@ describe('storage service', () => { it('expands directory prefixes and deletes all', async () => { mockProvider.listAllKeys.mockResolvedValue(['dir/a.txt', 'dir/b.txt']); mockProvider.deleteObjects.mockResolvedValue({ failed: [] }); - const result = await deleteObjects(userId, 'bucket', ['file.txt', 'dir/']); + const result = await deleteObjects(config, 'bucket', ['file.txt', 'dir/']); expect(mockProvider.listAllKeys).toHaveBeenCalledWith('dir/'); expect(mockProvider.deleteObjects).toHaveBeenCalledWith([ 'file.txt', @@ -228,15 +215,14 @@ describe('storage service', () => { it('uses directory key itself when no children found', async () => { mockProvider.listAllKeys.mockResolvedValue([]); mockProvider.deleteObjects.mockResolvedValue({ failed: [] }); - await deleteObjects(userId, 'bucket', ['empty/']); + await deleteObjects(config, 'bucket', ['empty/']); expect(mockProvider.deleteObjects).toHaveBeenCalledWith(['empty/']); }); it('returns empty result for no keys', async () => { - // No file keys, no dir prefixes after expansion mockProvider.listAllKeys.mockResolvedValue([]); mockProvider.deleteObjects.mockResolvedValue({ failed: [] }); - const result = await deleteObjects(userId, 'bucket', []); + const result = await deleteObjects(config, 'bucket', []); expect(result).toEqual({ failed: [] }); }); @@ -244,7 +230,7 @@ describe('storage service', () => { const err = makeS3Error('AccessDenied'); mockProvider.listAllKeys.mockResolvedValue([]); mockProvider.deleteObjects.mockRejectedValue(err); - await expect(deleteObjects(userId, 'bucket', ['file.txt'])).rejects.toThrow(err); + await expect(deleteObjects(config, 'bucket', ['file.txt'])).rejects.toThrow(err); expect(mockMapS3ErrorToHttp).toHaveBeenCalledWith(err, { bucket: 'bucket', operation: 'deleteObjects' @@ -254,7 +240,7 @@ describe('storage service', () => { it('rethrows non-S3 errors', async () => { const err = new Error('network'); mockProvider.deleteObjects.mockRejectedValue(err); - await expect(deleteObjects(userId, 'bucket', ['file.txt'])).rejects.toThrow('network'); + await expect(deleteObjects(config, 'bucket', ['file.txt'])).rejects.toThrow('network'); expect(mockMapS3ErrorToHttp).not.toHaveBeenCalled(); }); }); diff --git a/src/lib/server/storage/service.ts b/src/lib/server/storage/service.ts index c4550e94..c27b6532 100644 --- a/src/lib/server/storage/service.ts +++ b/src/lib/server/storage/service.ts @@ -1,40 +1,33 @@ import { S3ServiceException, ListBucketsCommand } from '@aws-sdk/client-s3'; -import { getUserConnection, setUserConnection, clearUserConnection } from './user-connections.js'; import { createS3Client } from './s3-client.js'; import { mapS3ErrorToHttp } from './s3-errors.js'; -import { getProviderForUser } from './utils.js'; +import { getProvider } from './utils.js'; import type { StoragePage, StorageMetadata } from '$lib/storage/types.js'; import type { ObjectDownload, DeleteObjectsResult } from './provider.js'; +import type { S3ConnectionConfig } from './types.js'; import { logger } from '$lib/server/logging'; const log = logger.child({ module: 'storage-service' }); -export { getUserConnection as getConnection }; -export { setUserConnection as saveConnection }; -export { clearUserConnection as clearConnection }; - -/** List all buckets accessible with the user's current connection. Returns [] when not connected. */ -export async function listBuckets(userId: string): Promise { - const config = getUserConnection(userId); - if (!config) return []; - +/** List all buckets accessible with the given connection config. */ +export async function listBuckets(config: S3ConnectionConfig): Promise { const client = createS3Client(config); const output = await client.send(new ListBucketsCommand({})); const buckets = (output.Buckets ?? []).map((b) => b.Name ?? '').filter(Boolean); - log.debug({ user_id: userId, bucket_count: buckets.length }, 'listed buckets'); + log.debug({ storage_type: config.type, bucket_count: buckets.length }, 'listed buckets'); return buckets; } -/** List objects at the given bucket/prefix for the user's current connection. Throws on S3 errors. */ +/** List objects at the given bucket/prefix. Throws on S3 errors. */ export async function listObjects( - userId: string, + config: S3ConnectionConfig, bucket: string, prefix: string, pageSize: number, continuationToken?: string | null ): Promise { - const provider = getProviderForUser(userId, bucket); + const provider = getProvider(config, bucket); try { return await provider.listObjects(prefix, pageSize, continuationToken ?? undefined); @@ -45,18 +38,19 @@ export async function listObjects( throw err; } } + /** Download a single object from the bucket, returning a stream and metadata for the HTTP response. */ export async function downloadObject( - userId: string, + config: S3ConnectionConfig, bucket: string, key: string ): Promise { - const provider = getProviderForUser(userId, bucket); + const provider = getProvider(config, bucket); try { - log.debug({ user_id: userId, bucket, key }, 'downloading object'); + log.debug({ bucket, key }, 'downloading object'); const download = await provider.getObject(key); - log.info({ user_id: userId, bucket, key }, 'object download started'); + log.info({ bucket, key }, 'object download started'); return download; } catch (err) { if (err instanceof S3ServiceException) { @@ -68,14 +62,14 @@ export async function downloadObject( /** Fetch metadata for a single object — used for lightweight pre-flight checks. */ export async function getObjectMetadata( - userId: string, + config: S3ConnectionConfig, bucket: string, key: string ): Promise { - const provider = getProviderForUser(userId, bucket); + const provider = getProvider(config, bucket); try { - log.debug({ user_id: userId, bucket, key }, 'getting object metadata'); + log.debug({ bucket, key }, 'getting object metadata'); return await provider.getMetadata(key); } catch (err) { if (err instanceof S3ServiceException) { @@ -87,20 +81,20 @@ export async function getObjectMetadata( /** Upload an object to the bucket, using multipart upload for large files. */ export async function uploadObject( - userId: string, + config: S3ConnectionConfig, bucket: string, key: string, body: ReadableStream | Buffer, contentType: string, contentLength?: number ): Promise { - const provider = getProviderForUser(userId, bucket); + const provider = getProvider(config, bucket); try { - log.debug({ user_id: userId, bucket, key, content_type: contentType }, 'uploading object'); + log.debug({ bucket, key, content_type: contentType }, 'uploading object'); await provider.putObject(key, body, contentType, contentLength); log.info( - { user_id: userId, bucket, key, content_type: contentType, content_length: contentLength }, + { bucket, key, content_type: contentType, content_length: contentLength }, 'object uploaded' ); } catch (err) { @@ -114,11 +108,11 @@ export async function uploadObject( /** Delete one or more objects from the bucket. Returns a result listing any keys that failed. * Directory keys (ending with '/') are expanded to all contained objects before deletion. */ export async function deleteObjects( - userId: string, + config: S3ConnectionConfig, bucket: string, keys: string[] ): Promise { - const provider = getProviderForUser(userId, bucket); + const provider = getProvider(config, bucket); try { // Expand any directory prefixes (keys ending with '/') to their contents. @@ -127,7 +121,7 @@ export async function deleteObjects( let allKeys = [...fileKeys]; for (const prefix of dirPrefixes) { - log.debug({ user_id: userId, bucket, prefix }, 'expanding directory prefix for deletion'); + log.debug({ bucket, prefix }, 'expanding directory prefix for deletion'); const children = await provider.listAllKeys(prefix); allKeys = allKeys.concat(children.length > 0 ? children : [prefix]); } @@ -136,10 +130,10 @@ export async function deleteObjects( return { failed: [] }; } - log.debug({ user_id: userId, bucket, key_count: allKeys.length }, 'deleting objects'); + log.debug({ bucket, key_count: allKeys.length }, 'deleting objects'); const result = await provider.deleteObjects(allKeys); log.info( - { user_id: userId, bucket, key_count: allKeys.length, failed_count: result.failed.length }, + { bucket, key_count: allKeys.length, failed_count: result.failed.length }, 'objects delete completed' ); return result; diff --git a/src/lib/server/storage/user-connections.test.ts b/src/lib/server/storage/user-connections.test.ts deleted file mode 100644 index 68e9ac66..00000000 --- a/src/lib/server/storage/user-connections.test.ts +++ /dev/null @@ -1,67 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { faker } from '@faker-js/faker'; -import { setUserConnection, getUserConnection, clearUserConnection } from './user-connections.js'; -import type { S3ConnectionConfig } from './types.js'; - -vi.mock('$lib/server/logging', () => ({ - logger: { child: () => ({ info: vi.fn(), debug: vi.fn() }) } -})); - -function makeConfig(): S3ConnectionConfig { - return { - type: 's3', - region: faker.location.countryCode(), - endpoint: faker.internet.url(), - accessKeyId: faker.string.alphanumeric(20), - secretAccessKey: faker.string.alphanumeric(40) - }; -} - -describe('user-connections', () => { - const userId = faker.string.uuid(); - - beforeEach(() => { - clearUserConnection(userId); - }); - - it('returns null for unknown user', () => { - expect(getUserConnection(faker.string.uuid())).toBeNull(); - }); - - it('stores and retrieves a connection', () => { - const config = makeConfig(); - setUserConnection(userId, config); - expect(getUserConnection(userId)).toEqual(config); - }); - - it('overwrites existing connection', () => { - setUserConnection(userId, makeConfig()); - const newConfig = makeConfig(); - setUserConnection(userId, newConfig); - expect(getUserConnection(userId)).toEqual(newConfig); - }); - - it('clears a connection', () => { - setUserConnection(userId, makeConfig()); - clearUserConnection(userId); - expect(getUserConnection(userId)).toBeNull(); - }); - - it('clearing non-existent user does not throw', () => { - expect(() => clearUserConnection(faker.string.uuid())).not.toThrow(); - }); - - it('supports multiple users independently', () => { - const user1 = faker.string.uuid(); - const user2 = faker.string.uuid(); - const config1 = makeConfig(); - const config2 = makeConfig(); - setUserConnection(user1, config1); - setUserConnection(user2, config2); - expect(getUserConnection(user1)).toEqual(config1); - expect(getUserConnection(user2)).toEqual(config2); - clearUserConnection(user1); - expect(getUserConnection(user1)).toBeNull(); - expect(getUserConnection(user2)).toEqual(config2); - }); -}); diff --git a/src/lib/server/storage/user-connections.ts b/src/lib/server/storage/user-connections.ts deleted file mode 100644 index bcfa9835..00000000 --- a/src/lib/server/storage/user-connections.ts +++ /dev/null @@ -1,20 +0,0 @@ -import type { S3ConnectionConfig } from './types.js'; -import { logger } from '$lib/server/logging'; - -const log = logger.child({ module: 'storage-user-connections' }); - -const userConnections = new Map(); - -export function setUserConnection(userId: string, config: S3ConnectionConfig): void { - userConnections.set(userId, config); - log.info({ user_id: userId, storage_type: config.type }, 'user storage connection saved'); -} - -export function getUserConnection(userId: string): S3ConnectionConfig | null { - return userConnections.get(userId) ?? null; -} - -export function clearUserConnection(userId: string): void { - userConnections.delete(userId); - log.info({ user_id: userId }, 'user storage connection cleared'); -} diff --git a/src/lib/server/storage/utils.test.ts b/src/lib/server/storage/utils.test.ts index 55a3a061..b83bfb17 100644 --- a/src/lib/server/storage/utils.test.ts +++ b/src/lib/server/storage/utils.test.ts @@ -1,42 +1,29 @@ import { describe, it, expect, vi } from 'vitest'; -import { faker } from '@faker-js/faker'; vi.mock('$lib/server/logging', () => ({ logger: { child: () => ({ info: vi.fn(), debug: vi.fn() }) } })); -const mockGetUserConnection = vi.fn(); -vi.mock('./user-connections.js', () => ({ - getUserConnection: (...args: unknown[]) => mockGetUserConnection(...args) -})); - const mockCreate = vi.fn().mockReturnValue({ listObjects: vi.fn() }); vi.mock('./factory.js', () => ({ StorageProviderFactory: { create: (...args: unknown[]) => mockCreate(...args) } })); -import { getProviderForUser } from './utils.js'; +import { getProvider } from './utils.js'; +import type { S3ConnectionConfig } from './types.js'; -describe('getProviderForUser', () => { - it('throws 401 when no connection exists', () => { - mockGetUserConnection.mockReturnValue(null); - expect(() => getProviderForUser(faker.string.uuid(), 'bucket')).toThrow( - expect.objectContaining({ status: 401 }) - ); - }); +const s3Config: S3ConnectionConfig = { type: 's3', region: 'us-east-1' }; +describe('getProvider', () => { it('throws 400 for non-s3 connection type', () => { - mockGetUserConnection.mockReturnValue({ type: 'hdfs', nameNode: 'nn', port: 9870, user: 'u' }); - expect(() => getProviderForUser(faker.string.uuid(), 'bucket')).toThrow( - expect.objectContaining({ status: 400 }) - ); + expect(() => + getProvider({ type: 'hdfs', nameNode: 'nn', port: 9870, user: 'u' } as never, 'bucket') + ).toThrow(expect.objectContaining({ status: 400 })); }); it('returns provider for valid s3 connection', () => { - const config = { type: 's3', region: 'us-east-1' }; - mockGetUserConnection.mockReturnValue(config); - const provider = getProviderForUser('user1', 'my-bucket'); - expect(mockCreate).toHaveBeenCalledWith({ ...config, bucket: 'my-bucket' }); + const provider = getProvider(s3Config, 'my-bucket'); + expect(mockCreate).toHaveBeenCalledWith({ ...s3Config, bucket: 'my-bucket' }); expect(provider).toBeDefined(); }); }); diff --git a/src/lib/server/storage/utils.ts b/src/lib/server/storage/utils.ts index c98a0d2c..a0ab2223 100644 --- a/src/lib/server/storage/utils.ts +++ b/src/lib/server/storage/utils.ts @@ -1,17 +1,16 @@ import { error } from '@sveltejs/kit'; -import { getUserConnection } from './user-connections.js'; import { StorageProviderFactory } from './factory.js'; import type { StorageProvider } from './provider.js'; +import type { S3ConnectionConfig } from './types.js'; -export function getProviderForUser(userId: string, bucket: string): StorageProvider { - const connection = getUserConnection(userId); - if (!connection) { - throw error(401, 'No storage connection configured'); - } - - if (connection.type !== 's3') { +/** + * Construct a bucket-scoped storage provider from the given connection config. + * Throws a 400 HTTP error if the connection type is not supported. + */ +export function getProvider(config: S3ConnectionConfig, bucket: string): StorageProvider { + if (config.type !== 's3') { throw error(400, 'Storage backend not supported'); } - return StorageProviderFactory.create({ ...connection, bucket }); + return StorageProviderFactory.create({ ...config, bucket }); } diff --git a/src/lib/storage/connection-storage.ts b/src/lib/storage/connection-storage.ts index 9bf1f3ea..bf868931 100644 --- a/src/lib/storage/connection-storage.ts +++ b/src/lib/storage/connection-storage.ts @@ -1,7 +1,14 @@ import type { z } from 'zod'; import type { StorageConnectionSchema } from './schemas.js'; -type StoredConnection = z.infer; +export type StoredConnection = z.infer; + +/** + * HTTP header name used to pass the S3 connection config from the browser to + * the backend API endpoints. Defined here (client-safe module) so both client + * and server code can import it without leaking server-only code to the browser. + */ +export const STORAGE_CONNECTION_HEADER = 'x-storage-connection'; const STORAGE_KEY = 'stackable_storage_connections'; @@ -62,3 +69,11 @@ export function removeConnectionLocally(data: StoredConnection): void { // localStorage may be unavailable } } + +/** + * Encode a connection config as a base64 JSON string suitable for the + * `X-Storage-Connection` request header sent to the backend. + */ +export function getConnectionHeader(connection: StoredConnection): string { + return btoa(JSON.stringify(connection)); +} diff --git a/src/lib/storage/download.ts b/src/lib/storage/download.ts index 7f7ad963..ef447a63 100644 --- a/src/lib/storage/download.ts +++ b/src/lib/storage/download.ts @@ -2,15 +2,21 @@ * Client-side utility for downloading a single S3 object via the server proxy. * * Strategy: - * 1. Send a HEAD request to validate credentials and access rights without - * transferring the object body (server uses HeadObject internally). + * 1. Fetch the object with the `X-Storage-Connection` header carrying the + * connection config from localStorage. * 2. On error: throw a `DownloadError` with a typed `code` so the caller can * display a localised message. - * 3. On success: trigger a native browser download via a programmatic anchor - * click. The browser streams the object directly to disk — no client-side - * buffering occurs regardless of file size. + * 3. On success: create a Blob URL and trigger a native browser download via a + * programmatic anchor click. + * + * Note: The response body is buffered as a Blob before the download link is + * constructed. This avoids exposing credentials in the URL (query-param approach) + * while keeping the implementation simple. For very large files this will use + * proportional browser memory — see TECH_DEBT.md for the long-term fix. */ +import { STORAGE_CONNECTION_HEADER } from '$lib/storage/connection-storage.js'; + export type DownloadErrorCode = | 'not_connected' | 'access_denied' @@ -43,33 +49,42 @@ function mapStatusToCode(status: number): DownloadErrorCode { /** * Download a single S3 object. * - * Performs a HEAD pre-flight to surface auth and access errors as typed - * `DownloadError` exceptions, then triggers a native browser download for - * the actual transfer so the file streams straight to disk. + * Fetches the object with the connection config header, buffers it as a Blob, + * then triggers a native browser download via a programmatic anchor click. * - * @throws {DownloadError} when the server returns a non-2xx response on the - * pre-flight check. + * @throws {DownloadError} when the server returns a non-2xx response. */ -export async function downloadObject(bucket: string, key: string): Promise { +export async function downloadObject( + bucket: string, + key: string, + connectionHeader: string +): Promise { const url = buildDownloadUrl(bucket, key); - // Pre-flight: validate credentials and access without fetching the body. - const check = await fetch(url, { method: 'HEAD' }); - if (!check.ok) { - const code = mapStatusToCode(check.status); - throw new DownloadError(code, `Download pre-flight failed with status ${check.status}`); + const response = await fetch(url, { + headers: { [STORAGE_CONNECTION_HEADER]: connectionHeader } + }); + + if (!response.ok) { + const code = mapStatusToCode(response.status); + throw new DownloadError(code, `Download failed with status ${response.status}`); } + const blob = await response.blob(); + const blobUrl = URL.createObjectURL(blob); + // Derive filename from the key (last path segment). const filename = key.split('/').filter(Boolean).pop() ?? key; - // Trigger a native browser download. The browser sends the session cookie - // automatically and streams the response body directly to disk. const anchor = document.createElement('a'); - anchor.href = url; + anchor.href = blobUrl; anchor.download = filename; anchor.style.display = 'none'; document.body.appendChild(anchor); anchor.click(); document.body.removeChild(anchor); + + // Release the object URL after a short delay to allow the browser to initiate + // the download before the URL is revoked. + setTimeout(() => URL.revokeObjectURL(blobUrl), 10_000); } diff --git a/src/lib/storage/state.svelte.ts b/src/lib/storage/state.svelte.ts index c15ccc6c..e3f8eb49 100644 --- a/src/lib/storage/state.svelte.ts +++ b/src/lib/storage/state.svelte.ts @@ -16,6 +16,7 @@ import { downloadObject, DownloadError } from '$lib/storage/download.js'; import { addToast } from '$lib/stores/toast.svelte.js'; import { ActionError, getActionErrorMessage } from './errors.js'; import { BookmarksState } from './bookmarks.svelte.js'; +import { loadConnectionLocally, getConnectionHeader } from '$lib/storage/connection-storage.js'; export class StorageState { // ── Core data (synced from server load) ── @@ -269,7 +270,12 @@ export class StorageState { this.bookmarks.recordFileVisit(this.bucket, f.key, f.size); } try { - await downloadObject(this.bucket, key); + const conn = loadConnectionLocally(); + if (!conn) { + addToast('error', m.storage_download_error_unknown()); + return; + } + await downloadObject(this.bucket, key, getConnectionHeader(conn)); } catch (err: unknown) { if (err instanceof DownloadError) { addToast('error', getActionErrorMessage(new ActionError(err.code, err.message))); @@ -357,7 +363,10 @@ export class StorageState { const params = new SvelteURLSearchParams({ bucket }); for (const key of keys) params.append('keys', key); - const res = await fetch(`/storage/api/delete?${params}`, { method: 'DELETE' }); + const conn = loadConnectionLocally(); + const headers: HeadersInit = conn ? { 'x-storage-connection': getConnectionHeader(conn) } : {}; + + const res = await fetch(`/storage/api/delete?${params}`, { method: 'DELETE', headers }); if (!res.ok) { let code: string; if (res.status === 401) code = 'not_connected'; diff --git a/src/lib/storage/upload.ts b/src/lib/storage/upload.ts index 8865b144..0f366405 100644 --- a/src/lib/storage/upload.ts +++ b/src/lib/storage/upload.ts @@ -9,8 +9,12 @@ * supported in Firefox. * - The file is sent as the raw request body — no base64 or multipart encoding. * The server streams it directly to S3, preserving binary integrity. + * - The connection config is passed via the `X-Storage-Connection` header + * (base64-encoded JSON), read by the server from the client's localStorage. */ +import { STORAGE_CONNECTION_HEADER } from '$lib/storage/connection-storage.js'; + export type UploadErrorCode = | 'not_connected' | 'access_denied' @@ -53,9 +57,16 @@ function mapStatusToUploadCode(status: number): UploadErrorCode { * so no object body is transferred. Returns `true` if the object exists, `false` * if it does not. Throws `UploadError` for auth or server errors. */ -export async function checkObjectExists(bucket: string, key: string): Promise { +export async function checkObjectExists( + bucket: string, + key: string, + connectionHeader: string +): Promise { const url = buildDownloadUrl(bucket, key); - const res = await fetch(url, { method: 'HEAD' }); + const res = await fetch(url, { + method: 'HEAD', + headers: { [STORAGE_CONNECTION_HEADER]: connectionHeader } + }); if (res.status === 200) return true; if (res.status === 404) return false; if (res.status === 401) throw new UploadError('not_connected', 'Not connected'); @@ -77,7 +88,8 @@ export function uploadFile( bucket: string, key: string, file: File, - onProgress: (pct: number) => void + onProgress: (pct: number) => void, + connectionHeader: string ): Promise { return new Promise((resolve, reject) => { const xhr = new XMLHttpRequest(); @@ -109,6 +121,7 @@ export function uploadFile( xhr.open('POST', url); xhr.setRequestHeader('Content-Type', file.type || 'application/octet-stream'); + xhr.setRequestHeader(STORAGE_CONNECTION_HEADER, connectionHeader); xhr.send(file); }); } diff --git a/src/routes/(app)/storage/+layout.server.ts b/src/routes/(app)/storage/+layout.server.ts index ce04ed41..17175c59 100644 --- a/src/routes/(app)/storage/+layout.server.ts +++ b/src/routes/(app)/storage/+layout.server.ts @@ -1,7 +1,5 @@ import type { LayoutServerLoad } from './$types'; import { error } from '@sveltejs/kit'; -import { getConnection, listBuckets } from '$lib/server/storage/service.js'; -import { getUserId } from '$lib/server/auth-utils.js'; import { storageBrowserEnabled } from '$lib/server/feature-flags.js'; export const load: LayoutServerLoad = async ({ locals }) => { @@ -9,18 +7,9 @@ export const load: LayoutServerLoad = async ({ locals }) => { throw error(404, 'Not found'); } - const userId = getUserId(locals); - const connection = getConnection(userId); + locals.logger.debug('loading storage layout'); - if (!connection) { - return { connected: false, buckets: [] as string[], connectionType: null }; - } - - try { - const buckets = await listBuckets(userId); - return { connected: true, buckets, connectionType: connection.type }; - } catch (err) { - locals.logger.warn({ err }, 'failed to list buckets for storage layout'); - return { connected: true, buckets: [] as string[], connectionType: connection.type }; - } + // Connected state and bucket list are determined client-side from localStorage + // and populated by the universal +layout.ts load after hydration. + return { connected: false, buckets: [] as string[], connectionType: null }; }; diff --git a/src/routes/(app)/storage/+layout.ts b/src/routes/(app)/storage/+layout.ts new file mode 100644 index 00000000..4b4c5e10 --- /dev/null +++ b/src/routes/(app)/storage/+layout.ts @@ -0,0 +1,35 @@ +import { browser } from '$app/environment'; +import { loadConnectionLocally, getConnectionHeader } from '$lib/storage/connection-storage.js'; +import type { LayoutLoad } from './$types'; + +const DISCONNECTED = { connected: false, buckets: [] as string[], connectionType: null }; + +/** + * Client-side load: reads the connection from localStorage and fetches the bucket + * list from the server. During SSR this returns the disconnected default — the + * layout re-runs on the client after hydration to populate the real state. + */ +export const load: LayoutLoad = async ({ fetch, url }) => { + if (!browser) return DISCONNECTED; + + // When the user has just explicitly disconnected, skip the auto-reconnect. + if (url.searchParams.has('disconnected')) return DISCONNECTED; + + const connection = loadConnectionLocally(); + if (!connection) return DISCONNECTED; + + const header = getConnectionHeader(connection); + + try { + const res = await fetch('/storage/api/buckets', { + headers: { 'x-storage-connection': header } + }); + + if (!res.ok) return DISCONNECTED; + + const buckets = (await res.json()) as string[]; + return { connected: true, buckets, connectionType: connection.type }; + } catch { + return DISCONNECTED; + } +}; diff --git a/src/routes/(app)/storage/+page.server.ts b/src/routes/(app)/storage/+page.server.ts index 48fdd74d..b383aec3 100644 --- a/src/routes/(app)/storage/+page.server.ts +++ b/src/routes/(app)/storage/+page.server.ts @@ -3,21 +3,13 @@ import { superValidate, message } from 'sveltekit-superforms'; import { zod4 as zod } from 'sveltekit-superforms/adapters'; import type { Actions, PageServerLoad } from './$types'; import { StorageConnectionSchema } from '$lib/storage/schemas.js'; -import { getUserId } from '$lib/server/auth-utils.js'; -import { - getConnection, - saveConnection, - clearConnection, - listBuckets -} from '$lib/server/storage/service.js'; +import { listBuckets } from '$lib/server/storage/service.js'; import type { S3ConnectionConfig } from '$lib/server/storage/types.js'; export const load: PageServerLoad = async ({ locals }) => { const connectionForm = await superValidate(zod(StorageConnectionSchema)); - const userId = getUserId(locals); - const connected = getConnection(userId) !== null; - locals.logger.debug({ connected }, 'loading storage page'); - return { connectionForm, connected }; + locals.logger.debug('loading storage page'); + return { connectionForm }; }; export const actions: Actions = { @@ -30,7 +22,6 @@ export const actions: Actions = { return fail(400, { form }); } - const userId = getUserId(locals); const { type, endpoint, pathStyle, region, accessKeyId, secretAccessKey } = form.data; if (type !== 's3') { @@ -47,11 +38,9 @@ export const actions: Actions = { }; try { - saveConnection(userId, config); - await listBuckets(userId); - log.info({ storage_type: type }, 'user storage connection verified and saved'); + await listBuckets(config); + log.info({ storage_type: type }, 'user storage connection verified'); } catch (err) { - clearConnection(userId); log.warn({ err }, 'storage connection test failed'); return message(form, 'Could not connect — check the endpoint and credentials.', { status: 400 @@ -62,9 +51,7 @@ export const actions: Actions = { }, disconnect: async ({ locals }) => { - const userId = getUserId(locals); - clearConnection(userId); - locals.logger.info({ user_id: userId }, 'user storage connection cleared'); + locals.logger.info('user storage connection cleared'); throw redirect(303, '/storage?disconnected=1'); } }; diff --git a/src/routes/(app)/storage/[bucket]/[...prefix]/+page.server.ts b/src/routes/(app)/storage/[bucket]/[...prefix]/+page.server.ts index df190b1b..32a557c6 100644 --- a/src/routes/(app)/storage/[bucket]/[...prefix]/+page.server.ts +++ b/src/routes/(app)/storage/[bucket]/[...prefix]/+page.server.ts @@ -1,26 +1,7 @@ import type { PageServerLoad } from './$types'; -import { listObjects } from '$lib/server/storage/service.js'; -import { getUserId } from '$lib/server/auth-utils.js'; -export const load: PageServerLoad = async ({ locals, params, url }) => { - const userId = getUserId(locals); +export const load: PageServerLoad = async ({ params }) => { const prefix = params.prefix ? params.prefix + '/' : ''; const bucket = params.bucket; - const continuationToken = url.searchParams.get('continuationToken'); - const pageSizeParam = url.searchParams.get('pageSize'); - const pageSize = pageSizeParam ? parseInt(pageSizeParam, 10) : undefined; - - locals.logger.debug( - { bucket, prefix, continuation_token: continuationToken, page_size: pageSize }, - 'loading storage bucket objects' - ); - - const objects = await listObjects( - userId, - bucket, - prefix, - pageSize ?? 25, - continuationToken ?? undefined - ); - return { objects, prefix, bucket }; + return { bucket, prefix }; }; diff --git a/src/routes/(app)/storage/[bucket]/[...prefix]/+page.ts b/src/routes/(app)/storage/[bucket]/[...prefix]/+page.ts new file mode 100644 index 00000000..6a53ef13 --- /dev/null +++ b/src/routes/(app)/storage/[bucket]/[...prefix]/+page.ts @@ -0,0 +1,46 @@ +import { browser } from '$app/environment'; +import { redirect } from '@sveltejs/kit'; +import { loadConnectionLocally, getConnectionHeader } from '$lib/storage/connection-storage.js'; +import type { PageLoad } from './$types'; +import type { StoragePage } from '$lib/storage/types.js'; + +const EMPTY_PAGE: StoragePage = { + objects: [], + hasNextPage: false, + currentPage: 1, + pageSize: 25 +}; + +/** + * Client-side load: fetches the object list from the server using the connection + * config from localStorage. Redirects to /storage if no connection is available. + * During SSR this returns an empty page — the load re-runs on the client. + */ +export const load: PageLoad = async ({ fetch, url, data }) => { + // Always pass server data through so PageData includes bucket/prefix. + const { bucket, prefix } = data; + + if (!browser) return { bucket, prefix, objects: EMPTY_PAGE }; + + const connection = loadConnectionLocally(); + if (!connection) throw redirect(303, '/storage'); + + const continuationToken = url.searchParams.get('continuationToken'); + const pageSizeParam = url.searchParams.get('pageSize'); + + const query = new URLSearchParams({ bucket, prefix: prefix ?? '' }); + if (continuationToken) query.set('continuationToken', continuationToken); + if (pageSizeParam) query.set('pageSize', pageSizeParam); + + const res = await fetch(`/storage/api/objects?${query}`, { + headers: { 'x-storage-connection': getConnectionHeader(connection) } + }); + + if (!res.ok) { + if (res.status === 401) throw redirect(303, '/storage'); + return { bucket, prefix, objects: EMPTY_PAGE }; + } + + const objects = (await res.json()) as StoragePage; + return { bucket, prefix, objects }; +}; diff --git a/src/routes/(app)/storage/[bucket]/[...prefix]/page.server.test.ts b/src/routes/(app)/storage/[bucket]/[...prefix]/page.server.test.ts index e525528c..1ef2b010 100644 --- a/src/routes/(app)/storage/[bucket]/[...prefix]/page.server.test.ts +++ b/src/routes/(app)/storage/[bucket]/[...prefix]/page.server.test.ts @@ -1,62 +1,29 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; - -vi.mock('$lib/server/storage/service.js', () => ({ - listObjects: vi.fn() -})); - -vi.mock('$lib/server/auth-utils.js', () => ({ - getUserId: vi.fn(() => 'test-user') -})); +import { describe, it, expect } from 'vitest'; import { load } from './+page.server.js'; -import { listObjects } from '$lib/server/storage/service.js'; -function mockEvent( - opts: { bucket?: string; prefix?: string; searchParams?: Record } = {} -) { - const url = new URL('http://localhost/storage/b1/some/prefix'); - for (const [k, v] of Object.entries(opts.searchParams ?? {})) { - url.searchParams.set(k, v); - } +function mockEvent(opts: { bucket?: string; prefix?: string } = {}) { return { locals: { logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, user: { id: 'test-user' } }, - params: { bucket: opts.bucket ?? 'my-bucket', prefix: opts.prefix ?? '' }, - url + params: { bucket: opts.bucket ?? 'my-bucket', prefix: opts.prefix ?? '' } } as unknown as Parameters[0]; } -describe('bucket page load', () => { - beforeEach(() => vi.clearAllMocks()); - - it('lists objects with default page size', async () => { - vi.mocked(listObjects).mockResolvedValue({ items: [], prefixes: [] } as unknown as Awaited< - ReturnType - >); - - const result = (await load(mockEvent())) as { bucket: string; prefix: string }; +import { vi } from 'vitest'; - expect(listObjects).toHaveBeenCalledWith('test-user', 'my-bucket', '', 25, undefined); - expect(result.bucket).toBe('my-bucket'); - expect(result.prefix).toBe(''); +describe('bucket page server load', () => { + it('returns bucket and prefix', async () => { + const result = await load(mockEvent()); + expect(result).toEqual({ bucket: 'my-bucket', prefix: '' }); }); it('adds trailing slash to prefix', async () => { - vi.mocked(listObjects).mockResolvedValue({ items: [] } as unknown as Awaited< - ReturnType - >); - - await load(mockEvent({ prefix: 'data/2024' })); - - expect(listObjects).toHaveBeenCalledWith('test-user', 'my-bucket', 'data/2024/', 25, undefined); + const result = await load(mockEvent({ prefix: 'data/2024' })); + expect(result).toEqual({ bucket: 'my-bucket', prefix: 'data/2024/' }); }); - it('passes continuationToken and pageSize', async () => { - vi.mocked(listObjects).mockResolvedValue({ items: [] } as unknown as Awaited< - ReturnType - >); - - await load(mockEvent({ searchParams: { continuationToken: 'abc', pageSize: '50' } })); - - expect(listObjects).toHaveBeenCalledWith('test-user', 'my-bucket', '', 50, 'abc'); + it('handles empty prefix', async () => { + const result = await load(mockEvent({ bucket: 'test-bucket', prefix: '' })); + expect(result).toEqual({ bucket: 'test-bucket', prefix: '' }); }); }); diff --git a/src/routes/(app)/storage/api/buckets/+server.ts b/src/routes/(app)/storage/api/buckets/+server.ts new file mode 100644 index 00000000..620f983a --- /dev/null +++ b/src/routes/(app)/storage/api/buckets/+server.ts @@ -0,0 +1,16 @@ +import { listBuckets } from '$lib/server/storage/service.js'; +import { requireConnection } from '$lib/server/storage/connection.js'; +import type { RequestHandler } from './$types'; + +/** + * GET /storage/api/buckets + * + * Returns the list of buckets accessible with the connection config supplied in + * the `X-Storage-Connection` request header (base64-encoded JSON). + */ +export const GET: RequestHandler = async ({ request, locals }) => { + const config = requireConnection(request); + const buckets = await listBuckets(config); + locals.logger.debug({ bucket_count: buckets.length }, 'bucket list returned'); + return Response.json(buckets); +}; diff --git a/src/routes/(app)/storage/api/delete/+server.ts b/src/routes/(app)/storage/api/delete/+server.ts index 174e84b4..4892bb35 100644 --- a/src/routes/(app)/storage/api/delete/+server.ts +++ b/src/routes/(app)/storage/api/delete/+server.ts @@ -1,7 +1,7 @@ import type { RequestHandler } from './$types'; import { error } from '@sveltejs/kit'; import { deleteObjects } from '$lib/server/storage/service.js'; -import { getUserId } from '$lib/server/auth-utils.js'; +import { requireConnection } from '$lib/server/storage/connection.js'; import { requireBucket } from '../params.js'; /** @@ -9,20 +9,22 @@ import { requireBucket } from '../params.js'; * * Deletes one or more S3 objects from the given bucket. * Authentication is enforced by the app-level auth guard in hooks.server.ts. + * + * The connection config is read from the `X-Storage-Connection` request header + * (base64-encoded JSON), set by the client from its localStorage entry. */ -export const DELETE: RequestHandler = async ({ locals, url }) => { +export const DELETE: RequestHandler = async ({ locals, url, request }) => { const bucket = requireBucket(url); + const config = requireConnection(request); const keys = url.searchParams.getAll('keys'); if (!keys.length) { throw error(400, 'Missing required query parameter: keys'); } - const userId = getUserId(locals); - locals.logger.debug({ bucket, key_count: keys.length }, 'delete request received'); - const result = await deleteObjects(userId, bucket, keys); + const result = await deleteObjects(config, bucket, keys); locals.logger.info( { bucket, key_count: keys.length, failed_count: result.failed.length }, diff --git a/src/routes/(app)/storage/api/delete/server.test.ts b/src/routes/(app)/storage/api/delete/server.test.ts index acda7403..35783ccb 100644 --- a/src/routes/(app)/storage/api/delete/server.test.ts +++ b/src/routes/(app)/storage/api/delete/server.test.ts @@ -5,13 +5,18 @@ vi.mock('$lib/server/storage/service.js', () => ({ deleteObjects: vi.fn() })); -vi.mock('$lib/server/auth-utils.js', () => ({ - getUserId: vi.fn(() => 'test-user') +vi.mock('$lib/server/storage/connection.js', () => ({ + requireConnection: vi.fn(() => ({ type: 's3', region: 'us-east-1' })), + STORAGE_CONNECTION_HEADER: 'x-storage-connection' })); import { DELETE } from './+server.js'; import { deleteObjects } from '$lib/server/storage/service.js'; +const CONNECTION_HEADER = { + 'x-storage-connection': btoa(JSON.stringify({ type: 's3', region: 'us-east-1' })) +}; + function mockEvent(searchParams: Record) { const url = new URL('http://localhost/storage/api/delete'); for (const [k, v] of Object.entries(searchParams)) { @@ -23,6 +28,7 @@ function mockEvent(searchParams: Record) { } return { url, + request: { headers: new Headers(CONNECTION_HEADER) }, locals: { logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, user: { id: 'test-user' } } } as unknown as Parameters[0]; } @@ -50,7 +56,7 @@ describe('DELETE /storage/api/delete', () => { ); const response = await DELETE(mockEvent({ bucket: 'b1', keys })); - expect(deleteObjects).toHaveBeenCalledWith('test-user', 'b1', keys); + expect(deleteObjects).toHaveBeenCalledWith(expect.objectContaining({ type: 's3' }), 'b1', keys); expect(await response.json()).toEqual(result); }); }); diff --git a/src/routes/(app)/storage/api/download/+server.ts b/src/routes/(app)/storage/api/download/+server.ts index 0a2c984a..e43df827 100644 --- a/src/routes/(app)/storage/api/download/+server.ts +++ b/src/routes/(app)/storage/api/download/+server.ts @@ -1,6 +1,6 @@ import type { RequestHandler } from './$types'; import { downloadObject, getObjectMetadata } from '$lib/server/storage/service.js'; -import { getUserId } from '$lib/server/auth-utils.js'; +import { requireConnection } from '$lib/server/storage/connection.js'; import { requireBucketKey } from '../params.js'; /** Derive the bare filename from a (possibly path-prefixed) object key. */ @@ -15,14 +15,17 @@ function filenameFromKey(key: string): string { * Authentication is enforced by the app-level auth guard in hooks.server.ts. * The S3 body stream is piped straight to the HTTP response — no server-side * buffering occurs. + * + * The connection config is read from the `X-Storage-Connection` request header + * (base64-encoded JSON), set by the client from its localStorage entry. */ -export const GET: RequestHandler = async ({ locals, url }) => { +export const GET: RequestHandler = async ({ locals, url, request }) => { const { bucket, key } = requireBucketKey(url); - const userId = getUserId(locals); + const config = requireConnection(request); locals.logger.debug({ bucket, key }, 'download request received'); - const download = await downloadObject(userId, bucket, key); + const download = await downloadObject(config, bucket, key); const filename = filenameFromKey(key); // RFC 5987 encoding for non-ASCII filenames in Content-Disposition @@ -53,16 +56,17 @@ export const GET: RequestHandler = async ({ locals, url }) => { * * Lightweight pre-flight that validates credentials and access rights using * a HeadObject call (no object body transferred). The client uses this before - * initiating a native browser download to surface auth/not-found errors as - * inline UI messages rather than browser download failures. + * initiating a download to surface auth/not-found errors as inline UI messages. + * + * The connection config is read from the `X-Storage-Connection` request header. */ -export const HEAD: RequestHandler = async ({ locals, url }) => { +export const HEAD: RequestHandler = async ({ locals, url, request }) => { const { bucket, key } = requireBucketKey(url); - const userId = getUserId(locals); + const config = requireConnection(request); locals.logger.debug({ bucket, key }, 'download pre-flight check'); - const meta = await getObjectMetadata(userId, bucket, key); + const meta = await getObjectMetadata(config, bucket, key); return new Response(null, { status: 200, diff --git a/src/routes/(app)/storage/api/download/server.test.ts b/src/routes/(app)/storage/api/download/server.test.ts index 0ed933f8..d0304e87 100644 --- a/src/routes/(app)/storage/api/download/server.test.ts +++ b/src/routes/(app)/storage/api/download/server.test.ts @@ -5,17 +5,23 @@ vi.mock('$lib/server/storage/service.js', () => ({ getObjectMetadata: vi.fn() })); -vi.mock('$lib/server/auth-utils.js', () => ({ - getUserId: vi.fn(() => 'test-user') +vi.mock('$lib/server/storage/connection.js', () => ({ + requireConnection: vi.fn(() => ({ type: 's3', region: 'us-east-1' })), + STORAGE_CONNECTION_HEADER: 'x-storage-connection' })); import { GET, HEAD } from './+server.js'; import { downloadObject, getObjectMetadata } from '$lib/server/storage/service.js'; +const CONNECTION_HEADER = { + 'x-storage-connection': btoa(JSON.stringify({ type: 's3', region: 'us-east-1' })) +}; + function mockEvent(params: string) { const url = new URL(`http://localhost/storage/api/download?${params}`); return { url, + request: { headers: new Headers(CONNECTION_HEADER) }, locals: { logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, user: { id: 'test-user' } } } as unknown as Parameters[0]; } diff --git a/src/routes/(app)/storage/api/objects/+server.ts b/src/routes/(app)/storage/api/objects/+server.ts new file mode 100644 index 00000000..d1fb56f9 --- /dev/null +++ b/src/routes/(app)/storage/api/objects/+server.ts @@ -0,0 +1,30 @@ +import { error } from '@sveltejs/kit'; +import { listObjects } from '$lib/server/storage/service.js'; +import { requireConnection } from '$lib/server/storage/connection.js'; +import type { RequestHandler } from './$types'; + +/** + * GET /storage/api/objects?bucket=&prefix=&pageSize=&continuationToken= + * + * Returns a page of objects in the given bucket/prefix using the connection + * config supplied in the `X-Storage-Connection` request header. + */ +export const GET: RequestHandler = async ({ request, url, locals }) => { + const config = requireConnection(request); + + const bucket = url.searchParams.get('bucket')?.trim(); + if (!bucket) throw error(400, 'Missing required query parameter: bucket'); + + const prefix = url.searchParams.get('prefix') ?? ''; + const continuationToken = url.searchParams.get('continuationToken'); + const pageSizeParam = url.searchParams.get('pageSize'); + const pageSize = pageSizeParam ? parseInt(pageSizeParam, 10) : 25; + + locals.logger.debug( + { bucket, prefix, continuation_token: continuationToken, page_size: pageSize }, + 'listing objects' + ); + + const page = await listObjects(config, bucket, prefix, pageSize, continuationToken); + return Response.json(page); +}; diff --git a/src/routes/(app)/storage/api/preview/+server.ts b/src/routes/(app)/storage/api/preview/+server.ts index 193ad363..f6c6e7f7 100644 --- a/src/routes/(app)/storage/api/preview/+server.ts +++ b/src/routes/(app)/storage/api/preview/+server.ts @@ -1,19 +1,25 @@ import { S3ServiceException } from '@aws-sdk/client-s3'; -import { getUserId } from '$lib/server/auth-utils.js'; import { mapS3ErrorToHttp } from '$lib/server/storage/s3-errors.js'; -import { getProviderForUser } from '$lib/server/storage/utils.js'; +import { getProvider } from '$lib/server/storage/utils.js'; +import { requireConnection } from '$lib/server/storage/connection.js'; // import { parquetPreview } from '$lib/server/storage/preview/parquet.js'; import { binaryPreview, KNOWN_BINARY_TYPES } from '$lib/server/storage/preview/binary.js'; import { streamPreview } from '$lib/server/storage/preview/stream.js'; import { requireBucketKey } from '../params.js'; import type { RequestHandler } from './$types'; -export const GET: RequestHandler = async ({ url, locals }) => { +/** + * GET /storage/api/preview?bucket=&key= + * + * The connection config is read from the `X-Storage-Connection` request header + * (base64-encoded JSON), set by the client from its localStorage entry. + */ +export const GET: RequestHandler = async ({ url, locals, request }) => { const log = locals.logger; - const userId = getUserId(locals); + const config = requireConnection(request); const { bucket, key } = requireBucketKey(url); - const provider = getProviderForUser(userId, bucket); + const provider = getProvider(config, bucket); try { const metadata = await provider.getMetadata(key); @@ -29,7 +35,7 @@ export const GET: RequestHandler = async ({ url, locals }) => { if (isParquet) { log.info( - { user_id: userId, bucket, key, content_type: rawContentType, total_size: totalSize }, + { bucket, key, content_type: rawContentType, total_size: totalSize }, 'parquet preview disabled' ); // TODO: re-enable once we have a more robust parquet preview solution in place @@ -39,7 +45,7 @@ export const GET: RequestHandler = async ({ url, locals }) => { // Skip body fetch for known-binary formats — client will show fallback immediately. if (KNOWN_BINARY_TYPES.has(rawContentType)) { log.info( - { user_id: userId, bucket, key, content_type: rawContentType }, + { bucket, key, content_type: rawContentType }, 'skipping preview fetch for known-binary type' ); return binaryPreview(rawContentType, totalSize); @@ -52,7 +58,8 @@ export const GET: RequestHandler = async ({ url, locals }) => { ? 'text/csv' : rawContentType; - return await streamPreview(provider, key, contentType, totalSize, userId, log); + // Pass a placeholder user identifier for logging purposes (no longer user-specific) + return await streamPreview(provider, key, contentType, totalSize, 'client', log); } catch (err) { if (err instanceof S3ServiceException) { mapS3ErrorToHttp(err, { bucket, key, operation: 'preview' }); diff --git a/src/routes/(app)/storage/api/preview/server.test.ts b/src/routes/(app)/storage/api/preview/server.test.ts index d0867554..79f72594 100644 --- a/src/routes/(app)/storage/api/preview/server.test.ts +++ b/src/routes/(app)/storage/api/preview/server.test.ts @@ -3,11 +3,12 @@ import { S3ServiceException } from '@aws-sdk/client-s3'; const mockGetMetadata = vi.fn(); vi.mock('$lib/server/storage/utils.js', () => ({ - getProviderForUser: vi.fn(() => ({ getMetadata: mockGetMetadata })) + getProvider: vi.fn(() => ({ getMetadata: mockGetMetadata })) })); -vi.mock('$lib/server/auth-utils.js', () => ({ - getUserId: vi.fn(() => 'test-user') +vi.mock('$lib/server/storage/connection.js', () => ({ + requireConnection: vi.fn(() => ({ type: 's3', region: 'us-east-1' })), + STORAGE_CONNECTION_HEADER: 'x-storage-connection' })); vi.mock('$lib/server/storage/preview/binary.js', () => ({ @@ -30,10 +31,15 @@ import { binaryPreview } from '$lib/server/storage/preview/binary.js'; import { streamPreview } from '$lib/server/storage/preview/stream.js'; import { mapS3ErrorToHttp } from '$lib/server/storage/s3-errors.js'; +const CONNECTION_HEADER = { + 'x-storage-connection': btoa(JSON.stringify({ type: 's3', region: 'us-east-1' })) +}; + function mockEvent(params: string) { const url = new URL(`http://localhost/storage/api/preview?${params}`); return { url, + request: { headers: new Headers(CONNECTION_HEADER) }, locals: { logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, user: { id: 'test-user' } } } as unknown as Parameters[0]; } @@ -59,7 +65,7 @@ describe('GET /storage/api/preview', () => { 'data.csv', 'text/csv', 100, - 'test-user', + 'client', expect.anything() ); }); @@ -74,7 +80,7 @@ describe('GET /storage/api/preview', () => { 'readme.txt', 'text/plain', 50, - 'test-user', + 'client', expect.anything() ); }); diff --git a/src/routes/(app)/storage/api/upload/+server.ts b/src/routes/(app)/storage/api/upload/+server.ts index 696e97c2..dc58c546 100644 --- a/src/routes/(app)/storage/api/upload/+server.ts +++ b/src/routes/(app)/storage/api/upload/+server.ts @@ -1,6 +1,6 @@ import { error } from '@sveltejs/kit'; import { uploadObject } from '$lib/server/storage/service.js'; -import { getUserId } from '$lib/server/auth-utils.js'; +import { requireConnection } from '$lib/server/storage/connection.js'; import { requireBucketKey } from '../params.js'; import type { RequestHandler } from './$types'; @@ -11,16 +11,12 @@ import type { RequestHandler } from './$types'; * @aws-sdk/lib-storage). The request body is piped to the S3 SDK without * buffering in server memory, regardless of file size. * - * Assumptions (v0): - * - Credentials come from the in-memory per-user S3 connection config - * (same as download/preview). No production credential vaulting. - * - No server-side file size limit is enforced; S3's 5 TB object limit applies. - * - A single Content-Type header value is trusted from the client and forwarded - * to S3 as-is. + * The connection config is read from the `X-Storage-Connection` request header + * (base64-encoded JSON), set by the client from its localStorage entry. */ export const POST: RequestHandler = async ({ locals, url, request }) => { const log = locals.logger; - const userId = getUserId(locals); + const config = requireConnection(request); const { bucket, key } = requireBucketKey(url); if (!request.body) { @@ -39,7 +35,7 @@ export const POST: RequestHandler = async ({ locals, url, request }) => { 'upload request received' ); - await uploadObject(userId, bucket, key, request.body, contentType, contentLength); + await uploadObject(config, bucket, key, request.body, contentType, contentLength); log.info( { bucket, key, content_type: contentType, content_length: contentLength }, diff --git a/src/routes/(app)/storage/api/upload/server.test.ts b/src/routes/(app)/storage/api/upload/server.test.ts index 0f3bb6ed..b8adfdb9 100644 --- a/src/routes/(app)/storage/api/upload/server.test.ts +++ b/src/routes/(app)/storage/api/upload/server.test.ts @@ -4,13 +4,18 @@ vi.mock('$lib/server/storage/service.js', () => ({ uploadObject: vi.fn() })); -vi.mock('$lib/server/auth-utils.js', () => ({ - getUserId: vi.fn(() => 'test-user') +vi.mock('$lib/server/storage/connection.js', () => ({ + requireConnection: vi.fn(() => ({ type: 's3', region: 'us-east-1' })), + STORAGE_CONNECTION_HEADER: 'x-storage-connection' })); import { POST } from './+server.js'; import { uploadObject } from '$lib/server/storage/service.js'; +const CONNECTION_HEADER = { + 'x-storage-connection': btoa(JSON.stringify({ type: 's3', region: 'us-east-1' })) +}; + function mockEvent(opts: { params?: string; body?: ReadableStream | null; @@ -20,7 +25,7 @@ function mockEvent(opts: { `http://localhost/storage/api/upload?${opts.params ?? 'bucket=b1&key=file.txt'}` ); const headers = new Headers( - opts.headers ?? { 'Content-Type': 'text/plain', 'Content-Length': '42' } + opts.headers ?? { 'Content-Type': 'text/plain', 'Content-Length': '42', ...CONNECTION_HEADER } ); const body = 'body' in opts ? opts.body : new ReadableStream(); return { @@ -52,13 +57,17 @@ describe('POST /storage/api/upload', () => { const res = await POST( mockEvent({ body, - headers: { 'Content-Type': 'image/png; charset=utf-8', 'Content-Length': '100' } + headers: { + 'Content-Type': 'image/png; charset=utf-8', + 'Content-Length': '100', + ...CONNECTION_HEADER + } }) ); expect(res.status).toBe(201); expect(uploadObject).toHaveBeenCalledWith( - 'test-user', + expect.objectContaining({ type: 's3' }), 'b1', 'file.txt', body, @@ -70,7 +79,7 @@ describe('POST /storage/api/upload', () => { it('defaults content type to application/octet-stream', async () => { vi.mocked(uploadObject).mockResolvedValue(undefined); - const res = await POST(mockEvent({ headers: {} })); + const res = await POST(mockEvent({ headers: { ...CONNECTION_HEADER } })); expect(res.status).toBe(201); expect(vi.mocked(uploadObject).mock.calls[0][4]).toBe('application/octet-stream'); diff --git a/src/routes/(app)/storage/layout.server.test.ts b/src/routes/(app)/storage/layout.server.test.ts index 25d1e89e..7edce12a 100644 --- a/src/routes/(app)/storage/layout.server.test.ts +++ b/src/routes/(app)/storage/layout.server.test.ts @@ -7,17 +7,7 @@ vi.mock('$lib/server/feature-flags.js', () => ({ } })); -vi.mock('$lib/server/storage/service.js', () => ({ - getConnection: vi.fn(), - listBuckets: vi.fn() -})); - -vi.mock('$lib/server/auth-utils.js', () => ({ - getUserId: vi.fn(() => 'test-user') -})); - import { load } from './+layout.server.js'; -import { getConnection, listBuckets } from '$lib/server/storage/service.js'; function mockEvent() { return { @@ -25,7 +15,7 @@ function mockEvent() { } as unknown as Parameters[0]; } -describe('storage layout load', () => { +describe('storage layout server load', () => { beforeEach(() => vi.clearAllMocks()); it('throws 404 when storage browser is disabled', async () => { @@ -33,37 +23,9 @@ describe('storage layout load', () => { await expect(load(mockEvent())).rejects.toThrow(expect.objectContaining({ status: 404 })); }); - it('returns connected:false when no connection', async () => { + it('returns disconnected default state', async () => { mockStorageBrowserEnabled.mockReturnValue(true); - vi.mocked(getConnection).mockReturnValue(null); - const result = await load(mockEvent()); expect(result).toEqual({ connected: false, buckets: [], connectionType: null }); }); - - it('returns buckets when connected', async () => { - mockStorageBrowserEnabled.mockReturnValue(true); - vi.mocked(getConnection).mockReturnValue({ type: 's3' } as unknown as ReturnType< - typeof getConnection - >); - vi.mocked(listBuckets).mockResolvedValue(['bucket-a', 'bucket-b']); - - const result = await load(mockEvent()); - expect(result).toEqual({ - connected: true, - buckets: ['bucket-a', 'bucket-b'], - connectionType: 's3' - }); - }); - - it('returns empty buckets when listBuckets fails', async () => { - mockStorageBrowserEnabled.mockReturnValue(true); - vi.mocked(getConnection).mockReturnValue({ type: 's3' } as unknown as ReturnType< - typeof getConnection - >); - vi.mocked(listBuckets).mockRejectedValue(new Error('network')); - - const result = await load(mockEvent()); - expect(result).toEqual({ connected: true, buckets: [], connectionType: 's3' }); - }); }); diff --git a/src/routes/(app)/storage/page.server.test.ts b/src/routes/(app)/storage/page.server.test.ts index d1766205..207b214c 100644 --- a/src/routes/(app)/storage/page.server.test.ts +++ b/src/routes/(app)/storage/page.server.test.ts @@ -1,16 +1,9 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; vi.mock('$lib/server/storage/service.js', () => ({ - getConnection: vi.fn(), - saveConnection: vi.fn(), - clearConnection: vi.fn(), listBuckets: vi.fn() })); -vi.mock('$lib/server/auth-utils.js', () => ({ - getUserId: vi.fn(() => 'test-user') -})); - vi.mock('$lib/storage/schemas.js', () => ({ StorageConnectionSchema: {} // superValidate is also mocked })); @@ -25,7 +18,7 @@ vi.mock('sveltekit-superforms/adapters', () => ({ })); import { load, actions } from './+page.server.js'; -import { getConnection, clearConnection, listBuckets } from '$lib/server/storage/service.js'; +import { listBuckets } from '$lib/server/storage/service.js'; import { superValidate } from 'sveltekit-superforms'; function mockLocals() { @@ -35,18 +28,14 @@ function mockLocals() { describe('storage page load', () => { beforeEach(() => vi.clearAllMocks()); - it('returns connectionForm and connected status', async () => { + it('returns connectionForm', async () => { vi.mocked(superValidate).mockResolvedValue({ data: {} } as unknown as Awaited< ReturnType >); - vi.mocked(getConnection).mockReturnValue({ type: 's3' } as unknown as ReturnType< - typeof getConnection - >); const result = (await load({ locals: mockLocals() - } as unknown as Parameters[0])) as { connected: boolean; connectionForm: unknown }; - expect(result.connected).toBe(true); + } as unknown as Parameters[0])) as { connectionForm: unknown }; expect(result.connectionForm).toBeDefined(); }); }); @@ -86,7 +75,7 @@ describe('storage page actions', () => { expect(result).toHaveProperty('message', 'HDFS connections are not yet supported'); }); - it('connect: clears connection and returns error on test failure', async () => { + it('connect: returns error message on connection test failure', async () => { vi.mocked(superValidate).mockResolvedValue({ valid: true, data: { @@ -104,7 +93,6 @@ describe('storage page actions', () => { request: new Request('http://localhost', { method: 'POST' }), locals: mockLocals() } as unknown as Parameters[0]); - expect(clearConnection).toHaveBeenCalledWith('test-user'); expect(result).toHaveProperty( 'message', 'Could not connect — check the endpoint and credentials.' @@ -133,7 +121,7 @@ describe('storage page actions', () => { ).rejects.toThrow(expect.objectContaining({ status: 303, location: '/storage' })); }); - it('disconnect: clears connection and redirects', async () => { + it('disconnect: redirects', async () => { await expect( actions.disconnect({ locals: mockLocals() } as unknown as Parameters< typeof actions.disconnect @@ -141,6 +129,5 @@ describe('storage page actions', () => { ).rejects.toThrow( expect.objectContaining({ status: 303, location: '/storage?disconnected=1' }) ); - expect(clearConnection).toHaveBeenCalledWith('test-user'); }); }); From 6ea6b163758ae656e0813d1ab3f8097ecabee663 Mon Sep 17 00:00:00 2001 From: F3l1x1vo Date: Wed, 3 Jun 2026 08:17:41 +0200 Subject: [PATCH 2/8] #185 update e2e tests for browser connections --- e2e/storage/helpers.ts | 28 ++++++++++ e2e/storage/permissions.spec.ts | 1 + e2e/storage/preview.spec.ts | 8 +-- e2e/storage/recent-items.spec.ts | 27 +++++----- .../storage/sidebar/BucketList.svelte | 8 +-- src/routes/(app)/storage/+error.svelte | 9 +++- .../storage/[bucket]/[...prefix]/+page.svelte | 51 ++++++++++++++++++- .../storage/[bucket]/[...prefix]/+page.ts | 13 ++++- 8 files changed, 120 insertions(+), 25 deletions(-) diff --git a/e2e/storage/helpers.ts b/e2e/storage/helpers.ts index 744a9246..95e26ae0 100644 --- a/e2e/storage/helpers.ts +++ b/e2e/storage/helpers.ts @@ -82,7 +82,35 @@ export async function connectAndOpenPrefix( await connectToStorage(page, credentials); await expect(page).toHaveURL('/storage'); await page.goto(bucketRoute(credentials.bucket, prefix)); + await waitForObjectsLoaded(page); +} + +/** + * Wait for the bucket object listing to be ready after a client-side load. + * With the new architecture, `waitForHydration` alone is insufficient because + * the object list is fetched client-side after hydration. This waits for either + * a table row or the empty-state message to appear, confirming the fetch has + * completed and the UI has updated. + */ +export async function waitForObjectsLoaded(page: Page) { + await waitForHydration(page); + await page + .locator('tbody tr') + .or(page.getByText('This bucket is empty')) + .first() + .waitFor({ timeout: 15_000 }); +} + +/** + * Wait for the storage landing page to display the connected state. + * With the new client-side layout load, navigating to `/storage` initially + * renders the SSR default (disconnected) state. This waits until the + * client-side bucket fetch has completed and the connected UI (recent items + * tabs) is visible. + */ +export async function waitForStorageConnected(page: Page) { await waitForHydration(page); + await expect(page.getByRole('tab', { name: 'Recent Files' })).toBeVisible({ timeout: 15_000 }); } export async function putTextObject( diff --git a/e2e/storage/permissions.spec.ts b/e2e/storage/permissions.spec.ts index 50de8f06..b9b2f467 100644 --- a/e2e/storage/permissions.spec.ts +++ b/e2e/storage/permissions.spec.ts @@ -127,6 +127,7 @@ test.describe('Storage S3 — Permissions', () => { }); await connectToStorage(page, noAccessCredentials); + await expect(page).toHaveURL('/storage'); await page.goto(bucketRoute(noAccessBucket)); await expect(page.getByText('403')).toBeVisible(); diff --git a/e2e/storage/preview.spec.ts b/e2e/storage/preview.spec.ts index e07fc162..84bd1b52 100644 --- a/e2e/storage/preview.spec.ts +++ b/e2e/storage/preview.spec.ts @@ -73,7 +73,7 @@ test.describe('Storage S3 — Preview', () => { await expect(page.getByRole('heading', { name: 'archive.zip' })).toBeVisible(); await expect(page.getByText('Preview unavailable')).toBeVisible(); - await expect(page.getByRole('link', { name: 'Download full file' })).toBeVisible(); + await expect(page.getByRole('button', { name: 'Download full file' })).toBeVisible(); } finally { await deleteKnownKeys(client, credentials.bucket, cleanupKeys); } @@ -103,7 +103,7 @@ test.describe('Storage S3 — Preview', () => { await expect(page.getByRole('heading', { name: 'data.bin' })).toBeVisible(); await expect(page.getByText('Binary content')).toBeVisible(); - await expect(page.getByRole('link', { name: 'Download full file' })).toBeVisible(); + await expect(page.getByRole('button', { name: 'Download full file' })).toBeVisible(); } finally { await deleteKnownKeys(client, credentials.bucket, cleanupKeys); } @@ -172,8 +172,8 @@ test.describe('Storage S3 — Preview', () => { // File size badge is displayed await expect(page.locator('.badge', { hasText: /B$/ })).toBeVisible(); - // Download link is available - await expect(page.getByRole('link', { name: 'Download full file' })).toBeVisible(); + // Download button is available + await expect(page.getByRole('button', { name: 'Download full file' })).toBeVisible(); // Close button works await page.getByRole('button', { name: 'Close' }).last().click(); diff --git a/e2e/storage/recent-items.spec.ts b/e2e/storage/recent-items.spec.ts index 7b30156e..9a53f2b7 100644 --- a/e2e/storage/recent-items.spec.ts +++ b/e2e/storage/recent-items.spec.ts @@ -4,7 +4,6 @@ import { hasGarageCredentials, requireGarageCredentials } from '../support/garage.js'; -import { waitForHydration } from '../support/helpers.js'; import { bucketRoute, connectAndOpenPrefix, @@ -12,7 +11,9 @@ import { deleteKnownKeys, putTextObject, rowByName, - uniquePrefix + uniquePrefix, + waitForObjectsLoaded, + waitForStorageConnected } from './helpers.js'; async function previewFile(page: Page, name: string) { @@ -35,11 +36,12 @@ test.describe('Storage S3 — Recent Items', () => { const credentials = requireGarageCredentials(); await connectToStorage(page, credentials); + await expect(page).toHaveURL('/storage'); await page.goto(bucketRoute(credentials.bucket)); - await waitForHydration(page); + await waitForObjectsLoaded(page); await page.goto('/storage'); - await waitForHydration(page); + await waitForStorageConnected(page); await page.getByRole('tab', { name: 'Recent Locations' }).click(); @@ -63,10 +65,8 @@ test.describe('Storage S3 — Recent Items', () => { await page.getByRole('button', { name: 'Close' }).last().click(); await page.goto('/storage'); - await waitForHydration(page); + await waitForStorageConnected(page); - const filesTab = page.getByRole('tab', { name: 'Recent Files' }); - await expect(filesTab).toBeVisible(); await expect(page.locator('tbody').getByText('recent.txt')).toBeVisible(); } finally { await deleteKnownKeys(client, credentials.bucket, cleanupKeys); @@ -88,12 +88,12 @@ test.describe('Storage S3 — Recent Items', () => { // Confirm it appears in Recent Files await page.goto('/storage'); - await waitForHydration(page); + await waitForStorageConnected(page); await expect(page.locator('tbody').getByText('to-delete.txt')).toBeVisible(); // Delete the file via the UI await page.goto(bucketRoute(credentials.bucket, prefix)); - await waitForHydration(page); + await waitForObjectsLoaded(page); await page.getByRole('button', { name: 'Toggle selection mode' }).click(); await page.getByLabel('Select to-delete.txt').check(); await page.getByRole('button', { name: 'Delete', exact: true }).click(); @@ -102,7 +102,7 @@ test.describe('Storage S3 — Recent Items', () => { // File should no longer appear in Recent Files await page.goto('/storage'); - await waitForHydration(page); + await waitForStorageConnected(page); await expect(page.locator('tbody').getByText('to-delete.txt')).not.toBeVisible(); } finally { await deleteKnownKeys(client, credentials.bucket, [key]); @@ -122,21 +122,20 @@ test.describe('Storage S3 — Recent Items', () => { // Navigate into the sub-directory so it appears in Recent Locations await connectAndOpenPrefix(page, credentials, `${prefix}sub/`); - await waitForHydration(page); // Preview the nested file so it appears in Recent Files await previewFile(page, 'nested.txt'); // Confirm both lists contain the entries await page.goto('/storage'); - await waitForHydration(page); + await waitForStorageConnected(page); await expect(page.locator('tbody').getByText('nested.txt')).toBeVisible(); await page.getByRole('tab', { name: 'Recent Locations' }).click(); await expect(page.locator('tbody').getByText('sub', { exact: true })).toBeVisible(); // Delete the parent directory via the UI await page.goto(bucketRoute(credentials.bucket, prefix)); - await waitForHydration(page); + await waitForObjectsLoaded(page); await page.getByRole('button', { name: 'Toggle selection mode' }).click(); await page.getByLabel('Select sub').check(); await page.getByRole('button', { name: 'Delete', exact: true }).click(); @@ -145,7 +144,7 @@ test.describe('Storage S3 — Recent Items', () => { // Neither the file nor the location should appear in the recent lists await page.goto('/storage'); - await waitForHydration(page); + await waitForStorageConnected(page); await expect(page.locator('tbody').getByText('nested.txt')).not.toBeVisible(); await page.getByRole('tab', { name: 'Recent Locations' }).click(); await expect(page.locator('tbody').getByText('sub', { exact: true })).not.toBeVisible(); diff --git a/src/lib/components/storage/sidebar/BucketList.svelte b/src/lib/components/storage/sidebar/BucketList.svelte index 30e2742a..ac5f9f4c 100644 --- a/src/lib/components/storage/sidebar/BucketList.svelte +++ b/src/lib/components/storage/sidebar/BucketList.svelte @@ -15,10 +15,12 @@ const storage = getStorageState(); - // Detect the active bucket from the URL so the highlight stays on when - // navigating into any sub-prefix within the bucket. + // page.url may be undefined in the error boundary state (when a client-side + // universal load throws and SvelteKit transitions to the error state). Guard + // with optional chaining to avoid crashing the layout and escalating the + // error to the root fallback handler. const activeBucket = $derived.by(() => { - const match = page.url.pathname.match(/^\/storage\/([^/]+)/); + const match = page.url?.pathname?.match(/^\/storage\/([^/]+)/); return match ? decodeURIComponent(match[1]) : null; }); const activePrefix = $derived(page.params.prefix ? page.params.prefix + '/' : ''); diff --git a/src/routes/(app)/storage/+error.svelte b/src/routes/(app)/storage/+error.svelte index e91a40b5..bcc2ce6a 100644 --- a/src/routes/(app)/storage/+error.svelte +++ b/src/routes/(app)/storage/+error.svelte @@ -4,7 +4,14 @@ import IconWarning from 'virtual:icons/material-symbols/warning'; import * as m from '$lib/paraglide/messages.js'; - const bucket = $derived(page.params.bucket ?? ''); + // page.params.bucket may be absent when a client-side universal load throws an + // error (the error boundary sits at the parent /storage level, and SvelteKit + // may not populate child-route params on the page store in that case). + // Fall back to parsing the bucket segment directly from the URL path. + const bucket = $derived( + page.params.bucket || + decodeURIComponent(page.url?.pathname?.split('/').filter(Boolean)[1] ?? '') + ); function bucketErrorMessage(status: number, name: string): string | null { if (!name) return null; diff --git a/src/routes/(app)/storage/[bucket]/[...prefix]/+page.svelte b/src/routes/(app)/storage/[bucket]/[...prefix]/+page.svelte index ef43e0db..49c8f26b 100644 --- a/src/routes/(app)/storage/[bucket]/[...prefix]/+page.svelte +++ b/src/routes/(app)/storage/[bucket]/[...prefix]/+page.svelte @@ -3,6 +3,8 @@ import { resolve } from '$app/paths'; import { getStorageState } from '$lib/storage/context.js'; import FileExplorer from '$lib/components/storage/explorer/FileExplorer.svelte'; + import IconWarning from 'virtual:icons/material-symbols/warning'; + import * as m from '$lib/paraglide/messages.js'; let { data } = $props(); const storage = getStorageState(); @@ -32,4 +34,51 @@ }); - +{#if data.accessDenied} + +
+ + +
+

+ {m.storage_error_title()} +

+

403

+

+ {m.storage_error_access_denied({ bucket: data.bucket })} +

+
+ +
+ + {m.storage_error_back_to_storage()} + + +
+
+{:else} + +{/if} diff --git a/src/routes/(app)/storage/[bucket]/[...prefix]/+page.ts b/src/routes/(app)/storage/[bucket]/[...prefix]/+page.ts index 6a53ef13..e38da795 100644 --- a/src/routes/(app)/storage/[bucket]/[...prefix]/+page.ts +++ b/src/routes/(app)/storage/[bucket]/[...prefix]/+page.ts @@ -1,5 +1,5 @@ import { browser } from '$app/environment'; -import { redirect } from '@sveltejs/kit'; +import { error, redirect } from '@sveltejs/kit'; import { loadConnectionLocally, getConnectionHeader } from '$lib/storage/connection-storage.js'; import type { PageLoad } from './$types'; import type { StoragePage } from '$lib/storage/types.js'; @@ -38,7 +38,16 @@ export const load: PageLoad = async ({ fetch, url, data }) => { if (!res.ok) { if (res.status === 401) throw redirect(303, '/storage'); - return { bucket, prefix, objects: EMPTY_PAGE }; + // For 403 we return an accessDenied flag rather than throwing error(). + // Throwing from a universal load during initial hydration (e.g. after + // page.goto) can bypass the +error.svelte boundary and fall through to the + // root fallback when data.connected=true causes BucketList to mount — a + // hydration-state mismatch that SvelteKit cannot safely recover from. + // Handling 403 inline in +page.svelte avoids the boundary entirely and + // keeps the sidebar visible so the user can navigate away. + if (res.status === 403) return { bucket, prefix, objects: EMPTY_PAGE, accessDenied: true as const }; + const body = (await res.json().catch(() => ({}))) as { message?: string }; + throw error(res.status, body.message ?? 'Failed to load objects'); } const objects = (await res.json()) as StoragePage; From c07a3bc46ab4a349bfc617fa2e24a18a11b7fcaa Mon Sep 17 00:00:00 2001 From: F3l1x1vo Date: Wed, 3 Jun 2026 10:03:00 +0200 Subject: [PATCH 3/8] #185 fix unit test --- .../modals/upload/UploadModal.svelte.spec.ts | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/lib/components/storage/modals/upload/UploadModal.svelte.spec.ts b/src/lib/components/storage/modals/upload/UploadModal.svelte.spec.ts index 5aace25a..40eac1ad 100644 --- a/src/lib/components/storage/modals/upload/UploadModal.svelte.spec.ts +++ b/src/lib/components/storage/modals/upload/UploadModal.svelte.spec.ts @@ -168,14 +168,14 @@ describe('UploadModal', () => { render(UploadModal, { ...defaultProps, prefix }); await selectAndUpload([createFile('report.csv')]); - expect(mockCheckObjectExists).toHaveBeenCalledWith(expect.any(String), 'data/report.csv'); + expect(mockCheckObjectExists).toHaveBeenCalledWith(expect.any(String), 'data/report.csv', expect.any(String)); }); it('should use filename directly when prefix is empty', async () => { render(UploadModal, { ...defaultProps, prefix: '' }); await selectAndUpload([createFile('report.csv')]); - expect(mockCheckObjectExists).toHaveBeenCalledWith(expect.any(String), 'report.csv'); + expect(mockCheckObjectExists).toHaveBeenCalledWith(expect.any(String), 'report.csv', expect.any(String)); }); }); @@ -196,7 +196,8 @@ describe('UploadModal', () => { defaultProps.bucket, 'a.txt', expect.any(File), - expect.any(Function) + expect.any(Function), + expect.any(String) ); }); @@ -337,7 +338,8 @@ describe('UploadModal', () => { expect.any(String), expect.any(String), expect.any(File), - expect.any(Function) + expect.any(Function), + expect.any(String) ); }); @@ -439,7 +441,8 @@ describe('UploadModal', () => { expect.any(String), 'folder/sub/data.csv', expect.any(File), - expect.any(Function) + expect.any(Function), + expect.any(String) ); }); }); @@ -510,12 +513,13 @@ describe('UploadModal', () => { render(UploadModal, { ...defaultProps, bucket }); await selectAndUpload([createFile('a.txt')]); - expect(mockCheckObjectExists).toHaveBeenCalledWith(bucket, expect.any(String)); + expect(mockCheckObjectExists).toHaveBeenCalledWith(bucket, expect.any(String), expect.any(String)); expect(mockUploadFile).toHaveBeenCalledWith( bucket, expect.any(String), expect.any(File), - expect.any(Function) + expect.any(Function), + expect.any(String) ); }); From 3dbcd2bdddbac3ecd5f579208473743c05e0d58a Mon Sep 17 00:00:00 2001 From: F3l1x1vo Date: Wed, 3 Jun 2026 10:14:08 +0200 Subject: [PATCH 4/8] #185 fix lint --- .../modals/upload/UploadModal.svelte.spec.ts | 18 +++++++++++++++--- .../storage/[bucket]/[...prefix]/+page.ts | 3 ++- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/src/lib/components/storage/modals/upload/UploadModal.svelte.spec.ts b/src/lib/components/storage/modals/upload/UploadModal.svelte.spec.ts index 40eac1ad..c2b31675 100644 --- a/src/lib/components/storage/modals/upload/UploadModal.svelte.spec.ts +++ b/src/lib/components/storage/modals/upload/UploadModal.svelte.spec.ts @@ -168,14 +168,22 @@ describe('UploadModal', () => { render(UploadModal, { ...defaultProps, prefix }); await selectAndUpload([createFile('report.csv')]); - expect(mockCheckObjectExists).toHaveBeenCalledWith(expect.any(String), 'data/report.csv', expect.any(String)); + expect(mockCheckObjectExists).toHaveBeenCalledWith( + expect.any(String), + 'data/report.csv', + expect.any(String) + ); }); it('should use filename directly when prefix is empty', async () => { render(UploadModal, { ...defaultProps, prefix: '' }); await selectAndUpload([createFile('report.csv')]); - expect(mockCheckObjectExists).toHaveBeenCalledWith(expect.any(String), 'report.csv', expect.any(String)); + expect(mockCheckObjectExists).toHaveBeenCalledWith( + expect.any(String), + 'report.csv', + expect.any(String) + ); }); }); @@ -513,7 +521,11 @@ describe('UploadModal', () => { render(UploadModal, { ...defaultProps, bucket }); await selectAndUpload([createFile('a.txt')]); - expect(mockCheckObjectExists).toHaveBeenCalledWith(bucket, expect.any(String), expect.any(String)); + expect(mockCheckObjectExists).toHaveBeenCalledWith( + bucket, + expect.any(String), + expect.any(String) + ); expect(mockUploadFile).toHaveBeenCalledWith( bucket, expect.any(String), diff --git a/src/routes/(app)/storage/[bucket]/[...prefix]/+page.ts b/src/routes/(app)/storage/[bucket]/[...prefix]/+page.ts index e38da795..47568c1d 100644 --- a/src/routes/(app)/storage/[bucket]/[...prefix]/+page.ts +++ b/src/routes/(app)/storage/[bucket]/[...prefix]/+page.ts @@ -45,7 +45,8 @@ export const load: PageLoad = async ({ fetch, url, data }) => { // hydration-state mismatch that SvelteKit cannot safely recover from. // Handling 403 inline in +page.svelte avoids the boundary entirely and // keeps the sidebar visible so the user can navigate away. - if (res.status === 403) return { bucket, prefix, objects: EMPTY_PAGE, accessDenied: true as const }; + if (res.status === 403) + return { bucket, prefix, objects: EMPTY_PAGE, accessDenied: true as const }; const body = (await res.json().catch(() => ({}))) as { message?: string }; throw error(res.status, body.message ?? 'Failed to load objects'); } From 99e7d62045435cb5bc590e3c71918e6f64c5a00e Mon Sep 17 00:00:00 2001 From: F3l1x1vo Date: Wed, 3 Jun 2026 12:38:49 +0200 Subject: [PATCH 5/8] #185 move request handling to middleware --- src/app.d.ts | 8 +++++++ src/hooks.server.ts | 22 +++++++++++++++++-- src/lib/server/storage/connection.ts | 17 ++++---------- .../(app)/storage/api/buckets/+server.ts | 8 +++---- .../(app)/storage/api/delete/+server.ts | 10 ++++----- .../(app)/storage/api/delete/server.test.ts | 11 +++++----- .../(app)/storage/api/download/+server.ts | 18 +++++++-------- .../(app)/storage/api/download/server.test.ts | 11 +++++----- .../(app)/storage/api/objects/+server.ts | 15 ++++++++----- .../(app)/storage/api/preview/+server.ts | 10 ++++----- .../(app)/storage/api/preview/server.test.ts | 11 +++++----- .../(app)/storage/api/upload/+server.ts | 8 +++---- .../(app)/storage/api/upload/server.test.ts | 11 +++++----- 13 files changed, 85 insertions(+), 75 deletions(-) diff --git a/src/app.d.ts b/src/app.d.ts index b5f6447a..222b0243 100644 --- a/src/app.d.ts +++ b/src/app.d.ts @@ -15,6 +15,14 @@ declare global { session: typeof auth.$Infer.Session.session | null; logger: import('pino').Logger; requestId: string; + /** + * Parsed S3 connection config extracted from the `x-storage-connection` + * request header by the `handleStorageConnection` middleware. + * Always non-null for requests to `/(app)/storage/api/*` routes + * (the middleware throws 401 before the handler runs if the header is absent). + * Null for all other routes. + */ + storageConfig: import('$lib/server/storage/types.js').S3ConnectionConfig | null; } // interface PageData {} // interface PageState {} diff --git a/src/hooks.server.ts b/src/hooks.server.ts index e0066ed7..e48a50a4 100644 --- a/src/hooks.server.ts +++ b/src/hooks.server.ts @@ -1,11 +1,12 @@ import { paraglideMiddleware } from '$lib/paraglide/server'; import { httpRequestDuration } from '$lib/server/metrics'; import { building, dev } from '$app/environment'; -import { redirect, type Handle, type HandleServerError } from '@sveltejs/kit'; +import { error, redirect, type Handle, type HandleServerError } from '@sveltejs/kit'; import { sequence } from '@sveltejs/kit/hooks'; import { svelteKitHandler } from 'better-auth/svelte-kit'; import { auth, oidcEnabled } from '$lib/server/auth'; import { requestLogger, logger } from '$lib/server/logging'; +import { getConnectionFromHeader } from '$lib/server/storage/connection.js'; // Allow self-signed TLS certificates in development (e.g. local Trino with self-signed certs). if (dev) { @@ -58,11 +59,28 @@ const handleAuthGuard: Handle = async ({ event, resolve }) => { return resolve(event); }; +/** + * Parse the `x-storage-connection` header (base64 JSON) for every request and + * store the result in `event.locals.storageConfig`. For routes under + * `/(app)/storage/api/` the header is mandatory — the middleware throws 401 + * before the handler runs if it is absent, so handlers can rely on + * `locals.storageConfig` being non-null. Throws 400 for a present but + * malformed / invalid header on any route. + */ +const handleStorageConnection: Handle = async ({ event, resolve }) => { + event.locals.storageConfig = getConnectionFromHeader(event.request); + if (event.locals.storageConfig === null && event.route.id?.startsWith('/(app)/storage/api/')) { + throw error(401, 'No storage connection configured'); + } + return resolve(event); +}; + export const handle = sequence( requestLogger, handleMetrics, handleParaglide, - ...(oidcEnabled ? [handleAuth, handleAuthGuard] : []) + ...(oidcEnabled ? [handleAuth, handleAuthGuard] : []), + handleStorageConnection ); export const handleError: HandleServerError = ({ error, event, status, message }) => { diff --git a/src/lib/server/storage/connection.ts b/src/lib/server/storage/connection.ts index 21e17b4c..2f606ba5 100644 --- a/src/lib/server/storage/connection.ts +++ b/src/lib/server/storage/connection.ts @@ -4,8 +4,6 @@ import { STORAGE_CONNECTION_HEADER } from '$lib/storage/connection-storage.js'; import type { S3ConnectionConfig } from './types.js'; import { logger } from '$lib/server/logging'; -export { STORAGE_CONNECTION_HEADER }; - const log = logger.child({ module: 'storage-connection' }); /** @@ -43,20 +41,13 @@ function parseConnectionPayload(raw: string): S3ConnectionConfig { /** * Extract the storage connection config from the `X-Storage-Connection` request header. - * Returns `null` if the header is absent. Throws on malformed or invalid payloads. + * Returns `null` if the header is absent. Throws 400 on malformed or invalid payloads. + * + * Used by the `handleStorageConnection` middleware in `hooks.server.ts` to populate + * `event.locals.storageConfig` before any storage API handler runs. */ export function getConnectionFromHeader(request: Request): S3ConnectionConfig | null { const header = request.headers.get(STORAGE_CONNECTION_HEADER); if (!header) return null; return parseConnectionPayload(header); } - -/** - * Extract the storage connection config from the `X-Storage-Connection` request header. - * Throws a 401 HTTP error if the header is absent. - */ -export function requireConnection(request: Request): S3ConnectionConfig { - const conn = getConnectionFromHeader(request); - if (!conn) throw error(401, 'No storage connection configured'); - return conn; -} diff --git a/src/routes/(app)/storage/api/buckets/+server.ts b/src/routes/(app)/storage/api/buckets/+server.ts index 620f983a..54994560 100644 --- a/src/routes/(app)/storage/api/buckets/+server.ts +++ b/src/routes/(app)/storage/api/buckets/+server.ts @@ -1,5 +1,4 @@ import { listBuckets } from '$lib/server/storage/service.js'; -import { requireConnection } from '$lib/server/storage/connection.js'; import type { RequestHandler } from './$types'; /** @@ -7,10 +6,11 @@ import type { RequestHandler } from './$types'; * * Returns the list of buckets accessible with the connection config supplied in * the `X-Storage-Connection` request header (base64-encoded JSON). + * The config is parsed and validated by the `handleStorageConnection` middleware + * in hooks.server.ts before this handler runs. */ -export const GET: RequestHandler = async ({ request, locals }) => { - const config = requireConnection(request); - const buckets = await listBuckets(config); +export const GET: RequestHandler = async ({ locals }) => { + const buckets = await listBuckets(locals.storageConfig!); locals.logger.debug({ bucket_count: buckets.length }, 'bucket list returned'); return Response.json(buckets); }; diff --git a/src/routes/(app)/storage/api/delete/+server.ts b/src/routes/(app)/storage/api/delete/+server.ts index 4892bb35..43369e83 100644 --- a/src/routes/(app)/storage/api/delete/+server.ts +++ b/src/routes/(app)/storage/api/delete/+server.ts @@ -1,7 +1,6 @@ import type { RequestHandler } from './$types'; import { error } from '@sveltejs/kit'; import { deleteObjects } from '$lib/server/storage/service.js'; -import { requireConnection } from '$lib/server/storage/connection.js'; import { requireBucket } from '../params.js'; /** @@ -10,12 +9,11 @@ import { requireBucket } from '../params.js'; * Deletes one or more S3 objects from the given bucket. * Authentication is enforced by the app-level auth guard in hooks.server.ts. * - * The connection config is read from the `X-Storage-Connection` request header - * (base64-encoded JSON), set by the client from its localStorage entry. + * The connection config is parsed and validated by the `handleStorageConnection` + * middleware in hooks.server.ts before this handler runs. */ -export const DELETE: RequestHandler = async ({ locals, url, request }) => { +export const DELETE: RequestHandler = async ({ locals, url }) => { const bucket = requireBucket(url); - const config = requireConnection(request); const keys = url.searchParams.getAll('keys'); if (!keys.length) { @@ -24,7 +22,7 @@ export const DELETE: RequestHandler = async ({ locals, url, request }) => { locals.logger.debug({ bucket, key_count: keys.length }, 'delete request received'); - const result = await deleteObjects(config, bucket, keys); + const result = await deleteObjects(locals.storageConfig!, bucket, keys); locals.logger.info( { bucket, key_count: keys.length, failed_count: result.failed.length }, diff --git a/src/routes/(app)/storage/api/delete/server.test.ts b/src/routes/(app)/storage/api/delete/server.test.ts index 35783ccb..7d2dcd9e 100644 --- a/src/routes/(app)/storage/api/delete/server.test.ts +++ b/src/routes/(app)/storage/api/delete/server.test.ts @@ -5,11 +5,6 @@ vi.mock('$lib/server/storage/service.js', () => ({ deleteObjects: vi.fn() })); -vi.mock('$lib/server/storage/connection.js', () => ({ - requireConnection: vi.fn(() => ({ type: 's3', region: 'us-east-1' })), - STORAGE_CONNECTION_HEADER: 'x-storage-connection' -})); - import { DELETE } from './+server.js'; import { deleteObjects } from '$lib/server/storage/service.js'; @@ -29,7 +24,11 @@ function mockEvent(searchParams: Record) { return { url, request: { headers: new Headers(CONNECTION_HEADER) }, - locals: { logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, user: { id: 'test-user' } } + locals: { + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, + user: { id: 'test-user' }, + storageConfig: { type: 's3', region: 'us-east-1' } + } } as unknown as Parameters[0]; } diff --git a/src/routes/(app)/storage/api/download/+server.ts b/src/routes/(app)/storage/api/download/+server.ts index e43df827..d8e09bb8 100644 --- a/src/routes/(app)/storage/api/download/+server.ts +++ b/src/routes/(app)/storage/api/download/+server.ts @@ -1,6 +1,5 @@ import type { RequestHandler } from './$types'; import { downloadObject, getObjectMetadata } from '$lib/server/storage/service.js'; -import { requireConnection } from '$lib/server/storage/connection.js'; import { requireBucketKey } from '../params.js'; /** Derive the bare filename from a (possibly path-prefixed) object key. */ @@ -16,16 +15,15 @@ function filenameFromKey(key: string): string { * The S3 body stream is piped straight to the HTTP response — no server-side * buffering occurs. * - * The connection config is read from the `X-Storage-Connection` request header - * (base64-encoded JSON), set by the client from its localStorage entry. + * The connection config is parsed and validated by the `handleStorageConnection` + * middleware in hooks.server.ts before this handler runs. */ -export const GET: RequestHandler = async ({ locals, url, request }) => { +export const GET: RequestHandler = async ({ locals, url }) => { const { bucket, key } = requireBucketKey(url); - const config = requireConnection(request); locals.logger.debug({ bucket, key }, 'download request received'); - const download = await downloadObject(config, bucket, key); + const download = await downloadObject(locals.storageConfig!, bucket, key); const filename = filenameFromKey(key); // RFC 5987 encoding for non-ASCII filenames in Content-Disposition @@ -58,15 +56,15 @@ export const GET: RequestHandler = async ({ locals, url, request }) => { * a HeadObject call (no object body transferred). The client uses this before * initiating a download to surface auth/not-found errors as inline UI messages. * - * The connection config is read from the `X-Storage-Connection` request header. + * The connection config is parsed and validated by the `handleStorageConnection` + * middleware in hooks.server.ts before this handler runs. */ -export const HEAD: RequestHandler = async ({ locals, url, request }) => { +export const HEAD: RequestHandler = async ({ locals, url }) => { const { bucket, key } = requireBucketKey(url); - const config = requireConnection(request); locals.logger.debug({ bucket, key }, 'download pre-flight check'); - const meta = await getObjectMetadata(config, bucket, key); + const meta = await getObjectMetadata(locals.storageConfig!, bucket, key); return new Response(null, { status: 200, diff --git a/src/routes/(app)/storage/api/download/server.test.ts b/src/routes/(app)/storage/api/download/server.test.ts index d0304e87..545b581b 100644 --- a/src/routes/(app)/storage/api/download/server.test.ts +++ b/src/routes/(app)/storage/api/download/server.test.ts @@ -5,11 +5,6 @@ vi.mock('$lib/server/storage/service.js', () => ({ getObjectMetadata: vi.fn() })); -vi.mock('$lib/server/storage/connection.js', () => ({ - requireConnection: vi.fn(() => ({ type: 's3', region: 'us-east-1' })), - STORAGE_CONNECTION_HEADER: 'x-storage-connection' -})); - import { GET, HEAD } from './+server.js'; import { downloadObject, getObjectMetadata } from '$lib/server/storage/service.js'; @@ -22,7 +17,11 @@ function mockEvent(params: string) { return { url, request: { headers: new Headers(CONNECTION_HEADER) }, - locals: { logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, user: { id: 'test-user' } } + locals: { + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, + user: { id: 'test-user' }, + storageConfig: { type: 's3', region: 'us-east-1' } + } } as unknown as Parameters[0]; } diff --git a/src/routes/(app)/storage/api/objects/+server.ts b/src/routes/(app)/storage/api/objects/+server.ts index d1fb56f9..463a5b63 100644 --- a/src/routes/(app)/storage/api/objects/+server.ts +++ b/src/routes/(app)/storage/api/objects/+server.ts @@ -1,6 +1,5 @@ import { error } from '@sveltejs/kit'; import { listObjects } from '$lib/server/storage/service.js'; -import { requireConnection } from '$lib/server/storage/connection.js'; import type { RequestHandler } from './$types'; /** @@ -8,10 +7,10 @@ import type { RequestHandler } from './$types'; * * Returns a page of objects in the given bucket/prefix using the connection * config supplied in the `X-Storage-Connection` request header. + * The config is parsed and validated by the `handleStorageConnection` middleware + * in hooks.server.ts before this handler runs. */ -export const GET: RequestHandler = async ({ request, url, locals }) => { - const config = requireConnection(request); - +export const GET: RequestHandler = async ({ url, locals }) => { const bucket = url.searchParams.get('bucket')?.trim(); if (!bucket) throw error(400, 'Missing required query parameter: bucket'); @@ -25,6 +24,12 @@ export const GET: RequestHandler = async ({ request, url, locals }) => { 'listing objects' ); - const page = await listObjects(config, bucket, prefix, pageSize, continuationToken); + const page = await listObjects( + locals.storageConfig!, + bucket, + prefix, + pageSize, + continuationToken + ); return Response.json(page); }; diff --git a/src/routes/(app)/storage/api/preview/+server.ts b/src/routes/(app)/storage/api/preview/+server.ts index f6c6e7f7..ffb51b24 100644 --- a/src/routes/(app)/storage/api/preview/+server.ts +++ b/src/routes/(app)/storage/api/preview/+server.ts @@ -1,7 +1,6 @@ import { S3ServiceException } from '@aws-sdk/client-s3'; import { mapS3ErrorToHttp } from '$lib/server/storage/s3-errors.js'; import { getProvider } from '$lib/server/storage/utils.js'; -import { requireConnection } from '$lib/server/storage/connection.js'; // import { parquetPreview } from '$lib/server/storage/preview/parquet.js'; import { binaryPreview, KNOWN_BINARY_TYPES } from '$lib/server/storage/preview/binary.js'; import { streamPreview } from '$lib/server/storage/preview/stream.js'; @@ -11,15 +10,14 @@ import type { RequestHandler } from './$types'; /** * GET /storage/api/preview?bucket=&key= * - * The connection config is read from the `X-Storage-Connection` request header - * (base64-encoded JSON), set by the client from its localStorage entry. + * The connection config is parsed and validated by the `handleStorageConnection` + * middleware in hooks.server.ts before this handler runs. */ -export const GET: RequestHandler = async ({ url, locals, request }) => { +export const GET: RequestHandler = async ({ url, locals }) => { const log = locals.logger; - const config = requireConnection(request); const { bucket, key } = requireBucketKey(url); - const provider = getProvider(config, bucket); + const provider = getProvider(locals.storageConfig!, bucket); try { const metadata = await provider.getMetadata(key); diff --git a/src/routes/(app)/storage/api/preview/server.test.ts b/src/routes/(app)/storage/api/preview/server.test.ts index 79f72594..10c18fdd 100644 --- a/src/routes/(app)/storage/api/preview/server.test.ts +++ b/src/routes/(app)/storage/api/preview/server.test.ts @@ -6,11 +6,6 @@ vi.mock('$lib/server/storage/utils.js', () => ({ getProvider: vi.fn(() => ({ getMetadata: mockGetMetadata })) })); -vi.mock('$lib/server/storage/connection.js', () => ({ - requireConnection: vi.fn(() => ({ type: 's3', region: 'us-east-1' })), - STORAGE_CONNECTION_HEADER: 'x-storage-connection' -})); - vi.mock('$lib/server/storage/preview/binary.js', () => ({ KNOWN_BINARY_TYPES: new Set(['application/zip']), binaryPreview: vi.fn(() => new Response(null, { headers: { 'X-Preview-Renderable': 'false' } })) @@ -40,7 +35,11 @@ function mockEvent(params: string) { return { url, request: { headers: new Headers(CONNECTION_HEADER) }, - locals: { logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, user: { id: 'test-user' } } + locals: { + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, + user: { id: 'test-user' }, + storageConfig: { type: 's3', region: 'us-east-1' } + } } as unknown as Parameters[0]; } diff --git a/src/routes/(app)/storage/api/upload/+server.ts b/src/routes/(app)/storage/api/upload/+server.ts index dc58c546..b6202da4 100644 --- a/src/routes/(app)/storage/api/upload/+server.ts +++ b/src/routes/(app)/storage/api/upload/+server.ts @@ -1,6 +1,5 @@ import { error } from '@sveltejs/kit'; import { uploadObject } from '$lib/server/storage/service.js'; -import { requireConnection } from '$lib/server/storage/connection.js'; import { requireBucketKey } from '../params.js'; import type { RequestHandler } from './$types'; @@ -11,12 +10,11 @@ import type { RequestHandler } from './$types'; * @aws-sdk/lib-storage). The request body is piped to the S3 SDK without * buffering in server memory, regardless of file size. * - * The connection config is read from the `X-Storage-Connection` request header - * (base64-encoded JSON), set by the client from its localStorage entry. + * The connection config is parsed and validated by the `handleStorageConnection` + * middleware in hooks.server.ts before this handler runs. */ export const POST: RequestHandler = async ({ locals, url, request }) => { const log = locals.logger; - const config = requireConnection(request); const { bucket, key } = requireBucketKey(url); if (!request.body) { @@ -35,7 +33,7 @@ export const POST: RequestHandler = async ({ locals, url, request }) => { 'upload request received' ); - await uploadObject(config, bucket, key, request.body, contentType, contentLength); + await uploadObject(locals.storageConfig!, bucket, key, request.body, contentType, contentLength); log.info( { bucket, key, content_type: contentType, content_length: contentLength }, diff --git a/src/routes/(app)/storage/api/upload/server.test.ts b/src/routes/(app)/storage/api/upload/server.test.ts index b8adfdb9..17644c13 100644 --- a/src/routes/(app)/storage/api/upload/server.test.ts +++ b/src/routes/(app)/storage/api/upload/server.test.ts @@ -4,11 +4,6 @@ vi.mock('$lib/server/storage/service.js', () => ({ uploadObject: vi.fn() })); -vi.mock('$lib/server/storage/connection.js', () => ({ - requireConnection: vi.fn(() => ({ type: 's3', region: 'us-east-1' })), - STORAGE_CONNECTION_HEADER: 'x-storage-connection' -})); - import { POST } from './+server.js'; import { uploadObject } from '$lib/server/storage/service.js'; @@ -31,7 +26,11 @@ function mockEvent(opts: { return { url, request: { body, headers }, - locals: { logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, user: { id: 'test-user' } } + locals: { + logger: { debug: vi.fn(), info: vi.fn(), warn: vi.fn() }, + user: { id: 'test-user' }, + storageConfig: { type: 's3', region: 'us-east-1' } + } } as unknown as Parameters[0]; } From e4b8b845277f62f420367e03ec94a59d89527fe0 Mon Sep 17 00:00:00 2001 From: dklOrdix Date: Wed, 3 Jun 2026 14:09:39 +0200 Subject: [PATCH 6/8] refactor: move service logic into StorageProvider interface and implementations --- src/lib/server/storage/hdfs-provider.ts | 4 + src/lib/server/storage/preview/stream.test.ts | 1 + src/lib/server/storage/provider.ts | 2 + src/lib/server/storage/s3-provider.test.ts | 181 ++++++++++++ src/lib/server/storage/s3-provider.ts | 260 ++++++++++++------ src/lib/server/storage/service.test.ts | 247 ----------------- src/lib/server/storage/service.ts | 146 ---------- src/lib/server/storage/utils.ts | 13 + src/routes/(app)/storage/+page.server.ts | 4 +- .../(app)/storage/api/buckets/+server.ts | 4 +- .../(app)/storage/api/delete/+server.ts | 4 +- .../(app)/storage/api/delete/server.test.ts | 12 +- .../(app)/storage/api/download/+server.ts | 6 +- .../(app)/storage/api/download/server.test.ts | 19 +- .../(app)/storage/api/objects/+server.ts | 8 +- .../(app)/storage/api/upload/+server.ts | 4 +- .../(app)/storage/api/upload/server.test.ts | 21 +- src/routes/(app)/storage/page.server.test.ts | 10 +- 18 files changed, 413 insertions(+), 533 deletions(-) delete mode 100644 src/lib/server/storage/service.test.ts delete mode 100644 src/lib/server/storage/service.ts diff --git a/src/lib/server/storage/hdfs-provider.ts b/src/lib/server/storage/hdfs-provider.ts index 5861498a..1a8df630 100644 --- a/src/lib/server/storage/hdfs-provider.ts +++ b/src/lib/server/storage/hdfs-provider.ts @@ -10,6 +10,10 @@ import type { StoragePage, StorageMetadata } from '$lib/storage/types.js'; export class HDFSStorageProvider implements StorageProvider { constructor(_config: HDFSConfig) {} + listBuckets(): Promise { + throw new Error('HDFS not implemented'); + } + listObjects( _prefix: string, _pageSize: number, diff --git a/src/lib/server/storage/preview/stream.test.ts b/src/lib/server/storage/preview/stream.test.ts index 382f19c7..6d11d0e7 100644 --- a/src/lib/server/storage/preview/stream.test.ts +++ b/src/lib/server/storage/preview/stream.test.ts @@ -7,6 +7,7 @@ const mockLog = { info: vi.fn(), debug: vi.fn(), warn: vi.fn() } as unknown as p function makeProvider(overrides: Partial = {}): StorageProvider { return { + listBuckets: vi.fn(), listObjects: vi.fn(), getObject: vi .fn() diff --git a/src/lib/server/storage/provider.ts b/src/lib/server/storage/provider.ts index 6156762d..c666165b 100644 --- a/src/lib/server/storage/provider.ts +++ b/src/lib/server/storage/provider.ts @@ -12,6 +12,8 @@ export interface ObjectDownload { /** Backend-agnostic interface for a bucket-scoped storage provider. */ export interface StorageProvider { + /** List all buckets accessible with the current connection credentials. */ + listBuckets(): Promise; /** * List objects using cursor-based pagination. `continuationToken` is the * provider-specific opaque token returned from a previous call. When diff --git a/src/lib/server/storage/s3-provider.test.ts b/src/lib/server/storage/s3-provider.test.ts index d67d613a..73ada8fe 100644 --- a/src/lib/server/storage/s3-provider.test.ts +++ b/src/lib/server/storage/s3-provider.test.ts @@ -509,3 +509,184 @@ describe('S3StorageProvider constructor', () => { ); }); }); + +// --------------------------------------------------------------------------- +// listBuckets +// --------------------------------------------------------------------------- + +describe('S3StorageProvider.listBuckets', () => { + let provider: S3StorageProvider; + let send: ReturnType; + + beforeEach(() => { + ({ provider, send } = makeProvider()); + }); + + it('returns bucket names', async () => { + send.mockResolvedValue({ Buckets: [{ Name: 'a' }, { Name: 'b' }] }); + expect(await provider.listBuckets()).toEqual(['a', 'b']); + }); + + it('filters out buckets with no name', async () => { + send.mockResolvedValue({ Buckets: [{ Name: 'a' }, { Name: undefined }, { Name: '' }] }); + expect(await provider.listBuckets()).toEqual(['a']); + }); + + it('handles null Buckets in response', async () => { + send.mockResolvedValue({ Buckets: null }); + expect(await provider.listBuckets()).toEqual([]); + }); + + it('maps AccessDenied S3 error to HTTP 403', async () => { + send.mockRejectedValue(makeS3Error('AccessDenied', 403)); + await expect(provider.listBuckets()).rejects.toMatchObject({ status: 403 }); + }); + + it('re-throws non-S3 errors', async () => { + const err = new Error('network'); + send.mockRejectedValue(err); + await expect(provider.listBuckets()).rejects.toThrow('network'); + }); +}); + +// --------------------------------------------------------------------------- +// Error handling — S3ServiceException → HTTP error mapping +// --------------------------------------------------------------------------- + +describe('S3StorageProvider error handling', () => { + let provider: S3StorageProvider; + let send: ReturnType; + + beforeEach(() => { + ({ provider, send } = makeProvider()); + }); + + it('listObjects: maps AccessDenied to HTTP 403', async () => { + send.mockRejectedValue(makeS3Error('AccessDenied', 403)); + await expect(provider.listObjects('', 10)).rejects.toMatchObject({ status: 403 }); + }); + + it('listObjects: maps NoSuchBucket to HTTP 404', async () => { + send.mockRejectedValue(makeS3Error('NoSuchBucket', 404)); + await expect(provider.listObjects('', 10)).rejects.toMatchObject({ status: 404 }); + }); + + it('listObjects: re-throws non-S3 errors', async () => { + send.mockRejectedValue(new Error('timeout')); + await expect(provider.listObjects('', 10)).rejects.toThrow('timeout'); + }); + + it('getObject: maps NoSuchKey to HTTP 404', async () => { + send.mockRejectedValue(makeS3Error('NoSuchKey', 404)); + await expect(provider.getObject('k')).rejects.toMatchObject({ status: 404 }); + }); + + it('getObject: maps AccessDenied to HTTP 403', async () => { + send.mockRejectedValue(makeS3Error('AccessDenied', 403)); + await expect(provider.getObject('k')).rejects.toMatchObject({ status: 403 }); + }); + + it('getObject: re-throws non-S3 errors', async () => { + send.mockRejectedValue(new Error('gone')); + await expect(provider.getObject('k')).rejects.toThrow('gone'); + }); + + it('getObjectRange: maps S3 error to HTTP error', async () => { + send.mockRejectedValue(makeS3Error('NoSuchKey', 404)); + await expect(provider.getObjectRange('k', 0, 100)).rejects.toMatchObject({ status: 404 }); + }); + + it('getObjectRange: re-throws non-S3 errors', async () => { + send.mockRejectedValue(new Error('network')); + await expect(provider.getObjectRange('k', 0, 100)).rejects.toThrow('network'); + }); + + it('getMetadata: maps NoSuchKey to HTTP 404', async () => { + send.mockRejectedValue(makeS3Error('NoSuchKey', 404)); + await expect(provider.getMetadata('k')).rejects.toMatchObject({ status: 404 }); + }); + + it('getMetadata: re-throws non-S3 errors', async () => { + send.mockRejectedValue(new Error('gone')); + await expect(provider.getMetadata('k')).rejects.toThrow('gone'); + }); + + it('putObject: maps AccessDenied to HTTP 403', async () => { + mockUploadDone.mockRejectedValue(makeS3Error('AccessDenied', 403)); + MockUpload.mockImplementationOnce(function (this: { done: typeof mockUploadDone }) { + this.done = mockUploadDone; + }); + await expect(provider.putObject('k', Buffer.from('x'), 'text/plain')).rejects.toMatchObject({ + status: 403 + }); + }); + + it('putObject: re-throws non-S3 errors', async () => { + mockUploadDone.mockRejectedValue(new Error('disk full')); + MockUpload.mockImplementationOnce(function (this: { done: typeof mockUploadDone }) { + this.done = mockUploadDone; + }); + await expect(provider.putObject('k', Buffer.from('x'), 'text/plain')).rejects.toThrow( + 'disk full' + ); + }); + + it('deleteObjects: maps AccessDenied to HTTP 403', async () => { + send.mockRejectedValue(makeS3Error('AccessDenied', 403)); + await expect(provider.deleteObjects(['file.txt'])).rejects.toMatchObject({ status: 403 }); + }); + + it('deleteObjects: re-throws non-S3 errors', async () => { + send.mockRejectedValue(new Error('network')); + await expect(provider.deleteObjects(['file.txt'])).rejects.toThrow('network'); + }); +}); + +// --------------------------------------------------------------------------- +// deleteObjects — directory expansion +// --------------------------------------------------------------------------- + +describe('S3StorageProvider.deleteObjects directory expansion', () => { + let provider: S3StorageProvider; + let send: ReturnType; + + beforeEach(() => { + ({ provider, send } = makeProvider()); + }); + + it('expands directory prefixes and deletes all contained keys', async () => { + // First call: ListObjectsV2 for dir/ + send.mockResolvedValueOnce({ + Contents: [{ Key: 'dir/a.txt' }, { Key: 'dir/b.txt' }], + IsTruncated: false + }); + // Second call: DeleteObjects + send.mockResolvedValueOnce({ Errors: [] }); + + const result = await provider.deleteObjects(['file.txt', 'dir/']); + + const deleteCall = send.mock.calls[1][0]; + expect(deleteCall.input.Delete.Objects).toEqual([ + { Key: 'file.txt' }, + { Key: 'dir/a.txt' }, + { Key: 'dir/b.txt' } + ]); + expect(result).toEqual({ failed: [] }); + }); + + it('uses directory key itself when no children found', async () => { + send.mockResolvedValueOnce({ IsTruncated: false }); // listAllKeys returns [] + send.mockResolvedValueOnce({ Errors: [] }); // deleteObjects + + await provider.deleteObjects(['empty/']); + + const deleteCall = send.mock.calls[1][0]; + expect(deleteCall.input.Delete.Objects).toEqual([{ Key: 'empty/' }]); + }); + + it('returns empty result for no keys', async () => { + const result = await provider.deleteObjects([]); + expect(result).toEqual({ failed: [] }); + expect(send).not.toHaveBeenCalled(); + }); +}); diff --git a/src/lib/server/storage/s3-provider.ts b/src/lib/server/storage/s3-provider.ts index e6fac66f..60052508 100644 --- a/src/lib/server/storage/s3-provider.ts +++ b/src/lib/server/storage/s3-provider.ts @@ -1,6 +1,7 @@ import { S3Client, S3ServiceException, + ListBucketsCommand, ListObjectsV2Command, GetObjectCommand, HeadObjectCommand, @@ -13,6 +14,7 @@ import type { S3Config } from './types.js'; import type { StoragePage, StorageObject, StorageMetadata } from '$lib/storage/types.js'; import { logger } from '$lib/server/logging'; import { createS3Client } from './s3-client.js'; +import { mapS3ErrorToHttp } from './s3-errors.js'; const log = logger.child({ module: 's3-provider' }); @@ -25,6 +27,21 @@ export class S3StorageProvider implements StorageProvider { this.client = client ?? createS3Client(config); } + async listBuckets(): Promise { + log.trace({ bucket: this.bucket }, 'S3 ListBuckets'); + try { + const output = await this.client.send(new ListBucketsCommand({})); + const buckets = (output.Buckets ?? []).map((b) => b.Name ?? '').filter(Boolean); + log.debug({ bucket_count: buckets.length }, 'listed buckets'); + return buckets; + } catch (err) { + if (err instanceof S3ServiceException) { + mapS3ErrorToHttp(err, { operation: 'listBuckets' }); + } + throw err; + } + } + async listObjects( prefix: string, pageSize: number, @@ -32,81 +49,113 @@ export class S3StorageProvider implements StorageProvider { ): Promise { log.trace({ bucket: this.bucket, prefix, page_size: pageSize }, 'S3 ListObjectsV2'); - const output: ListObjectsV2CommandOutput = await this.client.send( - new ListObjectsV2Command({ - Bucket: this.bucket, - Prefix: prefix || undefined, - Delimiter: '/', - MaxKeys: pageSize, - ContinuationToken: continuationToken ?? undefined - }) - ); + try { + const output: ListObjectsV2CommandOutput = await this.client.send( + new ListObjectsV2Command({ + Bucket: this.bucket, + Prefix: prefix || undefined, + Delimiter: '/', + MaxKeys: pageSize, + ContinuationToken: continuationToken ?? undefined + }) + ); - const objects: StorageObject[] = [ - ...(output.CommonPrefixes ?? []).map((cp) => ({ - key: cp.Prefix ?? '', - size: 0, - lastModified: new Date(0), - isDirectory: true, - contentType: undefined - })), - ...(output.Contents ?? []) - // Filter out the prefix itself (S3 sometimes echoes it back) - .filter((obj) => obj.Key !== prefix) - .map((obj) => ({ - key: obj.Key ?? '', - size: obj.Size ?? 0, - lastModified: obj.LastModified ?? new Date(0), - isDirectory: false, + const objects: StorageObject[] = [ + ...(output.CommonPrefixes ?? []).map((cp) => ({ + key: cp.Prefix ?? '', + size: 0, + lastModified: new Date(0), + isDirectory: true, contentType: undefined - })) - ]; - - return { - objects, - hasNextPage: output.IsTruncated ?? false, - currentPage: 1, - pageSize, - continuationToken: continuationToken ?? null, - nextContinuationToken: output.NextContinuationToken ?? null - }; + })), + ...(output.Contents ?? []) + // Filter out the prefix itself (S3 sometimes echoes it back) + .filter((obj) => obj.Key !== prefix) + .map((obj) => ({ + key: obj.Key ?? '', + size: obj.Size ?? 0, + lastModified: obj.LastModified ?? new Date(0), + isDirectory: false, + contentType: undefined + })) + ]; + + return { + objects, + hasNextPage: output.IsTruncated ?? false, + currentPage: 1, + pageSize, + continuationToken: continuationToken ?? null, + nextContinuationToken: output.NextContinuationToken ?? null + }; + } catch (err) { + if (err instanceof S3ServiceException) { + mapS3ErrorToHttp(err, { bucket: this.bucket, operation: 'listObjects' }); + } + throw err; + } } async getObject(key: string): Promise { log.trace({ bucket: this.bucket, key }, 'S3 GetObject'); - const output = await this.client.send(new GetObjectCommand({ Bucket: this.bucket, Key: key })); - if (!output.Body) { - throw new Error(`Object ${key} has no body`); + try { + const output = await this.client.send( + new GetObjectCommand({ Bucket: this.bucket, Key: key }) + ); + if (!output.Body) { + throw new Error(`Object ${key} has no body`); + } + return { + stream: output.Body.transformToWebStream(), + contentType: output.ContentType, + contentLength: output.ContentLength, + etag: output.ETag + }; + } catch (err) { + if (err instanceof S3ServiceException) { + mapS3ErrorToHttp(err, { bucket: this.bucket, key, operation: 'getObject' }); + } + throw err; } - return { - stream: output.Body.transformToWebStream(), - contentType: output.ContentType, - contentLength: output.ContentLength, - etag: output.ETag - }; } async getObjectRange(key: string, start: number, end: number): Promise { log.trace({ bucket: this.bucket, key, start, end }, 'S3 GetObject (range)'); - const output = await this.client.send( - new GetObjectCommand({ Bucket: this.bucket, Key: key, Range: `bytes=${start}-${end}` }) - ); - if (!output.Body) { - throw new Error(`Object ${key} has no body`); + try { + const output = await this.client.send( + new GetObjectCommand({ Bucket: this.bucket, Key: key, Range: `bytes=${start}-${end}` }) + ); + if (!output.Body) { + throw new Error(`Object ${key} has no body`); + } + return output.Body.transformToWebStream(); + } catch (err) { + if (err instanceof S3ServiceException) { + mapS3ErrorToHttp(err, { bucket: this.bucket, key, operation: 'getObjectRange' }); + } + throw err; } - return output.Body.transformToWebStream(); } async getMetadata(key: string): Promise { log.trace({ bucket: this.bucket, key }, 'S3 HeadObject'); - const output = await this.client.send(new HeadObjectCommand({ Bucket: this.bucket, Key: key })); - return { - size: output.ContentLength ?? 0, - lastModified: output.LastModified ?? new Date(0), - contentType: output.ContentType, - etag: output.ETag, - customMetadata: output.Metadata - }; + try { + const output = await this.client.send( + new HeadObjectCommand({ Bucket: this.bucket, Key: key }) + ); + return { + size: output.ContentLength ?? 0, + lastModified: output.LastModified ?? new Date(0), + contentType: output.ContentType, + etag: output.ETag, + customMetadata: output.Metadata + }; + } catch (err) { + if (err instanceof S3ServiceException) { + mapS3ErrorToHttp(err, { bucket: this.bucket, key, operation: 'getMetadata' }); + } + throw err; + } } async exists(key: string): Promise { @@ -134,44 +183,73 @@ export class S3StorageProvider implements StorageProvider { { bucket: this.bucket, key, content_type: contentType, content_length: contentLength }, 'S3 Upload' ); - const upload = new Upload({ - client: this.client, - queueSize: 4, - partSize: 5 * 1024 * 1024, - params: { - Bucket: this.bucket, - Key: key, - Body: body, - ContentType: contentType, - ...(contentLength !== undefined ? { ContentLength: contentLength } : {}) + try { + const upload = new Upload({ + client: this.client, + queueSize: 4, + partSize: 5 * 1024 * 1024, + params: { + Bucket: this.bucket, + Key: key, + Body: body, + ContentType: contentType, + ...(contentLength !== undefined ? { ContentLength: contentLength } : {}) + } + }); + await upload.done(); + } catch (err) { + if (err instanceof S3ServiceException) { + mapS3ErrorToHttp(err, { bucket: this.bucket, key, operation: 'putObject' }); } - }); - await upload.done(); + throw err; + } } async deleteObjects(keys: string[]): Promise { - log.trace({ bucket: this.bucket, key_count: keys.length }, 'S3 DeleteObjects'); - const output = await this.client.send( - new DeleteObjectsCommand({ - Bucket: this.bucket, - Delete: { - Objects: keys.map((key) => ({ Key: key })), - Quiet: true - } - }) - ); - const failed = (output.Errors ?? []).map((e) => ({ - key: e.Key ?? '', - code: e.Code, - message: e.Message - })); - if (failed.length > 0) { - log.warn( - { bucket: this.bucket, failed_count: failed.length }, - 'some objects failed to delete' + // Expand any directory prefixes (keys ending with '/') to their contents. + const dirPrefixes = keys.filter((k) => k.endsWith('/')); + const fileKeys = keys.filter((k) => !k.endsWith('/')); + + let allKeys = [...fileKeys]; + for (const prefix of dirPrefixes) { + log.debug({ bucket: this.bucket, prefix }, 'expanding directory prefix for deletion'); + const children = await this.listAllKeys(prefix); + allKeys = allKeys.concat(children.length > 0 ? children : [prefix]); + } + + if (allKeys.length === 0) { + return { failed: [] }; + } + + log.trace({ bucket: this.bucket, key_count: allKeys.length }, 'S3 DeleteObjects'); + try { + const output = await this.client.send( + new DeleteObjectsCommand({ + Bucket: this.bucket, + Delete: { + Objects: allKeys.map((key) => ({ Key: key })), + Quiet: true + } + }) ); + const failed = (output.Errors ?? []).map((e) => ({ + key: e.Key ?? '', + code: e.Code, + message: e.Message + })); + if (failed.length > 0) { + log.warn( + { bucket: this.bucket, failed_count: failed.length }, + 'some objects failed to delete' + ); + } + return { failed }; + } catch (err) { + if (err instanceof S3ServiceException) { + mapS3ErrorToHttp(err, { bucket: this.bucket, operation: 'deleteObjects' }); + } + throw err; } - return { failed }; } async listAllKeys(prefix: string): Promise { diff --git a/src/lib/server/storage/service.test.ts b/src/lib/server/storage/service.test.ts deleted file mode 100644 index fa4be91f..00000000 --- a/src/lib/server/storage/service.test.ts +++ /dev/null @@ -1,247 +0,0 @@ -import { describe, it, expect, vi, beforeEach } from 'vitest'; -import { faker } from '@faker-js/faker'; -import { S3ServiceException } from '@aws-sdk/client-s3'; - -vi.mock('$lib/server/logging', () => ({ - logger: { child: () => ({ info: vi.fn(), debug: vi.fn(), warn: vi.fn() }) } -})); - -const mockSend = vi.fn(); -vi.mock('./s3-client.js', () => ({ - createS3Client: () => ({ send: (...args: unknown[]) => mockSend(...args) }) -})); - -const mockProvider = { - listObjects: vi.fn(), - getObject: vi.fn(), - getObjectRange: vi.fn(), - getMetadata: vi.fn(), - exists: vi.fn(), - putObject: vi.fn(), - deleteObjects: vi.fn(), - listAllKeys: vi.fn() -}; -vi.mock('./utils.js', () => ({ - getProvider: () => mockProvider -})); - -// eslint-disable-next-line @typescript-eslint/no-unused-vars -const mockMapS3ErrorToHttp = vi.fn((err: unknown, _context?: unknown) => { - throw err; -}); -vi.mock('./s3-errors.js', () => ({ - mapS3ErrorToHttp: (err: unknown, context: unknown) => mockMapS3ErrorToHttp(err, context) -})); - -function makeS3Error(name = 'NoSuchKey'): S3ServiceException { - return new S3ServiceException({ name, message: name, $fault: 'client', $metadata: {} }); -} - -import { - listBuckets, - listObjects, - downloadObject, - getObjectMetadata, - uploadObject, - deleteObjects -} from './service.js'; -import type { S3ConnectionConfig } from './types.js'; - -const config: S3ConnectionConfig = { - type: 's3', - region: faker.location.countryCode() -}; - -describe('storage service', () => { - beforeEach(() => { - vi.clearAllMocks(); - }); - - describe('listBuckets', () => { - it('returns bucket names', async () => { - mockSend.mockResolvedValue({ Buckets: [{ Name: 'a' }, { Name: 'b' }] }); - expect(await listBuckets(config)).toEqual(['a', 'b']); - }); - - it('filters out buckets with no name', async () => { - mockSend.mockResolvedValue({ Buckets: [{ Name: 'a' }, { Name: undefined }, { Name: '' }] }); - expect(await listBuckets(config)).toEqual(['a']); - }); - - it('handles null Buckets in response', async () => { - mockSend.mockResolvedValue({ Buckets: null }); - expect(await listBuckets(config)).toEqual([]); - }); - }); - - describe('listObjects', () => { - it('proxies to provider', async () => { - const page = { objects: [], prefixes: [], nextToken: null }; - mockProvider.listObjects.mockResolvedValue(page); - const result = await listObjects(config, 'bucket', 'prefix/', 100); - expect(mockProvider.listObjects).toHaveBeenCalledWith('prefix/', 100, undefined); - expect(result).toEqual(page); - }); - - it('passes continuation token when provided', async () => { - const page = { objects: [], prefixes: [], nextToken: null }; - mockProvider.listObjects.mockResolvedValue(page); - await listObjects(config, 'bucket', '', 10, 'tok'); - expect(mockProvider.listObjects).toHaveBeenCalledWith('', 10, 'tok'); - }); - - it('passes undefined when continuation token is null', async () => { - const page = { objects: [], prefixes: [], nextToken: null }; - mockProvider.listObjects.mockResolvedValue(page); - await listObjects(config, 'bucket', '', 10, null); - expect(mockProvider.listObjects).toHaveBeenCalledWith('', 10, undefined); - }); - - it('throws S3ServiceException via mapS3ErrorToHttp', async () => { - const err = makeS3Error('AccessDenied'); - mockProvider.listObjects.mockRejectedValue(err); - await expect(listObjects(config, 'bucket', '', 10)).rejects.toThrow(err); - expect(mockMapS3ErrorToHttp).toHaveBeenCalledWith(err, { - bucket: 'bucket', - operation: 'listObjects' - }); - }); - - it('rethrows non-S3 errors', async () => { - const err = new Error('network'); - mockProvider.listObjects.mockRejectedValue(err); - await expect(listObjects(config, 'bucket', '', 10)).rejects.toThrow('network'); - expect(mockMapS3ErrorToHttp).not.toHaveBeenCalled(); - }); - }); - - describe('downloadObject', () => { - it('returns download from provider', async () => { - const download = { stream: new ReadableStream(), contentType: 'text/plain' }; - mockProvider.getObject.mockResolvedValue(download); - const result = await downloadObject(config, 'bucket', 'key.txt'); - expect(result).toEqual(download); - }); - - it('throws S3ServiceException via mapS3ErrorToHttp', async () => { - const err = makeS3Error('NoSuchKey'); - mockProvider.getObject.mockRejectedValue(err); - await expect(downloadObject(config, 'bucket', 'key.txt')).rejects.toThrow(err); - expect(mockMapS3ErrorToHttp).toHaveBeenCalledWith(err, { - bucket: 'bucket', - key: 'key.txt', - operation: 'getObject' - }); - }); - - it('rethrows non-S3 errors', async () => { - const err = new Error('timeout'); - mockProvider.getObject.mockRejectedValue(err); - await expect(downloadObject(config, 'bucket', 'k')).rejects.toThrow('timeout'); - expect(mockMapS3ErrorToHttp).not.toHaveBeenCalled(); - }); - }); - - describe('getObjectMetadata', () => { - it('proxies to provider', async () => { - const meta = { contentType: 'text/plain', contentLength: 42 }; - mockProvider.getMetadata.mockResolvedValue(meta); - expect(await getObjectMetadata(config, 'bucket', 'k')).toEqual(meta); - }); - - it('throws S3ServiceException via mapS3ErrorToHttp', async () => { - const err = makeS3Error('NoSuchKey'); - mockProvider.getMetadata.mockRejectedValue(err); - await expect(getObjectMetadata(config, 'bucket', 'k')).rejects.toThrow(err); - expect(mockMapS3ErrorToHttp).toHaveBeenCalledWith(err, { - bucket: 'bucket', - key: 'k', - operation: 'getMetadata' - }); - }); - - it('rethrows non-S3 errors', async () => { - const err = new Error('gone'); - mockProvider.getMetadata.mockRejectedValue(err); - await expect(getObjectMetadata(config, 'bucket', 'k')).rejects.toThrow('gone'); - expect(mockMapS3ErrorToHttp).not.toHaveBeenCalled(); - }); - }); - - describe('uploadObject', () => { - it('proxies to provider', async () => { - mockProvider.putObject.mockResolvedValue(undefined); - await uploadObject(config, 'bucket', 'k', Buffer.from('x'), 'text/plain', 1); - expect(mockProvider.putObject).toHaveBeenCalledWith('k', expect.any(Buffer), 'text/plain', 1); - }); - - it('throws S3ServiceException via mapS3ErrorToHttp', async () => { - const err = makeS3Error('AccessDenied'); - mockProvider.putObject.mockRejectedValue(err); - await expect( - uploadObject(config, 'bucket', 'k', Buffer.from('x'), 'text/plain') - ).rejects.toThrow(err); - expect(mockMapS3ErrorToHttp).toHaveBeenCalledWith(err, { - bucket: 'bucket', - key: 'k', - operation: 'putObject' - }); - }); - - it('rethrows non-S3 errors', async () => { - const err = new Error('disk full'); - mockProvider.putObject.mockRejectedValue(err); - await expect( - uploadObject(config, 'bucket', 'k', Buffer.from('x'), 'text/plain') - ).rejects.toThrow('disk full'); - expect(mockMapS3ErrorToHttp).not.toHaveBeenCalled(); - }); - }); - - describe('deleteObjects', () => { - it('expands directory prefixes and deletes all', async () => { - mockProvider.listAllKeys.mockResolvedValue(['dir/a.txt', 'dir/b.txt']); - mockProvider.deleteObjects.mockResolvedValue({ failed: [] }); - const result = await deleteObjects(config, 'bucket', ['file.txt', 'dir/']); - expect(mockProvider.listAllKeys).toHaveBeenCalledWith('dir/'); - expect(mockProvider.deleteObjects).toHaveBeenCalledWith([ - 'file.txt', - 'dir/a.txt', - 'dir/b.txt' - ]); - expect(result).toEqual({ failed: [] }); - }); - - it('uses directory key itself when no children found', async () => { - mockProvider.listAllKeys.mockResolvedValue([]); - mockProvider.deleteObjects.mockResolvedValue({ failed: [] }); - await deleteObjects(config, 'bucket', ['empty/']); - expect(mockProvider.deleteObjects).toHaveBeenCalledWith(['empty/']); - }); - - it('returns empty result for no keys', async () => { - mockProvider.listAllKeys.mockResolvedValue([]); - mockProvider.deleteObjects.mockResolvedValue({ failed: [] }); - const result = await deleteObjects(config, 'bucket', []); - expect(result).toEqual({ failed: [] }); - }); - - it('throws S3ServiceException via mapS3ErrorToHttp', async () => { - const err = makeS3Error('AccessDenied'); - mockProvider.listAllKeys.mockResolvedValue([]); - mockProvider.deleteObjects.mockRejectedValue(err); - await expect(deleteObjects(config, 'bucket', ['file.txt'])).rejects.toThrow(err); - expect(mockMapS3ErrorToHttp).toHaveBeenCalledWith(err, { - bucket: 'bucket', - operation: 'deleteObjects' - }); - }); - - it('rethrows non-S3 errors', async () => { - const err = new Error('network'); - mockProvider.deleteObjects.mockRejectedValue(err); - await expect(deleteObjects(config, 'bucket', ['file.txt'])).rejects.toThrow('network'); - expect(mockMapS3ErrorToHttp).not.toHaveBeenCalled(); - }); - }); -}); diff --git a/src/lib/server/storage/service.ts b/src/lib/server/storage/service.ts deleted file mode 100644 index c27b6532..00000000 --- a/src/lib/server/storage/service.ts +++ /dev/null @@ -1,146 +0,0 @@ -import { S3ServiceException, ListBucketsCommand } from '@aws-sdk/client-s3'; -import { createS3Client } from './s3-client.js'; -import { mapS3ErrorToHttp } from './s3-errors.js'; -import { getProvider } from './utils.js'; -import type { StoragePage, StorageMetadata } from '$lib/storage/types.js'; -import type { ObjectDownload, DeleteObjectsResult } from './provider.js'; -import type { S3ConnectionConfig } from './types.js'; -import { logger } from '$lib/server/logging'; - -const log = logger.child({ module: 'storage-service' }); - -/** List all buckets accessible with the given connection config. */ -export async function listBuckets(config: S3ConnectionConfig): Promise { - const client = createS3Client(config); - - const output = await client.send(new ListBucketsCommand({})); - const buckets = (output.Buckets ?? []).map((b) => b.Name ?? '').filter(Boolean); - log.debug({ storage_type: config.type, bucket_count: buckets.length }, 'listed buckets'); - return buckets; -} - -/** List objects at the given bucket/prefix. Throws on S3 errors. */ -export async function listObjects( - config: S3ConnectionConfig, - bucket: string, - prefix: string, - pageSize: number, - continuationToken?: string | null -): Promise { - const provider = getProvider(config, bucket); - - try { - return await provider.listObjects(prefix, pageSize, continuationToken ?? undefined); - } catch (err) { - if (err instanceof S3ServiceException) { - mapS3ErrorToHttp(err, { bucket, operation: 'listObjects' }); - } - throw err; - } -} - -/** Download a single object from the bucket, returning a stream and metadata for the HTTP response. */ -export async function downloadObject( - config: S3ConnectionConfig, - bucket: string, - key: string -): Promise { - const provider = getProvider(config, bucket); - - try { - log.debug({ bucket, key }, 'downloading object'); - const download = await provider.getObject(key); - log.info({ bucket, key }, 'object download started'); - return download; - } catch (err) { - if (err instanceof S3ServiceException) { - mapS3ErrorToHttp(err, { bucket, key, operation: 'getObject' }); - } - throw err; - } -} - -/** Fetch metadata for a single object — used for lightweight pre-flight checks. */ -export async function getObjectMetadata( - config: S3ConnectionConfig, - bucket: string, - key: string -): Promise { - const provider = getProvider(config, bucket); - - try { - log.debug({ bucket, key }, 'getting object metadata'); - return await provider.getMetadata(key); - } catch (err) { - if (err instanceof S3ServiceException) { - mapS3ErrorToHttp(err, { bucket, key, operation: 'getMetadata' }); - } - throw err; - } -} - -/** Upload an object to the bucket, using multipart upload for large files. */ -export async function uploadObject( - config: S3ConnectionConfig, - bucket: string, - key: string, - body: ReadableStream | Buffer, - contentType: string, - contentLength?: number -): Promise { - const provider = getProvider(config, bucket); - - try { - log.debug({ bucket, key, content_type: contentType }, 'uploading object'); - await provider.putObject(key, body, contentType, contentLength); - log.info( - { bucket, key, content_type: contentType, content_length: contentLength }, - 'object uploaded' - ); - } catch (err) { - if (err instanceof S3ServiceException) { - mapS3ErrorToHttp(err, { bucket, key, operation: 'putObject' }); - } - throw err; - } -} - -/** Delete one or more objects from the bucket. Returns a result listing any keys that failed. - * Directory keys (ending with '/') are expanded to all contained objects before deletion. */ -export async function deleteObjects( - config: S3ConnectionConfig, - bucket: string, - keys: string[] -): Promise { - const provider = getProvider(config, bucket); - - try { - // Expand any directory prefixes (keys ending with '/') to their contents. - const dirPrefixes = keys.filter((k) => k.endsWith('/')); - const fileKeys = keys.filter((k) => !k.endsWith('/')); - - let allKeys = [...fileKeys]; - for (const prefix of dirPrefixes) { - log.debug({ bucket, prefix }, 'expanding directory prefix for deletion'); - const children = await provider.listAllKeys(prefix); - allKeys = allKeys.concat(children.length > 0 ? children : [prefix]); - } - - if (allKeys.length === 0) { - return { failed: [] }; - } - - log.debug({ bucket, key_count: allKeys.length }, 'deleting objects'); - const result = await provider.deleteObjects(allKeys); - log.info( - { bucket, key_count: allKeys.length, failed_count: result.failed.length }, - 'objects delete completed' - ); - return result; - } catch (err) { - if (err instanceof S3ServiceException) { - mapS3ErrorToHttp(err, { bucket, operation: 'deleteObjects' }); - } - throw err; - } -} diff --git a/src/lib/server/storage/utils.ts b/src/lib/server/storage/utils.ts index a0ab2223..7efb2d95 100644 --- a/src/lib/server/storage/utils.ts +++ b/src/lib/server/storage/utils.ts @@ -14,3 +14,16 @@ export function getProvider(config: S3ConnectionConfig, bucket: string): Storage return StorageProviderFactory.create({ ...config, bucket }); } + +/** + * Construct a connection-scoped storage provider for operations that do not + * require a specific bucket (e.g. listing all buckets). + * Throws a 400 HTTP error if the connection type is not supported. + */ +export function getConnectionProvider(config: S3ConnectionConfig): StorageProvider { + if (config.type !== 's3') { + throw error(400, 'Storage backend not supported'); + } + + return StorageProviderFactory.create({ ...config, bucket: '' }); +} diff --git a/src/routes/(app)/storage/+page.server.ts b/src/routes/(app)/storage/+page.server.ts index b383aec3..a9a205f2 100644 --- a/src/routes/(app)/storage/+page.server.ts +++ b/src/routes/(app)/storage/+page.server.ts @@ -3,7 +3,7 @@ import { superValidate, message } from 'sveltekit-superforms'; import { zod4 as zod } from 'sveltekit-superforms/adapters'; import type { Actions, PageServerLoad } from './$types'; import { StorageConnectionSchema } from '$lib/storage/schemas.js'; -import { listBuckets } from '$lib/server/storage/service.js'; +import { getConnectionProvider } from '$lib/server/storage/utils.js'; import type { S3ConnectionConfig } from '$lib/server/storage/types.js'; export const load: PageServerLoad = async ({ locals }) => { @@ -38,7 +38,7 @@ export const actions: Actions = { }; try { - await listBuckets(config); + await getConnectionProvider(config).listBuckets(); log.info({ storage_type: type }, 'user storage connection verified'); } catch (err) { log.warn({ err }, 'storage connection test failed'); diff --git a/src/routes/(app)/storage/api/buckets/+server.ts b/src/routes/(app)/storage/api/buckets/+server.ts index 620f983a..0afdce95 100644 --- a/src/routes/(app)/storage/api/buckets/+server.ts +++ b/src/routes/(app)/storage/api/buckets/+server.ts @@ -1,4 +1,4 @@ -import { listBuckets } from '$lib/server/storage/service.js'; +import { getConnectionProvider } from '$lib/server/storage/utils.js'; import { requireConnection } from '$lib/server/storage/connection.js'; import type { RequestHandler } from './$types'; @@ -10,7 +10,7 @@ import type { RequestHandler } from './$types'; */ export const GET: RequestHandler = async ({ request, locals }) => { const config = requireConnection(request); - const buckets = await listBuckets(config); + const buckets = await getConnectionProvider(config).listBuckets(); locals.logger.debug({ bucket_count: buckets.length }, 'bucket list returned'); return Response.json(buckets); }; diff --git a/src/routes/(app)/storage/api/delete/+server.ts b/src/routes/(app)/storage/api/delete/+server.ts index 4892bb35..5ca7d213 100644 --- a/src/routes/(app)/storage/api/delete/+server.ts +++ b/src/routes/(app)/storage/api/delete/+server.ts @@ -1,6 +1,6 @@ import type { RequestHandler } from './$types'; import { error } from '@sveltejs/kit'; -import { deleteObjects } from '$lib/server/storage/service.js'; +import { getProvider } from '$lib/server/storage/utils.js'; import { requireConnection } from '$lib/server/storage/connection.js'; import { requireBucket } from '../params.js'; @@ -24,7 +24,7 @@ export const DELETE: RequestHandler = async ({ locals, url, request }) => { locals.logger.debug({ bucket, key_count: keys.length }, 'delete request received'); - const result = await deleteObjects(config, bucket, keys); + const result = await getProvider(config, bucket).deleteObjects(keys); locals.logger.info( { bucket, key_count: keys.length, failed_count: result.failed.length }, diff --git a/src/routes/(app)/storage/api/delete/server.test.ts b/src/routes/(app)/storage/api/delete/server.test.ts index 35783ccb..1418a311 100644 --- a/src/routes/(app)/storage/api/delete/server.test.ts +++ b/src/routes/(app)/storage/api/delete/server.test.ts @@ -1,8 +1,9 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { faker } from '@faker-js/faker'; -vi.mock('$lib/server/storage/service.js', () => ({ - deleteObjects: vi.fn() +const mockProvider = { deleteObjects: vi.fn() }; +vi.mock('$lib/server/storage/utils.js', () => ({ + getProvider: () => mockProvider })); vi.mock('$lib/server/storage/connection.js', () => ({ @@ -11,7 +12,6 @@ vi.mock('$lib/server/storage/connection.js', () => ({ })); import { DELETE } from './+server.js'; -import { deleteObjects } from '$lib/server/storage/service.js'; const CONNECTION_HEADER = { 'x-storage-connection': btoa(JSON.stringify({ type: 's3', region: 'us-east-1' })) @@ -51,12 +51,10 @@ describe('DELETE /storage/api/delete', () => { it('deletes objects and returns JSON result', async () => { const keys = [faker.system.fileName(), faker.system.fileName()]; const result = { deleted: keys, failed: [] }; - vi.mocked(deleteObjects).mockResolvedValue( - result as unknown as Awaited> - ); + mockProvider.deleteObjects.mockResolvedValue(result); const response = await DELETE(mockEvent({ bucket: 'b1', keys })); - expect(deleteObjects).toHaveBeenCalledWith(expect.objectContaining({ type: 's3' }), 'b1', keys); + expect(mockProvider.deleteObjects).toHaveBeenCalledWith(keys); expect(await response.json()).toEqual(result); }); }); diff --git a/src/routes/(app)/storage/api/download/+server.ts b/src/routes/(app)/storage/api/download/+server.ts index e43df827..f74dff8a 100644 --- a/src/routes/(app)/storage/api/download/+server.ts +++ b/src/routes/(app)/storage/api/download/+server.ts @@ -1,5 +1,5 @@ import type { RequestHandler } from './$types'; -import { downloadObject, getObjectMetadata } from '$lib/server/storage/service.js'; +import { getProvider } from '$lib/server/storage/utils.js'; import { requireConnection } from '$lib/server/storage/connection.js'; import { requireBucketKey } from '../params.js'; @@ -25,7 +25,7 @@ export const GET: RequestHandler = async ({ locals, url, request }) => { locals.logger.debug({ bucket, key }, 'download request received'); - const download = await downloadObject(config, bucket, key); + const download = await getProvider(config, bucket).getObject(key); const filename = filenameFromKey(key); // RFC 5987 encoding for non-ASCII filenames in Content-Disposition @@ -66,7 +66,7 @@ export const HEAD: RequestHandler = async ({ locals, url, request }) => { locals.logger.debug({ bucket, key }, 'download pre-flight check'); - const meta = await getObjectMetadata(config, bucket, key); + const meta = await getProvider(config, bucket).getMetadata(key); return new Response(null, { status: 200, diff --git a/src/routes/(app)/storage/api/download/server.test.ts b/src/routes/(app)/storage/api/download/server.test.ts index d0304e87..6f754b32 100644 --- a/src/routes/(app)/storage/api/download/server.test.ts +++ b/src/routes/(app)/storage/api/download/server.test.ts @@ -1,8 +1,8 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -vi.mock('$lib/server/storage/service.js', () => ({ - downloadObject: vi.fn(), - getObjectMetadata: vi.fn() +const mockProvider = { getObject: vi.fn(), getMetadata: vi.fn() }; +vi.mock('$lib/server/storage/utils.js', () => ({ + getProvider: () => mockProvider })); vi.mock('$lib/server/storage/connection.js', () => ({ @@ -11,7 +11,6 @@ vi.mock('$lib/server/storage/connection.js', () => ({ })); import { GET, HEAD } from './+server.js'; -import { downloadObject, getObjectMetadata } from '$lib/server/storage/service.js'; const CONNECTION_HEADER = { 'x-storage-connection': btoa(JSON.stringify({ type: 's3', region: 'us-east-1' })) @@ -31,12 +30,12 @@ describe('GET /storage/api/download', () => { it('streams download with correct headers', async () => { const stream = new ReadableStream(); - vi.mocked(downloadObject).mockResolvedValue({ + mockProvider.getObject.mockResolvedValue({ stream, contentType: 'text/csv', contentLength: 1234, etag: '"abc"' - } as unknown as Awaited>); + }); const res = await GET(mockEvent('bucket=b1&key=path/data.csv')); @@ -48,12 +47,12 @@ describe('GET /storage/api/download', () => { }); it('uses application/octet-stream when no content type', async () => { - vi.mocked(downloadObject).mockResolvedValue({ + mockProvider.getObject.mockResolvedValue({ stream: new ReadableStream(), contentType: undefined, contentLength: undefined, etag: undefined - } as unknown as Awaited>); + }); const res = await GET(mockEvent('bucket=b1&key=file.bin')); expect(res.headers.get('Content-Type')).toBe('application/octet-stream'); @@ -65,10 +64,10 @@ describe('HEAD /storage/api/download', () => { beforeEach(() => vi.clearAllMocks()); it('returns 200 with metadata headers', async () => { - vi.mocked(getObjectMetadata).mockResolvedValue({ + mockProvider.getMetadata.mockResolvedValue({ contentType: 'application/json', size: 999 - } as unknown as Awaited>); + }); const res = await HEAD(mockEvent('bucket=b1&key=data.json')); diff --git a/src/routes/(app)/storage/api/objects/+server.ts b/src/routes/(app)/storage/api/objects/+server.ts index d1fb56f9..547be682 100644 --- a/src/routes/(app)/storage/api/objects/+server.ts +++ b/src/routes/(app)/storage/api/objects/+server.ts @@ -1,5 +1,5 @@ import { error } from '@sveltejs/kit'; -import { listObjects } from '$lib/server/storage/service.js'; +import { getProvider } from '$lib/server/storage/utils.js'; import { requireConnection } from '$lib/server/storage/connection.js'; import type { RequestHandler } from './$types'; @@ -25,6 +25,10 @@ export const GET: RequestHandler = async ({ request, url, locals }) => { 'listing objects' ); - const page = await listObjects(config, bucket, prefix, pageSize, continuationToken); + const page = await getProvider(config, bucket).listObjects( + prefix, + pageSize, + continuationToken ?? undefined + ); return Response.json(page); }; diff --git a/src/routes/(app)/storage/api/upload/+server.ts b/src/routes/(app)/storage/api/upload/+server.ts index dc58c546..096b9488 100644 --- a/src/routes/(app)/storage/api/upload/+server.ts +++ b/src/routes/(app)/storage/api/upload/+server.ts @@ -1,5 +1,5 @@ import { error } from '@sveltejs/kit'; -import { uploadObject } from '$lib/server/storage/service.js'; +import { getProvider } from '$lib/server/storage/utils.js'; import { requireConnection } from '$lib/server/storage/connection.js'; import { requireBucketKey } from '../params.js'; import type { RequestHandler } from './$types'; @@ -35,7 +35,7 @@ export const POST: RequestHandler = async ({ locals, url, request }) => { 'upload request received' ); - await uploadObject(config, bucket, key, request.body, contentType, contentLength); + await getProvider(config, bucket).putObject(key, request.body, contentType, contentLength); log.info( { bucket, key, content_type: contentType, content_length: contentLength }, diff --git a/src/routes/(app)/storage/api/upload/server.test.ts b/src/routes/(app)/storage/api/upload/server.test.ts index b8adfdb9..f50a6cee 100644 --- a/src/routes/(app)/storage/api/upload/server.test.ts +++ b/src/routes/(app)/storage/api/upload/server.test.ts @@ -1,7 +1,8 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -vi.mock('$lib/server/storage/service.js', () => ({ - uploadObject: vi.fn() +const mockProvider = { putObject: vi.fn() }; +vi.mock('$lib/server/storage/utils.js', () => ({ + getProvider: () => mockProvider })); vi.mock('$lib/server/storage/connection.js', () => ({ @@ -10,7 +11,6 @@ vi.mock('$lib/server/storage/connection.js', () => ({ })); import { POST } from './+server.js'; -import { uploadObject } from '$lib/server/storage/service.js'; const CONNECTION_HEADER = { 'x-storage-connection': btoa(JSON.stringify({ type: 's3', region: 'us-east-1' })) @@ -51,7 +51,7 @@ describe('POST /storage/api/upload', () => { }); it('uploads and returns 201', async () => { - vi.mocked(uploadObject).mockResolvedValue(undefined); + mockProvider.putObject.mockResolvedValue(undefined); const body = new ReadableStream(); const res = await POST( @@ -66,22 +66,15 @@ describe('POST /storage/api/upload', () => { ); expect(res.status).toBe(201); - expect(uploadObject).toHaveBeenCalledWith( - expect.objectContaining({ type: 's3' }), - 'b1', - 'file.txt', - body, - 'image/png', - 100 - ); + expect(mockProvider.putObject).toHaveBeenCalledWith(body, 'image/png', 100); }); it('defaults content type to application/octet-stream', async () => { - vi.mocked(uploadObject).mockResolvedValue(undefined); + mockProvider.putObject.mockResolvedValue(undefined); const res = await POST(mockEvent({ headers: { ...CONNECTION_HEADER } })); expect(res.status).toBe(201); - expect(vi.mocked(uploadObject).mock.calls[0][4]).toBe('application/octet-stream'); + expect(mockProvider.putObject.mock.calls[0][1]).toBe('application/octet-stream'); }); }); diff --git a/src/routes/(app)/storage/page.server.test.ts b/src/routes/(app)/storage/page.server.test.ts index 207b214c..89efaa6b 100644 --- a/src/routes/(app)/storage/page.server.test.ts +++ b/src/routes/(app)/storage/page.server.test.ts @@ -1,7 +1,8 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -vi.mock('$lib/server/storage/service.js', () => ({ - listBuckets: vi.fn() +const mockConnectionProvider = { listBuckets: vi.fn() }; +vi.mock('$lib/server/storage/utils.js', () => ({ + getConnectionProvider: () => mockConnectionProvider })); vi.mock('$lib/storage/schemas.js', () => ({ @@ -18,7 +19,6 @@ vi.mock('sveltekit-superforms/adapters', () => ({ })); import { load, actions } from './+page.server.js'; -import { listBuckets } from '$lib/server/storage/service.js'; import { superValidate } from 'sveltekit-superforms'; function mockLocals() { @@ -87,7 +87,7 @@ describe('storage page actions', () => { secretAccessKey: 'sk' } } as unknown as Awaited>); - vi.mocked(listBuckets).mockRejectedValue(new Error('connection refused')); + mockConnectionProvider.listBuckets.mockRejectedValue(new Error('connection refused')); const result = await actions.connect({ request: new Request('http://localhost', { method: 'POST' }), @@ -111,7 +111,7 @@ describe('storage page actions', () => { secretAccessKey: 'sk' } } as unknown as Awaited>); - vi.mocked(listBuckets).mockResolvedValue(['b1']); + mockConnectionProvider.listBuckets.mockResolvedValue(['b1']); await expect( actions.connect({ From 739fbdbb0d33c990ca50e04b94955ba416c8f796 Mon Sep 17 00:00:00 2001 From: dklOrdix Date: Mon, 8 Jun 2026 09:13:57 +0200 Subject: [PATCH 7/8] refactor: rename listBuckets to listContainers across storage providers and tests --- src/lib/server/storage/hdfs-provider.ts | 2 +- src/lib/server/storage/preview/stream.test.ts | 2 +- src/lib/server/storage/provider.ts | 2 +- src/lib/server/storage/s3-provider.test.ts | 14 +- src/lib/server/storage/s3-provider.ts | 245 +++++++----------- src/routes/(app)/storage/+page.server.ts | 2 +- .../(app)/storage/api/buckets/+server.ts | 2 +- src/routes/(app)/storage/page.server.test.ts | 6 +- 8 files changed, 107 insertions(+), 168 deletions(-) diff --git a/src/lib/server/storage/hdfs-provider.ts b/src/lib/server/storage/hdfs-provider.ts index 1a8df630..faa5c07c 100644 --- a/src/lib/server/storage/hdfs-provider.ts +++ b/src/lib/server/storage/hdfs-provider.ts @@ -10,7 +10,7 @@ import type { StoragePage, StorageMetadata } from '$lib/storage/types.js'; export class HDFSStorageProvider implements StorageProvider { constructor(_config: HDFSConfig) {} - listBuckets(): Promise { + listContainers(): Promise { throw new Error('HDFS not implemented'); } diff --git a/src/lib/server/storage/preview/stream.test.ts b/src/lib/server/storage/preview/stream.test.ts index 6d11d0e7..458c16c8 100644 --- a/src/lib/server/storage/preview/stream.test.ts +++ b/src/lib/server/storage/preview/stream.test.ts @@ -7,7 +7,7 @@ const mockLog = { info: vi.fn(), debug: vi.fn(), warn: vi.fn() } as unknown as p function makeProvider(overrides: Partial = {}): StorageProvider { return { - listBuckets: vi.fn(), + listContainers: vi.fn(), listObjects: vi.fn(), getObject: vi .fn() diff --git a/src/lib/server/storage/provider.ts b/src/lib/server/storage/provider.ts index c666165b..2977e238 100644 --- a/src/lib/server/storage/provider.ts +++ b/src/lib/server/storage/provider.ts @@ -13,7 +13,7 @@ export interface ObjectDownload { /** Backend-agnostic interface for a bucket-scoped storage provider. */ export interface StorageProvider { /** List all buckets accessible with the current connection credentials. */ - listBuckets(): Promise; + listContainers(): Promise; /** * List objects using cursor-based pagination. `continuationToken` is the * provider-specific opaque token returned from a previous call. When diff --git a/src/lib/server/storage/s3-provider.test.ts b/src/lib/server/storage/s3-provider.test.ts index 73ada8fe..7856b1f6 100644 --- a/src/lib/server/storage/s3-provider.test.ts +++ b/src/lib/server/storage/s3-provider.test.ts @@ -511,10 +511,10 @@ describe('S3StorageProvider constructor', () => { }); // --------------------------------------------------------------------------- -// listBuckets +// listContainers // --------------------------------------------------------------------------- -describe('S3StorageProvider.listBuckets', () => { +describe('S3StorageProvider.listContainers', () => { let provider: S3StorageProvider; let send: ReturnType; @@ -524,28 +524,28 @@ describe('S3StorageProvider.listBuckets', () => { it('returns bucket names', async () => { send.mockResolvedValue({ Buckets: [{ Name: 'a' }, { Name: 'b' }] }); - expect(await provider.listBuckets()).toEqual(['a', 'b']); + expect(await provider.listContainers()).toEqual(['a', 'b']); }); it('filters out buckets with no name', async () => { send.mockResolvedValue({ Buckets: [{ Name: 'a' }, { Name: undefined }, { Name: '' }] }); - expect(await provider.listBuckets()).toEqual(['a']); + expect(await provider.listContainers()).toEqual(['a']); }); it('handles null Buckets in response', async () => { send.mockResolvedValue({ Buckets: null }); - expect(await provider.listBuckets()).toEqual([]); + expect(await provider.listContainers()).toEqual([]); }); it('maps AccessDenied S3 error to HTTP 403', async () => { send.mockRejectedValue(makeS3Error('AccessDenied', 403)); - await expect(provider.listBuckets()).rejects.toMatchObject({ status: 403 }); + await expect(provider.listContainers()).rejects.toMatchObject({ status: 403 }); }); it('re-throws non-S3 errors', async () => { const err = new Error('network'); send.mockRejectedValue(err); - await expect(provider.listBuckets()).rejects.toThrow('network'); + await expect(provider.listContainers()).rejects.toThrow('network'); }); }); diff --git a/src/lib/server/storage/s3-provider.ts b/src/lib/server/storage/s3-provider.ts index 60052508..7f2594fb 100644 --- a/src/lib/server/storage/s3-provider.ts +++ b/src/lib/server/storage/s3-provider.ts @@ -27,7 +27,7 @@ export class S3StorageProvider implements StorageProvider { this.client = client ?? createS3Client(config); } - async listBuckets(): Promise { + async listContainers(): Promise { log.trace({ bucket: this.bucket }, 'S3 ListBuckets'); try { const output = await this.client.send(new ListBucketsCommand({})); @@ -49,113 +49,81 @@ export class S3StorageProvider implements StorageProvider { ): Promise { log.trace({ bucket: this.bucket, prefix, page_size: pageSize }, 'S3 ListObjectsV2'); - try { - const output: ListObjectsV2CommandOutput = await this.client.send( - new ListObjectsV2Command({ - Bucket: this.bucket, - Prefix: prefix || undefined, - Delimiter: '/', - MaxKeys: pageSize, - ContinuationToken: continuationToken ?? undefined - }) - ); + const output: ListObjectsV2CommandOutput = await this.client.send( + new ListObjectsV2Command({ + Bucket: this.bucket, + Prefix: prefix || undefined, + Delimiter: '/', + MaxKeys: pageSize, + ContinuationToken: continuationToken ?? undefined + }) + ); - const objects: StorageObject[] = [ - ...(output.CommonPrefixes ?? []).map((cp) => ({ - key: cp.Prefix ?? '', - size: 0, - lastModified: new Date(0), - isDirectory: true, + const objects: StorageObject[] = [ + ...(output.CommonPrefixes ?? []).map((cp) => ({ + key: cp.Prefix ?? '', + size: 0, + lastModified: new Date(0), + isDirectory: true, + contentType: undefined + })), + ...(output.Contents ?? []) + // Filter out the prefix itself (S3 sometimes echoes it back) + .filter((obj) => obj.Key !== prefix) + .map((obj) => ({ + key: obj.Key ?? '', + size: obj.Size ?? 0, + lastModified: obj.LastModified ?? new Date(0), + isDirectory: false, contentType: undefined - })), - ...(output.Contents ?? []) - // Filter out the prefix itself (S3 sometimes echoes it back) - .filter((obj) => obj.Key !== prefix) - .map((obj) => ({ - key: obj.Key ?? '', - size: obj.Size ?? 0, - lastModified: obj.LastModified ?? new Date(0), - isDirectory: false, - contentType: undefined - })) - ]; - - return { - objects, - hasNextPage: output.IsTruncated ?? false, - currentPage: 1, - pageSize, - continuationToken: continuationToken ?? null, - nextContinuationToken: output.NextContinuationToken ?? null - }; - } catch (err) { - if (err instanceof S3ServiceException) { - mapS3ErrorToHttp(err, { bucket: this.bucket, operation: 'listObjects' }); - } - throw err; - } + })) + ]; + + return { + objects, + hasNextPage: output.IsTruncated ?? false, + currentPage: 1, + pageSize, + continuationToken: continuationToken ?? null, + nextContinuationToken: output.NextContinuationToken ?? null + }; } async getObject(key: string): Promise { log.trace({ bucket: this.bucket, key }, 'S3 GetObject'); - try { - const output = await this.client.send( - new GetObjectCommand({ Bucket: this.bucket, Key: key }) - ); - if (!output.Body) { - throw new Error(`Object ${key} has no body`); - } - return { - stream: output.Body.transformToWebStream(), - contentType: output.ContentType, - contentLength: output.ContentLength, - etag: output.ETag - }; - } catch (err) { - if (err instanceof S3ServiceException) { - mapS3ErrorToHttp(err, { bucket: this.bucket, key, operation: 'getObject' }); - } - throw err; + const output = await this.client.send(new GetObjectCommand({ Bucket: this.bucket, Key: key })); + if (!output.Body) { + throw new Error(`Object ${key} has no body`); } + return { + stream: output.Body.transformToWebStream(), + contentType: output.ContentType, + contentLength: output.ContentLength, + etag: output.ETag + }; } async getObjectRange(key: string, start: number, end: number): Promise { log.trace({ bucket: this.bucket, key, start, end }, 'S3 GetObject (range)'); - try { - const output = await this.client.send( - new GetObjectCommand({ Bucket: this.bucket, Key: key, Range: `bytes=${start}-${end}` }) - ); - if (!output.Body) { - throw new Error(`Object ${key} has no body`); - } - return output.Body.transformToWebStream(); - } catch (err) { - if (err instanceof S3ServiceException) { - mapS3ErrorToHttp(err, { bucket: this.bucket, key, operation: 'getObjectRange' }); - } - throw err; + const output = await this.client.send( + new GetObjectCommand({ Bucket: this.bucket, Key: key, Range: `bytes=${start}-${end}` }) + ); + if (!output.Body) { + throw new Error(`Object ${key} has no body`); } + return output.Body.transformToWebStream(); } async getMetadata(key: string): Promise { log.trace({ bucket: this.bucket, key }, 'S3 HeadObject'); - try { - const output = await this.client.send( - new HeadObjectCommand({ Bucket: this.bucket, Key: key }) - ); - return { - size: output.ContentLength ?? 0, - lastModified: output.LastModified ?? new Date(0), - contentType: output.ContentType, - etag: output.ETag, - customMetadata: output.Metadata - }; - } catch (err) { - if (err instanceof S3ServiceException) { - mapS3ErrorToHttp(err, { bucket: this.bucket, key, operation: 'getMetadata' }); - } - throw err; - } + const output = await this.client.send(new HeadObjectCommand({ Bucket: this.bucket, Key: key })); + return { + size: output.ContentLength ?? 0, + lastModified: output.LastModified ?? new Date(0), + contentType: output.ContentType, + etag: output.ETag, + customMetadata: output.Metadata + }; } async exists(key: string): Promise { @@ -183,73 +151,44 @@ export class S3StorageProvider implements StorageProvider { { bucket: this.bucket, key, content_type: contentType, content_length: contentLength }, 'S3 Upload' ); - try { - const upload = new Upload({ - client: this.client, - queueSize: 4, - partSize: 5 * 1024 * 1024, - params: { - Bucket: this.bucket, - Key: key, - Body: body, - ContentType: contentType, - ...(contentLength !== undefined ? { ContentLength: contentLength } : {}) - } - }); - await upload.done(); - } catch (err) { - if (err instanceof S3ServiceException) { - mapS3ErrorToHttp(err, { bucket: this.bucket, key, operation: 'putObject' }); + const upload = new Upload({ + client: this.client, + queueSize: 4, + partSize: 5 * 1024 * 1024, + params: { + Bucket: this.bucket, + Key: key, + Body: body, + ContentType: contentType, + ...(contentLength !== undefined ? { ContentLength: contentLength } : {}) } - throw err; - } + }); + await upload.done(); } async deleteObjects(keys: string[]): Promise { - // Expand any directory prefixes (keys ending with '/') to their contents. - const dirPrefixes = keys.filter((k) => k.endsWith('/')); - const fileKeys = keys.filter((k) => !k.endsWith('/')); - - let allKeys = [...fileKeys]; - for (const prefix of dirPrefixes) { - log.debug({ bucket: this.bucket, prefix }, 'expanding directory prefix for deletion'); - const children = await this.listAllKeys(prefix); - allKeys = allKeys.concat(children.length > 0 ? children : [prefix]); - } - - if (allKeys.length === 0) { - return { failed: [] }; - } - - log.trace({ bucket: this.bucket, key_count: allKeys.length }, 'S3 DeleteObjects'); - try { - const output = await this.client.send( - new DeleteObjectsCommand({ - Bucket: this.bucket, - Delete: { - Objects: allKeys.map((key) => ({ Key: key })), - Quiet: true - } - }) + log.trace({ bucket: this.bucket, key_count: keys.length }, 'S3 DeleteObjects'); + const output = await this.client.send( + new DeleteObjectsCommand({ + Bucket: this.bucket, + Delete: { + Objects: keys.map((key) => ({ Key: key })), + Quiet: true + } + }) + ); + const failed = (output.Errors ?? []).map((e) => ({ + key: e.Key ?? '', + code: e.Code, + message: e.Message + })); + if (failed.length > 0) { + log.warn( + { bucket: this.bucket, failed_count: failed.length }, + 'some objects failed to delete' ); - const failed = (output.Errors ?? []).map((e) => ({ - key: e.Key ?? '', - code: e.Code, - message: e.Message - })); - if (failed.length > 0) { - log.warn( - { bucket: this.bucket, failed_count: failed.length }, - 'some objects failed to delete' - ); - } - return { failed }; - } catch (err) { - if (err instanceof S3ServiceException) { - mapS3ErrorToHttp(err, { bucket: this.bucket, operation: 'deleteObjects' }); - } - throw err; } + return { failed }; } async listAllKeys(prefix: string): Promise { diff --git a/src/routes/(app)/storage/+page.server.ts b/src/routes/(app)/storage/+page.server.ts index a9a205f2..55cab9d9 100644 --- a/src/routes/(app)/storage/+page.server.ts +++ b/src/routes/(app)/storage/+page.server.ts @@ -38,7 +38,7 @@ export const actions: Actions = { }; try { - await getConnectionProvider(config).listBuckets(); + await getConnectionProvider(config).listContainers(); log.info({ storage_type: type }, 'user storage connection verified'); } catch (err) { log.warn({ err }, 'storage connection test failed'); diff --git a/src/routes/(app)/storage/api/buckets/+server.ts b/src/routes/(app)/storage/api/buckets/+server.ts index 0408290d..dadfa8e1 100644 --- a/src/routes/(app)/storage/api/buckets/+server.ts +++ b/src/routes/(app)/storage/api/buckets/+server.ts @@ -10,7 +10,7 @@ import type { RequestHandler } from './$types'; * in hooks.server.ts before this handler runs. */ export const GET: RequestHandler = async ({ locals }) => { - const buckets = await getConnectionProvider(locals.storageConfig!).listBuckets(); + const buckets = await getConnectionProvider(locals.storageConfig!).listContainers(); locals.logger.debug({ bucket_count: buckets.length }, 'bucket list returned'); return Response.json(buckets); }; diff --git a/src/routes/(app)/storage/page.server.test.ts b/src/routes/(app)/storage/page.server.test.ts index 89efaa6b..d3e4926e 100644 --- a/src/routes/(app)/storage/page.server.test.ts +++ b/src/routes/(app)/storage/page.server.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; -const mockConnectionProvider = { listBuckets: vi.fn() }; +const mockConnectionProvider = { listContainers: vi.fn() }; vi.mock('$lib/server/storage/utils.js', () => ({ getConnectionProvider: () => mockConnectionProvider })); @@ -87,7 +87,7 @@ describe('storage page actions', () => { secretAccessKey: 'sk' } } as unknown as Awaited>); - mockConnectionProvider.listBuckets.mockRejectedValue(new Error('connection refused')); + mockConnectionProvider.listContainers.mockRejectedValue(new Error('connection refused')); const result = await actions.connect({ request: new Request('http://localhost', { method: 'POST' }), @@ -111,7 +111,7 @@ describe('storage page actions', () => { secretAccessKey: 'sk' } } as unknown as Awaited>); - mockConnectionProvider.listBuckets.mockResolvedValue(['b1']); + mockConnectionProvider.listContainers.mockResolvedValue(['b1']); await expect( actions.connect({ From 265654aeca85e1f8358cd3942735971e0c9f1e49 Mon Sep 17 00:00:00 2001 From: dklOrdix Date: Mon, 8 Jun 2026 10:11:27 +0200 Subject: [PATCH 8/8] refactor: centralize S3 error handling with withS3Errors function --- src/lib/server/storage/s3-provider.ts | 185 +++++++++++++++++--------- 1 file changed, 121 insertions(+), 64 deletions(-) diff --git a/src/lib/server/storage/s3-provider.ts b/src/lib/server/storage/s3-provider.ts index 7f2594fb..594030bf 100644 --- a/src/lib/server/storage/s3-provider.ts +++ b/src/lib/server/storage/s3-provider.ts @@ -18,6 +18,18 @@ import { mapS3ErrorToHttp } from './s3-errors.js'; const log = logger.child({ module: 's3-provider' }); +/** Runs `fn` and maps any S3ServiceException to an HTTP error via `mapS3ErrorToHttp`. */ +async function withS3Errors( + fn: () => Promise, + context: Parameters[1] +): Promise { + try { + return await fn(); + } catch (err) { + mapS3ErrorToHttp(err, context); + } +} + export class S3StorageProvider implements StorageProvider { private readonly client: S3Client; private readonly bucket: string; @@ -29,17 +41,15 @@ export class S3StorageProvider implements StorageProvider { async listContainers(): Promise { log.trace({ bucket: this.bucket }, 'S3 ListBuckets'); - try { - const output = await this.client.send(new ListBucketsCommand({})); - const buckets = (output.Buckets ?? []).map((b) => b.Name ?? '').filter(Boolean); - log.debug({ bucket_count: buckets.length }, 'listed buckets'); - return buckets; - } catch (err) { - if (err instanceof S3ServiceException) { - mapS3ErrorToHttp(err, { operation: 'listBuckets' }); - } - throw err; - } + return withS3Errors( + async () => { + const output = await this.client.send(new ListBucketsCommand({})); + const buckets = (output.Buckets ?? []).map((b) => b.Name ?? '').filter(Boolean); + log.debug({ bucket_count: buckets.length }, 'listed buckets'); + return buckets; + }, + { operation: 'listBuckets' } + ); } async listObjects( @@ -48,15 +58,18 @@ export class S3StorageProvider implements StorageProvider { continuationToken?: string | null ): Promise { log.trace({ bucket: this.bucket, prefix, page_size: pageSize }, 'S3 ListObjectsV2'); - - const output: ListObjectsV2CommandOutput = await this.client.send( - new ListObjectsV2Command({ - Bucket: this.bucket, - Prefix: prefix || undefined, - Delimiter: '/', - MaxKeys: pageSize, - ContinuationToken: continuationToken ?? undefined - }) + const output = await withS3Errors( + () => + this.client.send( + new ListObjectsV2Command({ + Bucket: this.bucket, + Prefix: prefix || undefined, + Delimiter: '/', + MaxKeys: pageSize, + ContinuationToken: continuationToken ?? undefined + }) + ), + { bucket: this.bucket, operation: 'listObjects' } ); const objects: StorageObject[] = [ @@ -91,39 +104,58 @@ export class S3StorageProvider implements StorageProvider { async getObject(key: string): Promise { log.trace({ bucket: this.bucket, key }, 'S3 GetObject'); - const output = await this.client.send(new GetObjectCommand({ Bucket: this.bucket, Key: key })); - if (!output.Body) { - throw new Error(`Object ${key} has no body`); - } - return { - stream: output.Body.transformToWebStream(), - contentType: output.ContentType, - contentLength: output.ContentLength, - etag: output.ETag - }; + return withS3Errors( + async () => { + const output = await this.client.send( + new GetObjectCommand({ Bucket: this.bucket, Key: key }) + ); + if (!output.Body) { + throw new Error(`Object ${key} has no body`); + } + return { + stream: output.Body.transformToWebStream(), + contentType: output.ContentType, + contentLength: output.ContentLength, + etag: output.ETag + }; + }, + { bucket: this.bucket, key, operation: 'getObject' } + ); } async getObjectRange(key: string, start: number, end: number): Promise { log.trace({ bucket: this.bucket, key, start, end }, 'S3 GetObject (range)'); - const output = await this.client.send( - new GetObjectCommand({ Bucket: this.bucket, Key: key, Range: `bytes=${start}-${end}` }) + return withS3Errors( + async () => { + const output = await this.client.send( + new GetObjectCommand({ Bucket: this.bucket, Key: key, Range: `bytes=${start}-${end}` }) + ); + if (!output.Body) { + throw new Error(`Object ${key} has no body`); + } + return output.Body.transformToWebStream(); + }, + { bucket: this.bucket, key, operation: 'getObjectRange' } ); - if (!output.Body) { - throw new Error(`Object ${key} has no body`); - } - return output.Body.transformToWebStream(); } async getMetadata(key: string): Promise { log.trace({ bucket: this.bucket, key }, 'S3 HeadObject'); - const output = await this.client.send(new HeadObjectCommand({ Bucket: this.bucket, Key: key })); - return { - size: output.ContentLength ?? 0, - lastModified: output.LastModified ?? new Date(0), - contentType: output.ContentType, - etag: output.ETag, - customMetadata: output.Metadata - }; + return withS3Errors( + async () => { + const output = await this.client.send( + new HeadObjectCommand({ Bucket: this.bucket, Key: key }) + ); + return { + size: output.ContentLength ?? 0, + lastModified: output.LastModified ?? new Date(0), + contentType: output.ContentType, + etag: output.ETag, + customMetadata: output.Metadata + }; + }, + { bucket: this.bucket, key, operation: 'getMetadata' } + ); } async exists(key: string): Promise { @@ -163,32 +195,57 @@ export class S3StorageProvider implements StorageProvider { ...(contentLength !== undefined ? { ContentLength: contentLength } : {}) } }); - await upload.done(); + await withS3Errors(() => upload.done(), { bucket: this.bucket, key, operation: 'putObject' }); } async deleteObjects(keys: string[]): Promise { log.trace({ bucket: this.bucket, key_count: keys.length }, 'S3 DeleteObjects'); - const output = await this.client.send( - new DeleteObjectsCommand({ - Bucket: this.bucket, - Delete: { - Objects: keys.map((key) => ({ Key: key })), - Quiet: true + + if (keys.length === 0) { + return { failed: [] }; + } + + // Expand directory prefixes to their contained keys + const resolvedKeys: string[] = []; + for (const key of keys) { + if (key.endsWith('/')) { + const children = await this.listAllKeys(key); + if (children.length > 0) { + resolvedKeys.push(...children); + } else { + resolvedKeys.push(key); } - }) - ); - const failed = (output.Errors ?? []).map((e) => ({ - key: e.Key ?? '', - code: e.Code, - message: e.Message - })); - if (failed.length > 0) { - log.warn( - { bucket: this.bucket, failed_count: failed.length }, - 'some objects failed to delete' - ); + } else { + resolvedKeys.push(key); + } } - return { failed }; + + return withS3Errors( + async () => { + const output = await this.client.send( + new DeleteObjectsCommand({ + Bucket: this.bucket, + Delete: { + Objects: resolvedKeys.map((key) => ({ Key: key })), + Quiet: true + } + }) + ); + const failed = (output.Errors ?? []).map((e) => ({ + key: e.Key ?? '', + code: e.Code, + message: e.Message + })); + if (failed.length > 0) { + log.warn( + { bucket: this.bucket, failed_count: failed.length }, + 'some objects failed to delete' + ); + } + return { failed }; + }, + { bucket: this.bucket, operation: 'deleteObjects' } + ); } async listAllKeys(prefix: string): Promise {