Configure Next.js global fetch to use optional outbound proxy - #624
Configure Next.js global fetch to use optional outbound proxy#624jbeard4 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
The PR introduces startup-time networking behavior changes without sufficient safety guards and with a likely runtime compatibility regression, so it is not safe to approve yet.
Blocking issues
- Runtime compatibility risk:
undici@7.22.0(added inpackage.json/package-lock.json) requires Node>=20.18.1, but the repo does not declare/enforce this runtime inpackage.jsonengines or runtime config. On environments still on Node 18 or older Node 20 minors, importing/using this dependency frominstrumentation.tscan break server startup. - Availability regression:
instrumentation.tscallssetGlobalDispatcher(new ProxyAgent(proxyUrl))during app initialization without validation or error handling. A malformedPROXIMO_URL/HTTPS_PROXY/HTTP_PROXYcan throw at boot and take the app down instead of failing open. - Missing tests for risky global behavior: there are no tests covering proxy initialization paths (no proxy set, valid proxy, malformed proxy, non-Node runtime). Given this changes global
fetchbehavior process-wide, test coverage is required before approval.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e23b2eebcf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @@ -0,0 +1,19 @@ | |||
| import { ProxyAgent, setGlobalDispatcher } from "undici"; | |||
There was a problem hiding this comment.
Defer undici import until Node runtime is confirmed
The NEXT_RUNTIME guard in register() does not protect edge execution because undici is imported eagerly at module load time. In Next.js instrumentation, this file is evaluated before that guard runs, so any edge runtime context (for example, adding middleware or an edge route) can fail while resolving this Node-oriented dependency, causing build/startup errors before line 8 is reached. Load undici inside the nodejs branch (e.g., dynamic import) so non-Node runtimes never evaluate it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
The PR introduces global fetch proxying in production runtime without sufficient safety checks/tests, and it also hard-pins the project engine to Node 24.x, which is a high-risk compatibility change.
Blocking issues
instrumentation.tssets a global Undici dispatcher for all Node.jsfetchcalls when proxy env vars are present, but there is no handling for bypass rules (e.g.NO_PROXY) or internal destinations. This can unintentionally route internal/authenticated traffic through the proxy and cause behavioral/security regressions.instrumentation.tsdoes not validate or safely handle invalid proxy URLs before constructingnew ProxyAgent(proxyUrl). A malformed env value can throw during startup/instrumentation and break application boot.package.jsonadds"engines": { "node": "24.x" }with no accompanying compatibility evidence/tests. This is a broad runtime constraint change that can break CI/deploy environments still on Node 20/22.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d008d015e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "@types/react-dom": "19.1.9" | ||
| }, | ||
| "engines": { | ||
| "node": "24.x" |
There was a problem hiding this comment.
Broaden Node engine range to avoid install failures
Setting engines.node to 24.x unintentionally drops support for the Node versions this repo currently uses elsewhere (for example, .github/workflows/check.yaml is pinned to 20.x), and any environment that enforces engines (engine-strict, some CI/deploy setups) will fail npm install with EBADENGINE before tests or builds run. This commit’s runtime proxy change does not require a hard Node 24 pin, so the new constraint can block contributors and deployments on otherwise compatible Node versions.
Useful? React with 👍 / 👎.
Motivation
fetch()(Undici) does not honorHTTP_PROXY/HTTPS_PROXYautomatically which causes connection timeouts when direct egress is blocked in production (e.g. Heroku + Proximo).Description
instrumentation.tsstartup hook that readsPROXIMO_URL,HTTPS_PROXY, orHTTP_PROXYand, when set, configures Undici withnew ProxyAgent(proxyUrl)andsetGlobalDispatcher(...)so all server fetches use the proxy; the hook no-ops if no proxy is present or when not running in Node.js server runtime.undicitodependenciessoProxyAgent/setGlobalDispatcherare available at runtime.instrumentation.tswhich is the recommended centralized startup/agent hook for Next.js server runtime and runs once at server startup.Testing
undicivianpm install undiciwhich updatedpackage.jsonandpackage-lock.jsonsuccessfully.npm run check-types(tsc --noEmit) completed successfully with no new type errors.npm run lint(next lint) completed successfully and reported only pre-existing warnings unrelated to this change.Codex Task