fix(web-fetch): tighten ssrf config and cache#45872
Conversation
openclaw-barnacle
Bot
added
docs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7022767fbc
ℹ️ 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".
Greptile SummaryThis PR wires Key observations:
Confidence Score: 2/5
Prompt To Fix All With AIThis is a comment left during a code review.
Path: docs/tools/web.md
Line: 390-411
Comment:
**Patch artifacts embedded as literal content**
The entire new section from line 390 onward contains raw patch-format markers (`* +` and ` +`) as literal file content instead of clean markdown. This means the file will render incorrectly in every markdown viewer — e.g. `* +### SSRF policy for web_fetch` will appear as a bullet whose text is `+### SSRF policy for web_fetch` rather than a heading, and the JSON5 code block lines like ` +{` and `* tools: {` will show stray `+` and `*` characters instead of clean indented code.
The same corruption is present in `docs/zh-CN/tools/web.md` lines 259–280.
The section should read:
```markdown
### SSRF policy for web_fetch
`tools.web.fetch.ssrfPolicy` lets you tighten or relax the SSRF guard for `web_fetch` requests without affecting other tools. The optional fields mirror the browser-level settings:
- `allowPrivateNetwork`: legacy alias for `dangerouslyAllowPrivateNetwork`. Set to `true` to permit private/internal IP addresses.
- `dangerouslyAllowPrivateNetwork`: high-risk toggle that removes private/internal/special-use blocking. Enable only in trusted, isolated environments.
- `allowedHostnames`: explicitly allowed hostnames or IPs that bypass private network checks even when private access is blocked globally.
- `hostnameAllowlist`: pattern-based allowlist (e.g. `*.internal`) that shortlists which hostnames `web_fetch` is allowed to reach.
Example:
```json5
{
tools: {
web: {
fetch: {
ssrfPolicy: {
hostnameAllowlist: ["example.com", "*.example.internal"],
allowedHostnames: ["192.168.1.42"],
},
},
},
},
}
```
**Risk note:** ...
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/tools/web-guarded-fetch.ts
Line: 23
Comment:
**`ssrfPolicy` should be excluded from `WebToolEndpointFetchOptions`**
`WebToolEndpointFetchOptions` is defined as `Omit<WebToolGuardedFetchOptions, "policy" | "useEnvProxy">`, which still leaves `ssrfPolicy` accessible. This type is used by `withTrustedWebToolsEndpoint` and `withStrictWebToolsEndpoint` — callers that aren't supposed to override the per-request SSRF policy (especially `withTrustedWebToolsEndpoint`, which hardcodes `WEB_TOOLS_TRUSTED_NETWORK_SSRF_POLICY` including `allowRfc2544BenchmarkRange: true`).
Because `fetchWithWebToolsNetworkGuard` now resolves `policy: ssrfPolicy ?? rest.policy`, any caller that mistakenly (or intentionally) passes `ssrfPolicy` to `withTrustedWebToolsEndpoint` would silently replace the trusted policy, dropping `allowRfc2544BenchmarkRange` and potentially other flags.
Consider also omitting `ssrfPolicy` from the endpoint options type:
```suggestion
type WebToolEndpointFetchOptions = Omit<WebToolGuardedFetchOptions, "policy" | "useEnvProxy" | "ssrfPolicy">;
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: 7022767 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| * +### SSRF policy for web_fetch | ||
| * +`tools.web.fetch.ssrfPolicy` lets you tighten or relax the SSRF guard for `web_fetch` requests without affecting other tools. The optional fields mirror the browser-level settings: | ||
| * +- `allowPrivateNetwork`: legacy alias for `dangerouslyAllowPrivateNetwork`. Set to `true` to permit private/internal IP addresses. | ||
| +- `dangerouslyAllowPrivateNetwork`: high-risk toggle that removes private/internal/special-use blocking. Enable only in trusted, isolated environments. | ||
| +- `allowedHostnames`: explicitly allowed hostnames or IPs that bypass private network checks even when private access is blocked globally. | ||
| +- `hostnameAllowlist`: pattern-based allowlist (e.g. `*.internal`) that shortlists which hostnames `web_fetch` is allowed to reach. | ||
| * +Example: | ||
| * +```json5 | ||
| +{ | ||
| * tools: { | ||
| * web: { | ||
| * fetch: { | ||
| * ssrfPolicy: { | ||
| * hostnameAllowlist: ["example.com", "*.example.internal"], | ||
| * allowedHostnames: ["192.168.1.42"], | ||
| * }, | ||
| * }, | ||
| * }, | ||
| * }, | ||
| +} | ||
| +``` | ||
| * +**Risk note:** `dangerouslyAllowPrivateNetwork` (and its alias `allowPrivateNetwork`) undermines the default SSRF blocking, so avoid enabling it unless you fully trust the target network. Prefer allowlists/whitelists to expand access only for specific hosts. If you do need to relax the guard, keep a narrow `hostnameAllowlist` or `allowedHostnames` and monitor the usage closely. |
There was a problem hiding this comment.
Patch artifacts embedded as literal content
The entire new section from line 390 onward contains raw patch-format markers (* + and +) as literal file content instead of clean markdown. This means the file will render incorrectly in every markdown viewer — e.g. * +### SSRF policy for web_fetch will appear as a bullet whose text is +### SSRF policy for web_fetch rather than a heading, and the JSON5 code block lines like +{ and * tools: { will show stray + and * characters instead of clean indented code.
The same corruption is present in docs/zh-CN/tools/web.md lines 259–280.
The section should read:
### SSRF policy for web_fetch
`tools.web.fetch.ssrfPolicy` lets you tighten or relax the SSRF guard for `web_fetch` requests without affecting other tools. The optional fields mirror the browser-level settings:
- `allowPrivateNetwork`: legacy alias for `dangerouslyAllowPrivateNetwork`. Set to `true` to permit private/internal IP addresses.
- `dangerouslyAllowPrivateNetwork`: high-risk toggle that removes private/internal/special-use blocking. Enable only in trusted, isolated environments.
- `allowedHostnames`: explicitly allowed hostnames or IPs that bypass private network checks even when private access is blocked globally.
- `hostnameAllowlist`: pattern-based allowlist (e.g. `*.internal`) that shortlists which hostnames `web_fetch` is allowed to reach.
Example:
```json5
{
tools: {
web: {
fetch: {
ssrfPolicy: {
hostnameAllowlist: ["example.com", "*.example.internal"],
allowedHostnames: ["192.168.1.42"],
},
},
},
},
}Risk note: ...
<details><summary>Prompt To Fix With AI</summary>
`````markdown
This is a comment left during a code review.
Path: docs/tools/web.md
Line: 390-411
Comment:
**Patch artifacts embedded as literal content**
The entire new section from line 390 onward contains raw patch-format markers (`* +` and ` +`) as literal file content instead of clean markdown. This means the file will render incorrectly in every markdown viewer — e.g. `* +### SSRF policy for web_fetch` will appear as a bullet whose text is `+### SSRF policy for web_fetch` rather than a heading, and the JSON5 code block lines like ` +{` and `* tools: {` will show stray `+` and `*` characters instead of clean indented code.
The same corruption is present in `docs/zh-CN/tools/web.md` lines 259–280.
The section should read:
```markdown
### SSRF policy for web_fetch
`tools.web.fetch.ssrfPolicy` lets you tighten or relax the SSRF guard for `web_fetch` requests without affecting other tools. The optional fields mirror the browser-level settings:
- `allowPrivateNetwork`: legacy alias for `dangerouslyAllowPrivateNetwork`. Set to `true` to permit private/internal IP addresses.
- `dangerouslyAllowPrivateNetwork`: high-risk toggle that removes private/internal/special-use blocking. Enable only in trusted, isolated environments.
- `allowedHostnames`: explicitly allowed hostnames or IPs that bypass private network checks even when private access is blocked globally.
- `hostnameAllowlist`: pattern-based allowlist (e.g. `*.internal`) that shortlists which hostnames `web_fetch` is allowed to reach.
Example:
```json5
{
tools: {
web: {
fetch: {
ssrfPolicy: {
hostnameAllowlist: ["example.com", "*.example.internal"],
allowedHostnames: ["192.168.1.42"],
},
},
},
},
}
Risk note: ...
How can I resolve this? If you propose a fix, please make it concise.
dsantoreis
left a comment
dsantoreis
left a comment
There was a problem hiding this comment.
Good direction — loading SSRF policy from config (tools.web.fetch.ssrfPolicy) instead of ad-hoc execution args is cleaner and schema-driven.
The +133/-7 diff keeping existing tests untouched while making the policy injection explicit is the right approach. This is part of the broader SSRF guard family (#45749, #45781, #45799) where policy configuration is scattered across multiple paths.
Does this also affect the browser/playwright SSRF path, or only web-fetch? If only web-fetch, that's fine for now, but worth noting in the PR description that browser SSRF policy is a separate concern.
CharZhou
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f54dac6d52
ℹ️ 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".
| * Use allowedHostnames to permit specific internal hosts, or dangerouslyAllowPrivateNetwork | ||
| * to disable all private-network blocking (not recommended for production). | ||
| */ | ||
| ssrfPolicy?: SsrFPolicy; |
There was a problem hiding this comment.
ssrfPolicy is declared as SsrFPolicy here, which includes fields like allowRfc2544BenchmarkRange, but ToolsWebFetchSchema only accepts four SSRF keys (allowPrivateNetwork, dangerouslyAllowPrivateNetwork, allowedHostnames, hostnameAllowlist) and is .strict() in src/config/zod-schema.agent-runtime.ts. That means a typed config can compile yet fail validation at runtime when using a SsrFPolicy field that the schema rejects, so the type contract is now broader than the accepted config surface.
Useful? React with 👍 / 👎.
CharZhou
left a comment
CharZhou
left a comment
There was a problem hiding this comment.
@dsantoreis Thanks for the review! This change only affects the web_fetch tool (runtime + cache + docs + schema). Browser/playwright SSRF policy currently follows a different config flow, so I left that scope out for now; happy to follow up separately if we want to align those paths later.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ca31186827
ℹ️ 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".
This comment was marked as spam.
This comment was marked as spam.
CharZhou
left a comment
CharZhou
left a comment
There was a problem hiding this comment.
Thanks for the follow-up. Both the docs patch markers and the endpoint type issue were already addressed in the commits after that comment (see the trimmed SSRF sections in + and the definition that now omits "ssrfPolicy"). Those commits also generated TASK-007/TASK-008 reports for traceability, so the fixes are now part of the current diff — happy to signpost the exact report files if you need them.
2 participants
Summary\n- ensure SSRF policy flows through tools.web.fetch.ssrfPolicy everywhere (schema, docs, runtime) rather than ad-hoc execution arguments\n- tighten web_fetch cache keys to include ssrfPolicy so permissive policies can’t leak cached responses into stricter contexts\n- update docs/types/tests so the new config-driven SSRF guard is documented, well-typed, and validated\n\n## Testing\n- pnpm test -- --run src/agents/tools/web-fetch.ssrf.test.ts