Skip to content

Conversation

@cirospaciari
Copy link
Member

@cirospaciari cirospaciari commented Dec 5, 2025

What does this PR do?

  • Add contentDisposition option to S3 file uploads to control the Content-Disposition HTTP header
  • Support passing contentDisposition through all S3 upload paths (simple uploads, multipart uploads, and streaming uploads)
  • Add TypeScript types for the new option
    Fixes no way to set content disposition on s3file.write #25362

How did you verify your code works?

Test

@robobun
Copy link
Collaborator

robobun commented Dec 5, 2025

Updated 9:27 AM PT - Dec 8th, 2025

@cirospaciari, your commit 1c27fd3 has some failures in Build #33026 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 25363

That installs a local version of the PR into your bun-25363 executable, so you can run:

bun-25363 --bun

@cirospaciari cirospaciari changed the title feat(s3) contentDisposition support feat(s3): add Content-Disposition support for S3 uploads Dec 5, 2025
@cirospaciari cirospaciari marked this pull request as ready for review December 6, 2025 04:14
@cirospaciari cirospaciari requested a review from alii as a code owner December 6, 2025 04:14
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 6, 2025

Walkthrough

Adds Content-Disposition support across S3: a new optional contentDisposition option in type defs, header accessor, credential/signing and client APIs, multipart logic, JS call sites, and tests to ensure the header propagates into uploads and presigned responses.

Changes

Cohort / File(s) Summary
Type Definitions
packages/bun-types/s3.d.ts
Added optional `contentDisposition?: string
HTTP Headers
src/http/Headers.zig
Added pub fn getContentDisposition(this: *const Headers) ?[]const u8; refactored getContentType to delegate to get("content-type").
S3 Credentials & Signing
src/s3/credentials.zig
New content_disposition handling: public field on S3CredentialsWithOptions and SignResult, private _contentDispositionSlice, parsing/validation from options, deinit cleanup, and inclusion of content-disposition in header construction and canonical/signing paths.
S3 Client Core
src/s3/client.zig
Added content_disposition: ?[]const u8 parameter to upload() and uploadStream() signatures; propagate .content_disposition into request payloads and multipart initialization.
S3 Multipart Upload
src/s3/multipart.zig
Added content_disposition: ?[]const u8 field to MultiPartUpload; propagate through request initializations, enqueue/start/retry paths, and free in deinit.
JavaScript Integration
src/bun.js/webcore/Blob.zig, src/bun.js/webcore/fetch.zig
Pass aws_options.content_disposition into S3 upload / uploadStream calls; extract Content-Disposition from request headers (h.getContentDisposition()) for streaming uploads.
Tests
test/js/bun/s3/s3.test.ts
Added tests asserting Content-Disposition propagation for S3 write/presign and writer scenarios.

Pre-merge checks

✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately summarizes the main change: adding Content-Disposition support for S3 uploads, which is the primary objective.
Description check ✅ Passed The description follows the template and covers both required sections: what the PR does and how it was verified, though verification details are minimal.
Linked Issues check ✅ Passed The PR implements the requested feature from issue #25362 by adding contentDisposition option, supporting all S3 upload paths, and providing TypeScript types, fully addressing the coding requirements.
Out of Scope Changes check ✅ Passed All changes are directly related to implementing Content-Disposition support for S3 uploads as specified in the linked issue, with no unrelated modifications.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 8bc550f and 62b2f04.

📒 Files selected for processing (3)
  • src/bun.js/webcore/Blob.zig (9 hunks)
  • src/s3/client.zig (6 hunks)
  • test/js/bun/s3/s3.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

  • src/bun.js/webcore/Blob.zig
  • src/s3/client.zig
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

  • src/bun.js/webcore/Blob.zig
  • src/s3/client.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

  • src/bun.js/webcore/Blob.zig
  • src/s3/client.zig
test/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test, describe, expect) and import from bun:test
Use test.each and data-driven tests to reduce boilerplate when testing multiple similar cases

Files:

  • test/js/bun/s3/s3.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun:test with files that end in *.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always use port: 0 to get a random port
Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, use bunExe and bunEnv from harness to ensure the same build of Bun is used and debug logging is silenced
Use -e flag for single-file tests when spawning Bun processes
Use tempDir() from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, use Promise.withResolvers to create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Use describe blocks for grouping related tests
Always use await using or using to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Use describe.each() for parameterized tests
Use toMatchSnapshot() for snapshot testing
Use beforeAll(), afterEach(), beforeEach() for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup in afterEach()

Files:

  • test/js/bun/s3/s3.test.ts
test/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.test.{ts,tsx}: For single-file tests in Bun test suite, prefer using -e flag over tempDir
For multi-file tests in Bun test suite, prefer using tempDir and Bun.spawn
Always use port: 0 when spawning servers in tests - do not hardcode ports or use custom random port functions
Use normalizeBunSnapshot to normalize snapshot output in tests instead of manual output comparison
Never write tests that check for no 'panic', 'uncaught exception', or similar strings in test output - that is not a valid test
Use tempDir from harness to create temporary directories in tests - do not use tmpdirSync or fs.mkdtempSync
In tests, call expect(stdout).toBe(...) before expect(exitCode).toBe(0) when spawning processes for more useful error messages on failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met since you're testing the CONDITION, not TIME PASSING
Verify your test fails with USE_SYSTEM_BUN=1 bun test <file> and passes with bun bd test <file> - tests are not valid if they pass with USE_SYSTEM_BUN=1
Avoid shell commands in tests - do not use find or grep; use Bun's Glob and built-in tools instead
Test files must end in .test.ts or .test.tsx and be created in the appropriate test folder structure

Files:

  • test/js/bun/s3/s3.test.ts
🧠 Learnings (2)
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc

Applied to files:

  • test/js/bun/s3/s3.test.ts
📚 Learning: 2025-11-24T18:35:39.205Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Applies to **/js_*.zig : Use `bun.JSError!JSValue` for proper error propagation in JavaScript bindings

Applied to files:

  • src/s3/client.zig
🧬 Code graph analysis (1)
test/js/bun/s3/s3.test.ts (1)
test/js/bun/s3/s3-stream-leak-fixture.js (1)
  • s3file (9-9)
🔇 Additional comments (3)
src/bun.js/webcore/Blob.zig (1)

961-983: S3 upload/stream calls correctly thread content_disposition through all blob→S3 paths

The new content_disposition argument is wired consistently for:

  • Empty-destination S3 writes (writeFileWithEmptySourceToDestination),
  • Byte/file/S3 source→S3 destination (writeFileWithSourceDestination),
  • Response/Request locked-body S3 streaming, and
  • pipeReadableStreamToBlob S3 branch,

always using destination_blob.contentTypeOrMimeType() for content_type and aws_options.content_disposition (or null) for the new slot. This matches the updated signatures in bun.S3.upload / bun.S3.uploadStream and mirrors how content_type was already handled, so behavior and lifetimes look correct.

Also applies to: 1104-1193, 1385-1399, 1447-1459, 2402-2415

src/s3/client.zig (1)

227-249: content_disposition is cleanly integrated into upload, writableStream, and uploadStream

  • upload now takes content_disposition: ?[]const u8 and forwards it via .content_disposition = content_disposition into the simple-request payload, mirroring the existing content_type plumbing.
  • writableStream and uploadStream both duplicate content_disposition into heap-owned storage on MultiPartUpload (bun.default_allocator.dupe), so the value remains valid for the lifetime of the multipart task and is independent of caller buffers.
  • The new parameter ordering (..., content_type, content_disposition, acl, proxy, storage_class, ...) matches all updated call sites you’ve added elsewhere.

Overall, the API extension and memory ownership for content_disposition look correct and consistent with the existing design.

Also applies to: 251-309, 431-487

test/js/bun/s3/s3.test.ts (1)

411-447: Header assertion strictness (optional)

The assertions currently use strict equality:

expect(response.headers.get("content-disposition")).toBe('attachment; filename="test.txt"');

If any provider normalizes this header (e.g., adding spaces or reordering parameters), these tests will fail even though behavior is correct. Using toContain or a regexp-based check would make the tests more tolerant while still validating that the value round-trips.

Likely an incorrect or invalid review comment.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/s3/client.zig (1)

251-328: writableStream() missing content_disposition parameter.

The writableStream() function creates a writable stream for S3 uploads but does not accept or propagate content_disposition. This creates an inconsistency since upload() and uploadStream() both support the new option. Users of the writable stream API won't be able to set the Content-Disposition header.

Consider adding content_disposition support:

 pub fn writableStream(
     this: *S3Credentials,
     path: []const u8,
     globalThis: *jsc.JSGlobalObject,
     options: MultiPartUploadOptions,
     content_type: ?[]const u8,
+    content_disposition: ?[]const u8,
     proxy: ?[]const u8,
     storage_class: ?StorageClass,
 ) bun.JSError!jsc.JSValue {

And propagate it to the MultiPartUpload task initialization around line 299.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 23383b3 and 2c1032b.

📒 Files selected for processing (8)
  • packages/bun-types/s3.d.ts (1 hunks)
  • src/bun.js/webcore/Blob.zig (7 hunks)
  • src/bun.js/webcore/fetch.zig (1 hunks)
  • src/http/Headers.zig (1 hunks)
  • src/s3/client.zig (4 hunks)
  • src/s3/credentials.zig (13 hunks)
  • src/s3/multipart.zig (5 hunks)
  • test/js/bun/s3/s3.test.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

  • src/s3/multipart.zig
  • src/s3/client.zig
  • src/bun.js/webcore/Blob.zig
  • src/http/Headers.zig
  • src/s3/credentials.zig
  • src/bun.js/webcore/fetch.zig
src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

  • src/s3/multipart.zig
  • src/s3/client.zig
  • src/bun.js/webcore/Blob.zig
  • src/http/Headers.zig
  • src/s3/credentials.zig
  • src/bun.js/webcore/fetch.zig
**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

  • src/s3/multipart.zig
  • src/s3/client.zig
  • src/bun.js/webcore/Blob.zig
  • src/http/Headers.zig
  • src/s3/credentials.zig
  • src/bun.js/webcore/fetch.zig
test/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test, describe, expect) and import from bun:test
Use test.each and data-driven tests to reduce boilerplate when testing multiple similar cases

Files:

  • test/js/bun/s3/s3.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun:test with files that end in *.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always use port: 0 to get a random port
Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, use bunExe and bunEnv from harness to ensure the same build of Bun is used and debug logging is silenced
Use -e flag for single-file tests when spawning Bun processes
Use tempDir() from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, use Promise.withResolvers to create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Use describe blocks for grouping related tests
Always use await using or using to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Use describe.each() for parameterized tests
Use toMatchSnapshot() for snapshot testing
Use beforeAll(), afterEach(), beforeEach() for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup in afterEach()

Files:

  • test/js/bun/s3/s3.test.ts
test/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.test.{ts,tsx}: For single-file tests in Bun test suite, prefer using -e flag over tempDir
For multi-file tests in Bun test suite, prefer using tempDir and Bun.spawn
Always use port: 0 when spawning servers in tests - do not hardcode ports or use custom random port functions
Use normalizeBunSnapshot to normalize snapshot output in tests instead of manual output comparison
Never write tests that check for no 'panic', 'uncaught exception', or similar strings in test output - that is not a valid test
Use tempDir from harness to create temporary directories in tests - do not use tmpdirSync or fs.mkdtempSync
In tests, call expect(stdout).toBe(...) before expect(exitCode).toBe(0) when spawning processes for more useful error messages on failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met since you're testing the CONDITION, not TIME PASSING
Verify your test fails with USE_SYSTEM_BUN=1 bun test <file> and passes with bun bd test <file> - tests are not valid if they pass with USE_SYSTEM_BUN=1
Avoid shell commands in tests - do not use find or grep; use Bun's Glob and built-in tools instead
Test files must end in .test.ts or .test.tsx and be created in the appropriate test folder structure

Files:

  • test/js/bun/s3/s3.test.ts
🧠 Learnings (5)
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

  • test/js/bun/s3/s3.test.ts
📚 Learning: 2025-11-24T18:37:30.259Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc

Applied to files:

  • test/js/bun/s3/s3.test.ts
📚 Learning: 2025-11-24T18:36:08.558Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/zig-javascriptcore-classes.mdc:0-0
Timestamp: 2025-11-24T18:36:08.558Z
Learning: Applies to **/*.zig : Implement resource cleanup using `deinit()` method that releases resources, followed by `finalize()` called by the GC that invokes `deinit()` and frees the pointer

Applied to files:

  • src/s3/credentials.zig
📚 Learning: 2025-11-24T18:35:39.205Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Applies to **/js_*.zig : Always implement proper cleanup in `deinit()` and `finalize()` methods for JavaScript binding classes

Applied to files:

  • src/s3/credentials.zig
📚 Learning: 2025-10-17T20:50:58.644Z
Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.

Applied to files:

  • src/bun.js/webcore/fetch.zig
🧬 Code graph analysis (1)
test/js/bun/s3/s3.test.ts (1)
test/js/bun/s3/s3-stream-leak-fixture.js (1)
  • s3file (9-9)
🔇 Additional comments (19)
packages/bun-types/s3.d.ts (1)

284-300: New contentDisposition option is well-integrated into S3Options

Name, type, and JSDoc examples are consistent with existing type/ACL options and align with the intended Content-Disposition semantics across S3File.write, S3Client.write, and presign options.

src/bun.js/webcore/fetch.zig (1)

1295-1308: Content-Disposition correctly forwarded for S3 streaming uploads

Passing if (headers) |h| h.getContentDisposition() else null into bun.S3.uploadStream in the S3+ReadableStream path mirrors how Content-Type is handled and keeps the new argument ordering consistent with other S3 uploadStream call sites.

src/http/Headers.zig (1)

79-84: Header helpers for Content-Disposition/Content-Type look correct

Both getContentDisposition and the refactored getContentType cleanly reuse the generic get with lowercase header names, relying on the existing case-insensitive lookup, and return stable slices tied to the Headers buffer.

src/bun.js/webcore/Blob.zig (1)

968-983: Content-Disposition is consistently propagated through all S3 write paths in Blob.zig

aws_options.content_disposition is now passed to S3.upload/S3.uploadStream in every S3 write scenario across all 7 call sites in this file: empty writes, byte-buffer uploads, file/s3-to-s3 copies, Response/Request streaming, and pipeReadableStreamToBlob. The parameter placement matches the updated function signatures—immediately after content-type and before proxy URL for S3.upload, and after storage_class but before proxy URL for S3.uploadStream. This ensures user-specified contentDisposition from options flows through to the underlying S3 operations consistently.

src/s3/multipart.zig (5)

118-118: LGTM!

The content_disposition field is correctly added with optional semantics matching content_type.


280-284: LGTM!

The cleanup correctly frees content_disposition when present, following the established pattern for content_type.


310-310: LGTM!

The content_disposition is correctly propagated in the retry path for single uploads.


599-599: LGTM!

Correctly propagates content_disposition when initiating multipart uploads.


676-676: LGTM!

Correctly propagates content_disposition for single-file uploads in the buffered path.

src/s3/client.zig (4)

232-232: LGTM!

The content_disposition parameter is correctly added to the upload function signature.


245-245: LGTM!

The content_disposition is correctly propagated to the S3 request payload.


439-439: LGTM!

The content_disposition parameter is correctly added to the uploadStream function signature.


476-476: LGTM!

The content_disposition is correctly allocated and assigned to the MultiPartUpload task, following the same pattern as content_type on line 475.

src/s3/credentials.zig (6)

215-229: LGTM!

The contentDisposition option is correctly parsed from JS options following the established pattern for other string options. The backing slice is properly stored for later cleanup.


385-387: LGTM!

The cleanup correctly frees content_disposition using bun.freeSensitive, consistent with other sensitive data handling in this function.


901-1032: LGTM!

The canonical request construction correctly includes the content-disposition header in alphabetical order, matching the signed_headers computation. The header is properly formatted as content-disposition:{value}\n.


1095-1105: LGTM!

The headers are correctly constructed:

  • x-amz-storage-class header is properly added when storage_class is set
  • content-disposition header uses lowercase naming (HTTP/2 standard)
  • Memory is allocated safely with bun.handleOom

1123-1143: LGTM!

The S3CredentialsWithOptions struct is correctly extended with content_disposition and its backing slice _contentDispositionSlice. The deinit method properly cleans up the slice, following the established pattern for other credential options.


632-757: Alphabetical header ordering for AWS SigV4 is correct.

Verified all 32 unique signed_headers strings extracted from lines 632-757. Each maintains proper alphabetical ordering across all conditional branches: content-dispositioncontent-md5hostx-amz-aclx-amz-content-sha256x-amz-datex-amz-security-tokenx-amz-storage-class. The nested conditionals correctly include/exclude headers based on parameter presence while preserving AWS Signature Version 4 compliance.

Copy link
Contributor

@paperclover paperclover left a comment

Choose a reason for hiding this comment

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

thank you ciro. don't forget to tag the other issues this closes.

Copy link
Member

@alii alii left a comment

Choose a reason for hiding this comment

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

types

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

  • Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 2c1032b and 8bc550f.

📒 Files selected for processing (1)
  • packages/bun-types/s3.d.ts (1 hunks)

@cirospaciari cirospaciari requested a review from alii December 8, 2025 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

no way to set content disposition on s3file.write

5 participants