diff --git a/src/bun.js/webcore/S3Client.zig b/src/bun.js/webcore/S3Client.zig index 941b8afec671..cb75e5433a62 100644 --- a/src/bun.js/webcore/S3Client.zig +++ b/src/bun.js/webcore/S3Client.zig @@ -151,12 +151,39 @@ pub const S3Client = struct { } return globalThis.throwInvalidArguments("Expected a path to presign", .{}); }; - errdefer path.deinit(); + defer 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); - defer blob.detach(); - return S3File.getPresignUrlFrom(&blob, globalThis, options); + + // Compute credentials and sign the request directly without + // constructing a temporary blob. Creating and then cleaning up + // a blob store while an error is being thrown from signRequest + // corrupts the exception scope chain and crashes during GC. + // + // Match getPresignUrlFrom behavior: acl/storage_class are only + // used as defaults when extra options are provided (the old code + // only called s3.getCredentialsWithOptions when extra_options + // was non-null). Without options, they default to null. + const has_options = options != null and options.?.isObject(); + var aws_options = try S3Credentials.getCredentialsWithOptions( + ptr.credentials.*, + ptr.options, + options, + if (has_options) ptr.acl else null, + if (has_options) ptr.storage_class else null, + ptr.request_payer, + globalThis, + ); + defer aws_options.deinit(); + + // Normalize the path the same way Store.S3.path() does: + // URL.parse().s3Path(), strip trailing backslash, strip leading slash. + var s3_path = bun.URL.parse(path.slice()).s3Path(); + if (s3_path.len > 0 and s3_path[s3_path.len - 1] == '\\') + s3_path = s3_path[0 .. s3_path.len - 1]; + if (s3_path.len > 0 and (s3_path[0] == '/' or s3_path[0] == '\\')) + s3_path = s3_path[1..]; + return S3File.presignFromCredentials(globalThis, s3_path, options, &aws_options); } pub fn exists(ptr: *@This(), globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSError!JSValue { diff --git a/src/bun.js/webcore/S3File.zig b/src/bun.js/webcore/S3File.zig index 29d0524f7dba..7189484d5d90 100644 --- a/src/bun.js/webcore/S3File.zig +++ b/src/bun.js/webcore/S3File.zig @@ -67,7 +67,7 @@ pub fn presign(globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.J // accept a path or a blob var path_or_blob = try PathOrBlob.fromJSNoCopy(globalThis, &args); - errdefer { + defer { if (path_or_blob == .path) { path_or_blob.path.deinit(); } @@ -83,9 +83,23 @@ 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(); - var blob = try constructS3FileInternalStore(globalThis, path.path, options); - defer blob.deinit(); - return try getPresignUrlFrom(&blob, globalThis, options); + + // Compute credentials and sign directly without constructing + // a temporary blob. Creating and cleaning up a blob store + // while an error is being thrown from signRequest corrupts + // the exception scope chain and crashes during GC. + const existing_credentials = globalThis.bunVM().transpiler.env.getS3Credentials(); + var aws_options = try S3.S3Credentials.getCredentialsWithOptions(existing_credentials, .{}, options, null, null, false, globalThis); + defer aws_options.deinit(); + + // Normalize the path the same way Store.S3.path() does: + // URL.parse().s3Path(), strip trailing backslash, strip leading slash. + var s3_path = bun.URL.parse(path.path.slice()).s3Path(); + if (s3_path.len > 0 and s3_path[s3_path.len - 1] == '\\') + s3_path = s3_path[0 .. s3_path.len - 1]; + if (s3_path.len > 0 and (s3_path[0] == '/' or s3_path[0] == '\\')) + s3_path = s3_path[1..]; + return try presignFromCredentials(globalThis, s3_path, options, &aws_options); }, .blob => return try getPresignUrlFrom(&path_or_blob.blob, globalThis, args.nextEat()), } @@ -464,6 +478,44 @@ pub const S3BlobStatTask = struct { } }; +/// Compute a presigned URL from already-resolved credentials, without +/// constructing a blob. Used by S3Client.presign and S3File.presign to +/// avoid the interaction between deferred blob cleanup and error throwing +/// from signRequest that corrupts the exception scope chain. +pub fn presignFromCredentials(globalThis: *jsc.JSGlobalObject, request_path: []const u8, extra_options: ?JSValue, credentialsWithOptions: *S3.S3CredentialsWithOptions) bun.JSError!JSValue { + var method: bun.http.Method = .GET; + var expires: usize = 86400; + + if (extra_options) |options| { + if (options.isObject()) { + if (try options.getTruthyComptime(globalThis, "method")) |method_| { + method = try Method.fromJS(globalThis, method_) orelse { + return globalThis.throwInvalidArguments("method must be GET, PUT, DELETE or HEAD when using s3 protocol", .{}); + }; + } + if (try options.getOptional(globalThis, "expiresIn", i32)) |expires_| { + if (expires_ <= 0) return globalThis.throwInvalidArguments("expiresIn must be greater than 0", .{}); + expires = @intCast(expires_); + } + } + } + + const result = credentialsWithOptions.credentials.signRequest(.{ + .path = request_path, + .method = method, + .acl = credentialsWithOptions.acl, + .storage_class = credentialsWithOptions.storage_class, + .request_payer = credentialsWithOptions.request_payer, + .content_disposition = credentialsWithOptions.content_disposition, + .content_type = credentialsWithOptions.content_type, + .content_encoding = credentialsWithOptions.content_encoding, + }, false, .{ .expires = expires }) catch |sign_err| { + return S3.throwSignError(sign_err, globalThis); + }; + defer result.deinit(); + return bun.String.createUTF8ForJS(globalThis, result.url); +} + pub fn getPresignUrlFrom(this: *Blob, globalThis: *jsc.JSGlobalObject, extra_options: ?JSValue) bun.JSError!JSValue { if (!this.isS3()) { return globalThis.ERR(.INVALID_THIS, "presign is only possible for s3:// files", .{}).throw(); @@ -489,7 +541,7 @@ pub fn getPresignUrlFrom(this: *Blob, globalThis: *jsc.JSGlobalObject, extra_opt }; } if (try options.getOptional(globalThis, "expiresIn", i32)) |expires_| { - if (expires_ <= 0) return globalThis.throwInvalidArguments("expiresIn must be greather than 0", .{}); + if (expires_ <= 0) return globalThis.throwInvalidArguments("expiresIn must be greater than 0", .{}); expires = @intCast(expires_); } } @@ -505,6 +557,7 @@ pub fn getPresignUrlFrom(this: *Blob, globalThis: *jsc.JSGlobalObject, extra_opt .request_payer = credentialsWithOptions.request_payer, .content_disposition = credentialsWithOptions.content_disposition, .content_type = credentialsWithOptions.content_type, + .content_encoding = credentialsWithOptions.content_encoding, }, false, .{ .expires = expires }) catch |sign_err| { return S3.throwSignError(sign_err, globalThis); }; diff --git a/src/s3/credentials.zig b/src/s3/credentials.zig index d6a8632611b7..7c25d8b34579 100644 --- a/src/s3/credentials.zig +++ b/src/s3/credentials.zig @@ -156,7 +156,7 @@ pub const S3Credentials = struct { new_credentials.changed_credentials = true; } } else { - return globalObject.throwInvalidArgumentTypeValue("bucket", "string", js_value); + return globalObject.throwInvalidArgumentTypeValue("sessionToken", "string", js_value); } } } 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 000000000000..85de54ff498e --- /dev/null +++ b/test/js/bun/s3/s3-presign-missing-credentials.test.ts @@ -0,0 +1,48 @@ +import { expect, test } from "bun:test"; +import { bunEnv, bunExe } from "harness"; + +// Regression test: S3 presign with missing credentials should throw +// ERR_S3_MISSING_CREDENTIALS instead of crashing. + +// Spawn subprocesses with S3 credential env vars explicitly unset so +// the tests are not affected by ambient AWS credentials in the host. +const cleanEnv = { + ...bunEnv, + AWS_ACCESS_KEY_ID: undefined, + AWS_SECRET_ACCESS_KEY: undefined, + S3_ACCESS_KEY_ID: undefined, + S3_SECRET_ACCESS_KEY: undefined, + AWS_SESSION_TOKEN: undefined, + S3_SESSION_TOKEN: undefined, + S3_ENDPOINT: undefined, + S3_BUCKET: undefined, + S3_REGION: undefined, + AWS_ENDPOINT: undefined, + AWS_BUCKET: undefined, + AWS_REGION: undefined, +}; + +test("S3 presign with missing credentials throws instead of crashing", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + try { Bun.s3.presign("test-path"); } catch(e) { console.log(e.code); } + try { new Bun.S3Client().presign("test-path"); } catch(e) { console.log(e.code); } + try { Bun.S3Client.presign("test-path"); } catch(e) { console.log(e.code); } + Bun.gc(true); + console.log("ok"); + `, + ], + env: cleanEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + + expect(stderr).not.toContain("error:"); + expect(stdout.trim()).toBe("ERR_S3_MISSING_CREDENTIALS\nERR_S3_MISSING_CREDENTIALS\nERR_S3_MISSING_CREDENTIALS\nok"); + expect(exitCode).toBe(0); +});