diff --git a/src/bun.js/webcore/S3Client.zig b/src/bun.js/webcore/S3Client.zig index 941b8afec671..79ab49971e53 100644 --- a/src/bun.js/webcore/S3Client.zig +++ b/src/bun.js/webcore/S3Client.zig @@ -135,7 +135,7 @@ pub const S3Client = struct { } return globalThis.throwInvalidArguments("Expected a path", .{}); }; - errdefer path.deinit(); + // constructS3FileWithS3CredentialsAndOptions takes ownership of path. const options = args.nextEat(); var blob = Blob.new(try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer)); return blob.toJS(globalThis); @@ -151,8 +151,7 @@ pub const S3Client = struct { } return globalThis.throwInvalidArguments("Expected a path to presign", .{}); }; - errdefer path.deinit(); - + // constructS3FileWithS3CredentialsAndOptions takes ownership of path. const options = args.nextEat(); var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); defer blob.detach(); @@ -169,7 +168,7 @@ pub const S3Client = struct { } return globalThis.throwInvalidArguments("Expected a path to check if it exists", .{}); }; - errdefer path.deinit(); + // constructS3FileWithS3CredentialsAndOptions takes ownership of path. const options = args.nextEat(); var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); defer blob.detach(); @@ -186,7 +185,7 @@ pub const S3Client = struct { } return globalThis.throwInvalidArguments("Expected a path to check the size of", .{}); }; - errdefer path.deinit(); + // constructS3FileWithS3CredentialsAndOptions takes ownership of path. const options = args.nextEat(); var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); defer blob.detach(); @@ -203,7 +202,7 @@ pub const S3Client = struct { } return globalThis.throwInvalidArguments("Expected a path to check the stat of", .{}); }; - errdefer path.deinit(); + // constructS3FileWithS3CredentialsAndOptions takes ownership of path. const options = args.nextEat(); var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); defer blob.detach(); @@ -214,16 +213,22 @@ pub const S3Client = struct { const arguments = callframe.arguments_old(3).slice(); var args = jsc.CallFrame.ArgumentsSlice.init(globalThis.bunVM(), arguments); defer args.deinit(); - const path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse { + var path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse { return globalThis.ERR(.MISSING_ARGS, "Expected a path to write to", .{}).throw(); }; + // Guard against early return before constructS3FileWithS3CredentialsAndOptions + // takes ownership of path. errdefer path.deinit(); const data = args.nextEat() orelse { return globalThis.ERR(.MISSING_ARGS, "Expected a Blob-y thing to write", .{}).throw(); }; const options = args.nextEat(); - var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); + // constructS3FileWithS3CredentialsAndOptions takes ownership of path + // (has its own errdefer) — neutralize ours before the call. + const owned_path = path; + path = .{ .string = bun.PathString.empty }; + var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, owned_path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); defer blob.detach(); var blob_internal: PathOrBlob = .{ .blob = blob }; return Blob.writeFileInternal(globalThis, &blob_internal, data, .{ @@ -251,7 +256,7 @@ pub const S3Client = struct { const path: jsc.Node.PathLike = try jsc.Node.PathLike.fromJS(globalThis, &args) orelse { return globalThis.ERR(.MISSING_ARGS, "Expected a path to unlink", .{}).throw(); }; - errdefer path.deinit(); + // constructS3FileWithS3CredentialsAndOptions takes ownership of path. const options = args.nextEat(); var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); defer blob.detach(); diff --git a/src/bun.js/webcore/S3File.zig b/src/bun.js/webcore/S3File.zig index 29d0524f7dba..4459d6c143f5 100644 --- a/src/bun.js/webcore/S3File.zig +++ b/src/bun.js/webcore/S3File.zig @@ -83,6 +83,8 @@ pub fn presign(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.J return globalThis.throwInvalidArguments("Expected a S3 or path to presign", .{}); } const options = args.nextEat(); + // constructS3FileInternalStore takes ownership of path. + path_or_blob = .{ .blob = .initEmpty(globalThis) }; var blob = try constructS3FileInternalStore(globalThis, path.path, options); defer blob.deinit(); return try getPresignUrlFrom(&blob, globalThis, options); @@ -113,6 +115,8 @@ pub fn unlink(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS return globalThis.throwInvalidArguments("Expected a S3 or path to delete", .{}); } const options = args.nextEat(); + // constructS3FileInternalStore takes ownership of path. + path_or_blob = .{ .blob = .initEmpty(globalThis) }; var blob = try constructS3FileInternalStore(globalThis, path.path, options); defer blob.deinit(); return try blob.store.?.data.s3.unlink(blob.store.?, globalThis, options); @@ -150,6 +154,8 @@ pub fn write(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSE if (path == .fd) { return globalThis.throwInvalidArguments("Expected a S3 or path to upload", .{}); } + // constructS3FileInternalStore takes ownership of path. + path_or_blob = .{ .blob = .initEmpty(globalThis) }; var blob = try constructS3FileInternalStore(globalThis, path.path, options); defer blob.deinit(); @@ -189,6 +195,8 @@ pub fn size(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr if (path == .fd) { return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{}); } + // constructS3FileInternalStore takes ownership of path. + path_or_blob = .{ .blob = .initEmpty(globalThis) }; var blob = try constructS3FileInternalStore(globalThis, path.path, options); defer blob.deinit(); @@ -222,6 +230,8 @@ pub fn exists(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS if (path == .fd) { return globalThis.throwInvalidArguments("Expected a S3 or path to check if it exists", .{}); } + // constructS3FileInternalStore takes ownership of path. + path_or_blob = .{ .blob = .initEmpty(globalThis) }; var blob = try constructS3FileInternalStore(globalThis, path.path, options); defer blob.deinit(); @@ -253,6 +263,12 @@ pub fn constructS3FileWithS3CredentialsAndOptions( default_storage_class: ?bun.S3.StorageClass, default_request_payer: bool, ) bun.JSError!Blob { + // This function takes ownership of `path`. If we fail before passing + // it to initS3/initS3WithReferencedCredentials (which consume it via + // toThreadSafe), we must clean it up ourselves. + var path_to_clean = path; + errdefer path_to_clean.deinit(); + var aws_options = try S3.S3Credentials.getCredentialsWithOptions(default_credentials.*, default_options, options, default_acl, default_storage_class, default_request_payer, globalObject); defer aws_options.deinit(); @@ -263,6 +279,9 @@ pub fn constructS3FileWithS3CredentialsAndOptions( break :brk bun.handleOom(Blob.Store.initS3WithReferencedCredentials(path, null, default_credentials, bun.default_allocator)); } }; + // Path has been consumed by initS3/initS3WithReferencedCredentials + // via toThreadSafe — neutralize errdefer to prevent double-free. + path_to_clean = .{ .string = bun.PathString.empty }; errdefer store.deinit(); store.data.s3.options = aws_options.options; store.data.s3.acl = aws_options.acl; @@ -304,9 +323,15 @@ pub fn constructS3FileWithS3Credentials( options: ?jsc.JSValue, existing_credentials: S3.S3Credentials, ) bun.JSError!Blob { + // This function takes ownership of `path`. + var path_to_clean = path; + errdefer path_to_clean.deinit(); + var aws_options = try S3.S3Credentials.getCredentialsWithOptions(existing_credentials, .{}, options, null, null, false, globalObject); defer aws_options.deinit(); const store = bun.handleOom(Blob.Store.initS3(path, null, aws_options.credentials, bun.default_allocator)); + // Path consumed by initS3 via toThreadSafe. + path_to_clean = .{ .string = bun.PathString.empty }; errdefer store.deinit(); store.data.s3.options = aws_options.options; store.data.s3.acl = aws_options.acl; @@ -573,6 +598,8 @@ pub fn stat(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr if (path == .fd) { return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{}); } + // constructS3FileInternalStore takes ownership of path. + path_or_blob = .{ .blob = .initEmpty(globalThis) }; var blob = try constructS3FileInternalStore(globalThis, path.path, options); defer blob.deinit(); diff --git a/src/bun.js/webcore/blob/Store.zig b/src/bun.js/webcore/blob/Store.zig index ca01a387410b..d2ac6874e990 100644 --- a/src/bun.js/webcore/blob/Store.zig +++ b/src/bun.js/webcore/blob/Store.zig @@ -67,7 +67,8 @@ pub fn external(ptr: ?*anyopaque, _: ?*anyopaque, _: usize) callconv(.c) void { } pub fn initS3WithReferencedCredentials(pathlike: node.PathLike, mime_type: ?MimeType, credentials: *bun.S3.S3Credentials, allocator: std.mem.Allocator) !*Store { var path = pathlike; - // this actually protects/refs the pathlike + // toThreadSafe takes ownership of the underlying string — callers + // must not deinit their copy after this call. path.toThreadSafe(); const store = Blob.Store.new(.{ @@ -96,7 +97,8 @@ pub fn initS3WithReferencedCredentials(pathlike: node.PathLike, mime_type: ?Mime pub fn initS3(pathlike: node.PathLike, mime_type: ?MimeType, credentials: bun.S3.S3Credentials, allocator: std.mem.Allocator) !*Store { var path = pathlike; - // this actually protects/refs the pathlike + // toThreadSafe takes ownership of the underlying string — callers + // must not deinit their copy after this call. path.toThreadSafe(); const store = Blob.Store.new(.{ diff --git a/test/js/bun/s3/s3-presign-error.test.ts b/test/js/bun/s3/s3-presign-error.test.ts new file mode 100644 index 000000000000..3625901f9086 --- /dev/null +++ b/test/js/bun/s3/s3-presign-error.test.ts @@ -0,0 +1,36 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; + +test("s3 presign with missing credentials throws instead of crashing", async () => { + // Scrub AWS credential/config env vars so the test always hits the + // missing-credentials path regardless of ambient host configuration. + const env: Record = {}; + for (const [key, value] of Object.entries(bunEnv)) { + if (!key.startsWith("AWS_") && !key.startsWith("S3_") && !key.startsWith("BUN_S3_")) { + env[key] = value as string; + } + } + + // Test instance method (constructS3FileWithS3CredentialsAndOptions): + // - initS3WithReferencedCredentials (no credential overrides) + // - initS3 (with per-request credentials that still lack endpoint/bucket) + // Test static method (constructS3FileWithS3Credentials): + // - Bun.S3Client.presign (static path) + const code = [ + `try { Bun.s3.presign("mykey"); } catch(e) { console.log(e.code); }`, + `try { Bun.s3.presign("mykey", { accessKeyId: "x", secretAccessKey: "y" }); } catch(e) { console.log(e.code); }`, + `try { Bun.S3Client.presign("mykey"); } catch(e) { console.log(e.code); }`, + ].join("\n"); + + await using proc = Bun.spawn({ + cmd: [bunExe(), "-e", code], + env, + stdout: "pipe", + stderr: "inherit", + }); + + const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); + + expect(stdout.trim()).toBe("ERR_S3_MISSING_CREDENTIALS\nERR_S3_INVALID_PATH\nERR_S3_MISSING_CREDENTIALS"); + expect(exitCode).toBe(0); +});