fix(fetch): ignore proxy object without url property #25414

robobun · 2025-12-08T07:38:45Z

Summary

When a URL object is passed as the proxy option, or when a proxy object lacks a "url" property, ignore it instead of throwing an error
This fixes a regression introduced in 1.3.4 where libraries like taze that pass URL objects as proxy values would fail

Test plan

Added test: "proxy as URL object should be ignored (no url property)" - passes a URL object directly as proxy
Updated test: "proxy object without url is ignored (regression fetch in bun 1.3.4 may contains break changes #25413)" - proxy object with headers but no url
Updated test: "proxy object with null url is ignored (regression fetch in bun 1.3.4 may contains break changes #25413)" - proxy object where url is null
All 29 proxy tests pass

🤖 Generated with Claude Code

When a URL object is passed as the proxy option, or when a proxy object lacks a "url" property, ignore it instead of throwing an error. This fixes a regression introduced in 1.3.4 where libraries like taze that pass URL objects as proxy values would fail with: "fetch() proxy object requires a 'url' property" URL objects don't have a "url" property (they have "href"), so the strict validation was incorrectly rejecting valid use cases. Fixes #25413 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>

robobun · 2025-12-08T07:38:54Z

^{Updated 12:34 PM PT - Dec 8th, 2025}

❌ @Jarred-Sumner, your commit f0db0fe has 6 failures in Build #33034 (All Failures):

test/v8/v8.test.ts - 1 failing on 🐧 13 x64-asan
test/js/node/test/parallel/test-http-url.parse-https.request.js - code 1 on 🐧 13 x64-asan
test/js/node/test/parallel/test-fs-readdir-stack-overflow.js - timeout on 🍎 13 aarch64
test/js/bun/resolve/load-same-js-file-a-lot.test.ts - 1 failing on 🐧 13 x64-asan
test/regression/issue/11297/11297.test.ts - 1 failing on 🪟 2019 x64
test/integration/next-pages/test/dev-server.test.ts - 1 failing on 🍎 13 aarch64

🧪 To try this PR locally:

bunx bun-pr 25414

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

bun-25414 --bun

coderabbitai · 2025-12-08T07:45:06Z

Walkthrough

Modified proxy object validation in Bun's fetch implementation to gracefully handle missing, null, or undefined proxy URLs. Previously, the code required proxy.url to be a non-empty string and threw errors on invalid values. Now, invalid proxy URLs are silently ignored and requests proceed without a proxy.

Changes

Cohort / File(s)	Summary
Proxy handling relaxation `src/bun.js/webcore/fetch.zig`, `test/js/bun/http/proxy.test.ts`	Relaxed proxy object URL validation to ignore missing, null, or undefined proxy URLs instead of throwing errors. Updated tests from expecting errors to expecting successful requests without proxies when URLs are invalid.

Possibly related PRs

feat(fetch): add proxy object format with headers support #25090: Previously added full object and headers support for proxy-object handling in the same fetch.zig file; this PR builds upon it by relaxing URL validation constraints.

Suggested reviewers

nektro
alii

Pre-merge checks

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix(fetch): ignore proxy object without url property' clearly and specifically describes the main change addressing the regression in proxy handling.
Description check	✅ Passed	The PR description includes a comprehensive summary and test plan following the template structure, with details about the regression fix and test coverage.
Linked Issues check	✅ Passed	The code changes successfully implement the required fix: proxy objects without url properties are now ignored instead of throwing errors, addressing the regression reported in issue #25413.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing the proxy handling regression and related tests; no unrelated modifications are present.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Disabled knowledge base sources:

Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 3af0d23 and 59b8bac.

📒 Files selected for processing (2)

src/bun.js/webcore/fetch.zig (1 hunks)
test/js/bun/http/proxy.test.ts (2 hunks)

🧰 Additional context used

📓 Path-based instructions (6)

test/**/*.{js,ts,jsx,tsx}

📄 CodeRabbit inference engine (.cursor/rules/writing-tests.mdc)

test/**/*.{js,ts,jsx,tsx}: Write tests as JavaScript and TypeScript files using Jest-style APIs (test, describe, expect) and import from bun:test
Use test.each and data-driven tests to reduce boilerplate when testing multiple similar cases

Files:

test/js/bun/http/proxy.test.ts

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}

📄 CodeRabbit inference engine (test/CLAUDE.md)

test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Use bun:test with files that end in *.test.{ts,js,jsx,tsx,mjs,cjs}
Do not write flaky tests. Never wait for time to pass in tests; always wait for the condition to be met instead of using an arbitrary amount of time
Never use hardcoded port numbers in tests. Always use port: 0 to get a random port
Prefer concurrent tests over sequential tests using test.concurrent or describe.concurrent when multiple tests spawn processes or write files, unless it's very difficult to make them concurrent
When spawning Bun processes in tests, use bunExe and bunEnv from harness to ensure the same build of Bun is used and debug logging is silenced
Use -e flag for single-file tests when spawning Bun processes
Use tempDir() from harness to create temporary directories with files for multi-file tests instead of creating files manually
Prefer async/await over callbacks in tests
When callbacks must be used and it's just a single callback, use Promise.withResolvers to create a promise that can be resolved or rejected from a callback
Do not set a timeout on tests. Bun already has timeouts
Use Buffer.alloc(count, fill).toString() instead of 'A'.repeat(count) to create repetitive strings in tests, as ''.repeat is very slow in debug JavaScriptCore builds
Use describe blocks for grouping related tests
Always use await using or using to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc
Always check exit codes and test error scenarios in error tests
Use describe.each() for parameterized tests
Use toMatchSnapshot() for snapshot testing
Use beforeAll(), afterEach(), beforeEach() for setup/teardown in tests
Track resources (servers, clients) in arrays for cleanup in afterEach()

Files:

test/js/bun/http/proxy.test.ts

test/**/*.test.{ts,tsx}

📄 CodeRabbit inference engine (CLAUDE.md)

test/**/*.test.{ts,tsx}: For single-file tests in Bun test suite, prefer using -e flag over tempDir
For multi-file tests in Bun test suite, prefer using tempDir and Bun.spawn
Always use port: 0 when spawning servers in tests - do not hardcode ports or use custom random port functions
Use normalizeBunSnapshot to normalize snapshot output in tests instead of manual output comparison
Never write tests that check for no 'panic', 'uncaught exception', or similar strings in test output - that is not a valid test
Use tempDir from harness to create temporary directories in tests - do not use tmpdirSync or fs.mkdtempSync
In tests, call expect(stdout).toBe(...) before expect(exitCode).toBe(0) when spawning processes for more useful error messages on failure
Do not write flaky tests - do not use setTimeout in tests; instead await the condition to be met since you're testing the CONDITION, not TIME PASSING
Verify your test fails with USE_SYSTEM_BUN=1 bun test <file> and passes with bun bd test <file> - tests are not valid if they pass with USE_SYSTEM_BUN=1
Avoid shell commands in tests - do not use find or grep; use Bun's Glob and built-in tools instead
Test files must end in .test.ts or .test.tsx and be created in the appropriate test folder structure

Files:

test/js/bun/http/proxy.test.ts

src/**/*.{cpp,zig}

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

src/**/*.{cpp,zig}: Use bun bd or bun run build:debug to build debug versions for C++ and Zig source files; creates debug build at ./build/debug/bun-debug
Run tests using bun bd test <test-file> with the debug build; never use bun test directly as it will not include your changes
Execute files using bun bd <file> <...args>; never use bun <file> directly as it will not include your changes
Enable debug logs for specific scopes using BUN_DEBUG_$(SCOPE)=1 environment variable
Code generation happens automatically as part of the build process; no manual code generation commands are required

Files:

src/bun.js/webcore/fetch.zig

src/**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/building-bun.mdc)

Use bun.Output.scoped(.${SCOPE}, .hidden) for creating debug logs in Zig code

Implement core functionality in Zig, typically in its own directory in src/

src/**/*.zig: Private fields in Zig are fully supported using the # prefix: struct { #foo: u32 };
Use decl literals in Zig for declaration initialization: const decl: Decl = .{ .binding = 0, .value = 0 };
Prefer @import at the bottom of the file (auto formatter will move them automatically)

Be careful with memory management in Zig code - use defer for cleanup with allocators

Files:

src/bun.js/webcore/fetch.zig

**/*.zig

📄 CodeRabbit inference engine (.cursor/rules/zig-javascriptcore-classes.mdc)

**/*.zig: Expose generated bindings in Zig structs using pub const js = JSC.Codegen.JS<ClassName> with trait conversion methods: toJS, fromJS, and fromJSDirect
Use consistent parameter name globalObject instead of ctx in Zig constructor and method implementations
Use bun.JSError!JSValue return type for Zig methods and constructors to enable proper error handling and exception propagation
Implement resource cleanup using deinit() method that releases resources, followed by finalize() called by the GC that invokes deinit() and frees the pointer
Use JSC.markBinding(@src()) in finalize methods for debugging purposes before calling deinit()
For methods returning cached properties in Zig, declare external C++ functions using extern fn and callconv(JSC.conv) calling convention
Implement getter functions with naming pattern get<PropertyName> in Zig that accept this and globalObject parameters and return JSC.JSValue
Access JavaScript CallFrame arguments using callFrame.argument(i), check argument count with callFrame.argumentCount(), and get this with callFrame.thisValue()
For reference-counted objects, use .deref() in finalize instead of destroy() to release references to other JS objects

Files:

src/bun.js/webcore/fetch.zig

🧠 Learnings (10)

📓 Common learnings

Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.

Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23169
File: src/bun.js/bindings/BunIDLConvert.h:29-42
Timestamp: 2025-10-01T21:49:27.862Z
Learning: In Bun's IDL bindings (src/bun.js/bindings/BunIDLConvert.h), IDLStrictNull intentionally treats both undefined and null as null (using isUndefinedOrNull()), matching WebKit's IDLNull & IDLNullable behavior. This is the correct implementation and should not be changed to only accept null.

📚 Learning: 2025-10-18T05:23:24.403Z

Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/telemetry-server.test.ts:91-100
Timestamp: 2025-10-18T05:23:24.403Z
Learning: In the Bun codebase, telemetry tests (test/js/bun/telemetry-*.test.ts) should focus on telemetry API behavior: configure/disable/isEnabled, callback signatures and invocation, request ID correlation, and error handling. HTTP protocol behaviors like status code normalization (e.g., 200 with empty body → 204) should be tested in HTTP server tests (test/js/bun/http/), not in telemetry tests. Keep separation of concerns: telemetry tests verify the telemetry API contract; HTTP tests verify HTTP semantics.

Applied to files:

test/js/bun/http/proxy.test.ts

📚 Learning: 2025-10-19T04:55:33.099Z

Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: test/js/bun/http/node-telemetry.test.ts:27-203
Timestamp: 2025-10-19T04:55:33.099Z
Learning: In test/js/bun/http/node-telemetry.test.ts and the Bun.telemetry._node_binding API, after the architecture refactor, the _node_binding interface only contains two methods: handleIncomingRequest(req, res) and handleWriteHead(res, statusCode). The handleRequestFinish hook and other lifecycle hooks were removed during simplification. Both current methods are fully tested.

Applied to files:

test/js/bun/http/proxy.test.ts

📚 Learning: 2025-10-20T01:38:02.660Z

Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/BunFetchInstrumentation.ts:126-131
Timestamp: 2025-10-20T01:38:02.660Z
Learning: In BunFetchInstrumentation.ts, the force-restore to ORIGINAL_FETCH in the disable() method is intentionally kept (despite appearing unsafe) because it's required for proper test cleanup when instrumentation is repeatedly enabled/disabled. Without it, 15 distributed tracing and context propagation tests fail. Shimmer's unwrap() doesn't reliably restore the original fetch in Bun's globalThis context. The isBunOtelPatched safety check ensures the restore only happens when the current fetch is still ours, preventing clobbering of other tools' wrappers.

Applied to files:

test/js/bun/http/proxy.test.ts
src/bun.js/webcore/fetch.zig

📚 Learning: 2025-11-24T18:35:08.612Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/dev-server-tests.mdc:0-0
Timestamp: 2025-11-24T18:35:08.612Z
Learning: Applies to test/bake/**/*.test.ts : Use `dev.fetch()` for running HTTP requests in tests

Applied to files:

test/js/bun/http/proxy.test.ts

📚 Learning: 2025-11-24T18:36:59.706Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output

Applied to files:

test/js/bun/http/proxy.test.ts

📚 Learning: 2025-10-08T13:48:02.430Z

Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.

Applied to files:

test/js/bun/http/proxy.test.ts

📚 Learning: 2025-11-24T18:37:30.259Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:37:30.259Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Always use `await using` or `using` to ensure proper resource cleanup in tests for APIs like Bun.listen, Bun.connect, Bun.spawn, Bun.serve, etc

Applied to files:

test/js/bun/http/proxy.test.ts

📚 Learning: 2025-10-17T20:50:58.644Z

Learnt from: taylordotfish
Repo: oven-sh/bun PR: 23755
File: src/bun.js/api/bun/socket/Handlers.zig:154-159
Timestamp: 2025-10-17T20:50:58.644Z
Learning: In Bun socket configuration error messages (src/bun.js/api/bun/socket/Handlers.zig), use the user-facing JavaScript names "data" and "drain" instead of internal field names "onData" and "onWritable", as these are the names users see in the API according to SocketConfig.bindv2.ts.

Applied to files:

src/bun.js/webcore/fetch.zig

📚 Learning: 2025-11-24T18:35:39.205Z

Learnt from: CR
Repo: oven-sh/bun PR: 0
File: .cursor/rules/registering-bun-modules.mdc:0-0
Timestamp: 2025-11-24T18:35:39.205Z
Learning: Applies to src/bun.js/api/BunObject.zig : Implement getter functions in `src/bun.js/api/BunObject.zig` that return your feature, and export them in the `exportAll()` function

Applied to files:

src/bun.js/webcore/fetch.zig

🔇 Additional comments (3)

test/js/bun/http/proxy.test.ts (2)

503-515: Updated regression tests correctly cover ignored proxy-object cases

These two tests clearly lock in the new behavior that proxy objects without a url (missing or explicitly null) are treated as “no proxy” instead of throwing, which matches the regression report and desired API semantics. No issues from my side.

Also applies to: 517-528

706-722: URL-instance proxy regression is well covered

This test precisely captures the reported regression for proxy being a URL object (no url property). Verifying that the request succeeds without error is enough to guard against the previous breaking change; the implementation detail that the proxy is ignored is exercised via the Zig changes and other proxy tests.

src/bun.js/webcore/fetch.zig (1)

635-712: Proxy-object handling now matches intended “ignore when no usable url” semantics

The updated extract_proxy logic for object proxies looks solid:

Objects without a url property, or with url set to undefined/null, leave proxy unset so the request proceeds without a proxy (fixes the fetch in bun 1.3.4 may contains break changes #25413 regression and the proxy: new URL(...) case).

When proxy.url is a non-empty string, it’s parsed identically to the string-proxy path, including url_type updates, and invalid URLs still throw with fetch() proxy URL is invalid.

Non-string or empty-string proxy.url cleanly fail with fetch() proxy.url must be a non-empty string, preserving strictness when a value is provided.

Proxy headers are only parsed when a valid proxy URL is present, and proxy_headers ownership is correctly transferred to FetchTasklet (the local defer is neutralized by nulling proxy_headers before return).

Treating null/undefined proxy.url as “absent” is consistent with how optional fields are generally handled elsewhere in Bun. Based on learnings, this is aligned with the project’s null/undefined semantics.

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

Araxeus · 2025-12-08T15:17:51Z

I don't think this PR fixes the issue, i've tested it with bunx bun-pr 25414 and the error still occurs

bun-25414 check
ERROR: FetchError: request to https://api.github.com/repos/junegunn/fzf/contents/LICENSE?Authorization=bearer%20***&ref=0.38.0 failed, reason: fetch() proxy object requires a 'url' property:
error: script "check" exited with code 1

Jarred-Sumner · 2025-12-08T19:42:32Z

@Araxeus try running bun-25414 --bun check

Araxeus · 2025-12-08T19:46:02Z

it works now, my bad didn't know the --bun was so important sorry

github-actions bot added the claude label Dec 8, 2025

Araxeus mentioned this pull request Dec 8, 2025

bunx bun-pr failing with stderr: "bun: command not found: unzip\n" #25419

Open

Merge branch 'main' into claude/fix-fetch-proxy-url-object

f0db0fe

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(fetch): ignore proxy object without url property #25414

fix(fetch): ignore proxy object without url property #25414

robobun commented Dec 8, 2025

Uh oh!

robobun commented Dec 8, 2025 •

edited

Loading

Uh oh!

coderabbitai bot commented Dec 8, 2025

Uh oh!

Araxeus commented Dec 8, 2025

Uh oh!

Jarred-Sumner commented Dec 8, 2025

Uh oh!

Araxeus commented Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fix(fetch): ignore proxy object without url property #25414

Are you sure you want to change the base?

fix(fetch): ignore proxy object without url property #25414

Conversation

robobun commented Dec 8, 2025

Summary

Test plan

Uh oh!

robobun commented Dec 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coderabbitai bot commented Dec 8, 2025

Walkthrough

Changes

Possibly related PRs

Suggested reviewers

Pre-merge checks

Uh oh!

Araxeus commented Dec 8, 2025

Uh oh!

Jarred-Sumner commented Dec 8, 2025

Uh oh!

Araxeus commented Dec 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

robobun commented Dec 8, 2025 •

edited

Loading