fix(opencode): close webfetch SSRF redirect bypass and harden IP checks#9
Conversation
Ported from upstream anomalyco#36377 (security substance only).
|
Warning Review limit reached
Next review available in: 58 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughWebfetch adds SSRF validation for URLs and DNS results, manually validates redirects, retries Cloudflare challenges, enforces streaming response limits, and exports corrected HTML extraction logic with focused tests. ChangesWebfetch security and extraction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant WebFetchTool
participant URLValidation
participant WebServer
WebFetchTool->>URLValidation: validate initial URL
WebFetchTool->>WebServer: fetch with manual redirect handling
WebServer-->>WebFetchTool: response or redirect
WebFetchTool->>URLValidation: validate redirect target
WebFetchTool->>WebServer: follow validated redirect
WebServer-->>WebFetchTool: stream response body
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
@coderabbitai full review |
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 44 minutes. |
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/opencode/src/tool/webfetch.ts (1)
105-131: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAvoid leaking resolved private IP in the thrown error, and prefer a functional check over the manual loop.
record.address(the internal-network IP a hostname resolves to) is embedded directly in the error message, which can propagate back to an untrusted caller (e.g. an LLM agent relaying tool output) and aid network reconnaissance. Also, theforloop overrecordscan be replaced by.some()/.find()per the array-methods guideline.🔒 Proposed fix
const records = await dns.lookup(host, { all: true }) - for (const record of records) { - if (isPrivateIp(record.address)) { - throw new Error( - `Refusing to fetch host that resolves to a private address: ${host} -> ${record.address}. ${ALLOW_PRIVATE_HINT}`, - ) - } - } + if (records.some((record) => isPrivateIp(record.address))) { + throw new Error(`Refusing to fetch host that resolves to a private address: ${host}. ${ALLOW_PRIVATE_HINT}`) + }As per coding guidelines, "Prefer functional array methods over
forloops, and use type guards onfilterto preserve downstream inference."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/src/tool/webfetch.ts` around lines 105 - 131, Update assertPublicUrl to replace the manual records loop with a functional check such as some or find, while preserving rejection when any resolved address is private. Remove record.address from the thrown error message so it reports only the hostname and generic refusal context without exposing resolved IPs.Source: Coding guidelines
packages/opencode/test/tool/webfetch.test.ts (1)
124-162: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing regression test for the PR's core fix: redirect hop pointing to a private target.
The three new tests cover redirect-follow-to-public, redirect-limit, and initial-URL rejection, but none exercises a redirect chain where an intermediate hop's
locationpoints at a private/loopback address. That's the exact bypass this PR closes (public URL 302s to an internal address), so it deserves direct coverage rather than relying on the initial-URL check alone.✅ Suggested additional test
it.instance("rejects redirects to private targets when the SSRF guard is enabled", () => withFetch( (req) => { const path = new URL(req.url).pathname if (path === "/start") return new Response(null, { status: 302, headers: { location: "http://127.0.0.1/" } }) return new Response("landed", { status: 200 }) }, (url) => Effect.gen(function* () { const exit = yield* Effect.exit( exec({ url: new URL("/start", url).toString(), format: "text" }, { allowPrivate: false }), ) expect(exit._tag).toBe("Failure") }), ), )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/opencode/test/tool/webfetch.test.ts` around lines 124 - 162, Add a regression test alongside the existing redirect tests that starts from a public `/start` URL, returns a redirect whose `location` targets `http://127.0.0.1/`, and executes `exec` with `allowPrivate: false`. Assert via `Effect.exit` that the result is a failure, covering SSRF validation of redirect destinations rather than only the initial URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/opencode/src/tool/webfetch.ts`:
- Around line 170-182: Replace the current assertPublicUrl-only protection in
the web fetch flow with a transport-layer HttpClient/dispatcher that resolves
and validates the target, then pins and reuses the validated address for the
connection and redirect hops. Integrate this custom transport into the fetch
request path while preserving the allowPrivate bypass and existing error
handling.
---
Nitpick comments:
In `@packages/opencode/src/tool/webfetch.ts`:
- Around line 105-131: Update assertPublicUrl to replace the manual records loop
with a functional check such as some or find, while preserving rejection when
any resolved address is private. Remove record.address from the thrown error
message so it reports only the hostname and generic refusal context without
exposing resolved IPs.
In `@packages/opencode/test/tool/webfetch.test.ts`:
- Around line 124-162: Add a regression test alongside the existing redirect
tests that starts from a public `/start` URL, returns a redirect whose
`location` targets `http://127.0.0.1/`, and executes `exec` with `allowPrivate:
false`. Assert via `Effect.exit` that the result is a failure, covering SSRF
validation of redirect destinations rather than only the initial URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7e148c35-1f09-48c4-a800-573531583df9
📒 Files selected for processing (4)
packages/opencode/src/tool/webfetch.tspackages/opencode/test/tool/webfetch-extract.test.tspackages/opencode/test/tool/webfetch-ssrf.test.tspackages/opencode/test/tool/webfetch.test.ts
| // SSRF guard: block loopback / link-local / private targets. Redirect | ||
| // hops are re-validated in the fetch loop below. Remaining known gap: | ||
| // DNS rebinding (a hostname that resolves public here but private when | ||
| // the client re-resolves it) — closing it fully requires pinning the | ||
| // validated IP into the connection, which fetch does not expose. | ||
| const allowPrivate = yield* AllowPrivateFetch | ||
| if (!allowPrivate) { | ||
| yield* Effect.tryPromise({ | ||
| try: () => assertPublicUrl(params.url), | ||
| catch: (error) => (error instanceof Error ? error : new Error(String(error))), | ||
| }) | ||
| } | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
Does effect's FetchHttpClient or undici support pinning a resolved IP address (custom DNS lookup) per request to prevent DNS rebinding?
💡 Result:
Yes, it is possible to pin a resolved IP address in undici to prevent DNS rebinding [1][2][3]. While Effect's FetchHttpClient layer is designed for environments like Bun, browsers, or the global Node.js fetch [4], the underlying undici library (used by Node.js or when using NodeHttpClient) provides the necessary control via its Agent configuration [5][6]. To pin an IP address and close the DNS-rebinding time-of-check to time-of-use (TOCTOU) gap, you must provide a custom lookup function within the connect options of an undici Agent [1][7][3]. This hook allows you to perform your own DNS resolution (or use dns.lookup), validate the IP address (e.g., against a blocklist of private/internal ranges), and then return that validated IP so the socket connects directly to it, bypassing any further resolution [1][8]. Example implementation: import { Agent } from 'undici'; import dns from 'node:dns'; // Create a custom Agent with a pinned DNS lookup const safeAgent = new Agent({ connect: { lookup: (hostname, options, callback) => { // 1. Perform custom resolution dns.lookup(hostname, options, (err, address, family) => { if (err) return callback(err, address, family); // 2. Validate address (e.g., check for private IP ranges) if (isUnsafe(address)) { return callback(new Error('SSRF blocked'), address, family); } // 3. Return the validated, pinned address callback(null, address, family); }); }, }, }); // Use the agent for your request // const client = new Client(origin, { dispatcher: safeAgent }); // or // await fetch(url, { dispatcher: safeAgent }); Note that if you are using Effect, you should ensure you are utilizing the undici-compatible HTTP client (e.g., NodeHttpClient.layerUndici) rather than the generic FetchHttpClient layer, as the latter might rely on the global fetch environment where dispatcher injection may be restricted or have different compatibility characteristics [9][4][3]. Using undici directly allows you to control the socket-level connection details required for robust SSRF defense [1][7].
Citations:
- 1: https://uniauth.id/blog/dns-rebinding-safe-http-client
- 2: https://github.com/jeswr/guarded-fetch
- 3: 1111philo/plato@19929a1
- 4: https://gist.github.com/schickling/86e507a891932996066114db9e84e21b
- 5: https://github.com/nodejs/undici/blob/main/docs/docs/api/Client.md
- 6: Feature request: allow a custom dns lookup function to be supplied nodejs/undici#421
- 7: harden fetch_url against DNS rebinding (TOCTOU) yoloyash/overtchat#1
- 8: driftstackdev/driftstack-api@b1dd945
- 9: FetchHttpClient: OTLP protobuf exporter fails with "invalid content-length header" against undici 8 Effect-TS/effect-smol#2186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target file and nearby redirect/URL validation logic.
sed -n '1,320p' packages/opencode/src/tool/webfetch.ts
# Find the HTTP client implementation and any undici/custom-dispatcher usage.
rg -n "FetchHttpClient|NodeHttpClient|undici|dispatcher|lookup|assertPublicUrl|redirect" packages/opencode/src -SRepository: CreatorGhost/TheCode
Length of output: 24147
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant module shapes before reading specific slices.
ast-grep outline packages/opencode/src/tool/webfetch.ts --view expanded || true
ast-grep outline packages/opencode/src -g '*.ts' --view expanded | head -n 200 || trueRepository: CreatorGhost/TheCode
Length of output: 1171
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Probe whether a concrete dispatcher/lookup hook is available in the codebase.
rg -n "new Agent|lookup:|custom.*lookup|setGlobalDispatcher|Agent\(" packages/opencode/src -SRepository: CreatorGhost/TheCode
Length of output: 1874
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether the package uses Effect's FetchHttpClient or NodeHttpClient elsewhere.
rg -n "FetchHttpClient|NodeHttpClient|layerUndici|HttpClient" packages/opencode/src packages/opencode -g '*.ts' -SRepository: CreatorGhost/TheCode
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read package dependency info if available to infer runtime/fetch backend.
cat package.json 2>/dev/null || true
find packages/opencode -maxdepth 2 -name package.json -print -exec sed -n '1,220p' {} \;Repository: CreatorGhost/TheCode
Length of output: 11129
🌐 Web query:
Effect unstable/http FetchHttpClient.RequestInit dispatcher custom lookup undici pin IP
💡 Result:
To pin an IP address or use a custom DNS lookup in Node.js using Undici, you must provide a custom lookup function within the connect options of a dispatcher (such as Client, Pool, or Agent) [1][2]. The standard approach involves creating an instance of a dispatcher with a connect configuration that overrides the default DNS lookup behavior [2]. Example using Undici Client: import { Client } from 'undici'; import dns from 'node:dns'; const client = new Client('https://example.com', { connect: { lookup: (hostname, options, callback) => { // Pin to a specific IP address callback(null, [{ address: '1.2.3.4', family: 4 }]); } } }); In the context of the Effect framework (e.g., @effect/platform/FetchHttpClient), you can provide a custom dispatcher by configuring the underlying HTTP client [3]. Because FetchHttpClient relies on Undici for its implementation, you can instantiate an Agent or Client with the custom connect options shown above and pass it to your fetch requests or provide it via the Effect layer [1][3]. Note that the dispatcher option in fetch or RequestInit is used to override the default global dispatcher [4]. Ensure that when you use a custom dispatcher, it is configured correctly to handle the requirements of your specific application [5]. The lookup function signature follows the standard Node.js dns.lookup callback pattern [6][7].
Citations:
- 1: Feature request: allow a custom dns lookup function to be supplied nodejs/undici#421
- 2: https://github.com/nodejs/undici/blob/main/docs/docs/api/Client.md
- 3: https://foldkit.dev/core/commands
- 4: override request dispatcher from init nodejs/undici#2928
- 5: https://github.com/nodejs/undici/blob/main/docs/docs/api/Agent.md
- 6: https://undici.nodejs.org/api/Dispatcher
- 7: https://undici-docs.vramana.dev/docs/next/api/Dispatcher
🌐 Web query:
Bun fetch dispatcher lookup undici custom Agent DNS rebinding
💡 Result:
In Bun, using a custom Undici Agent to control DNS resolution—a common technique to prevent DNS rebinding—has historically faced compatibility issues because Bun's native fetch did not always fully respect custom dispatchers or their connect functions [1][2]. However, as of August 2025, Bun has implemented support for custom connect functions within the dispatcher option for fetch [3]. This improvement ensures that when you provide an Undici Agent (configured with a custom connect lookup) to the fetch dispatcher option, Bun correctly utilizes that logic to resolve hostnames [3]. To implement a DNS-rebinding-safe client in Bun, you can follow this pattern: 1. Perform your own DNS lookup for the target hostname [4]. 2. Validate the resolved IP address against your security policy (e.g., ensuring it is not a private or loopback address) [4]. 3. Create an Undici Agent with a custom connect.lookup function that returns your pre-validated IP address [4]. 4. Pass this Agent as the dispatcher option to your fetch request [4]. Example implementation: import { Agent } from "undici"; import dns from "dns/promises"; // 1. Resolve and validate IP manually const { address } = await dns.lookup("example.com"); // 2. Define custom dispatcher const dispatcher = new Agent({ connect: { lookup: (_hostname, _options, cb) => { // Force the agent to use the validated IP cb(null, address, 4); }, }, }); // 3. Use in fetch const response = await fetch("https://example.com", { dispatcher }); While Bun continues to improve its Undici compatibility to support libraries like fetch-socks or custom agents [5][2], always verify your specific Bun version if you encounter unexpected behavior, as some features were implemented recently as part of maintaining ecosystem compatibility [3][5].
Citations:
- 1: Bun's built-in fetch does not honor custom HTTPS agent with localAddress option oven-sh/bun#18157
- 2:
undici.Agent'sconnectoption goes unused, makingfetch-socksnot work oven-sh/bun#21492 - 3: fix: Implement custom connect function support in fetch and fix 21492 oven-sh/bun#21533
- 4: https://uniauth.id/blog/dns-rebinding-safe-http-client
- 5: feat(undici): implement Pool, Client, Agent, and stream() oven-sh/bun#27338
🌐 Web query:
"effect/unstable/http" "FetchHttpClient.RequestInit" RequestInit service dispatcher
💡 Result:
In Effect, FetchHttpClient is an implementation of the HttpClient service provided by the @effect/platform package [1][2]. The RequestInit configuration for FetchHttpClient allows you to customize the underlying Fetch API request settings [3][1]. To customize the FetchHttpClient, you provide a configuration object using the FetchHttpClient.RequestInit tag [1][2]. This is typically done by creating a Layer that provides this configuration to the runtime [1][2]. Example usage: import { FetchHttpClient, HttpClient } from "@effect/platform" import { Effect, Layer } from "effect" const CustomFetchLive = FetchHttpClient.layer.pipe( Layer.provide( Layer.succeed(FetchHttpClient.RequestInit, { credentials: "include", cache: "force-cache" // Example of customizing cache behavior [4] }))) const program = Effect.gen(function* { const client = yield* HttpClient.HttpClient const response = yield* client.get("https://jsonplaceholder.typicode.com/posts/1") const json = yield* response.json console.log(json) }).pipe( Effect.scoped, Effect.provide(CustomFetchLive)) Regarding a "service dispatcher," the HttpClient service in Effect functions as an abstraction over HTTP operations [5]. It does not typically use a "dispatcher" by that specific name in its public API; rather, it uses the Effect environment (Context) to provide the implementation (e.g., FetchHttpClient) and uses standard Effect piping and layering for dependency injection [1][2][5]. The HttpClient interface provides methods like get, post, put, and del, which are executed using the provided client implementation [5][6]. If you require custom dispatching or middleware logic, you can use HttpClient.mapRequest, HttpClient.mapRequestEffect, or create custom Layers that wrap or augment the base HttpClient implementation [5][6][7].
Citations:
- 1: https://npmx.dev/package/@effect/platform/v/%5E0.69.2
- 2: https://npmx.dev/package/@effect/platform/v/%5E0.68.0
- 3: https://github.com/Effect-TS/effect/blob/main/packages/platform/README.md
- 4: FetchHttpClient is preventing NextJS from caching responses Effect-TS/effect#4922
- 5: https://effect-ts.github.io/effect/platform/HttpClient.ts.html
- 6: https://github.com/Effect-TS/effect/blob/70ce155c/packages/platform/src/HttpClient.ts
- 7: https://github.com/Effect-TS/effect/blob/4708bb8e/packages/platform/src/HttpClient.ts
🌐 Web query:
site:effect.website FetchHttpClient RequestInit dispatcher undici
💡 Result:
In the context of the Effect ecosystem, particularly within @effect/platform-node, the undici library is utilized as a backend for HTTP requests [1]. When using FetchHttpClient, which serves as an implementation of the HttpClient interface, the underlying communication is handled by undici [1][2]. Regarding the use of dispatcher with FetchHttpClient and RequestInit: 1. Undici Dispatcher: The undici library uses a Dispatcher to manage HTTP requests. When FetchHttpClient is employed, it is typically configured via a layer (e.g., FetchHttpClient.layer or layerUndici) [3][4]. If you need to customize the undici client (such as providing a specific dispatcher), this is generally handled at the layer creation level when configuring the HttpClient service, rather than through individual RequestInit objects [3][2]. 2. RequestInit: The standard RequestInit interface (used by the native fetch API) does not natively support an undici-specific dispatcher property. RequestInit is intended for standard fetch options like method, headers, and body [5]. 3. Implementation Detail: Because FetchHttpClient is an Effect-based abstraction, configuration that requires underlying engine details—like a custom undici.Dispatcher—is managed through Effect's dependency injection system (Layers) [3][2]. You should configure the HttpClient layer with your required undici settings (like a custom Agent or Dispatcher) when providing the layer to your application, ensuring that all requests made through the injected HttpClient utilize the desired configuration [3][2]. If you are encountering issues where a global undici dispatcher is not being picked up or needs to be explicitly defined, verify that your layer configuration in the Effect program correctly injects the HttpClient with the undici client configured to your specifications [6][4].
Citations:
- 1: https://effect.website/blog/this-week-in-effect/2024/03/29/
- 2: https://effect.website/docs/ai/tool-use/
- 3: https://effect.website/blog/releases/effect/38/
- 4: https://effect.website/blog/effect-ai/
- 5: https://effect.website/docs/batching/
- 6: https://effect.website/blog/this-week-in-effect/2024/08/09/
Close the DNS-rebinding gap at the transport layer. assertPublicUrl() only validates one DNS answer before the request; the fetch path can still re-resolve for the actual connection and each redirect hop. FetchHttpClient.RequestInit doesn’t expose per-request IP pinning, so this needs a custom HttpClient/dispatcher layer that reuses the validated address for the socket.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/opencode/src/tool/webfetch.ts` around lines 170 - 182, Replace the
current assertPublicUrl-only protection in the web fetch flow with a
transport-layer HttpClient/dispatcher that resolves and validates the target,
then pins and reuses the validated address for the connection and redirect hops.
Integrate this custom transport into the fetch request path while preserving the
allowPrivate bypass and existing error handling.
- Replace manual for loop with records.some() in assertPublicUrl - Remove resolved IP from error message to prevent info leak - Add regression test for redirect-to-private-target SSRF bypass
CreatorGhost
force-pushed
the
webfetch-ssrf
branch
from
0bfba0b to
ec81ae4
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Selective port of upstream anomalyco/opencode#36377.
What's ported
isPrivateIp: full IPv4 reserved ranges (CGN, TEST-NETs, benchmarking, multicast, broadcast), uncompressed/mixed-notation IPv6 loopback, fe80::/10, mapped-IPv4 forms.OPENCODE_WEBFETCH_ALLOW_PRIVATE=1escape hatch for intentional internal fetches.webfetch-ssrf.test.ts,webfetch-extract.test.ts.Deviations from upstream PR
.opencode/GitHub triage tooling (upstream repo automation, not relevant to this fork).permission.ts/wildcard.tsdoc-comment-only changes and thesemantics.test.tsthat documents an older permission API our tree no longer exports.Verified: 76 tests pass across the three webfetch suites;
bun typecheckclean in packages/opencode.Summary by CodeRabbit
Security
Reliability
Tests