Skip to content

node-fetch: report Response.type as default for an HTTP error status - #43966

Open
breken-ai wants to merge 1 commit into
oven-sh:mainfrom
breken-ai:fix/node-fetch-response-type
Open

breken-ai wants to merge 1 commit into
oven-sh:mainfrom
breken-ai:fix/node-fetch-response-type

Conversation

@breken-ai

Copy link
Copy Markdown

What does this PR do?

The builtin node-fetch shim's Response.type returned "error" for any response that is not ok. So a fetched 404 or 500, or new Response(body, { status: 404 }), reported "error". In node-fetch every Response starts as "default", whatever its status. Only Response.error() sets "error" (src/response.js#L38-L39, #L121-L125). Code that uses res.type === "error" to spot a network failure treated every HTTP error status as one.

The fix is the one line in the type getter in src/js/thirdparty/node-fetch.ts: return super.type === "error" ? "error" : "default";. A response that is natively "error" stays "error". Everything else is "default", as in node-fetch. This also covers the case where fetched responses get a spec type such as "basic" from the native side.

import fetch, { Response } from "node-fetch";
const res = await fetch(urlThatReturns404);
console.log(res.status, res.type, new Response("x", { status: 500 }).type);
// node-fetch 3.3.2 on Node 22: 404 default default
// Bun 1.4.2:                   404 error error

How did you verify your code works?

New test in test/js/node/http/node-fetch.test.js: "node-fetch Response.type is default for an HTTP error status". It checks constructed 404/500/200 responses, a fetched 404 from a local Bun.serve({ port: 0 }), and a response that is natively "error". If the getter always returns "default", the test fails on that last case.

Red on Bun 1.4.2:

Expected: "default"
Received: "error"
(fail) node-fetch Response.type is default for an HTTP error status

I did not build Bun locally (no bun bd on this machine), so the green half was checked by loading the patched module into Bun 1.4.2:

  • Red on the release build. USE_SYSTEM_BUN=1 bun test test/js/node/http/node-fetch.test.js -t "<new test>" with Bun 1.4.2 (744846f), whose builtin node-fetch is the same as main at 29d9638 for these lines: fails with the output below.
  • Green on the patched source. A small script turns src/js/thirdparty/node-fetch.ts into a user module and changes only the builtin-only syntax ($cpp(...) → the global Response/Request/Blob/Headers/FormData/File, .$call → .call, $isJSArray → Array.isArray, new $Buffer → Buffer.from, Promise.$resolve → Promise.resolve, and a Readable.fromWeb stand-in for _ReadableFromWeb). A --preload then mock.module("node-fetch", ...)s it, and the same test file runs against it. The new test fails on the transformed main source and passes on the transformed patched source. The rest of the file gives the same results before and after the patch. The 12 body-stream tests that fail under the stand-in fail the same way in both runs, because the stand-in is not _ReadableFromWeb.
  • The debug build in CI is the real green check.
  • Reference: node-fetch 3.3.2 under Node 22.14.0, output below.
  • prettier --check and oxlint --config=oxlint.json pass on the changed files. tsc --noEmit -p src/js gives no new errors in node-fetch.ts.

🤖 Generated with Claude Code

node-fetch starts every Response as "default" and only Response.error()
sets "error". The shim returned "error" for any response that is not ok,
so a fetched 404 or 500 looked like a network error.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@claude claude Bot left a comment

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.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@coderabbitai

coderabbitai Bot commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Review in Change Stack →

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

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: Advanced

Run ID: e6c9c14e-1c47-4c1f-ad3a-19f227bca5cf

📥 Commits

Reviewing files that changed from the base of the PR and between 29d9638 and 681cb38.

📒 Files selected for processing (2)
  • src/js/thirdparty/node-fetch.ts
  • test/js/node/http/node-fetch.test.js

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


Walkthrough

Response.type now reports error only when the underlying response type is error. Tests cover constructed responses, a native error response, and a fetched 404 response.

Changes

Response Type Reporting

Layer / File(s) Summary
Response type getter and validation
src/js/thirdparty/node-fetch.ts, test/js/node/http/node-fetch.test.js
The getter returns error only when the underlying response type is error. Tests check constructed responses with statuses 404, 500, and the default status, a native error response, and a fetched 404.

Suggested reviewers: robobun

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 681cb

The change reports the intended response types, with tests covering non-OK HTTP responses and native error responses. No material merge-readiness risk is evident.

Security Architecture Review

Security architecture risk: 🔵 Low · up to 681cb

HTTP error responses will now report “default” rather than “error.” The reviewed change does not show a bypass of a security control, but applications outside this repository may rely on the previous value.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The observable change can reach applications using the public Response.type property. The resolved in-repository downstream use is the test; behavior of external consumers is not established.

Trust Boundaries and Controls

  • inferred — No authorization, credential, or other sensitive operation is present in the changed getter. Repository evidence does not establish whether an external caller uses this classification as a security control.
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the primary change: reporting Response.type as "default" for HTTP error statuses.
Description check ✅ Passed The description includes both required sections. It explains the behavior change, implementation, expected behavior, test coverage, verification results, and remaining test limitations.
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.

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

breken-ai added a commit to breken-ai/bun that referenced this pull request Sep 25, 2026
…onse

Response.redirect() now returns a node-fetch Response, and the old type
getter reported "error" for any status that is not ok, so a 301 from
Response.redirect() reported "error". Take the getter from oven-sh#43966: only a
response that is natively "error" reports "error", as in node-fetch.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant