diff --git a/packages/pushduck/src/__tests__/presigned-url-signing.test.ts b/packages/pushduck/src/__tests__/presigned-url-signing.test.ts index 48a1b319..75d6a822 100644 --- a/packages/pushduck/src/__tests__/presigned-url-signing.test.ts +++ b/packages/pushduck/src/__tests__/presigned-url-signing.test.ts @@ -33,38 +33,33 @@ describe("Presigned URL Signing", () => { secretAccessKey: "test-secret", }; - it("should include Content-Type and x-amz-acl from defaults in the signed request", async () => { + it("should sign only x-amz-acl; Content-Type is unsigned; metadata is excluded", async () => { const { config } = createUploadConfig() .provider("aws", baseProvider) .defaults({ acl: "public-read" }) .build(); - const options = { + const result = await generatePresignedUploadUrl(config, { key: "test.txt", contentType: "text/plain", - metadata: { - userId: "123", - }, - }; - - const result = await generatePresignedUploadUrl(config, options); + metadata: { userId: "123" }, + }); - // Verify result fields - expect(result.fields).toEqual({ - "Content-Type": "text/plain", + // requiredHeaders has ACL and Content-Type only — metadata is NOT returned to client + expect(result.requiredHeaders).toEqual({ "x-amz-acl": "public-read", - "x-amz-meta-userId": "123", + "Content-Type": "text/plain", }); + expect(result.requiredHeaders?.["x-amz-meta-userId"]).toBeUndefined(); - // Verify sign() was called with correct headers - expect(signMock).toHaveBeenCalled(); + // sign() receives ONLY the ACL const request = signMock.mock.calls[0][0] as Request; - expect(request.headers.get("Content-Type")).toBe("text/plain"); expect(request.headers.get("x-amz-acl")).toBe("public-read"); - expect(request.headers.get("x-amz-meta-userId")).toBe("123"); + expect(request.headers.get("Content-Type")).toBeNull(); + expect(request.headers.get("x-amz-meta-userId")).toBeNull(); }); - it("should fallback to provider.acl if defaults.acl is missing", async () => { + it("should fall back to provider.acl when defaults.acl is not set", async () => { const { config } = createUploadConfig() .provider("aws", { ...baseProvider, acl: "authenticated-read" }) .build(); @@ -75,7 +70,7 @@ describe("Presigned URL Signing", () => { expect(request.headers.get("x-amz-acl")).toBe("authenticated-read"); }); - it("should prioritize defaults.acl over provider.acl", async () => { + it("should prefer defaults.acl over provider.acl", async () => { const { config } = createUploadConfig() .provider("aws", { ...baseProvider, acl: "private" }) .defaults({ acl: "public-read" }) @@ -87,46 +82,50 @@ describe("Presigned URL Signing", () => { expect(request.headers.get("x-amz-acl")).toBe("public-read"); }); - it("should handle multiple metadata fields", async () => { + it("should never include metadata in requiredHeaders or the signed request", async () => { const { config } = createUploadConfig() .provider("aws", baseProvider) + .defaults({ acl: "public-read" }) .build(); - const options = { + const result = await generatePresignedUploadUrl(config, { key: "test.txt", metadata: { "user-id": "123", "project-id": "456", "is-public": "true", }, - }; + }); - await generatePresignedUploadUrl(config, options); + // Metadata is excluded from requiredHeaders — server-side values never reach the browser + expect(result.requiredHeaders?.["x-amz-meta-user-id"]).toBeUndefined(); + expect(result.requiredHeaders?.["x-amz-meta-project-id"]).toBeUndefined(); + expect(result.requiredHeaders?.["x-amz-meta-is-public"]).toBeUndefined(); + // Metadata is also not in the signed request const request = signMock.mock.calls[0][0] as Request; - expect(request.headers.get("x-amz-acl")).toBe("private"); - expect(request.headers.get("x-amz-meta-user-id")).toBe("123"); - expect(request.headers.get("x-amz-meta-project-id")).toBe("456"); - expect(request.headers.get("x-amz-meta-is-public")).toBe("true"); + expect(request.headers.get("x-amz-meta-user-id")).toBeNull(); + expect(request.headers.get("x-amz-meta-project-id")).toBeNull(); }); - it("should include only provided options in fields or default ACL", async () => { + it("should sign only ACL with no Content-Type or metadata provided", async () => { + // AWS provider defaults to acl: "private" when none is explicitly set const { config } = createUploadConfig() .provider("aws", baseProvider) .build(); - const options = { - key: "test-minimal.txt", - }; + const result = await generatePresignedUploadUrl(config, { key: "test-minimal.txt" }); - await generatePresignedUploadUrl(config, options); + // Only the default ACL — no Content-Type or metadata + expect(result.requiredHeaders).toEqual({ "x-amz-acl": "private" }); + // sign() receives only the ACL const request = signMock.mock.calls[0][0] as Request; expect(request.headers.get("x-amz-acl")).toBe("private"); expect(request.headers.get("Content-Type")).toBeNull(); }); - it("router should include fields in generatePresignedUrls response", async () => { + it("router should include requiredHeaders in generatePresignedUrls response", async () => { const { s3, config } = createUploadConfig() .provider("aws", baseProvider) .defaults({ acl: "private" }) @@ -143,15 +142,22 @@ describe("Presigned URL Signing", () => { ); expect(results[0].success).toBe(true); - expect(results[0].fields).toBeDefined(); - - // Verify sign() was called via router + expect(results[0].requiredHeaders).toBeDefined(); + // ACL is signed and in requiredHeaders + expect(results[0].requiredHeaders?.["x-amz-acl"]).toBe("private"); + // Content-Type is unsigned but in requiredHeaders + expect(results[0].requiredHeaders?.["Content-Type"]).toBe("image/jpeg"); + // Metadata (originalName, routeName) is NOT in requiredHeaders + expect(results[0].requiredHeaders?.["x-amz-meta-originalName"]).toBeUndefined(); + expect(results[0].requiredHeaders?.["x-amz-meta-routeName"]).toBeUndefined(); + + // sign() only received the ACL — not Content-Type, not metadata const request = signMock.mock.calls[0][0] as Request; expect(request.headers.get("x-amz-acl")).toBe("private"); - expect(request.headers.get("Content-Type")).toBe("image/jpeg"); + expect(request.headers.get("Content-Type")).toBeNull(); }); - it("should skip x-amz-acl for Cloudflare R2", async () => { + it("should skip x-amz-acl entirely for Cloudflare R2", async () => { const { config } = createUploadConfig() .provider("cloudflareR2", { ...baseProvider, @@ -161,7 +167,28 @@ describe("Presigned URL Signing", () => { .defaults({ acl: "public-read" }) .build(); - await generatePresignedUploadUrl(config, { key: "test.txt" }); + const result = await generatePresignedUploadUrl(config, { key: "test.txt" }); + + // No ACL in requiredHeaders for R2 + expect(result.requiredHeaders?.["x-amz-acl"]).toBeUndefined(); + + // sign() received no ACL header + const request = signMock.mock.calls[0][0] as Request; + expect(request.headers.get("x-amz-acl")).toBeNull(); + }); + + it("should skip x-amz-acl entirely for MinIO", async () => { + const { config } = createUploadConfig() + .provider("minio", { + ...baseProvider, + endpoint: "http://localhost:9000", + }) + .defaults({ acl: "public-read" }) + .build(); + + const result = await generatePresignedUploadUrl(config, { key: "test.txt" }); + + expect(result.requiredHeaders?.["x-amz-acl"]).toBeUndefined(); const request = signMock.mock.calls[0][0] as Request; expect(request.headers.get("x-amz-acl")).toBeNull(); diff --git a/packages/pushduck/src/core/config/upload-config.ts b/packages/pushduck/src/core/config/upload-config.ts index 8d2bc8ef..ea0eb296 100644 --- a/packages/pushduck/src/core/config/upload-config.ts +++ b/packages/pushduck/src/core/config/upload-config.ts @@ -261,8 +261,11 @@ export interface UploadConfig { maxFileSize?: string | number; /** Allowed MIME types (e.g., ['image/*', 'application/pdf']) */ allowedFileTypes?: string[]; - /** S3 ACL setting ('public-read', 'private', etc.) */ - acl?: string; + /** + * S3 ACL for uploaded objects. + * Not supported on Cloudflare R2 or MinIO — ignored on those providers. + */ + acl?: import("../providers/providers").S3AclValue; /** Default metadata attached to all uploads */ metadata?: Record; }; diff --git a/packages/pushduck/src/core/providers/providers.ts b/packages/pushduck/src/core/providers/providers.ts index 9de8d5cb..46c7e3f0 100644 --- a/packages/pushduck/src/core/providers/providers.ts +++ b/packages/pushduck/src/core/providers/providers.ts @@ -72,6 +72,21 @@ // Provider Types // ======================================== +/** + * Valid S3 ACL values for controlling object access. + * Note: Cloudflare R2 and MinIO do not support per-object ACLs — use bucket-level + * access settings instead. Setting `acl` has no effect on those providers. + */ +export type S3AclValue = + | "private" + | "public-read" + | "public-read-write" + | "authenticated-read" + | "bucket-owner-read" + | "bucket-owner-full-control" + | "log-delivery-write" + | (string & {}); + /** * Base configuration interface for all cloud storage providers. * Contains common properties shared across all provider implementations. @@ -85,8 +100,11 @@ export interface BaseProviderConfig { region?: string; /** Name of the storage bucket/container */ bucket: string; - /** Access Control List permissions (e.g., 'public-read', 'private') */ - acl?: string; + /** + * S3 ACL for uploaded objects. + * Not supported on Cloudflare R2 or MinIO — ignored on those providers. + */ + acl?: S3AclValue; /** Custom domain for file URLs (e.g., 'cdn.example.com') */ customDomain?: string; /** Force path-style URLs instead of virtual-hosted style */ diff --git a/packages/pushduck/src/core/router/router-v2.ts b/packages/pushduck/src/core/router/router-v2.ts index 2599ea9a..f1b71405 100644 --- a/packages/pushduck/src/core/router/router-v2.ts +++ b/packages/pushduck/src/core/router/router-v2.ts @@ -939,7 +939,7 @@ export class S3Router { file, presignedUrl: presignedResult.url, key: presignedResult.key, - fields: presignedResult.fields, + requiredHeaders: presignedResult.requiredHeaders, metadata: fileMetadata, }); } catch (error) { @@ -1047,7 +1047,8 @@ export interface PresignedUrlResponse { file: S3FileMetadata; presignedUrl?: string; key?: string; - fields?: Record; + /** Headers the client must send with the PUT request. See {@link PresignedUrlResult.requiredHeaders}. */ + requiredHeaders?: Record; metadata?: any; error?: string; } diff --git a/packages/pushduck/src/core/storage/client.ts b/packages/pushduck/src/core/storage/client.ts index 5335b889..c4056fe4 100644 --- a/packages/pushduck/src/core/storage/client.ts +++ b/packages/pushduck/src/core/storage/client.ts @@ -578,19 +578,14 @@ export interface PresignedUrlOptions { * * @example * ```typescript - * const result: PresignedUrlResult = { - * url: 'https://bucket.s3.amazonaws.com/uploads/file.jpg?AWSAccessKeyId=...', - * key: 'uploads/file.jpg', - * fields: { - * 'Content-Type': 'image/jpeg', - * 'x-amz-meta-user-id': '123', - * }, - * }; + * const result = await generatePresignedUploadUrl(config, { + * key: 'uploads/photo.jpg', + * contentType: 'image/jpeg', + * }); * - * // Use the URL for direct upload * await fetch(result.url, { * method: 'PUT', - * headers: result.fields, + * headers: result.requiredHeaders, * body: file, * }); * ``` @@ -600,8 +595,23 @@ export interface PresignedUrlResult { url: string; /** The S3 object key where the file will be stored */ key: string; - /** Signed headers that must be sent with the PUT upload request (e.g. x-amz-acl, Content-Type, x-amz-meta-*) */ - fields?: Record; + /** + * Headers the client must include in the PUT request. + * + * Signed headers (e.g. `x-amz-acl`) are part of the AWS signature and must + * be sent with exact values. Unsigned headers (`Content-Type`, `x-amz-meta-*`) + * are passed through by S3 without signature verification. + * + * @example + * ```typescript + * await fetch(result.url, { + * method: 'PUT', + * headers: result.requiredHeaders, + * body: file, + * }); + * ``` + */ + requiredHeaders?: Record; } /** @@ -700,12 +710,12 @@ export async function generatePresignedDownloadUrl( * ``` * */ -/** Providers that do not support standard S3 ACLs (x-amz-acl) */ +/** Providers where x-amz-acl causes a 400/501 error — use bucket-level access settings instead */ const ACL_UNSUPPORTED_PROVIDERS = new Set(["cloudflare-r2", "minio"]); /** - * Internal helper to prepare S3 upload headers (ACL, Content-Type, Metadata). - * Ensures consistency between presigned URL generation and direct server-side uploads. + * Prepares headers for a direct server-side S3 upload (PUT via aws4fetch). + * All headers are set directly on the request — no signature split needed. * * @internal */ @@ -723,21 +733,63 @@ function prepareS3UploadHeaders( headers["Content-Type"] = options.contentType; } - const acl = uploadConfig.defaults?.acl || providerAcl; - // Cloudflare R2 and MinIO do not support x-amz-acl headers and will return 400/501 errors + const acl = uploadConfig.defaults?.acl ?? providerAcl; if (acl && !ACL_UNSUPPORTED_PROVIDERS.has(provider)) { headers["x-amz-acl"] = acl; } if (options.metadata) { - Object.entries(options.metadata).forEach(([key, value]) => { + for (const [key, value] of Object.entries(options.metadata)) { headers[`x-amz-meta-${key}`] = value; - }); + } } return headers; } +/** + * Prepares headers for a presigned PUT upload, split into two groups: + * + * - `signed`: included in the AWS Signature V4 (`X-Amz-SignedHeaders`). + * The client MUST send these with exact values or S3 rejects the request. + * Only `x-amz-acl` goes here — it cannot be applied unless it is signed. + * + * - `unsigned`: the client should send these, but AWS does not verify them + * against the signature. `Content-Type` and `x-amz-meta-*` go here. + * S3 stores them as-is without signature verification, which avoids + * requiring CORS changes for metadata headers on existing deployments. + * + * @internal + */ +function preparePresignHeaders( + uploadConfig: UploadConfig, + options: { + contentType?: string; + metadata?: Record; + } +): { signed: Record; unsigned: Record } { + const { provider, acl: providerAcl } = uploadConfig.provider; + const signed: Record = {}; + const unsigned: Record = {}; + + // ACL must be signed — AWS SigV4 will not apply it unless it is in SignedHeaders + const acl = uploadConfig.defaults?.acl ?? providerAcl; + if (acl && !ACL_UNSUPPORTED_PROVIDERS.has(provider)) { + signed["x-amz-acl"] = acl; + } + + // Content-Type goes unsigned — S3 stores it without signature verification. + // Metadata (x-amz-meta-*) is intentionally excluded here. Returning metadata + // to the browser client would expose server-side values (userId, routeName, etc.) + // set by middleware. Storing metadata on S3 objects requires a server-side + // completion step (see issue #189). This matches pre-#164 behavior. + if (options.contentType) { + unsigned["Content-Type"] = options.contentType; + } + + return { signed, unsigned }; +} + export async function generatePresignedUploadUrl( uploadConfig: UploadConfig, options: PresignedUrlOptions @@ -758,26 +810,30 @@ export async function generatePresignedUploadUrl( // Add expiration as query parameter url.searchParams.set("X-Amz-Expires", expiresIn.toString()); - // Prepare headers for signing. These MUST be sent by the client in the actual PUT request. - const headers = prepareS3UploadHeaders(uploadConfig, options); + // Split headers into signed (ACL only) and unsigned (Content-Type, metadata). + // Only signed headers appear in X-Amz-SignedHeaders — unsigned headers are sent + // by the client but not verified against the signature. + const { signed, unsigned } = preparePresignHeaders(uploadConfig, options); - // Create a signed request for PUT operation - // Passing headers to sign() ensures they are included in the signature (SigV4) const signedRequest = await awsClient.sign( new Request(url.toString(), { method: "PUT", - headers: headers, + headers: signed, }), { aws: { signQuery: true }, }, ); + // Merge signed + unsigned so the client has a single flat map to apply. + const requiredHeaders = { ...signed, ...unsigned }; + if (config.debug) { logger.presignedUrl(options.key, { originalUrl: s3Url, signedUrl: signedRequest.url, - headers, + signedHeaders: signed, + unsignedHeaders: unsigned, config: { region: config.region, endpoint: config.endpoint, @@ -790,7 +846,7 @@ export async function generatePresignedUploadUrl( return { url: signedRequest.url, key: options.key, - fields: Object.keys(headers).length > 0 ? headers : undefined, + requiredHeaders: Object.keys(requiredHeaders).length > 0 ? requiredHeaders : undefined, }; } catch (error) { logger.error("Failed to generate presigned URL", error, { diff --git a/packages/pushduck/src/hooks/use-upload-route.ts b/packages/pushduck/src/hooks/use-upload-route.ts index 411cc0f0..78725d73 100644 --- a/packages/pushduck/src/hooks/use-upload-route.ts +++ b/packages/pushduck/src/hooks/use-upload-route.ts @@ -197,7 +197,7 @@ async function uploadToS3( blob: Blob, contentType: string, presignedUrl: string, - fields?: Record, + requiredHeaders?: Record, onProgress?: (progress: number, uploadSpeed?: number, eta?: number) => void ): Promise { @@ -230,13 +230,12 @@ async function uploadToS3( xhr.open("PUT", presignedUrl); - // Set signed headers (includes x-amz-acl, Content-Type, metadata, etc.) - if (fields) { - Object.entries(fields).forEach(([key, value]) => { + if (requiredHeaders) { + for (const [key, value] of Object.entries(requiredHeaders)) { xhr.setRequestHeader(key, value); - }); + } } else { - // Fallback for backward compatibility + // Older server version — no requiredHeaders returned, set Content-Type directly xhr.setRequestHeader("Content-Type", contentType); } @@ -681,7 +680,7 @@ export function useUploadRoute>( blob, fileMeta.type, result.presignedUrl, - result.fields, + result.requiredHeaders, (progress, uploadSpeed, eta) => updateFileProgress(fileState.id, progress, uploadSpeed, eta) );