feat(web-fetch): add ssrfPolicy.allowCidrs for custom CIDR allowlisting#28657
feat(web-fetch): add ssrfPolicy.allowCidrs for custom CIDR allowlisting#28657xiaoyaner0201 wants to merge 3 commits intoopenclaw:mainfrom
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0718852bd4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| } | ||
|
|
||
| const allowPrivateNetwork = resolveAllowPrivateNetwork(params.policy); | ||
| const parsedCidrs = parseCidrStrings(params.policy?.allowCidrs); |
There was a problem hiding this comment.
Preserve legacy RFC2544 opt-in compatibility
resolvePinnedHostnameWithPolicy now derives the SSRF override only from params.policy?.allowCidrs, so existing callers that still pass allowRfc2544BenchmarkRange lose their exemption and 198.18.0.0/15 becomes blocked again. I verified this still exists in-tree (for example src/telegram/bot/delivery.ts uses allowRfc2544BenchmarkRange: true), which means fake-IP proxy deployments can regress back to blocked media/download fetches even after this change.
Useful? React with 👍 / 👎.
| if (base.kind() !== "ipv4") { | ||
| continue; |
There was a problem hiding this comment.
Honor allowCidrs for IPv6 ranges
This loop ignores every non-IPv4 CIDR, but the new policy parsing/type path accepts IPv6 CIDRs as valid inputs, so entries like fd00::/8 are silently ignored and IPv6 special-use addresses remain unconditionally blocked. That makes the new allowCidrs option inconsistent for IPv6-only/internal-network environments because user-provided config appears valid but has no effect.
Useful? React with 👍 / 👎.
Greptile SummaryAdded flexible CIDR-based allowlisting for SSRF protection to fix Key changes:
Architecture:
Validation:
Confidence Score: 5/5
Last reviewed commit: 0718852 |
|
This pull request has been automatically marked as stale due to inactivity. |
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c62411cb40
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| url: params.url, | ||
| maxRedirects: params.maxRedirects, | ||
| timeoutSeconds: params.timeoutSeconds, | ||
| policy: params.ssrfPolicy, |
There was a problem hiding this comment.
Block Firecrawl fallback for allowlisted private targets
Passing params.ssrfPolicy into fetchWithWebToolsNetworkGuard lets web_fetch reach private/internal URLs when allowCidrs (or allowPrivateNetwork) is configured, but the existing fallback path still calls maybeFetchFirecrawlWebFetchPayload on fetch errors/non-OK responses and forwards the same URL to Firecrawl (runWebFetch in this file). In environments where Firecrawl is enabled, this newly allows internal hostnames/paths/query strings to be sent to an external service, which is a security/privacy regression compared to prior behavior where those targets were blocked before any fallback.
Useful? React with 👍 / 👎.
Add tools.web.fetch.ssrfPolicy config with: - allowCidrs: string[] — CIDR ranges to allow through SSRF protection (e.g. '198.18.0.0/15' for fake-IP proxies, '10.0.0.0/8' for internal nets) This is a generic alternative to the specific allowRfc2544BenchmarkRange approach. CIDR strings are parsed once per request with ipaddr.parseCIDR; invalid CIDRs are warned and skipped. IPv6 CIDRs are gracefully handled when checking IPv4 addresses. Changes: - Config: ssrfPolicy.allowCidrs in zod schema, types, labels, help - Runtime: thread allowCidrs from config → resolveSsrfPolicy → runWebFetch → fetchWithSsrFGuard → resolvePinnedHostnameWithPolicy - SSRF: parseCidrStrings() helper, updated type signatures throughout - IP: isBlockedSpecialUseIpv4Address checks allowCidrs before blocked ranges All defaults remain restrictive. dangerouslyAllowPrivateNetwork remains as the nuclear option for full private network bypass. Closes openclaw#28271, closes openclaw#26945. Related: openclaw#28349, openclaw#25229.
…owRfc2544BenchmarkRange
Tests were still referencing the old allowRfc2544BenchmarkRange property
which was replaced by the generic allowCidrs array. Updated both
ssrf.test.ts and ssrf.pinning.test.ts to use the new API with
ipaddr.parseCIDR('198.18.0.0/15') for equivalent RFC2544 allowlisting.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd321ac097
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const [base, prefix] = cidr; | ||
| if (base.kind() !== "ipv4") { |
There was a problem hiding this comment.
Guard allowCidrs entries before dereferencing CIDR base
This loop assumes each allowCidrs entry is a parsed tuple and immediately calls base.kind(), but the exported policy shape uses string CIDRs; if a caller forwards SsrFPolicy.allowCidrs directly to isPrivateIpAddress/isBlockedHostnameOrIp (both exported through plugin-sdk), base becomes a string and this throws TypeError: base.kind is not a function instead of applying SSRF checks. That creates a runtime crash path for JS/untyped integrations using the new allowCidrs field.
Useful? React with 👍 / 👎.
|
This pull request has been automatically marked as stale due to inactivity. |
Real-world use case: Telegram channel media download blocked by SSRF behind private proxyJust confirming this PR is still very much needed, and adding a concrete real-world scenario that is currently blocked. Our setup
The problemWhen the Telegram channel receives a message with inline media (photo/video), OpenClaw attempts to download the media file from OpenClaw's SSRF protection blocks this with: The blocked hostname is Current workaround attemptsWe tried adding {
"tools": {
"web": {
"fetch": {
"enabled": true,
"ssrfPolicy": {
"allowCidrs": ["192.168.0.0/16"]
}
}
}
}
}This had no effect — the field appears to not be recognized in v2026.4.2. We also tried ImpactTelegram media (photos, videos, documents) cannot be received — every inbound media message triggers the SSRF block. This affects every user who runs OpenClaw behind a private proxy (common in China for accessing Telegram, and common for users running Clash/mihomo/Surge in fake-IP mode). RequestThis PR (or a released version containing it) would solve the problem by allowing: {
"tools": {
"web": {
"fetch": {
"ssrfPolicy": {
"allowCidrs": ["192.168.0.0/16", "198.18.0.0/15"]
}
}
}
}
}Is there anything blocking this from being merged? Happy to help test. The March 7 rebase update with 13/13 tests passing sounds very close to ready. |
Summary
web_fetchis completely broken in fake-IP proxy environments (Surge Enhanced Mode, Clash fake-ip, Quantumult X, etc.) because the SSRF guard blocks RFC 2544 benchmark range (198.18.0.0/15) addresses used for DNS interception. There is no config path to opt out. Internal network fetching (e.g. local docs servers) is also impossible.web_fetchat all. Users wanting to access internal documentation servers are also blocked.tools.web.fetch.ssrfPolicy.allowCidrs— an array of CIDR strings that should be allowed through SSRF protection. This is a generic approach: instead of adding specific booleans for each IP range (likeallowRfc2544BenchmarkRange), users specify exactly which CIDRs they trust.dangerouslyAllowPrivateNetworknuclear option is untouched. Browser SSRF policy is unaffected.Config example:
{ "tools": { "web": { "fetch": { "ssrfPolicy": { "allowCidrs": ["198.18.0.0/15"] } } } } }More examples:
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
allowRfc2544BenchmarkRangeapproach — this PR is a more generic alternative)allowPrivateNetworkapproach — complementary, this adds fine-grained control)User-visible / Behavior Changes
tools.web.fetch.ssrfPolicy.allowCidrs(string array, optional, default empty)web_fetchSecurity Impact (required)
browser.ssrfPolicy.allowPrivateNetwork,dangerouslyAllowPrivateNetwork)Repro + Verification
Environment
{ "tools": { "web": { "fetch": { "ssrfPolicy": { "allowCidrs": ["198.18.0.0/15"] } } } } }Steps
web_fetchto access any public URL (e.g.https://example.com)198.18.x.x(fake-IP range) → SSRF guard blocks itExpected
With
allowCidrs: ["198.18.0.0/15"],web_fetchshould succeed.Actual (before fix)
Blocked: resolves to private/internal/special-use IP addressfor every URL.Actual (after fix)
web_fetchsucceeds for all public URLs, includingcursor.com,baidu.com, etc.Evidence
npx vitest run src/infra/net/fetch-guard.ssrf.test.ts— 8/8 passed (including updated RFC2544 opt-in test)npx vitest run src/shared/net/ip.test.ts— 4/4 passednpm run build— clean, no type errorsnpx oxfmt --check— cleanweb_fetch https://cursor.com/pricingsucceeds withallowCidrs: ["198.18.0.0/15"]behind SurgeHuman Verification (required)
Compatibility / Migration
tools.web.fetch.ssrfPolicy.allowCidrsFailure Recovery (if this breaks)
ssrfPolicy.allowCidrsfrom config → behavior reverts to default restrictive SSRF policyRisks and Mitigations
0.0.0.0/0)dangerouslyAllowPrivateNetworkalready provides full bypass.allowCidrsis strictly more restrictive since each range must be individually listed.ipaddr.parseCIDRis trivial. Could be cached if needed in future.