diff --git a/src/runtime/webcore/S3Client.zig b/src/runtime/webcore/S3Client.zig index 941b8afec67..966db81d016 100644 --- a/src/runtime/webcore/S3Client.zig +++ b/src/runtime/webcore/S3Client.zig @@ -145,7 +145,7 @@ pub const S3Client = struct { const arguments = callframe.arguments_old(2).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 { if (args.len() == 0) { return globalThis.ERR(.MISSING_ARGS, "Expected a path to presign", .{}).throw(); } @@ -155,6 +155,7 @@ pub const S3Client = struct { const options = args.nextEat(); var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); + path = .{ .string = bun.PathString.empty }; defer blob.detach(); return S3File.getPresignUrlFrom(&blob, globalThis, options); } @@ -163,7 +164,7 @@ pub const S3Client = struct { const arguments = callframe.arguments_old(2).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 { if (args.len() == 0) { return globalThis.ERR(.MISSING_ARGS, "Expected a path to check if it exists", .{}).throw(); } @@ -172,6 +173,7 @@ pub const S3Client = struct { errdefer path.deinit(); const options = args.nextEat(); var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); + path = .{ .string = bun.PathString.empty }; defer blob.detach(); return S3File.S3BlobStatTask.exists(globalThis, &blob); } @@ -180,7 +182,7 @@ pub const S3Client = struct { const arguments = callframe.arguments_old(2).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 { if (args.len() == 0) { return globalThis.ERR(.MISSING_ARGS, "Expected a path to check the size of", .{}).throw(); } @@ -189,6 +191,7 @@ pub const S3Client = struct { errdefer path.deinit(); const options = args.nextEat(); var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); + path = .{ .string = bun.PathString.empty }; defer blob.detach(); return S3File.S3BlobStatTask.size(globalThis, &blob); } @@ -197,7 +200,7 @@ pub const S3Client = struct { const arguments = callframe.arguments_old(2).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 { if (args.len() == 0) { return globalThis.ERR(.MISSING_ARGS, "Expected a path to check the stat of", .{}).throw(); } @@ -206,6 +209,7 @@ pub const S3Client = struct { errdefer path.deinit(); const options = args.nextEat(); var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); + path = .{ .string = bun.PathString.empty }; defer blob.detach(); return S3File.S3BlobStatTask.stat(globalThis, &blob); } @@ -214,7 +218,7 @@ 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(); }; errdefer path.deinit(); @@ -224,6 +228,7 @@ pub const S3Client = struct { const options = args.nextEat(); var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); + path = .{ .string = bun.PathString.empty }; defer blob.detach(); var blob_internal: PathOrBlob = .{ .blob = blob }; return Blob.writeFileInternal(globalThis, &blob_internal, data, .{ @@ -248,12 +253,13 @@ pub const S3Client = struct { const arguments = callframe.arguments_old(2).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 unlink", .{}).throw(); }; errdefer path.deinit(); const options = args.nextEat(); var blob = try S3File.constructS3FileWithS3CredentialsAndOptions(globalThis, path, options, ptr.credentials, ptr.options, ptr.acl, ptr.storage_class, ptr.request_payer); + path = .{ .string = bun.PathString.empty }; defer blob.detach(); return blob.store.?.data.s3.unlink(blob.store.?, globalThis, options); } diff --git a/src/runtime/webcore/S3File.zig b/src/runtime/webcore/S3File.zig index 41e900fb987..6feff4b0e7e 100644 --- a/src/runtime/webcore/S3File.zig +++ b/src/runtime/webcore/S3File.zig @@ -84,6 +84,7 @@ pub fn presign(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.J } const options = args.nextEat(); var blob = try constructS3FileInternalStore(globalThis, path.path, options); + path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } }; defer blob.deinit(); return try getPresignUrlFrom(&blob, globalThis, options); }, @@ -114,6 +115,7 @@ pub fn unlink(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS } const options = args.nextEat(); var blob = try constructS3FileInternalStore(globalThis, path.path, options); + path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } }; defer blob.deinit(); return try blob.store.?.data.s3.unlink(blob.store.?, globalThis, options); }, @@ -151,6 +153,7 @@ pub fn write(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSE return globalThis.throwInvalidArguments("Expected a S3 or path to upload", .{}); } var blob = try constructS3FileInternalStore(globalThis, path.path, options); + path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } }; defer blob.deinit(); var blob_internal: PathOrBlob = .{ .blob = blob }; @@ -190,6 +193,7 @@ pub fn size(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{}); } var blob = try constructS3FileInternalStore(globalThis, path.path, options); + path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } }; defer blob.deinit(); return S3BlobStatTask.size(globalThis, &blob); @@ -223,6 +227,7 @@ pub fn exists(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JS return globalThis.throwInvalidArguments("Expected a S3 or path to check if it exists", .{}); } var blob = try constructS3FileInternalStore(globalThis, path.path, options); + path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } }; defer blob.deinit(); return S3BlobStatTask.exists(globalThis, &blob); @@ -256,6 +261,9 @@ pub fn constructS3FileWithS3CredentialsAndOptions( 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(); + // initS3 transfers ownership of `path` to the store. Nothing below may + // return an error: the caller's `errdefer path.deinit()` still aliases + // the same string the store now owns. const store = brk: { if (aws_options.changed_credentials) { break :brk bun.handleOom(Blob.Store.initS3(path, null, aws_options.credentials, bun.default_allocator)); @@ -263,38 +271,13 @@ pub fn constructS3FileWithS3CredentialsAndOptions( break :brk bun.handleOom(Blob.Store.initS3WithReferencedCredentials(path, null, default_credentials, bun.default_allocator)); } }; - errdefer store.deinit(); store.data.s3.options = aws_options.options; store.data.s3.acl = aws_options.acl; store.data.s3.storage_class = aws_options.storage_class; store.data.s3.request_payer = aws_options.request_payer; var blob = Blob.initWithStore(store, globalObject); - if (options) |opts| { - if (opts.isObject()) { - if (try opts.getTruthyComptime(globalObject, "type")) |file_type| { - inner: { - if (file_type.isString()) { - var allocator = bun.default_allocator; - var str = try file_type.toSlice(globalObject, bun.default_allocator); - defer str.deinit(); - const slice = str.slice(); - if (!strings.isAllASCII(slice)) { - break :inner; - } - blob.content_type_was_set = true; - if (globalObject.bunVM().mimeType(str.slice())) |entry| { - blob.content_type = entry.value; - break :inner; - } - const content_type_buf = bun.handleOom(allocator.alloc(u8, slice.len)); - blob.content_type = strings.copyLowercase(slice, content_type_buf); - blob.content_type_allocated = true; - } - } - } - } - } + setBlobContentType(&blob, aws_options.content_type, globalObject); return blob; } @@ -306,41 +289,33 @@ pub fn constructS3FileWithS3Credentials( ) bun.JSError!Blob { var aws_options = try S3.S3Credentials.getCredentialsWithOptions(existing_credentials, .{}, options, null, null, false, globalObject); defer aws_options.deinit(); + // initS3 transfers ownership of `path` to the store. Nothing below may + // return an error: the caller's `errdefer path.deinit()` still aliases + // the same string the store now owns. const store = bun.handleOom(Blob.Store.initS3(path, null, aws_options.credentials, bun.default_allocator)); - errdefer store.deinit(); store.data.s3.options = aws_options.options; store.data.s3.acl = aws_options.acl; store.data.s3.storage_class = aws_options.storage_class; store.data.s3.request_payer = aws_options.request_payer; var blob = Blob.initWithStore(store, globalObject); - if (options) |opts| { - if (opts.isObject()) { - if (try opts.getTruthyComptime(globalObject, "type")) |file_type| { - inner: { - if (file_type.isString()) { - var allocator = bun.default_allocator; - var str = try file_type.toSlice(globalObject, bun.default_allocator); - defer str.deinit(); - const slice = str.slice(); - if (!strings.isAllASCII(slice)) { - break :inner; - } - blob.content_type_was_set = true; - if (globalObject.bunVM().mimeType(str.slice())) |entry| { - blob.content_type = entry.value; - break :inner; - } - const content_type_buf = bun.handleOom(allocator.alloc(u8, slice.len)); - blob.content_type = strings.copyLowercase(slice, content_type_buf); - blob.content_type_allocated = true; - } - } - } - } - } + setBlobContentType(&blob, aws_options.content_type, globalObject); return blob; } + +fn setBlobContentType(blob: *Blob, content_type: ?[]const u8, globalObject: *jsc.JSGlobalObject) void { + const slice = content_type orelse return; + if (!strings.isAllASCII(slice)) return; + blob.content_type_was_set = true; + if (globalObject.bunVM().mimeType(slice)) |entry| { + blob.content_type = entry.value; + return; + } + const content_type_buf = bun.handleOom(bun.default_allocator.alloc(u8, slice.len)); + blob.content_type = strings.copyLowercase(slice, content_type_buf); + blob.content_type_allocated = true; +} + fn constructS3FileInternal( globalObject: *jsc.JSGlobalObject, path: jsc.Node.PathLike, @@ -574,6 +549,7 @@ pub fn stat(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSEr return globalThis.throwInvalidArguments("Expected a S3 or path to get size", .{}); } var blob = try constructS3FileInternalStore(globalThis, path.path, options); + path_or_blob = .{ .path = .{ .path = .{ .string = bun.PathString.empty } } }; defer blob.deinit(); return S3BlobStatTask.stat(globalThis, &blob); diff --git a/test/js/bun/s3/s3-presign-missing-credentials.test.ts b/test/js/bun/s3/s3-presign-missing-credentials.test.ts new file mode 100644 index 00000000000..690e7739aa8 --- /dev/null +++ b/test/js/bun/s3/s3-presign-missing-credentials.test.ts @@ -0,0 +1,51 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; + +// The path string's ref is transferred to the blob store; when the operation +// fails synchronously after that point the errdefer cleanup must not deref it +// a second time. +test("S3Client path methods throw instead of crashing on synchronous errors", async () => { + const env = { ...bunEnv }; + for (const k of [ + "S3_ACCESS_KEY_ID", + "S3_SECRET_ACCESS_KEY", + "S3_BUCKET", + "S3_ENDPOINT", + "S3_REGION", + "S3_SESSION_TOKEN", + "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY", + "AWS_BUCKET", + "AWS_ENDPOINT", + "AWS_REGION", + "AWS_SESSION_TOKEN", + ]) { + delete env[k]; + } + + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + const out = []; + try { Bun.S3Client.presign("myfile"); } catch (e) { out.push(e.code); } + try { new Bun.S3Client({}).presign("myfile"); } catch (e) { out.push(e.code); } + try { Bun.S3Client.write("myfile", null); } catch (e) { out.push(e.code); } + try { new Bun.S3Client({}).write("myfile", null); } catch (e) { out.push(e.code); } + console.log(out.join(":")); + `, + ], + env, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).toBe(""); + expect(stdout.trim()).toBe( + "ERR_S3_MISSING_CREDENTIALS:ERR_S3_MISSING_CREDENTIALS:ERR_INVALID_ARG_TYPE:ERR_INVALID_ARG_TYPE", + ); + expect(exitCode).toBe(0); +});