-
Notifications
You must be signed in to change notification settings - Fork 3.8k
feat(s3): add Content-Disposition support for S3 uploads #25363
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
Updated 9:27 AM PT - Dec 8th, 2025
❌ @cirospaciari, your commit 1c27fd3 has some failures in 🧪 To try this PR locally: bunx bun-pr 25363That installs a local version of the PR into your bun-25363 --bun |
WalkthroughAdds Content-Disposition support across S3: a new optional Changes
Pre-merge checks✅ Passed checks (4 passed)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (6)src/**/*.{cpp,zig}📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Files:
src/**/*.zig📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)
Files:
**/*.zig📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
Files:
test/**/*.{js,ts,jsx,tsx}📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)
Files:
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
test/**/*.test.{ts,tsx}📄 CodeRabbit inference engine (CLAUDE.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-11-24T18:37:30.259ZApplied to files:
📚 Learning: 2025-11-24T18:35:39.205ZApplied to files:
🧬 Code graph analysis (1)test/js/bun/s3/s3.test.ts (1)
🔇 Additional comments (3)
Comment |
There was a problem hiding this 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()missingcontent_dispositionparameter.The
writableStream()function creates a writable stream for S3 uploads but does not accept or propagatecontent_disposition. This creates an inconsistency sinceupload()anduploadStream()both support the new option. Users of the writable stream API won't be able to set the Content-Disposition header.Consider adding
content_dispositionsupport: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.
📒 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}: Usebun bdorbun run build:debugto build debug versions for C++ and Zig source files; creates debug build at./build/debug/bun-debug
Run tests usingbun bd test <test-file>with the debug build; never usebun testdirectly as it will not include your changes
Execute files usingbun bd <file> <...args>; never usebun <file>directly as it will not include your changes
Enable debug logs for specific scopes usingBUN_DEBUG_$(SCOPE)=1environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required
Files:
src/s3/multipart.zigsrc/s3/client.zigsrc/bun.js/webcore/Blob.zigsrc/http/Headers.zigsrc/s3/credentials.zigsrc/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 codeImplement 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@importat 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.zigsrc/s3/client.zigsrc/bun.js/webcore/Blob.zigsrc/http/Headers.zigsrc/s3/credentials.zigsrc/bun.js/webcore/fetch.zig
**/*.zig
📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)
**/*.zig: Expose generated bindings in Zig structs usingpub const js = JSC.Codegen.JS<ClassName>with trait conversion methods:toJS,fromJS, andfromJSDirect
Use consistent parameter nameglobalObjectinstead ofctxin Zig constructor and method implementations
Usebun.JSError!JSValuereturn type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup usingdeinit()method that releases resources, followed byfinalize()called by the GC that invokesdeinit()and frees the pointer
UseJSC.markBinding(@src())in finalize methods for debugging purposes before callingdeinit()
For methods returning cached properties in Zig, declare external C++ functions usingextern fnandcallconv(JSC.conv)calling convention
Implement getter functions with naming patternget<PropertyName>in Zig that acceptthisandglobalObjectparameters and returnJSC.JSValue
Access JavaScript CallFrame arguments usingcallFrame.argument(i), check argument count withcallFrame.argumentCount(), and getthiswithcallFrame.thisValue()
For reference-counted objects, use.deref()in finalize instead ofdestroy()to release references to other JS objects
Files:
src/s3/multipart.zigsrc/s3/client.zigsrc/bun.js/webcore/Blob.zigsrc/http/Headers.zigsrc/s3/credentials.zigsrc/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 frombun:test
Usetest.eachand 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}: Usebun:testwith 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 useport: 0to get a random port
Prefer concurrent tests over sequential tests usingtest.concurrentordescribe.concurrentwhen multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, usebunExeandbunEnvfromharnessto ensure the same build of Bun is used and debug logging is silenced
Use-eflag for single-file tests when spawning Bun processes
UsetempDir()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, usePromise.withResolversto create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
UseBuffer.alloc(count, fill).toString()instead of'A'.repeat(count)to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Usedescribeblocks for grouping related tests
Always useawait usingorusingto 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
Usedescribe.each()for parameterized tests
UsetoMatchSnapshot()for snapshot testing
UsebeforeAll(),afterEach(),beforeEach()for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup inafterEach()
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-eflag overtempDir
For multi-file tests in Bun test suite, prefer usingtempDirandBun.spawn
Always useport: 0when spawning servers in tests - do not hardcode ports or use custom random port functions
UsenormalizeBunSnapshotto 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
UsetempDirfromharnessto create temporary directories in tests - do not usetmpdirSyncorfs.mkdtempSync
In tests, callexpect(stdout).toBe(...)beforeexpect(exitCode).toBe(0)when spawning processes for more useful error messages on failure
Do not write flaky tests - do not usesetTimeoutin tests; insteadawaitthe condition to be met since you're testing the CONDITION, not TIME PASSING
Verify your test fails withUSE_SYSTEM_BUN=1 bun test <file>and passes withbun bd test <file>- tests are not valid if they pass withUSE_SYSTEM_BUN=1
Avoid shell commands in tests - do not usefindorgrep; use Bun's Glob and built-in tools instead
Test files must end in.test.tsor.test.tsxand 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: NewcontentDispositionoption is well-integrated into S3OptionsName, type, and JSDoc examples are consistent with existing
type/ACL options and align with the intendedContent-Dispositionsemantics acrossS3File.write,S3Client.write, and presign options.src/bun.js/webcore/fetch.zig (1)
1295-1308: Content-Disposition correctly forwarded for S3 streaming uploadsPassing
if (headers) |h| h.getContentDisposition() else nullintobun.S3.uploadStreamin the S3+ReadableStream path mirrors howContent-Typeis 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 correctBoth
getContentDispositionand the refactoredgetContentTypecleanly reuse the genericgetwith 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_dispositionis now passed toS3.upload/S3.uploadStreamin 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, andpipeReadableStreamToBlob. The parameter placement matches the updated function signatures—immediately after content-type and before proxy URL forS3.upload, and after storage_class but before proxy URL forS3.uploadStream. This ensures user-specifiedcontentDispositionfrom options flows through to the underlying S3 operations consistently.src/s3/multipart.zig (5)
118-118: LGTM!The
content_dispositionfield is correctly added with optional semantics matchingcontent_type.
280-284: LGTM!The cleanup correctly frees
content_dispositionwhen present, following the established pattern forcontent_type.
310-310: LGTM!The
content_dispositionis correctly propagated in the retry path for single uploads.
599-599: LGTM!Correctly propagates
content_dispositionwhen initiating multipart uploads.
676-676: LGTM!Correctly propagates
content_dispositionfor single-file uploads in the buffered path.src/s3/client.zig (4)
232-232: LGTM!The
content_dispositionparameter is correctly added to theuploadfunction signature.
245-245: LGTM!The
content_dispositionis correctly propagated to the S3 request payload.
439-439: LGTM!The
content_dispositionparameter is correctly added to theuploadStreamfunction signature.
476-476: LGTM!The
content_dispositionis correctly allocated and assigned to the MultiPartUpload task, following the same pattern ascontent_typeon line 475.src/s3/credentials.zig (6)
215-229: LGTM!The
contentDispositionoption 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_dispositionusingbun.freeSensitive, consistent with other sensitive data handling in this function.
901-1032: LGTM!The canonical request construction correctly includes the
content-dispositionheader in alphabetical order, matching thesigned_headerscomputation. The header is properly formatted ascontent-disposition:{value}\n.
1095-1105: LGTM!The headers are correctly constructed:
x-amz-storage-classheader is properly added when storage_class is setcontent-dispositionheader uses lowercase naming (HTTP/2 standard)- Memory is allocated safely with
bun.handleOom
1123-1143: LGTM!The
S3CredentialsWithOptionsstruct is correctly extended withcontent_dispositionand its backing slice_contentDispositionSlice. Thedeinitmethod 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-disposition→content-md5→host→x-amz-acl→x-amz-content-sha256→x-amz-date→x-amz-security-token→x-amz-storage-class. The nested conditionals correctly include/exclude headers based on parameter presence while preserving AWS Signature Version 4 compliance.
paperclover
left a comment
There was a problem hiding this 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.
alii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
types
There was a problem hiding this 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.
📒 Files selected for processing (1)
packages/bun-types/s3.d.ts(1 hunks)
What does this PR do?
contentDispositionoption to S3 file uploads to control theContent-DispositionHTTP headercontentDispositionthrough all S3 upload paths (simple uploads, multipart uploads, and streaming uploads)Fixes no way to set content disposition on s3file.write #25362
How did you verify your code works?
Test