Conversation
|
Updated 8:40 PM PT - Aug 27th, 2026
✅ @robobun, your commit d72a8f238708c9ec32570f4151b1b669f8811aa8 passed in 🧪 To try this PR locally: bunx bun-pr 32046That installs a local version of the PR into your bun-32046 --bun |
WalkthroughProxy resolution was moved from many S3 call sites into request-time logic: S3 client APIs dropped proxy parameters and a new resolve_proxy_url helper applies NO_PROXY/env selection when the signed URL is available. Call sites now omit proxy wiring and tests validate behavior. ChangesS3 proxy resolution refactoring
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Linked Issues checkExplanation The PR fully addresses issue Full details: Description checkExplanation The description explains the problem, root cause, fix, behavior changes, and verification results. It provides the required implementation and testing information, although it uses different headings from the repository template. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/js/bun/s3/s3-proxy.test.ts`:
- Around line 117-122: The assertion is masking real stderr when exitCode === 0
by using `stderr: exitCode === 0 ? "" : stderr`; update the expect(...) object
to use the actual stderr value instead of conditionally blanking it (e.g., use
`stderr: stderr` or just `stderr`) so unexpected stderr is surfaced; apply this
change to the repeated assertions that construct the expected object containing
`stdout, exitCode, stderr, proxyHits` (the expect(...).toEqual calls around the
occurrences you flagged).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: db81fbb0-bf15-47a2-80f8-b13bbddfdbd7
📒 Files selected for processing (9)
src/runtime/server/RequestContext.rssrc/runtime/webcore/Blob.rssrc/runtime/webcore/ReadableStream.rssrc/runtime/webcore/S3File.rssrc/runtime/webcore/blob/Store.rssrc/runtime/webcore/s3/client.rssrc/runtime/webcore/s3/multipart.rssrc/runtime/webcore/s3/simple_request.rstest/js/bun/s3/s3-proxy.test.ts
💤 Files with no reviewable changes (3)
- src/runtime/webcore/S3File.rs
- src/runtime/webcore/ReadableStream.rs
- src/runtime/webcore/blob/Store.rs
👮 Files not reviewed due to content moderation or server errors (3)
- src/runtime/webcore/s3/simple_request.rs
- src/runtime/webcore/s3/client.rs
- src/runtime/webcore/s3/multipart.rs
5d12ba2 to
fd77777
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found, but this changes proxy-resolution behavior across the S3 request paths (HTTPS_PROXY selection, NO_PROXY now applied to explicit { proxy }, env proxies now consulted for streaming s3 fetch), so it's worth a human pass on the intended semantics.
Extended reasoning...
Overview
The PR centralizes S3 proxy resolution into resolve_proxy_url in simple_request.rs, called at the three AsyncHTTP setup sites (execute_simple_s3_request, list_objects, download_stream) once the signed request URL is known. The ~15 call sites that previously threaded get_http_proxy(true, None, None) through stat/download/upload/delete/writable_stream/etc. are deleted along with the corresponding parameters, and MultiPartUpload::proxy_url() now returns None for an empty override so per-part requests fall through to env resolution. RequestContext.rs drops the now-unused global_this binding in the HEAD path. A new test file exercises write/stream/list/writer/fetch-stream against local mock endpoint+proxy servers in child processes.
Security risks
Proxy selection determines where outbound S3 traffic is routed, so a regression could send credentials/data to the wrong host or bypass a mandated egress proxy. The change aligns S3 with the existing fetch/FetchTasklet semantics (get_http_proxy_for + is_no_proxy), which reduces surprise, but it does introduce three documented behavior changes: https S3 endpoints now consult HTTPS_PROXY instead of HTTP_PROXY; an explicit { proxy } is now subject to NO_PROXY; and streaming fetch("s3://…") without an explicit proxy now reads env proxies where it previously ignored them. None of these look exploitable, but they are user-visible networking changes.
Level of scrutiny
This is production runtime networking code (Rust) spanning eight source files plus a test. Most of the diff is mechanical parameter-removal, and the new helper is small and mirrors fetch's resolution, but the behavior deltas and the breadth of touched call sites put this beyond what I'd auto-approve as a trivial change.
Other factors
The bug-hunting pass found nothing; CodeRabbit's only nit (conditional stderr masking in tests) was addressed in bf863ff and the thread is resolved. Test coverage looks solid (8 cases covering each request-construction site, the multipart path, explicit-proxy override, and scheme selection). No CODEOWNERS match these paths.
|
Confirming the three behavior deltas are intentional, and all three bring S3 in line with how
Test coverage in |
|
CI status for whoever picks this up: the diff is green, the red is CI infrastructure. Build #66075 failed with zero jobs exiting nonzero. Every I have spent my automatic retrigger on an earlier build, so a maintainer re-run of CI should clear this. Rebased on current main; happy to rebase again if it drifts. |
The spawned fixture aborted on the x64-asan lane during child teardown, and piping its stderr hid the reason. Run the same assertions in-process, the way s3-storage-class.test.ts and s3-requester-pays.test.ts already do, so a crash surfaces its own trace. Neutralize HTTP_PROXY around the suite because the S3 client does not honor NO_PROXY (#32046); those vars are unset on CI.
The spawned fixture aborted on the x64-asan lane during child teardown, and piping its stderr hid the reason. Run the same assertions in-process, the way s3-storage-class.test.ts and s3-requester-pays.test.ts already do, so a crash surfaces its own trace. Neutralize HTTP_PROXY around the suite because the S3 client does not honor NO_PROXY (#32046); those vars are unset on CI.
|
This came up again independently: on current main (54d6d16) every S3 call site still resolves the proxy with This PR is the fix for it, so no second PR is being opened. Note that the branch has drifted: it is about 1000 commits behind main and a trial merge now conflicts in |
fd77777 to
3957645
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — my earlier nit on the https test title was addressed in 52b8db5.
What was reviewed:
resolve_proxy_urluses the sharedget_http_proxy_for/is_no_proxyenv helpers; verifiedProxySettings::from_explicit(http/lib.rs:706) already applies NO_PROXY to fetch's explicit proxy, so the "explicit proxy subject to NO_PROXY" delta genuinely matches fetch.- All three request-construction sites (
execute_simple_s3_request,list_objects,download_stream) plus the multipart path are covered by tests;proxy_url()returningNoneon empty correctly threads through to per-part env resolution. - Owned
Box<[u8]>return keeps the existing lifetime invariant (env href can be freed by aprocess.envwrite); no new lifetime extensions.
Extended reasoning...
Overview
Fixes #32045: S3 requests ignored NO_PROXY and always read HTTP_PROXY regardless of endpoint scheme, because ~15 call sites resolved the proxy with get_http_proxy(true, None, None) before the signed request URL (and thus the effective host/scheme) was known. The fix deletes those lookups and introduces resolve_proxy_url(url, explicit) in simple_request.rs, called at the three AsyncHTTP setup sites where the signed URL is available. It routes through the same EnvLoader::get_http_proxy_for / is_no_proxy helpers that fetch already uses. MultiPartUpload::proxy_url() now returns None when the stored override is empty so per-part requests fall through to env resolution. 9 files, +241/−158, mostly deletions.
Security risks
None identified. Honoring NO_PROXY makes requests connect directly to hosts the user has explicitly excluded — this narrows proxy usage per the user's own env, it does not expand it. No auth, TLS, or credential-handling paths are touched; reject_unauthorized and signing are unchanged.
Level of scrutiny
Moderate. This is networking routing but not memory-safety-critical or auth-adjacent. The new function is ~15 lines calling existing shared helpers; the rest is deleting a duplicated anti-pattern. Three behavior changes are documented (https→HTTPS_PROXY, explicit proxy subject to NO_PROXY, streaming fetch("s3://…") now reads env proxies), all of which converge S3 with existing fetch behavior — I verified the "matches FetchTasklet" claim against ProxySettings::from_explicit in src/http/lib.rs, which does capture and apply NO_PROXY to fetch's explicit { proxy }.
Other factors
- All prior review feedback is resolved: coderabbit's stderr-masking (fixed in ed2f20e), comment-cop's paragraph-comment flags (shortened in a2a878f), and my test-title nit (renamed in 52b8db5).
- 8 tests in
s3-proxy.test.tscover write/stream/list/writer (all four request paths), the pass-through case, explicit-proxy + NO_PROXY both ways, and https scheme selection; per the PR evidence 6 fail on the unfixed build and all 8 pass with the fix under debug ASAN. - The owned
Box<[u8]>return preserves the pre-existing lifetime contract onS3HttpSimpleTask::proxy_url(env-derived href can be freed by a concurrentprocess.envwrite), and the resolved value is stored on the task before thedetach_lifetime_refextension, so no new lifetime hazards. - Verified no remaining
get_http_proxy(true, None, None)sites in the S3 path.
52b8db5 to
bcb977f
Compare
There was a problem hiding this comment.
I reviewed this PR and the bug hunting system found no issues; my earlier nit about the https test title was addressed in bcb977f. Because this ships three intentional behavior deltas to S3 proxy routing (https endpoints now consult HTTPS_PROXY instead of HTTP_PROXY, explicit {proxy} is now subject to NO_PROXY, and streaming fetch("s3://…") now reads env proxies), a maintainer sign-off on those semantics would be worthwhile.
Checked: resolve_proxy_url uses the same get_http_proxy_for/is_no_proxy helpers as install/upgrade/WebSocket; the owned Box<[u8]> return avoids the env-href lifetime hazard the old call sites documented. Verified MultiPartUpload::proxy_url() returning None on empty correctly routes each part request through env resolution in execute_simple_s3_request. The three AsyncHTTP setup sites plus the multipart path are all covered by the new subprocess tests.
Extended reasoning...
Overview
The PR moves S3 proxy resolution from ~15 call sites (Blob.rs, S3File.rs, Store.rs, ReadableStream.rs, RequestContext.rs) — which all called get_http_proxy(true, None, None) before the effective request host was known — to the three AsyncHTTP construction points (execute_simple_s3_request, list_objects, download_stream) where the signed URL is available. A new resolve_proxy_url helper (simple_request.rs:553-569) applies the same get_http_proxy_for + is_no_proxy logic that fetch, install, upgrade, and WebSocket already use. MultiPartUpload::proxy_url() now returns None when empty so per-part requests fall through to env resolution. A 204-line test file exercises write/stream/list/writer × NO_PROXY, the positive proxy path, explicit-proxy ± NO_PROXY, and https scheme selection, all in child processes.
Security risks
Proxy resolution affects where credential-bearing S3 requests are sent, so incorrect routing could leak signed requests to an unintended proxy. The change reduces that risk relative to the old code (which sent NO_PROXY-listed hosts to the proxy anyway). resolve_proxy_url is called on the JS thread before scheduling, reads env via the process-singleton loader, and returns an owned copy — no cross-thread borrow of env storage. No new user input parsing.
Level of scrutiny
Medium. The mechanics are largely deletions plus an 18-line helper that composes existing primitives. What warrants a human look is the three intentional behavior deltas the author documented: (1) https S3 endpoints — the AWS default — now consult HTTPS_PROXY instead of HTTP_PROXY; (2) an explicit {proxy} on fetch("s3://…") is now filtered by NO_PROXY; (3) streaming s3 fetch without an explicit proxy now reads env proxies where it previously ignored them. All three are corrections that align S3 with fetch, but they change observable behavior for existing setups.
Other factors
All prior review threads are resolved (CodeRabbit's stderr-masking, comment-cop's comment-length flags, my test-title nit). The gate evidence shows 6/8 tests fail on the unfixed ASAN build and 8/8 pass with the fix. The PR was rebased onto current main after conflicts and re-verified. No CODEOWNERS file gates these paths that I could find.
bcb977f to
a826fc3
Compare
There was a problem hiding this comment.
LGTM — the earlier nit (https-proxy test title) is addressed and no bugs were found on re-review.
What was reviewed:
resolve_proxy_urlsemantics vs fetch'sProxySettings::from_explicit/from_env+resolve()— both apply NO_PROXY to explicit proxies and pick HTTP_PROXY/HTTPS_PROXY by scheme via the same env loader helpers.- All three AsyncHTTP setup sites (
execute_simple_s3_request,list_objects,download_stream) now resolve against the signed URL and store an owned copy on the task; the multipartproxy_url()accessor returnsNonefor empty so per-part requests fall through to env resolution. - Removed
proxy_urlparams traced through every caller (Blob/S3File/Store/ReadableStream/RequestContext/client) — no leftover env lookups or unused imports;upload_streamretains the explicit-override channel for thefetch("s3://…", { proxy })path.
Extended reasoning...
Overview
The PR moves S3 proxy resolution from ~15 call sites (which passed get_http_proxy(true, None, None) — no hostname, hardcoded http) to the three points where the signed request URL is known. A new resolve_proxy_url(url, explicit) helper in simple_request.rs mirrors fetch's hop-0 resolution: an explicit proxy is checked against env.is_no_proxy(hostname, host); otherwise env.get_http_proxy_for(url) picks by scheme with NO_PROXY applied. The result is stored as an owned Box<[u8]> on the task (documented: process.env writes can free the env loader's borrowed href). Net -158 production lines (parameter/lookup removal) + a 204-line test file.
Security risks
Proxy resolution is mildly security-relevant, but this change tightens behavior: NO_PROXY exclusions that were previously ignored for S3 are now honored, and https endpoints stop reading HTTP_PROXY (which was the wrong variable). The helper reuses the same EnvLoader::is_no_proxy / get_http_proxy_for that fetch, WebSocket (Bun__isNoProxy), and the CLI already use, so no new parsing surface. No auth/crypto/permissions code touched.
Level of scrutiny
Medium. Most of the diff is mechanical parameter removal; the new logic is ~15 lines that delegate to existing shared helpers. I cross-checked resolve_proxy_url against ProxySettings::from_explicit/from_env + resolve() in src/http/lib.rs (what FetchTasklet::queue uses) and the semantics match for the initial request. S3 does not pass proxy_settings for redirect-hop re-evaluation, but that is pre-existing and unchanged by this PR.
Other factors
The PR carries three intentional behavior deltas (https→HTTPS_PROXY, explicit proxy subject to NO_PROXY, streaming s3 fetch now reads env proxies) that were called out and justified as aligning with fetch's established behavior. The 8-test suite covers all three request-construction sites, the multipart path, explicit-proxy NO_PROXY (both matched and unmatched), and https scheme selection; it fails 6/8 on the unfixed build and passes 8/8 with the fix. All prior review feedback (CodeRabbit stderr assertion, comment-cop length, my test-title nit) is resolved. The commits since my last review are the nit fix, comment shortening, and a CI retrigger — no substantive change.
…t URL
S3 call sites resolved the proxy with get_http_proxy(true, None, None),
which skips the NO_PROXY filter (no hostname to match against) and reads
HTTP_PROXY even for https endpoints. fetch resolves the proxy from the
request URL, so S3Client behaved differently from fetch in proxied
environments.
Resolve the proxy where the signed request URL is known instead: the
three AsyncHTTP setup sites (execute_simple_s3_request, list_objects,
download_stream) now call resolve_proxy_url(), which applies NO_PROXY
and selects HTTP_PROXY vs HTTPS_PROXY from the URL scheme, matching
fetch. The explicit proxy from fetch("s3://...", { proxy }) is still
honored, subject to NO_PROXY like fetch's own explicit proxy handling.
The per-call-site env lookups are deleted.
Fixes #32045
a826fc3 to
d72a8f2
Compare
|
Superseded by #42692, which landed the same per-request proxy resolution for S3 (by scheme, with Coverage note: on main, |
1 participant
Fixes #32045
Problem
With
HTTP_PROXYset andNO_PROXY=localhost,127.0.0.1,fetchconnects directly to a localhost endpoint while S3Client sends the request to the proxy:Cause
Every S3 call site resolved the proxy with
EnvLoader::get_http_proxy(true, None, None). Withhostname: Nonethe NO_PROXY filter is skipped entirely, andis_http: truereadsHTTP_PROXYeven when the S3 endpoint is https (soHTTPS_PROXYwas never consulted for https endpoints either). The effective request host is only known after signing (endpoint option, virtual hosted style, bucket, region), so the call sites could not pass it.Fix
Resolve the proxy where the signed request URL is known: the three AsyncHTTP setup sites (
execute_simple_s3_request,list_objects,download_stream) now call a sharedresolve_proxy_url(url, explicit)which matches fetch's behavior:fetch("s3://…", { proxy })) is used as-is, but still subject to NO_PROXY, same asFetchTaskletdoes for fetch's explicit proxyHTTP_PROXY/HTTPS_PROXYis selected from the URL scheme with the NO_PROXY filter applied (EnvLoader::get_http_proxy_for)The hostname-less
get_http_proxy(true, None, None)lookups at the ~15 call sites (Blob, S3File, Store, ReadableStream, RequestContext) are deleted along with the proxy threading through the S3 client function signatures. The explicit-override channel stays for the fetch s3 streaming-upload path.Behavior notes:
HTTPS_PROXYinstead ofHTTP_PROXY, matching fetch and the usual convention. PreviouslyHTTPS_PROXYwas ignored for S3 andHTTP_PROXYapplied to https endpoints.fetch("s3://…", { body: stream })without an explicit proxy previously ignored proxy env vars entirely; it now resolves them like every other S3 request (the non-streaming s3 fetch path already did via FetchTasklet).Tests
test/js/bun/s3/s3-proxy.test.tsruns each operation in a child process (proxy env vars are read from the process env) against a local mock endpoint and a local mock proxy that record hits:HTTP_PROXYwhen the endpoint host is inNO_PROXY(covers all three request-construction sites and the multipart path)HTTP_PROXYwhenNO_PROXYdoes not match (proxy support still works)NO_PROXYdoes not match, bypassed when it doesHTTP_PROXYset connects directlyOn the unfixed build: 6 fail, 2 pass (the two pass-through sanity tests). With the fix: 8 pass. The previously failing tests fail with output like:
Rebase notes (conflicts resolved against current main)
Main's S3 rework landed between revisions (XML response parsing, shared-reference task setup, VM teardown tickets, narrowed field visibility), so the rebase onto current main had conflicts in 5 files. Resolution:
simple_request.rs: main now sets task fields in the struct literal and takes only a shared reference to the task afterwards, soresolve_proxy_urlis called on the signed URL before task creation and the result passed as theproxy_urlfield, instead of assigning through&mutafter.client.rs/multipart.rs: re-applied the parameter removals and the empty-means-envproxy_url()accessor on top of main'spub(crate)visibility andJsResultsignatures.RequestContext.rs: kept main's new HEAD framing logic and theserver/global_thisbindings (now consumed by the Locked-body arm on main); only the proxy argument to the S3 stat call is removed.Blob.rs: dropped the proxy lookup; main had already hoisted thepoll_ref.ref_my earlier revision touched.Verified after rebase: the 8 proxy tests pass on the debug build. As an end-to-end check in a container that itself sets
HTTP_PROXY+NO_PROXY=localhost,...: on main 55 tests intest/js/bun/s3/fail (mock-server requests are sent to the proxy), with this change 0 fail.[review] gate passed · iteration 8 · 9 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 8
evidence per changed file
root cause · written by the author bot
S3 request paths called the environment proxy lookup with no hostname, so the NO_PROXY exclusion list was never consulted and requests to excluded hosts were still routed through HTTP_PROXY, with https endpoints also incorrectly reading HTTP_PROXY. The fix removes proxy resolution from the S3 call sites and instead resolves the proxy at the three points where the signed request URL is known, using a new resolve_proxy_url helper that applies NO_PROXY to explicit proxies and otherwise selects HTTP_PROXY or HTTPS_PROXY by scheme via the same env loader helpers fetch uses. This makes S3Client h…