test(s3): cover the default region of a client against the local S3 server by robobun · Pull Request #44110 · oven-sh/bun

robobun · 2026-09-27T09:50:41Z

Behaviour change: none

Problem

After test: replace the MinIO container with an S3 server on Bun.serve #44054 each client in test/js/bun/s3/s3.test.ts has region: "us-east-1", because the local server checks the region of each signature. No test sends the default region of a client to a server that checks it.
withoutDefaultType (test/packages/s3-server/test/helpers.ts:30) removes a header that only a debug build of Bun sends. Nothing in the package names server: don't send a fallback content-type on detached responses #30997, which corrects Bun.serve, so the helper can stay after that correction.

Fix

Add one test to test/packages/s3-server/test/auth.test.ts. A client without a region reads, writes and presigns. The server has the region us-east-1. It refuses each request, and its error names the region of the signature, auto.
The client runs in a child process without S3_REGION and AWS_REGION, in a directory without a .env file. The machine of the test runner does not change the result.
The comment at withoutDefaultType names server: don't send a fallback content-type on detached responses #30997.
Verified: bun bd test test/packages/s3-server/test/auth.test.ts and a release build, on Linux x64: each 149 pass, 0 fail.

Background

Bun takes the region of an S3 client from its options, then from S3_REGION or AWS_REGION (src/dotenv/env_loader.rs:269). Without them, guess_region (src/s3_signing/credentials.rs:1115) reads it from the endpoint. For an endpoint that is not Amazon S3 or R2 the region is auto.
auth.test.ts has the cases of a signature for a region that the server refuses. This test is one more of them, with Bun.S3Client as the client.
Considered s3.test.ts for the test, and the client in the process of the test. The Notes have the reason against each.

Notes

This PR changes one test file and one comment. No file under src/ changes, so Bun behaves as before.
The child process of the new test also has no S3_SESSION_TOKEN and no AWS_SESSION_TOKEN. The error of the server is the region 'auto' is wrong; expecting 'us-east-1'. The test compares the complete error document of the presigned request. Only RequestId and HostId are replaced before.
The new test passes with AWS_REGION and S3_REGION in the environment of the test runner (on Windows also in lowercase, at the time when the test was in server.test.ts), and with a .env file that sets them in the directory of the test runner. With the client in the process of the test, or in a child process in the directory of the test runner, that .env file made the test fail: the client signed for us-west-2.
Why not s3.test.ts: since test: replace the MinIO container with an S3 server on Bun.serve #44054 each test of that file runs under bun bd test, because the server needs no container. On my machine (load average 400 to 650) tests of that file pass the default timeout of 5 s in a debug build, with and without the new test. In 4 runs these were should be able to set content-type of the Bun.S3Client group (2 runs), the 2 tests of http endpoint should work when using env variables (3 runs), and does not UAF when a ReadableStream body errors after enqueue (1 run). With --timeout=270000, the per-test timeout of the ASAN lane, the file passes: 308 pass with the new test in it. A first form of this PR had the test there.
This PR sets no timeout.
Windows: the same test passed on Windows x64 (release build) when it was in server.test.ts. I did not run it there in auth.test.ts.
A first form of this PR had the test in server.test.ts. That file has 8 tests that start the server as a process. In a debug build on a machine with a high load, 5 of them took more than 60 s. auth.test.ts starts no other process. The new test took 1.2 s and 7.2 s there under bun bd test.
test: replace the MinIO container with an S3 server on Bun.serve #44054 merged at 827c607. A first form of this test was written after that head and was not part of the merge.

[auto-merge] gate passed · iteration 4 · 2 files touched

passes on PR (with fix)

Test-only change.

Debug/ASAN (expected pass):
$ bun bd test 'test/packages/s3-server/test/auth.test.ts'
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "test/packages/s3-server/test/auth.test.ts"
rustup spent 7s installing the pinned toolchain (nightly-2026-09-15); it was missing or incomplete on this machine
bun test v1.4.3 (367d939d9)

test/packages/s3-server/test/auth.test.ts:
(pass) the examples of the AWS documentation > GET Object [6743.46ms]
(pass) the examples of the AWS documentation > PUT Object [500.66ms]
(pass) the examples of the AWS documentation > GET Bucket Lifecycle [288.72ms]
(pass) the examples of the AWS documentation > GET Bucket (List Objects) [1514.65ms]
(pass) the examples of the AWS documentation > presigned GET Object [456.83ms]
(pass) the examples of the AWS documentation > PUT Object in chunks [339.84ms]
(pass) the examples of the AWS documentation > PUT Object in chunks with a trailing header [1144.56ms]
(pass) the Authorization header > with no change [108.18ms]
(pass) the Authorization header > with the signature in uppercase [71.51ms]
(pass) the Authorization header > with the Date header and no x-amz-date [130.59ms]
(pass) the Authorization header > with no Credential [109.92ms]
(pass) the Authorization header > with no SignedHeaders [88.61ms]
(pass) the Authorization header > with no Signature [87.37ms]
(pass) the Authorization header > with a credential of four parts [77.79ms]
(pass) the Authorization header > with a scope date of seven digits [85.25ms]
(pass) the Authorization header > with another service [77.83ms]
(pass) the Authorization header > with another terminator [56.07ms]
(pass) the Authorization header > with a scope date that is not the date of the request [54.51ms]
(pass) the Authorization header > with Signature Version 2 [90.73ms]
(pass) the Authorization header > with Signature Version 2 in the query string and no header [145.45ms]
(pass) the Authorization header > with the Bearer scheme [272.43ms]
(pass) the Authorization header > with no value [131.16ms]
(pass) the Author
... (truncated)
Exit: 0

diff hotspot

test/packages/s3-server/test/auth.test.ts | 56 +++++++++++++++++++++++++++++++
 test/packages/s3-server/test/helpers.ts   |  3 ++
 2 files changed, 59 insertions(+)

gate history · 2 passed · 3 rejected · iteration 4

evidence per changed file

file                                       reads  edits  tests
test/packages/s3-server/test/auth.test.ts      0      0     18
test/packages/s3-server/test/helpers.ts        1      2     26

Each client of the file has a region, because the server checks it. This test keeps the default region of a client covered. It runs the client in a child process without S3_REGION and AWS_REGION, so the environment of the test does not change the result. The server refuses the signature, and its error names the region of the signature. The comment at withoutDefaultType names the pull request that has the correction for Bun.serve.

coderabbitai · 2026-09-27T09:53:54Z

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

@coderabbitai resume to resume automatic reviews.
@coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

▶️ Resume reviews
🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: oven-sh/bun/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: abe839ce-1206-4ec0-b0fe-eef86579a57f

📥 Commits

Reviewing files that changed from the base of the PR and between 1cb13c9 and 3b3f552.

📒 Files selected for processing (1)

test/packages/s3-server/test/server.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

Walkthrough

The changes add a subprocess test for S3 requests without a configured region and update a test-helper comment about a future removal.

Changes

S3 region fallback test

Layer / File(s)	Summary
Verify region fallback in S3 requests `test/packages/s3-server/test/server.test.ts`	The test launches a subprocess without region or session-token environment variables. It checks read, write, and presigned-read responses against a us-east-1 server and verifies that the bucket remains empty.

S3 test helper note

Layer / File(s)	Summary
Record future test-helper removal `test/packages/s3-server/test/helpers.ts`	The comment notes that `withoutDefaultType` and `TestClient` can be removed after an upstream Bun.serve content-type correction.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 3b3f5

The new test checks that requests without a configured region use the expected signing region. No merge-blocking issue was identified.

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the main change: testing the default region of an S3 client against the local S3 server.
Description check	✅ Passed	The description explains the problem, fix, verification steps, test results, and implementation background. It does not use the template headings exactly, but it provides the required information in e…

_{Comment @coderabbitai help to get the list of available commands.}

robobun · 2026-09-27T09:57:38Z

Status

How I reproduced the problem:

git grep -n "region" test/js/bun/s3/s3.test.ts on main: each client that sends a request to the local server has a region. In test/packages/s3-server/test/auth.test.ts, each case of a region that the server refuses has a client with a wrong region. No test covers a client without a region.

How I verified the change:

bun bd test test/packages/s3-server/test/auth.test.ts on Linux x64, 2 runs: each 149 pass, 0 fail.
Release build on Linux x64, 2 runs: each 149 pass, 0 fail.
The new test passes with AWS_REGION and S3_REGION in the environment of the test runner, and with a .env file that sets them in its directory.
Windows: I did not run the test in its new file there. The same test passed on Windows x64 when it was in server.test.ts.

claude

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline findings, I also checked whether pinning the region auto certifies a bug rather than intended behavior: guess_region in src/s3_signing/credentials.rs:1115 deliberately returns auto for any explicit endpoint that is not amazonaws.com or R2, and the us-east-1 default that docs/runtime/s3.mdx:408 describes applies only when no endpoint is given, so the expected string matches the designed default for a custom endpoint.

Extended reasoning...

Test-only change: one new subprocess test in test/js/bun/s3/s3.test.ts against the local s3-server, plus a JSDoc note in test/packages/s3-server/test/helpers.ts; no src/ changes and no security-sensitive surface. The default-region value the test pins was traced to the non-AWS-endpoint branch of guess_region and is consistent with the docs' no-endpoint us-east-1 default, so it is not a snapshot of buggy behavior.

…ile is The child process had the directory of the test runner. A .env file there with AWS_REGION or S3_REGION gave the client that region. The child now runs in the temporary directory of the tests, and the names of the variables that it must not have are compared without their case. The test compares the complete error document of the presigned request, and it runs beside the other tests that start a process.

claude

LGTM — the second commit addresses each point from the earlier review, and the new test is hermetic and exact.

What was reviewed:

Traced the expected strings against the local server: signature.ts:233 builds the region message with Region detail, credentialParameterError prefixes the X-Amz-Credential text, and errorResponse emits application/xml with RequestId (16 uppercase hex) and HostId (42 bytes base64 = 56 chars), matching the normalizing regex.
Checked the child spawn: pipes drained with Promise.all, await using, cwd: testDir (a tempdir with only index.mjs, so no dotenv file), and the region/session-token variables stripped case-insensitively from a spread bunEnv.
The comment change in helpers.ts is docs-only and links the upstream PR.

Extended reasoning...

The diff is test-only: one new it.concurrent test in test/js/bun/s3/s3.test.ts that spawns bun -e against the in-repo s3-server with no region configured and asserts the exact AuthorizationHeaderMalformed / AuthorizationQueryParametersError responses, plus a three-line doc comment in test/packages/s3-server/test/helpers.ts. It touches no runtime source and no security-sensitive surface; the only network traffic is to the local test server. The assertion strings and the RequestId/HostId regex were verified against the server's actual construction in signature.ts, context.ts and server.ts. The three optional comments from the prior run were all applied in commit 1c65f4e, no other reviewer has an open objection, and the changed files are not covered by CODEOWNERS.

Since the local server replaced the MinIO container, each test of the file runs under bun bd test. A debug build runs them many times slower than a release build. On a machine with a high load, three tests that move no data passed the default timeout of 5 seconds. CI does not run debug builds and passes --timeout to each test file.

claude

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Beyond the inline note on the file-wide debug timeout, I re-checked the three points from the earlier round against the current diff and they are addressed: the child now runs with cwd: testDir so no dotenv file can supply a region, the presigned body is pinned to the full <Error> document with only RequestId/HostId normalized, and the test is it.concurrent like its siblings.

Extended reasoning...

Test-only change adding one subprocess test to test/js/bun/s3/s3.test.ts plus a docstring line in test/packages/s3-server/test/helpers.ts; it touches no src/ code and no security-sensitive surface. The earlier-round concerns (dotenv leakage via inherited cwd, substring-only body assertion, serial execution) are resolved in the code. The remaining inline finding is the isDebug-gated setDefaultTimeout(60_000), which conflicts with test/CLAUDE.md's rule against setting test timeouts, so a maintainer should weigh whether to keep it.

test/CLAUDE.md has the rule that a test sets no timeout. Three older tests of the file take more than 5 seconds under bun bd test on a machine with a high load. Pass --timeout to bun bd test on such a machine, as the CI runner does.

robobun · 2026-09-27T12:01:56Z

^{Updated 5:09 PM PT - Sep 28th, 2026}

✅ @robobun, your commit 6c8c7215714e07d59508d86b76896fbbcb816ad6 passed in Build #121391! 🎉

🧪 To try this PR locally:

bunx bun-pr 44110

That installs a local version of the PR into your bun-44110 executable, so you can run:

bun-44110 --bun

claude

LGTM — the points from the earlier rounds are all addressed in the code now (child runs with cwd: testDir, the presigned 400 is pinned to the exact <Error> document with only the ids normalized, the test is it.concurrent, and the file-level setDefaultTimeout is gone).

What was reviewed:

The id normalization regex against the server: randomHex(8).toUpperCase() gives 16 hex chars and 42 random bytes in base64 give 56 unpadded chars, so the pattern matches every response.
Expected messages against malformedHeader/credentialParameterError in test/packages/s3-server/src/signature.ts, and the auto default in src/s3_signing/credentials.rs for a non-AWS endpoint.
Hermeticity: only the local s3-server is contacted, pipes are drained with Promise.all, and stdout/stderr are asserted before exitCode.

Extended reasoning...

The diff adds one subprocess test to test/js/bun/s3/s3.test.ts that spawns bun -e without S3_REGION/AWS_REGION and asserts the local S3 server refuses read, write and presigned GET with region 'auto', plus a three-line doc comment in test/packages/s3-server/test/helpers.ts. No file under src/ changes and no security-sensitive surface is touched. Every objection raised in the three prior review rounds is visibly resolved in the current code, the assertions are exact and match the server implementation, and the changed files are not covered by CODEOWNERS, which decided approve.

…ckage The test is next to the test of a client with a wrong region now, and s3.test.ts is as on main. Under bun bd test on a machine with a high load, older tests of s3.test.ts pass the default timeout of 5 seconds, with and without this test.

claude

LGTM — the moved test now addresses everything raised earlier, and this revision is a self-contained, hermetic test-only change.

What was reviewed:

The new test in test/packages/s3-server/test/server.test.ts: it runs in the concurrent describe, spawns the child in a fresh tempDir so no dotenv file can supply a region, strips the region and session-token variables from a spread bunEnv, drains stdout/stderr/exited together, and asserts one structured value covering both SDK errors and the full presigned <Error> document with only the per-request ids normalized (matches credentialParameterError in src/signature.ts).
Cleanup ordering: await using server, using dir, await using child are all declared before the assertions.
The withoutDefaultType change in helpers.ts is a comment only; s3.test.ts and its debug-build timeout are no longer touched.

Extended reasoning...

The diff touches two files under test/packages/s3-server/test: a comment on withoutDefaultType in helpers.ts and one new test in server.test.ts that spawns bun -e against the in-repo mock S3 server to check that a client without a configured region signs for 'auto' and is refused by a region-enforcing server. No src/ code and no security-sensitive surface is affected; the test contacts only the local mock server. All inline concerns from earlier runs (dotenv leakage via cwd, substring assertion on the presigned body, serial execution, and the file-wide setDefaultTimeout in s3.test.ts) are resolved in the code itself by the move to the package's own test file, and there are no CODEOWNERS entries covering these paths. The bug hunt exited on dry_streak with no findings.

robobun · 2026-09-28T19:55:49Z

@robobun wake up!!

Jarred-Sumner · 2026-09-28T20:40:30Z

@robobun wake up!!

… of auth.test.ts auth.test.ts has the cases of a signature for a region that the server refuses. The test of a client without a region is one more of them. server.test.ts is as on main.

claude

LGTM — the moved test is hermetic and the points raised on earlier versions are addressed in the code as it now stands.

What was reviewed:

The new test in test/packages/s3-server/test/auth.test.ts: child runs in an empty tempDir cwd with region/session variables filtered from a spread bunEnv (harness vars such as BUN_DEBUG_QUIET_LOGS and PATH survive the filter), pipes are drained concurrently, stderr is asserted before the exit code, and the presigned 400 body is compared as a full <Error> document with only RequestId/HostId normalized.
The expected strings match the server's signature.ts error text and errors.ts codes; S3_* credentials set for the child take precedence over any AWS_* values left in the environment.
Confirmed test/packages/s3-server/test/*.test.ts is discovered by scripts/runner.node.ts, so the test runs in CI; no per-test timeout is added and no file under src/ changes.

Extended reasoning...

The diff adds one test to the in-repo mock S3 server's auth tests and a two-line doc comment in helpers.ts; no runtime source is touched, so it has no user-facing or security-sensitive surface. The test spawns one bun -e child via bunExe/bunEnv against a local server, uses await using/using for cleanup, and asserts exact error codes, messages, status, content-type and bucket emptiness. Earlier inline feedback from this bot (missing cwd, substring-only XML check, non-concurrent placement, file-wide timeout) is reflected in the current code, and no CODEOWNERS entry covers the changed paths. The hunt ran dry with no findings and the change is small and self-contained, which decided approve.

robobun · 2026-09-28T23:08:15Z

Awake. The two red checks were not from the new test, and the branch has no conflict with main.

The debug run of server.test.ts failed on 5 older tests that start the server as a process. On a machine with a high load each took more than 60 s. The new test passed there.
The new test is now in auth.test.ts, beside the cases of a region that the server refuses. That file starts no other process. bun bd test test/packages/s3-server/test/auth.test.ts passes: 149 pass, 0 fail, 2 runs.
buildkite/bun was red for test/js/bun/spawn/spawn.test.ts on one debian 13 x64-asan shard. This PR does not change that test. 6c8c721 starts a new build.

robobun assigned Jarred-Sumner Sep 27, 2026

github-actions Bot added the claude label Sep 27, 2026

claude Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread test/js/bun/s3/s3.test.ts Outdated

Comment thread test/js/bun/s3/s3.test.ts Outdated

Comment thread test/js/bun/s3/s3.test.ts Outdated

claude Bot reviewed Sep 27, 2026

View reviewed changes

robobun added 2 commits September 27, 2026 10:55

ci: retrigger

2dc7271

claude Bot reviewed Sep 27, 2026

View reviewed changes

Comment thread test/js/bun/s3/s3.test.ts Outdated

claude Bot reviewed Sep 27, 2026

View reviewed changes

s3-server: put the test of the default region beside the region cases…

6c8c721

… of auth.test.ts auth.test.ts has the cases of a signature for a region that the server refuses. The test of a client without a region is one more of them. server.test.ts is as on main.

claude Bot reviewed Sep 28, 2026

View reviewed changes

Conversation

robobun commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Background

Uh oh!

coderabbitai Bot commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews paused

Walkthrough

Changes

Uh oh!

robobun commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

robobun commented Sep 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

robobun commented Sep 28, 2026

Uh oh!

Jarred-Sumner commented Sep 28, 2026

Uh oh!

claude Bot left a comment

Choose a reason for hiding this comment

Uh oh!

robobun commented Sep 28, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

robobun commented Sep 27, 2026 •

edited

Loading

coderabbitai Bot commented Sep 27, 2026 •

edited

Loading

robobun commented Sep 27, 2026 •

edited

Loading

robobun commented Sep 27, 2026 •

edited

Loading