fix(runtime): avoid infinite recursion in fetch for external URLs#4153
fix(runtime): avoid infinite recursion in fetch for external URLs#4153pi0 merged 1 commit intonitrojs:mainfrom
Conversation
The exported `fetch` function in `src/runtime/internal/app.ts` shadows the global `fetch`, causing it to recursively call itself when handling non-root-relative URLs (e.g., full URLs or URL objects). This results in a `RangeError: Maximum call stack size exceeded`. Fix: use `globalThis.fetch` explicitly for external requests. Added regression test with URL object in test fixture.
|
Someone is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe fetch function in the runtime now delegates to the global fetch implementation instead of recursively calling itself, preventing re-entry loops. A new test case verifies fetch functionality with URL objects. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/fixture/server/routes/fetch.ts (1)
13-15: Add an absolute-string URL regression case to match the reported repro.Nice coverage for
URLobjects; consider adding a"http://localhost/..."string case too, since that was also part of the stack-overflow repro path.Suggested diff
"nitro/runtime.fetch.url": await runtimeFetch(new URL("http://localhost/api/hello")).then( (res) => res.json() ), + "nitro/runtime.fetch.absoluteString": await runtimeFetch("http://localhost/api/hello").then( + (res) => res.json() + ),As per coding guidelines:
test/**/*.{ts,js}: Keep tests deterministic and environment-independent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/fixture/server/routes/fetch.ts` around lines 13 - 15, Add a deterministic regression test using an absolute-string URL alongside the existing URL object case: update the test to call runtimeFetch with the string "http://localhost/api/hello" (e.g., add another map entry similar to "nitro/runtime.fetch.url" but for the string case) and await its .then(res => res.json()) so the suite covers both new URL(...) and plain "http://localhost/..." call paths; locate the call to runtimeFetch in the test (the existing "nitro/runtime.fetch.url" entry) and add the analogous string-based entry there.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/fixture/server/routes/fetch.ts`:
- Around line 13-15: Add a deterministic regression test using an
absolute-string URL alongside the existing URL object case: update the test to
call runtimeFetch with the string "http://localhost/api/hello" (e.g., add
another map entry similar to "nitro/runtime.fetch.url" but for the string case)
and await its .then(res => res.json()) so the suite covers both new URL(...) and
plain "http://localhost/..." call paths; locate the call to runtimeFetch in the
test (the existing "nitro/runtime.fetch.url" entry) and add the analogous
string-based entry there.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a877b280-87bb-4694-b1d5-2fc95e419451
📒 Files selected for processing (2)
src/runtime/internal/app.tstest/fixture/server/routes/fetch.ts
commit: |
Description
The exported
fetchfunction insrc/runtime/internal/app.tsshadows the globalfetch, causing it to recursively call itself when handling non-root-relative URLs (e.g., full URL strings,URLobjects, orRequestobjects for external endpoints). This results in aRangeError: Maximum call stack size exceeded.Root Cause
In the
fetch()function, the fallback branch for external URLs callsfetch(resource, init)— which resolves to the local function itself rather than the nativeglobalThis.fetch:Fix
Explicitly use
globalThis.fetchto delegate external requests to the runtime's native fetch implementation:Changes
src/runtime/internal/app.ts: Changedfetch(resource, init)→globalThis.fetch(resource, init)on the external URL fallback path.test/fixture/server/routes/fetch.ts: Added a regression test case using aURLobject to verify external fetch does not recurse.How to reproduce
Call
fetch(new URL('https://example.com'))orfetch('https://example.com')from any Nitro route handler — it crashes with a stack overflow.Linked Issues
N/A (discovered during code review)