Skip to content

fix(storage): sign only ACL in presigned URLs, rename fields to requiredHeaders #188

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
abhay-ramesh merged 4 commits into from
Apr 7, 2026
Merged

fix(storage): sign only ACL in presigned URLs, rename fields to requiredHeaders #188

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
032ba6b
fix(storage): sign only ACL in presigned URLs, rename fields to requi…
abhay-ramesh Apr 4, 2026
df3cb32
fix(storage): exclude metadata from presigned URL requiredHeaders
abhay-ramesh Apr 4, 2026
50f1302
fix(types): allow arbitrary strings in S3AclValue for custom providers
abhay-ramesh Apr 4, 2026
c4d932b
Merge branch 'main' into fix/acl-presigned-headers
abhay-ramesh Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 65 additions & 38 deletions packages/pushduck/src/__tests__/presigned-url-signing.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -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" })
Expand All @@ -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" })
Expand All @@ -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,
Expand All @@ -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();
Expand Down
7 changes: 5 additions & 2 deletions packages/pushduck/src/core/config/upload-config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, any>;
};
Expand Down
21 changes: 19 additions & 2 deletions packages/pushduck/src/core/providers/providers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,20 @@
// 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";
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

Does Amazon S3 support the canned ACL value aws-exec-read, and what are the full set of canned ACL values?

💡 Result:

Yes, Amazon S3 supports the canned ACL value aws-exec-read for both buckets and objects. It grants FULL_CONTROL to the owner and READ access to Amazon EC2 for retrieving Amazon Machine Image (AMI) bundles. The full set of supported canned ACL values, applicable to buckets and/or objects as noted, are: - private: Owner gets FULL_CONTROL. No one else has access (default for both buckets and objects). - public-read: Owner gets FULL_CONTROL. AllUsers group gets READ (both). - public-read-write: Owner gets FULL_CONTROL. AllUsers group gets READ and WRITE (both; not recommended for buckets). - aws-exec-read: Owner gets FULL_CONTROL. Amazon EC2 gets READ (both). - authenticated-read: Owner gets FULL_CONTROL. AuthenticatedUsers group gets READ (both). - bucket-owner-read: Object owner gets FULL_CONTROL. Bucket owner gets READ (objects only; ignored for buckets). - bucket-owner-full-control: Object owner and bucket owner both get FULL_CONTROL (objects only; ignored for buckets). - log-delivery-write: LogDelivery group gets WRITE and READ_ACP (buckets only).

Citations:

Add the missing aws-exec-read canned ACL.

The S3AclValue union omits aws-exec-read, a valid Amazon S3 canned ACL that grants FULL_CONTROL to the owner and READ access to Amazon EC2 for AMI bundle retrieval.

➕ Complete the ACL union 
 export type S3AclValue =
   | "private"
   | "public-read"
   | "public-read-write"
+  | "aws-exec-read"
   | "authenticated-read"
   | "bucket-owner-read"
   | "bucket-owner-full-control"
   | "log-delivery-write";
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@packages/pushduck/src/core/providers/providers.ts` around lines 80 - 87, The
S3AclValue union in providers.ts is missing the valid canned ACL
"aws-exec-read"; update the S3AclValue type (the union declaration named
S3AclValue) to include the string literal "aws-exec-read" alongside the other
ACL values so the type covers this Amazon S3 canned ACL.


/**
* Base configuration interface for all cloud storage providers.
* Contains common properties shared across all provider implementations.
Expand All @@ -85,8 +99,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 */
Expand Down
5 changes: 3 additions & 2 deletions packages/pushduck/src/core/router/router-v2.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,7 @@ export class S3Router<TRoutes extends S3RouterDefinition> {
file,
presignedUrl: presignedResult.url,
key: presignedResult.key,
fields: presignedResult.fields,
requiredHeaders: presignedResult.requiredHeaders,
metadata: fileMetadata,
});
} catch (error) {
Expand Down Expand Up @@ -1047,7 +1047,8 @@ export interface PresignedUrlResponse {
file: S3FileMetadata;
presignedUrl?: string;
key?: string;
fields?: Record<string, string>;
/** Headers the client must send with the PUT request. See {@link PresignedUrlResult.requiredHeaders}. */
requiredHeaders?: Record<string, string>;
metadata?: any;
error?: string;
}
Expand Down
Loading
Loading