Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
36 commits
Select commit Hold shift + click to select a range
f27681e
Fix crash in S3 presign with missing credentials
robobun Mar 22, 2026
f635d50
[autofix.ci] apply automated fixes
autofix-ci[bot] Mar 22, 2026
be69ab4
retry CI
robobun Mar 22, 2026
9240986
Fix crash in S3 presign by computing URL without temporary blob
robobun Mar 22, 2026
ab261c9
[autofix.ci] apply automated fixes
autofix-ci[bot] Mar 22, 2026
2fdb044
fix typo: greather -> greater
robobun Mar 22, 2026
d486956
fix path leak and s3:// prefix normalization in presignFromCredentials
robobun Mar 22, 2026
1362929
retry CI
robobun Mar 22, 2026
10d7d7a
add content_encoding to presignFromCredentials and stderr assertion t…
robobun Mar 22, 2026
a24c656
[autofix.ci] apply automated fixes
autofix-ci[bot] Mar 22, 2026
3ee6137
add content_encoding to getPresignUrlFrom to match presignFromCredent…
robobun Mar 22, 2026
e584550
use idiomatic proc.stdout.text() in test
robobun Mar 22, 2026
84e2ab9
[autofix.ci] apply automated fixes
autofix-ci[bot] Mar 22, 2026
10860f2
strip leading slash after s3Path() to match Store.path() normalization
robobun Mar 22, 2026
45f86de
remove strict stderr check that fails under ASAN
robobun Mar 22, 2026
0b9bf69
[autofix.ci] apply automated fixes
autofix-ci[bot] Mar 22, 2026
48826c8
retry CI
robobun Mar 22, 2026
c26bfda
retry CI - infra flake
robobun Mar 22, 2026
43d7a33
retry CI
robobun Mar 22, 2026
be45713
Fix path leak: errdefer → defer for path cleanup in S3 functions
robobun Mar 23, 2026
68b24aa
Revert errdefer→defer changes, keep stderr assertion
robobun Mar 23, 2026
9276810
retry CI
robobun Mar 23, 2026
db30bb5
retry CI — prior build failures are infra (Darwin expired) and pre-ex…
robobun Mar 23, 2026
f8822f8
Add trailing backslash strip to match Store.S3.path() normalization
robobun Mar 23, 2026
c55e10c
retry CI
robobun Mar 23, 2026
18e384f
retry CI
robobun Mar 23, 2026
ee54d4f
retry CI — prior infra outage
robobun Mar 23, 2026
97b10aa
Fix sessionToken validation error message typo
robobun Mar 23, 2026
7aa8f27
retry CI
robobun Mar 23, 2026
1d6ccc2
retry CI
robobun Mar 24, 2026
cb81769
Fix S3Client.presign to not include acl/storage_class by default
robobun Mar 24, 2026
914518d
Use client acl/storage_class as defaults only when options are provided
robobun Mar 24, 2026
bb780ee
retry CI
robobun Mar 24, 2026
8ce8dee
retry CI
robobun Mar 24, 2026
41e276d
retry CI
robobun Mar 24, 2026
d230939
retry gate check — prior failure was Docker container crash, not test…
robobun Mar 24, 2026
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 31 additions & 4 deletions src/bun.js/webcore/S3Client.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Comment thread
robobun marked this conversation as resolved.
}

Comment thread
robobun marked this conversation as resolved.
pub fn exists(ptr: *@This(), globalThis: *jsc.JSGlobalObject, callframe: *jsc.CallFrame) bun.JSError!JSValue {
Expand Down
63 changes: 58 additions & 5 deletions src/bun.js/webcore/S3File.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand All @@ -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);
Comment thread
robobun marked this conversation as resolved.
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);
Comment thread
robobun marked this conversation as resolved.
},
.blob => return try getPresignUrlFrom(&path_or_blob.blob, globalThis, args.nextEat()),
}
Comment thread
claude[bot] marked this conversation as resolved.
Comment thread
claude[bot] marked this conversation as resolved.
Expand Down Expand Up @@ -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", .{});
Comment thread
claude[bot] marked this conversation as resolved.
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();
Comment thread
claude[bot] marked this conversation as resolved.
return bun.String.createUTF8ForJS(globalThis, result.url);
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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();
Expand All @@ -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_);
}
}
Expand All @@ -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);
};
Expand Down
2 changes: 1 addition & 1 deletion src/s3/credentials.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
Expand Down
48 changes: 48 additions & 0 deletions test/js/bun/s3/s3-presign-missing-credentials.test.ts
Original file line number Diff line number Diff line change
@@ -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");
Comment thread
robobun marked this conversation as resolved.
expect(exitCode).toBe(0);
});
Comment thread
claude[bot] marked this conversation as resolved.
Loading