Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
23 changes: 14 additions & 9 deletions src/bun.js/webcore/S3Client.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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();
Expand All @@ -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 };
Comment thread
claude[bot] marked this conversation as resolved.
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, .{
Expand Down Expand Up @@ -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();
Expand Down
27 changes: 27 additions & 0 deletions src/bun.js/webcore/S3File.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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();

Expand All @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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();

Comment on lines 598 to 605

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟣 The stat() function in S3File.zig contains two error messages copied from size() that say "Expected a S3 or path to get size" instead of referencing stat -- callers who pass a non-S3 blob or a file descriptor to S3.stat() will see a misleading error message. This is a pre-existing copy-paste error, but the PR directly modifies this function body (adding the ownership-transfer lines at lines 601-602), making it an adjacent opportunity to fix the messages.

Extended reasoning...

What the bug is

The stat() function in src/bun.js/webcore/S3File.zig contains two error messages copied verbatim from the size() function and never updated:

  1. Blob validation check: throwInvalidArguments("Expected a S3 or path to get size")
  2. .fd path check: throwInvalidArguments("Expected a S3 or path to get size")

Both should say something like "Expected a S3 or path to stat" to correctly describe what stat() does.

The specific code path that triggers it

Any call to S3.stat() or Bun.s3.stat() that fails one of these guards produces the misleading message:

  • Passing a non-S3 blob (e.g., a regular file blob) hits the blob validation check
  • Passing a file descriptor path hits the .fd guard

In both cases the user sees an error about "get size" even though they called stat, not size.

Why existing code does not prevent it

This is purely a copy-paste oversight. The size() function at lines 180-210 uses identical guard messages. When stat() was created from size() as a template, these strings were never updated. There is no automated check that validates error message text against the function name.

Impact

The impact is confusion at the JavaScript API surface: a user calling Bun.s3.stat(blob) with a non-S3 blob or a file descriptor receives an error message that tells them they cannot "get size" of that path, which is irrelevant to what they were trying to do. While not a correctness bug, this is a DX issue that makes debugging harder.

How to fix it

Change both error strings in stat() from "Expected a S3 or path to get size" to "Expected a S3 or path to stat" (or similar stat-specific wording). The size() function's identical messages are correct for that function and should remain unchanged.

Step-by-step proof

  1. User calls Bun.s3.stat(regularFileBlob) where regularFileBlob.store.data != .s3
  2. Execution reaches the blob validation check in stat(): throwInvalidArguments("Expected a S3 or path to get size")
  3. User receives error: "Expected a S3 or path to get size"
  4. User is confused -- they called stat, not size; the error message does not match the operation they attempted
  5. The size() function contains the identical string for its own guard, which is correct for that function
  6. The PR adds 2 lines directly inside stat() (the path_or_blob neutralization at lines 601-602), placing the change immediately adjacent to both incorrect error messages

Expand Down
6 changes: 4 additions & 2 deletions src/bun.js/webcore/blob/Store.zig
Original file line number Diff line number Diff line change
Expand Up @@ -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(.{
Expand Down Expand Up @@ -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(.{
Expand Down
36 changes: 36 additions & 0 deletions test/js/bun/s3/s3-presign-error.test.ts
Original file line number Diff line number Diff line change
@@ -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 () => {
Comment thread
claude[bot] marked this conversation as resolved.
// Scrub AWS credential/config env vars so the test always hits the
// missing-credentials path regardless of ambient host configuration.
const env: Record<string, string> = {};
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);
});
Loading