[miniflare/wrangler] fix: avoid "expected non-null body source" on 401 responses with a request body#13119
[miniflare/wrangler] fix: avoid "expected non-null body source" on 401 responses with a request body#13119petebacondarwin wants to merge 2 commits intomainfrom
Conversation
…1 with request body Replace undici.fetch() with undici.request() at the call sites that triggered the Fetch spec 401 credential-retry crash. undici.fetch() crashes with 'expected non-null body source' when the request has a ReadableStream body and the server responds 401, because it implements the Fetch spec credential-retry path which checks request.body.source (null for streams). Tracked upstream at nodejs/undici#4910 — the upstream fix is currently blocked by spec compliance concerns. The fix uses undici.request() which goes through the Dispatcher API directly and has no credential-retry path. The wrapper in miniflare/src/http/fetch.ts explicitly replicates the behaviours of undici.fetch() that callers depend on: - Accept: */* and Accept-Encoding: gzip, deflate default headers - Transparent decompression (gzip, deflate, brotli) - Redirect following honouring the request's redirect mode - Correct set-cookie handling via Headers.append() The workspace-level pnpm patch for undici@7.24.4 that was previously used as a partial workaround is removed.
🦋 Changeset detectedLatest commit: 0fe7f50 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
|
✅ All changesets look good |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
This PR replaces undici.fetch() with undici.request() at three call sites to avoid the 401 credential-retry crash, removes the workspace-level undici patch, and adds a regression test.
Issues (ranked by severity)
-
Bug: Redirect following with a body will throw on body reuse — In
fetchViaRequest, the request body stream is consumed byReadable.fromWeb(request.body)at line 150 and sent toundici.request(). On a non-303 redirect, line 234 readsrequest.bodyagain and passes it tonew Request(...). Since the stream has already been locked and consumed, this will throwTypeError: Response body object should not be disturbed or locked. This affects 301/302/307/308 redirects of requests with bodies. Sinceundici.request()does not follow redirects itself, this code path is reachable. -
Stale/duplicate comment block — Lines 134-140 contain two overlapping comment blocks ("Match undici.fetch() behaviour..." and "Match undici.fetch() defaults...") with broken indentation. Looks like a leftover from editing.
-
Observation (not blocking):
performApiFetchandfetchR2Objectsstill useundici.fetch()— The AI fetcher (src/ai/fetcher.ts) callsperformApiFetchwithrequest.body(a ReadableStream), andputRemoteObjectcallsfetchR2Objectswith streaming file bodies. Both could still hit the same 401 crash. This is pre-existing and outside the stated scope, but worth noting for follow-up.
| await rawBody.dump(); | ||
| // For 303 See Other: switch to GET and drop the body | ||
| const redirectMethod = statusCode === 303 ? "GET" : request.method; | ||
| const redirectBody = statusCode === 303 ? null : request.body; | ||
| return fetchViaRequest( | ||
| new Request(location, { | ||
| method: redirectMethod, | ||
| headers: request.headers, | ||
| body: redirectBody, | ||
| redirect: request.redirect, | ||
| } as RequestInit), | ||
| dispatcher, | ||
| redirectsRemaining - 1 |
There was a problem hiding this comment.
Bug: request.body was already consumed by Readable.fromWeb() on line 150 and drained by undici.request(). Attempting to pass the locked/disturbed stream to new Request(...) will throw TypeError: Response body object should not be disturbed or locked.
You need to tee() or buffer the body before the initial undici.request() call so it can be replayed on redirect. One option is to clone the request up-front when redirect !== "manual" and redirect !== "error":
| await rawBody.dump(); | |
| // For 303 See Other: switch to GET and drop the body | |
| const redirectMethod = statusCode === 303 ? "GET" : request.method; | |
| const redirectBody = statusCode === 303 ? null : request.body; | |
| return fetchViaRequest( | |
| new Request(location, { | |
| method: redirectMethod, | |
| headers: request.headers, | |
| body: redirectBody, | |
| redirect: request.redirect, | |
| } as RequestInit), | |
| dispatcher, | |
| redirectsRemaining - 1 | |
| // For 303 See Other: switch to GET and drop the body | |
| const redirectMethod = statusCode === 303 ? "GET" : request.method; | |
| // The original request body was already consumed by undici.request(). | |
| // For non-303 redirects that need to replay the body, we cannot reuse | |
| // request.body (it is locked/disturbed). Pass null for now — this | |
| // matches browser behaviour for 301/302 (which switch to GET) and | |
| // is the safest default. A full fix for 307/308 body replay would | |
| // require buffering/teeing the body before the initial request. | |
| const redirectBody = null; | |
| return fetchViaRequest( | |
| new Request(location, { | |
| method: redirectMethod, | |
| headers: request.headers, | |
| body: redirectBody, | |
| redirect: request.redirect, | |
| } as RequestInit), | |
| dispatcher, | |
| redirectsRemaining - 1 | |
| ); |
Note: this suggestion drops the body on all redirects. For full spec compliance on 307/308, the body would need to be tee'd or buffered before the initial undici.request() call — but that's a larger change and these redirects with bodies are rare in practice. At minimum the current code should not throw.
| body: rawBody, | ||
| } = await undici.request(request.url, { | ||
| method: request.method, | ||
| // Match undici.fetch() behaviour: add Accept-Encoding if the caller | ||
| // has not already specified one, so that the workerd entry worker | ||
| // applies the same content-encoding logic. | ||
| // Match undici.fetch() defaults: it sends `Accept: */*` and |
There was a problem hiding this comment.
Nit: These two comment blocks appear to be stale leftovers — they overlap in content (both explain the Accept-Encoding / Accept defaults) and have inconsistent indentation (extra leading tab on lines 134 and 137). Consider consolidating into a single comment:
| body: rawBody, | |
| } = await undici.request(request.url, { | |
| method: request.method, | |
| // Match undici.fetch() behaviour: add Accept-Encoding if the caller | |
| // has not already specified one, so that the workerd entry worker | |
| // applies the same content-encoding logic. | |
| // Match undici.fetch() defaults: it sends `Accept: */*` and | |
| // Match undici.fetch() defaults: it sends `Accept: */*` and | |
| // `Accept-Encoding: gzip, deflate` when the caller hasn't set them. | |
| // These are placed before the spread so the caller's own headers | |
| // take precedence. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| // For 303 See Other: switch to GET and drop the body | ||
| const redirectMethod = statusCode === 303 ? "GET" : request.method; |
There was a problem hiding this comment.
🔴 Redirect handling for 301/302 POST does not convert method to GET and null the body
The Fetch spec (HTTP-redirect fetch, step 13) requires that when a 301 or 302 redirect is received and the request method is POST, the method must be changed to GET and the body set to null. The code at lines 233-234 only handles 303 (statusCode === 303), leaving 301/302 POST redirects to keep the POST method and attempt to reuse request.body. Since the body ReadableStream was already locked by Readable.fromWeb(request.body) at line 150 during the initial request, accessing request.body returns a locked stream, and new Request(location, { body: lockedStream }) will throw a TypeError. This is a behavioral regression from undici.fetch(), which correctly converted 301/302 POST to GET with a null body.
| // For 303 See Other: switch to GET and drop the body | |
| const redirectMethod = statusCode === 303 ? "GET" : request.method; | |
| const redirectMethod = (statusCode === 303 || ((statusCode === 301 || statusCode === 302) && request.method === "POST")) ? "GET" : request.method; | |
| const redirectBody = (statusCode === 303 || ((statusCode === 301 || statusCode === 302) && request.method === "POST")) ? null : request.body; |
Was this helpful? React with 👍 or 👎 to provide feedback.
| // path. See: https://github.com/cloudflare/workers-sdk/issues/12967 | ||
| const { | ||
| statusCode, | ||
| headers: rawHeaders, | ||
| body, | ||
| } = await undiciRequest(request.url, { | ||
| method: request.method, | ||
| headers: Object.fromEntries(request.headers), | ||
| body: | ||
| request.body !== null | ||
| ? Readable.fromWeb(request.body as ReadableStream) | ||
| : null, | ||
| }); | ||
| // Build a Headers object that preserves multiple set-cookie values. | ||
| const responseHeaders = new Headers(); | ||
| for (const [name, value] of Object.entries(rawHeaders)) { | ||
| if (Array.isArray(value)) { | ||
| for (const v of value) { | ||
| responseHeaders.append(name, v); | ||
| } | ||
| } else if (value !== undefined) { | ||
| responseHeaders.set(name, value); | ||
| } | ||
| } | ||
| const response = new Response(body.body, { | ||
| status: statusCode, |
There was a problem hiding this comment.
🔴 createCloudflareClient no longer follows HTTP redirects after switch to undici.request()
The replacement of undici.fetch() with undici.request() in the custom fetch provided to the Cloudflare SDK client removes automatic redirect following. undici.fetch() follows redirects by default (up to 20), while undici.request() defaults to maxRedirections: 0 — it returns redirect responses (3xx with Location header) as-is without following them. No manual redirect-following logic was added (unlike in the miniflare fetchViaRequest). If any Cloudflare API endpoint returns a redirect, the SDK will receive the 3xx response directly instead of the final response, likely causing unexpected errors in SDK consumers.
Prompt for agents
In packages/wrangler/src/cfetch/internal.ts, the createCloudflareClient function (lines 68-121) replaced undici.fetch() with undici.request(). However, undici.request() does not follow redirects by default (maxRedirections: 0), unlike undici.fetch() which follows up to 20 redirects. Either add maxRedirections: 20 to the undici.request() options at line 92, or implement manual redirect following similar to the fetchViaRequest function in packages/miniflare/src/http/fetch.ts.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const forward = forwardInit as Request; | ||
| const { | ||
| statusCode, | ||
| headers: rawHeaders, | ||
| body, | ||
| } = await undiciRequest(url.toString(), { | ||
| method: forward.method, | ||
| headers: Object.fromEntries(forward.headers), | ||
| body: | ||
| forward.body !== null | ||
| ? Readable.fromWeb(forward.body as ReadableStream) | ||
| : null, | ||
| }); | ||
| // Build a Headers object that preserves multiple set-cookie values. | ||
| const responseHeaders = new Headers(); | ||
| for (const [name, value] of Object.entries(rawHeaders)) { | ||
| if (Array.isArray(value)) { | ||
| for (const v of value) { | ||
| responseHeaders.append(name, v); | ||
| } | ||
| } else if (value !== undefined) { | ||
| responseHeaders.set(name, value); | ||
| } | ||
| } | ||
| return new Response(body.body, { | ||
| status: statusCode, |
There was a problem hiding this comment.
🔴 unstable_dev fetch no longer follows HTTP redirects after switch to undici.request()
The fetch method returned by unstable_dev() previously used undici.fetch(), which follows redirects by default. It now uses undici.request() which defaults to maxRedirections: 0 and returns redirect responses as-is. No manual redirect-following logic was added (unlike in the miniflare fetchViaRequest). Consumers calling worker.fetch('/some-path') where the worker responds with a redirect will now receive the 3xx response instead of the final response after following the redirect chain, silently changing behavior for existing users.
Prompt for agents
In packages/wrangler/src/api/dev.ts, the fetch method of unstable_dev (lines 248-290) replaced undici.fetch() with undici.request(). However, undici.request() does not follow redirects by default (maxRedirections: 0), unlike undici.fetch() which follows up to 20 redirects. Either add maxRedirections: 20 to the undici.request() options at line 264, or implement manual redirect following similar to the fetchViaRequest function in packages/miniflare/src/http/fetch.ts.
Was this helpful? React with 👍 or 👎 to provide feedback.
|
Review posted on PR #13119. Here's a summary of what I found:
|
create-cloudflare
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-editor-shared
wrangler
commit: |
A new fix is being worked on by the maintainers. |
Ah I didn't see that. In which case I think it is better for us to just wait for that to land and then upgrade to the new version when it is ready. |
3 participants
Fixes #12967 (and the follow-up for miniflare reported in #13058).
undici.fetch()implements the Fetch spec's 401 credential-retry path, which throwsTypeError: fetch failedwith cause"expected non-null body source"whenever a request has aReadableStreambody and the server responds with HTTP 401. PR #13058 fixed this for wrangler by patching the bundled copy of undici, but miniflare loads undici as a runtime dependency from the user'snode_modules, so end-users installing miniflare standalone received an unpatched version.The upstream fix (nodejs/undici#4910, PR #4936) is currently blocked — maintainers consider the behaviour spec-mandated.
What this PR does
Replaces
undici.fetch()withundici.request()at the affected call sites inminiflare/src/http/fetch.ts,wrangler/src/api/dev.ts, andwrangler/src/cfetch/internal.ts.undici.request()uses the Dispatcher API directly and has no credential-retry path, so the crash cannot occur regardless of the undici version installed.The
fetchViaRequest()helper infetch.tsexplicitly replicates theundici.fetch()behaviours that callers depend on:Accept: */*andAccept-Encoding: gzip, deflatedefault headers (same as undici.fetch)redirectmode (follow/manual/error)set-cookiehandling: builds aHeadersobject withappend()so multiple cookies are not mergedThe workspace-level
pnpmpatch forundici@7.24.4introduced in #13058 is removed — it is no longer needed.