feat(mcp-web-fetch): expose allowed_domains and blocked_domains on web_fetch schema (#380) - #393
Open
NanluQingshi wants to merge 3 commits into
Open
Conversation
…b_fetch schema (ceilf6#380) The engine already supported allowHosts/denyHosts host filters, but the MCP tool schema/handler did not expose them. Add optional allowed_domains and blocked_domains to the web_fetch tool, mapped to the engine's allowHosts/denyHosts, so the agent can pass domain restrictions aligned with Anthropic web fetch semantics for least-privilege fetches. SSRF defaults are unchanged; the filters are additive and optional.
Owner
|
/review |
1 similar comment
Owner
|
/review |
Owner
|
@ceilf6/repo-guard |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
代码评审报告: feat(mcp-web-fetch): expose allowed_domains and blocked_domains on web_fetch schema (#380)
风险等级: 中
处理建议: 请求修改
决策摘要: 管线本身是安全的加法式改动（省略参数时行为不变，重定向每跳都会复用过滤器），但新增的 schema 描述把"精确主机名相等"包装成"与 Anthropic web fetch 一致"的域名过滤，blocked_domains: ['example.com'] 不会拦截 www.example.com（安全控制方向上是 fail-open）；同时仓库内没有任何消费者读取 inputSchema，agent 事实上仍无法传这两个字段——即 issue 的 problem statement 未被真正解决。两处最小修复都在 diff 内（改描述 + 修正 CHANGELOG 声明或补上 planner 参数通路）。
级联分析
- 变更符号:
webFetchSchema.inputSchema.properties（新增 2 个属性）、handleWebFetchTool（新增 2 个 arg 映射）。函数签名、返回形状、错误行为均未变。 - 受影响流程:
handleWebFetchTool→fetchUrl（engine.ts:107）→parseAndValidateUrl（url-safety.ts:192）。首跳与每个重定向跳（engine.ts:119-122、156-159）都传入相同的allowHosts/denyHosts，不存在重定向绕过过滤器的缺口。deny 先于 allow 检查（url-safety.ts:218 早于 222），schema 中"优先级高于 allowed_domains"的描述属实。 - 变更集外调用方: 全仓仅
packages/runtime-node/src/mcp-clients.ts:24导入本包，且只导入handleWebFetchTool，在mcp-clients.ts:147-148原样透传args——不会剥离新字段，无破坏。webFetchSchema/allWebFetchSchemas虽从index.ts:12-17导出，但没有任何仓库内消费者（text search）；packages/mcp-web-fetch无 server/bin 入口（package.json 无bin），也不作为独立 MCP server 对外提供tools/list。 - 置信度: degraded — PR 自述 GitNexus
detect_changes未能运行，本次结论基于 Read/Grep 文本证据；调用面很窄（单一跨包导入），但无图谱交叉验证。
问题发现
-
[高] 新增 schema 描述与引擎实际匹配语义不符，黑名单方向 fail-open
- 证据:
tools.ts:44-45、50-51向模型声明"与 Anthropic web fetch 的 allowed_domains/blocked_domains 一致"；但url-safety.ts:218与223是h.toLowerCase() === lowerHost精确相等比较，无后缀/子域匹配、无 scheme 或端口归一化。因此blocked_domains: ['example.com']不拦截www.example.com、evil.example.com；若调用方按 Anthropic 文档习惯写成['https://example.com']，黑名单将完全不生效且静默通过。白名单方向反过来是 fail-closed（example.com不放行docs.example.com），安全但会造成"配了却抓不到"的困惑。 - 受影响调用方/流程: 任何直接调用
handleWebFetchTool并依赖blocked_domains做最小权限约束的调用方；这是新增的公共工具契约文本，模型会按描述行事。 - 最小可行修复: 保持行为不变（issue 非目标明确禁止改 URL safety policy），把两条 description 改成明确的"精确主机名匹配，子域需逐一列出，不含 scheme/端口"。若确实要 Anthropic 的子域语义，应作为独立 issue 修改
parseAndValidateUrl并同步 url-safety 测试，不要在描述里先行承诺。
- 证据:
-
[高] issue 的 problem statement 未被解决：agent 仍无法传这两个字段
- 证据: issue #380 的问题陈述是"the agent cannot pass the domain restrictions"。但 (a)
packages/core/src/executor/types.ts:15的MCPClient.listTools()返回类型只有{ name, description }，全仓无任何位置读取webFetchSchema.inputSchema；(b)packages/runtime-node/src/mcp-clients.ts:154-171是手工维护的工具清单，只有名称和描述，不含参数 schema；(c) agent 的 web_fetch 步骤参数受packages/core/src/llm/schemas.ts:19-37的STEP_PARAMS_SCHEMA约束，这是一个封闭对象（有url，无allowed_domains/blocked_domains），zod 默认 strip 未知键；(d)packages/core/src/executor/executor.ts:155透传的正是这份被 strip 过的step.params。全仓allowed_domains|blocked_domains仅出现在 tools.ts、新测试、CHANGELOG 三处（text search）。 - 受影响调用方/流程: LLM 规划 → executor → mcp-clients →
handleWebFetchTool的唯一 agent 路径。当前该能力只对直接编程调用者可用。 - 最小可行修复: 二选一，由维护者决定范围——要么在
STEP_PARAMS_SCHEMA补上这两个可选数组字段（小改动，贴合 issue 的问题陈述）；要么保持本 PR 的 schema-only 范围，但把 CHANGELOG 与 PR 描述中"The agent can now pass domain restrictions"改成准确表述（引擎能力已在工具契约层暴露，agent 侧接线待跟进），并开跟进 issue。不要让 CHANGELOG 宣称一个当前不成立的用户可见效果。
- 证据: issue #380 的问题陈述是"the agent cannot pass the domain restrictions"。但 (a)
-
[中] 空数组语义未文档化，
allowed_domains: []等于"不限制"- 证据:
url-safety.ts:222用opts.allowHosts && opts.allowHosts.length > 0判定，空数组直接跳过白名单校验。调用方程序化构造白名单、结果为空时，期待"什么都不许抓"，实际得到"全部放行"。 - 受影响调用方/流程: 任何动态构造
allowed_domains的调用方；schema 描述目前对此只字未提。 - 最小可行修复: 在
allowed_domains描述中写明"空数组视为不启用白名单"。行为改动（空数组=全拒）会触及 URL safety policy，属 issue 非目标，不建议在本 PR 做。
- 证据:
-
[低] 非数组入参只会抛出不透明的 TypeError
- 证据:
tools.ts:75-76直接as string[] | undefined，无运行时校验。模型误传blocked_domains: "example.com"时，url-safety.ts:218的.some抛TypeError，被tools.ts:83-88兜住，返回denyHosts.some is not a function之类的错误文案。结果是 fail-closed（不会发出请求），所以不是安全缺口，但诊断信息很差，且与数值参数由clampLimit（engine.ts:50-53）做防御的既有模式不一致。 - 受影响调用方/流程: LLM 生成的工具调用参数（数组类型最易被写错）。
- 最小可行修复: 在 handler 里做一次归一化——非数组丢弃、元素过滤为非空字符串后再传给引擎。
- 证据:
行级发现
- [packages/mcp-web-fetch/src/tools.ts:51] 描述声称与 Anthropic
blocked_domains一致，但url-safety.ts:218是精确主机名相等，['example.com']不会拦截www.example.com；改为明确说明"精确主机名匹配，子域需逐条列出，不含 scheme/端口"。 - [packages/mcp-web-fetch/src/tools.ts:45] 同样问题 + 空数组语义：
url-safety.ts:222对[]直接跳过白名单校验，描述里需写明"空数组视为不启用白名单"，避免调用方误以为是"全部拒绝"。 - [packages/mcp-web-fetch/src/tools.ts:75]
as string[] | undefined无运行时校验，非数组入参会在引擎内抛.some is not a function；建议先归一化为字符串数组（非数组则视为未提供），与clampLimit的防御模式对齐。 - [packages/mcp-web-fetch/src/tools.domain-filters.test.ts:33] 这条用例与
tools.security.test.ts:5-10的 URL、断言正则完全相同，属重复覆盖；换成子域匹配用例（blocked_domains: ['example.com']+https://www.example.com/）才能锁定真实语义。 - [CHANGELOG.md:9] "The agent can now pass domain restrictions" 与当前代码不符：
core/src/executor/types.ts:15的listTools不带 schema，core/src/llm/schemas.ts:19-37的封闭STEP_PARAMS_SCHEMA会 strip 掉这两个键；请改为准确表述或在本 PR 补上 planner 侧字段。
Karpathy 评审
- 假设: PR 假设"暴露 schema 字段 = agent 可用"，该假设不成立（见发现 2）；同时假设引擎的 host 过滤与 Anthropic 的 domain 语义等价，实际是精确主机名相等（见发现 1）。两个假设都未在代码或测试中被验证。
- 简洁性: 良好。纯 passthrough 映射，无新抽象、无新层级、无配置膨胀，复用引擎既有的
allowHosts/denyHosts，没有重复造 helper。 - 结构质量: 无退化。
tools.ts89 行，schema 属性与既有format/timeoutMs/maxBytes同构，没有新增分支、flag 或特例路径。snake_case 与既有 camelCase 参数混用属 issue 明确要求（对齐 Anthropic 命名），不作为 finding。 - 变更范围: 精准。3 个文件、无无关重构、无格式噪声，每一行都能追溯到 issue 的 Scope；未触碰 SSRF、私网拦截、重定向、clamp 逻辑，符合非目标约束。
- 验证: 强度不足以匹配契约文本。4 条测试只覆盖精确匹配的拒绝路径，没有一条覆盖最容易出错的子域行为；第 4 条与既有 SSRF 测试重复；没有正向用例证明白名单内的 URL 能通过过滤器（handler 层无
fetchImpl注入口，可在 engine 层用FetchOptions.fetchImpl补，engine.ts:32）。
缺失覆盖
- 子域语义用例：
blocked_domains: ['example.com']+https://www.example.com/，断言当前的精确匹配行为（会放行），把这一意外语义锁进测试并与修正后的描述对齐。 - 白名单正向路径：在 engine 层用
fetchImpl注入 mock，验证allowHosts: ['example.com']下https://example.com/能通过校验并完成抓取；当前 3 条行为测试全是拒绝断言。 - 重定向 + 过滤器组合：首跳在白名单内、
Location指向白名单外主机时应被engine.ts:156-159拒绝——这是过滤器最有价值的防护场景，目前无覆盖。 - 畸形入参：
blocked_domains传字符串或包含空串的数组时，应有可读的错误信息而非.some is not a function。 - 若采纳发现 2 的接线方案：需要一条 planner/executor 层测试，证明
allowed_domains能从计划步骤参数一路到达handleWebFetchTool而不被 zod strip。
| type: 'array', | ||
| items: { type: 'string' }, | ||
| description: | ||
| '可选域名黑名单。提供后拒绝抓取列表中的域名（与 Anthropic web fetch 的 blocked_domains 一致）。优先级高于 allowed_domains。', |
There was a problem hiding this comment.
描述声称与 Anthropic blocked_domains 一致，但 url-safety.ts:218 是精确主机名相等，['example.com'] 不会拦截 www.example.com；改为明确说明"精确主机名匹配，子域需逐条列出，不含 scheme/端口"。
| type: 'array', | ||
| items: { type: 'string' }, | ||
| description: | ||
| '可选域名白名单。提供后仅允许抓取列表中的域名（与 Anthropic web fetch 的 allowed_domains 一致）。用于最小权限场景，限制 Agent 只能访问受信任的文档站点。', |
There was a problem hiding this comment.
同样问题 + 空数组语义：url-safety.ts:222 对 [] 直接跳过白名单校验，描述里需写明"空数组视为不启用白名单"，避免调用方误以为是"全部拒绝"。
| format: args.format as 'text' | 'html' | undefined, | ||
| timeoutMs: args.timeoutMs as number | undefined, | ||
| maxBytes: args.maxBytes as number | undefined, | ||
| allowHosts: args.allowed_domains as string[] | undefined, |
There was a problem hiding this comment.
as string[] | undefined 无运行时校验，非数组入参会在引擎内抛 .some is not a function；建议先归一化为字符串数组（非数组则视为未提供），与 clampLimit 的防御模式对齐。
| expect(result.error).toMatch(/not in allow list/i); | ||
| }); | ||
|
|
||
| it('omitting both filters preserves existing behavior (SSRF guard still applies)', async () => { |
There was a problem hiding this comment.
这条用例与 tools.security.test.ts:5-10 的 URL、断言正则完全相同，属重复覆盖；换成子域匹配用例（blocked_domains: ['example.com'] + https://www.example.com/）才能锁定真实语义。
|
|
||
| ### Added | ||
|
|
||
| - **mcp-web-fetch**: Exposed optional `allowed_domains` and `blocked_domains` on the `web_fetch` MCP tool schema, mapped to the engine's existing `allowHosts`/`denyHosts` host filters. The agent can now pass domain restrictions aligned with Anthropic web fetch semantics, enabling least-privilege fetches without changing SSRF defaults. |
There was a problem hiding this comment.
"The agent can now pass domain restrictions" 与当前代码不符：core/src/executor/types.ts:15 的 listTools 不带 schema，core/src/llm/schemas.ts:19-37 的封闭 STEP_PARAMS_SCHEMA 会 strip 掉这两个键；请改为准确表述或在本 PR 补上 planner 侧字段。
…ut (ceilf6#380) Address PR ceilf6#393 review feedback so the agent can actually pass domain restrictions end-to-end, with no new failure modes. - Thread allowed_domains/blocked_domains through STEP_PARAMS_SCHEMA and ErrorRecoveryPlanSchema (optional z.array(z.string())). The closed zod object previously stripped these keys before they reached handleWebFetchTool, so the capability was only available to direct programmatic callers — the planner/executor wiring now preserves them. - Rewrite schema descriptions to state exact-hostname matching semantics (no scheme/port; subdomains must be listed individually) and that an empty array disables the filter rather than denying all. Drops the misleading "aligned with Anthropic" claim. - Add normalizeHostList in engine.ts (mirrors the clampLimit defensive pattern) so non-array input is normalized to undefined instead of throwing ".some is not a function" inside the engine. Applied on both the initial URL and every redirect hop. - Tests: replace the redundant SSRF-omission case with subdomain-semantics, empty-array, and malformed-input coverage; add a url-safety exact-match subdomain test; add a core schema test proving the fields survive GeneratedPlanSchema.parse (not stripped). - CHANGELOG: rephrase to accurately reflect agent-usable end-to-end behavior. Quality gate run manually (pnpm is environmentally broken on Node 18: corepack pins the unpublished pnpm@9.0.0). biome + tsc clean; affected mcp-web-fetch / core llm suites pass. Pre-existing env-only failures (langgraph crypto, zod z.file, mcp-filesense fixture cleanup) reproduce identically on the baseline commit and are unrelated to this change. Co-Authored-By: Claude <noreply@anthropic.com>
Owner
|
/review |
There was a problem hiding this comment.
代码评审报告: feat(mcp-web-fetch): expose allowed_domains and blocked_domains on web_fetch schema (#380)
风险等级: 中
处理建议: 请求修改
决策摘要: 功能映射与包内级联是正确的，但 PR 在两个 generateObject 结构化输出 schema 中首次引入了 .optional() 字段，而该文件既有约定是"全部必填 + 空值哨兵"；这条链路完全没有验证证据（PR 只跑了 packages/mcp-web-fetch），建议改成必填空数组后再合。
级联分析
- 变更符号:
webFetchSchema/handleWebFetchTool（tools.ts）、fetchUrl+ 新增normalizeHostList（engine.ts）、STEP_PARAMS_SCHEMA与ErrorRecoveryPlanSchema.recoverySteps[].params（core/llm/schemas.ts） - 受影响流程: planner 结构化出计划 →
normalizePlan→Planner.convertLLMSteps（planner.ts:323 直接透传llmStep.params）→Executor展开{...step.params}（executor/executor.ts:155,177）→mcp-clients.ts:147→handleWebFetchTool→fetchUrl→parseAndValidateUrl（url-safety.ts:218-226）。链路端到端成立，CHANGELOG 关于"此前被 zod 闭合对象剥离"的说明属实。 - 变更集外调用方:
allowHosts/denyHosts仅由fetchUrl与包内测试使用（text search，无包外消费者）；STEP_PARAMS_SCHEMA由GeneratedPlanSchema/StepExpansionSchema共用，generateObject调用点为 plan-generation.ts:321、plan-generation.ts:468、code-generation.ts:398（ErrorRecoveryPlanSchema）。url-safety.ts未改动，SSRF 默认行为保持不变。 - 置信度: medium（基于文件阅读 + text search，无代码图谱；PR 自述
detect_changes未能运行。provider 侧 JSON Schema 转换行为无法在本 checkout 验证——依赖未安装）
问题发现
-
[高] 首次向结构化输出 schema 引入 optional 字段，与既有"全部必填"约定冲突且零验证
- 证据:
STEP_PARAMS_SCHEMA其余 15 个字段全部必填并用哨兵值表达"不适用"，描述文字明写"所有字段必填，不适用的字符串填空字符串"（schemas.ts:19-51）。本 PR 新增的allowed_domains/blocked_domains是该文件中唯二进入generateObject路径的.optional()字段（GeneratedPatchSchema虽有 optional，但 grep 显示它没有任何generateObject调用点，因此不构成先例）。llm-service.ts:130-134使用openai.chat(model)（AI SDK v5，ai@^5.0.199/@ai-sdk/openai@^2.0.106），OpenAI 严格 structured outputs 要求required覆盖全部 properties；optional 字段在该模式下是已知的不兼容点。本环境未安装依赖，无法实证，PR 也未提供任何 provider 侧或 core 侧验证。 - 受影响调用方/流程:
generatePlan（plan-generation.ts:468）、generatePlanInTwoPhases（plan-generation.ts:321）、generateErrorRecoveryPlan（code-generation.ts:398）——即 OpenAI provider 下的主计划生成路径。 - 最小可行修复: 去掉
.optional()，改为必填z.array(z.string())，描述沿用文件现有约定"不适用时填空数组"。行为完全等价：空数组在normalizeHostList中已折叠为undefined，url-safety.ts:222也把空 allow 列表视为不启用。这样既消除 provider 兼容风险，又与同文件其余字段保持一致。若坚持保留 optional，请补一次真实 provider structured-output 调用或 JSON Schema 转换断言作为证据。
- 证据:
-
[中]
normalizeHostList只挡住了类型错误，挡不住最常见的格式错误，deny 列表会静默失效- 证据: engine.ts:66-75 只做
Array.isArray+typeof string+trim；url-safety.ts:218用完整 hostname 全等比较。模型按 schema 描述"不含 scheme/端口"是软约束，实际最常见的错误输入是"https://evil.com"或"evil.com:443"。这类值进入blocked_domains后永远匹配不上，黑名单静默变成空操作（fail-open）；进入allowed_domains则变成全量拒绝。该函数的注释自称是为"模型写错"兜底，但恰恰漏了这一类。 - 受影响调用方/流程: 所有带域名过滤的
web_fetch调用；SSRF 默认防护不受影响，但最小权限这一层会无声失效。 - 最小可行修复: 在
normalizeHostList中对每个条目做一次规范化——若含://用new URL(entry).hostname，否则剥离尾部:port、结尾.，并toLowerCase()；无法解析的条目丢弃。同时为"https://example.com"形式补一条测试。
- 证据: engine.ts:66-75 只做
-
[中] 精确主机名匹配与 Anthropic 同名参数的语义不一致，容易造成 agent 自我拒绝
- 证据: tools.ts:41-52 的描述已明确说明"子域需逐条列出"，url-safety.test.ts:97-115 也固化了该语义。但 issue #380 的显式约束是"Keep naming aligned with Anthropic's documented tool parameters"——采用同名参数却是更严格的精确匹配。模型对
allowed_domains: ["example.com"]的自然预期是覆盖docs.example.com，实际会在抓文档站时被自己设的白名单挡下（Host is not in allow list），且这是硬失败而非降级。 - 受影响调用方/流程: planner 生成的最小权限
web_fetch步骤；失败发生在网络请求前，会直接进入 executor 的错误路径。 - 最小可行修复: 二选一并明确记录——(a) 保持精确匹配（符合 issue 的"不改 URL 安全策略"非目标），但在
STEP_PARAMS_SCHEMA的描述里也把"子域不自动包含"写全（当前 core 侧描述已有一句，但比 tools.ts 弱），并在 CHANGELOG 中标注与 Anthropic 语义的差异；(b) 改为后缀匹配host === d || host.endsWith('.' + d)。注意 (b) 会放宽 allow 列表语义，属于安全策略变更，需单独 PR。
- 证据: tools.ts:41-52 的描述已明确说明"子域需逐条列出"，url-safety.test.ts:97-115 也固化了该语义。但 issue #380 的显式约束是"Keep naming aligned with Anthropic's documented tool parameters"——采用同名参数却是更严格的精确匹配。模型对
-
[中] 验证范围没有覆盖实际改动的包，PR 描述与 diff 不符
- 证据: PR 描述称"Single file change in
packages/mcp-web-fetch"、"No engine, SSRF, or url-safety logic changed"、"No other packages touched"，并据此给出 LOW 风险与"非关键骨架变更"的结论。实际 diff 改了packages/mcp-web-fetch/src/engine.ts、packages/core/src/llm/schemas.ts、packages/core/src/llm/plan-generation.test.ts、packages/mcp-web-fetch/src/url-safety.test.ts。验证一节只列了packages/mcp-web-fetch的 48 个测试，packages/core一次都没跑——而问题 1 正好落在 core 里。 - 受影响调用方/流程: 评审者的风险判断依据；
packages/core的 planner/executor 测试集。 - 最小可行修复: 更新 PR 描述与 GitNexus 影响摘要以反映真实范围，并补跑
packages/core的测试（至少plan-generation、planner、executor）后把结果贴进验证一节。
- 证据: PR 描述称"Single file change in
行级发现
- [packages/core/src/llm/schemas.ts:38] 该文件其余字段一律必填 + 哨兵值，这里的
.optional()是进入generateObject的首个可选字段，OpenAI 严格 structured outputs 下有兼容风险；改成必填z.array(z.string())+ "不适用时填空数组"，行为等价（空数组已被normalizeHostList折叠为undefined）。 - [packages/core/src/llm/schemas.ts:140] 同上，
ErrorRecoveryPlanSchema走code-generation.ts:398的generateObject，需与STEP_PARAMS_SCHEMA采取一致处理，不要一处 optional 一处必填。 - [packages/mcp-web-fetch/src/engine.ts:71] 只
trim()无法处理"https://example.com"/"example.com:443"这类模型最常写错的格式，blocked_domains会静默失效；此处补一次 hostname 规范化（含://时取new URL(entry).hostname，否则剥离端口/结尾点并小写）。 - [packages/mcp-web-fetch/src/tools.ts:45] 与 Anthropic 同名参数的子域语义不一致，模型给出
example.com却抓docs.example.com时会被自己的白名单硬拒；若保持精确匹配，请把这条约束同步写进STEP_PARAMS_SCHEMA的描述并在 CHANGELOG 标注差异。
Karpathy 评审
- 假设: 隐含假设"planner 结构化输出可以接受 optional 字段"，未验证（问题 1）；隐含假设"模型会按描述给出裸 hostname"，
normalizeHostList的兜底并未覆盖该假设失效的主要形态（问题 2）。 - 简洁性: 映射本身足够薄——两个 schema 属性 + 两行透传，没有多余抽象。
normalizeHostList是合理的边界防御，放在 engine（安全边界）而非 tools 层是正确取舍。 - 结构质量: 无退化。没有新增分支扩散、没有薄 wrapper、没有重复 canonical helper，文件规模均在可扫描范围内（tools.ts 89 行、engine.ts 273 行、schemas.ts 175 行）。
ErrorRecoveryPlanSchema内联复制 params 形状是本就存在的重复，本 PR 只是跟随，不算新增债务。 - 变更范围: 基本外科手术式，但比 PR 自述宽（跨 2 个包 3 个源文件）。engine 的规范化与 core 的 schema 透传都是让功能真正端到端可用所必需，属于合理扩展，问题在于描述没有同步（问题 4）。
- 验证: 包内测试设计得好——4 个 handler 级测试全部在网络请求前短路，确实无网络依赖，且反向覆盖了 mapping 是否写反（
deniedvsnot in allow list）。缺口在 core 侧：新增的GeneratedPlanSchema.parse测试只证明 zod 运行时不剥离字段，没有证明 provider 侧 JSON Schema 转换可用，而后者才是问题 1 的风险面。
缺失覆盖
- provider 侧结构化输出兼容性：对
GeneratedPlanSchema做一次 JSON Schema 转换断言（例如断言required覆盖全部 properties，或按当前 provider 约定断言可选字段的表示形式），否则问题 1 无法被 CI 捕获。 packages/core全量测试结果：schemas.ts 被改动但该包测试未在 PR 中报告运行。- 域名格式健壮性：
allowed_domains: ['https://example.com']、['example.com:443']、['EXAMPLE.com ']三类输入的行为断言（当前normalizeHostList对前两类会静默产生永不匹配的过滤器）。 - 正向放行路径：目前只有"被拒绝"方向的 handler 级断言；建议补一条
allowed_domains命中后仍能进入 fetch 阶段的断言（可经fetchImpl注入或直接在 url-safety 层断言，避免真实网络）。
| changeDescription: z.string().describe('要做的修改描述（不适用时填空字符串）'), | ||
| allowed_domains: z | ||
| .array(z.string()) | ||
| .optional() |
There was a problem hiding this comment.
该文件其余字段一律必填 + 哨兵值，这里的 .optional() 是进入 generateObject 的首个可选字段，OpenAI 严格 structured outputs 下有兼容风险；改成必填 z.array(z.string()) + "不适用时填空数组"，行为等价（空数组已被 normalizeHostList 折叠为 undefined）。
| changeDescription: z.string().describe('要做的修改描述（不适用时填空字符串）'), | ||
| allowed_domains: z | ||
| .array(z.string()) | ||
| .optional() |
There was a problem hiding this comment.
同上，ErrorRecoveryPlanSchema 走 code-generation.ts:398 的 generateObject，需与 STEP_PARAMS_SCHEMA 采取一致处理，不要一处 optional 一处必填。
| const cleaned: string[] = []; | ||
| for (const entry of value) { | ||
| if (typeof entry !== 'string') continue; | ||
| const trimmed = entry.trim(); |
There was a problem hiding this comment.
只 trim() 无法处理 "https://example.com" / "example.com:443" 这类模型最常写错的格式，blocked_domains 会静默失效；此处补一次 hostname 规范化（含 :// 时取 new URL(entry).hostname，否则剥离端口/结尾点并小写）。
| type: 'array', | ||
| items: { type: 'string' }, | ||
| description: | ||
| '可选域名白名单，仅 web_fetch 适用，可省略。采用精确主机名匹配（不含 scheme/端口）：提供后仅允许抓取主机名恰好出现在列表中的 URL，子域需逐条列出（如 example.com 不会自动放行 www.example.com）。空数组视为不启用白名单（而非全部拒绝）。用于最小权限场景，限制 Agent 只能访问受信任的文档站点。', |
There was a problem hiding this comment.
与 Anthropic 同名参数的子域语义不一致，模型给出 example.com 却抓 docs.example.com 时会被自己的白名单硬拒；若保持精确匹配，请把这条约束同步写进 STEP_PARAMS_SCHEMA 的描述并在 CHANGELOG 标注差异。
Use z.array(z.string()).nullish() instead of .optional() for allowed_domains/blocked_domains in STEP_PARAMS_SCHEMA and ErrorRecoveryPlanSchema. Models sometimes emit null for optional fields; .optional() only accepts undefined and would reject null, forcing an object-repair retry. .nullish() accepts null/undefined, and the engine's normalizeHostList already treats null identically to undefined (filter disabled). Adds a regression test proving null survives GeneratedPlanSchema.parse without error. Co-Authored-By: Claude <noreply@anthropic.com>
Owner
|
/review |
There was a problem hiding this comment.
代码评审报告: feat(mcp-web-fetch): expose allowed_domains and blocked_domains on web_fetch schema (#380)
风险等级: 中
处理建议: 请求修改
决策摘要: 功能设计本身正确、契约是附加式的，主路径未发现破坏性问题；但畸形过滤器输入会让新加的安全限制静默失效（fail-open，并被新测试固化），且 PR 自述范围与实际 diff 不符——占一半改动的 packages/core LLM 契约没有任何验证证据，建议补这两项后合入。
级联分析
- 变更符号:
webFetchSchema.inputSchema.properties、handleWebFetchTool、fetchUrl、新增导出normalizeHostList(engine.ts:66)、STEP_PARAMS_SCHEMA、ErrorRecoveryPlanSchema.recoverySteps.params - 受影响流程: planner 结构化输出（
GeneratedPlanSchema/StepExpansionSchema/ErrorRecoveryPlanSchema，覆盖全部 13 个 action，不只 web_fetch）→Executor.executeStep（executor.ts:155{...step.params}原样透传）→ExecutorToolCallHandler.callTool→WebMCPClient.callTool(mcp-clients.ts:147) →handleWebFetchTool→fetchUrl→parseAndValidateUrl（首跳 + 每个重定向跳） - 变更集外调用方:
fetchUrl仅被tools.ts:71与engine.test.ts调用；parseAndValidateUrl的 host filter 仅由 engine 传入——包内闭合，无外部签名破坏。FetchOptions类型未变，既有编程式调用方兼容。models/frontagent-planner/prompts/schema.json是GeneratedPlanSchema的镜像副本，未同步（该副本本就已漂移，action enum 里连web_fetch都没有）。 - 置信度: medium（无代码图谱：PR 自述
detect_changes未能运行，本环境亦无索引。结论来自 grep + 直接读取全部调用点；@ai-sdk/openai的 strict structured-output 行为因 node_modules 未安装而无法实测，仅通过仓库内既有先例交叉验证）
问题发现
-
[中] 畸形过滤器输入让 deny list 静默失效（fail-open），且被新测试固化
- 证据:
normalizeHostList(engine.ts:66-75) 对任何非数组输入返回undefined。改动前blocked_domains: 'evil.com'（裸字符串）会在opts?.denyHosts?.some(...)(url-safety.ts:218) 抛 TypeError，被handleWebFetchTool的 catch 转成失败——请求不会发出（fail-closed）。改动后同样输入被当作"未提供过滤器"，抓取照常进行（fail-open）。tools.domain-filters.test.ts:46-57把这一行为写成了期望。同理，['evil.com', 123]中的非字符串项被静默丢弃。 - 受影响调用方/流程: 任何直接调用
handleWebFetchTool的 MCP host（runtime-node 内的 executor 路径参数未做校验，见 tool-call-handler.ts:74）；FrontAgent 自身走 zod 校验，可达性较低，但这正是该特性存在意义的场景——调用方打算限制却被静默忽略。 - 最小可行修复:
normalizeHostList区分"缺省"与"畸形"：value == null返回undefined；非数组、或数组中含非字符串项时抛UrlSafetyError('Invalid domain filter: ...')（handler 已有 catch，会转成success:false）。空数组仍保留"不启用"语义。同步调整该测试断言为期望明确报错。
- 证据:
-
[中] PR 自述范围与验证证据不覆盖实际 diff 的一半
- 证据: PR 描述称 "Single file change in
packages/mcp-web-fetch"、"No engine ... changed"、"No other packages touched"、风险 LOW；实际 diff 改了engine.ts、packages/core/src/llm/schemas.ts（planner 的 LLM 输出契约，被GeneratedPlanSchema/StepExpansionSchema/ErrorRecoveryPlanSchema共用，影响全部 action 的规划调用）以及packages/core/src/llm/plan-generation.test.ts。Verification 段落只列了packages/mcp-web-fetch的 48 tests / tsc / biome，pnpm quality:precommit因环境问题未跑。 - 受影响调用方/流程: 所有
LLMService.generateObject规划调用。注：STEP_PARAMS_SCHEMA原本每个字段都必填（描述明写"所有字段必填"），本 PR 引入其首批可选字段；我检查后确认code-quality-subagent.ts:160,163已在同一generateObject通道上使用.optional()，因此没有证据表明会触发 provider 的 strict-schema 报错——但这一判断依赖 provider 行为，需要真实跑一次核心包的规划测试来兜底。 - 最小可行修复: 补跑并贴出
packages/core的 vitest + tsc 结果（至少packages/core/src/llm），并把 PR 描述/GitNexus 影响摘要更正为实际范围（含 core 的 LLM 契约变更），不要以 "single file / LOW" 的前提让人评审。
- 证据: PR 描述称 "Single file change in
-
[中]
blocked_domains采用精确匹配，对子域 fail-open，与其借用的 Anthropic 语义存在落差- 证据:
parseAndValidateUrl(url-safety.ts:218) 只做全等比较，url-safety.test.ts:97-113明确固化denyHosts: ['example.com']不拦截www.example.com。issue #380 要求"命名对齐 Anthropic 文档参数"，同时非目标是"不改 URL safety policy"——所以取名对齐、语义不对齐是必然结果。白名单方向是 fail-closed（安全），黑名单方向是 fail-open（列了evil.com仍会抓www.evil.com）。 - 受影响调用方/流程: 使用该黑名单做最小权限约束的 agent/调用方。当前缓解手段只有文档——
tools.ts:47-52与schemas.ts:42-47的描述都明确写了"子域需逐条列出"，模型侧可见，缓解到位。 - 最小可行修复: 本 PR 内不必改（符合 issue 非目标）；建议在 CHANGELOG/issue 中留下明确的后续项：deny 侧改为后缀匹配
host === h || host.endsWith('.' + h)，或提供显式的*.example.com语法。请维护者确认接受当前语义。
- 证据:
-
[低]
ErrorRecoveryPlanSchema的 params 副本继续扩大重复- 证据: schemas.ts:126-147 内联了一份
STEP_PARAMS_SCHEMA的近似副本（本就缺query/filePattern/globOnly/maxResults），本 PR 又把两个新字段以不同措辞的 describe 复制了进去（schemas.ts:138-145 vs 36-47）。 - 受影响调用方/流程:
analyzeErrorsAndGenerateRecovery(code-generation.ts:398)；两份描述会随时间继续漂移。 - 最小可行修复: 恢复步骤的
params直接复用STEP_PARAMS_SCHEMA（或抽出共享 base），本次即可顺手收敛，diff 反而更小。
- 证据: schemas.ts:126-147 内联了一份
-
[低] 规划提示词没提这两个参数，端到端能力实际难以被触发
- 证据: 两阶段展开的"参数填写指南"里
plan-generation.ts:277仍是"web_fetch: params 需要 url"；WebMCPClient.listTools()(mcp-clients.ts:166-169) 只返回 name/description，不下发webFetchSchema的 inputSchema。模型能看到的只有STEP_PARAMS_SCHEMA的字段描述。 - 受影响调用方/流程: CHANGELOG 声称"the agent can actually pass domain restrictions end-to-end"——链路确实通了，但没有任何提示引导模型使用。
- 最小可行修复: 在 plan-generation.ts:277 追加一行，例如"抓取受信任文档站点时可加
allowed_domains（精确主机名）"。
- 证据: 两阶段展开的"参数填写指南"里
行级发现
- [packages/mcp-web-fetch/src/engine.ts:67] 非数组输入返回
undefined会把调用方明确设置的 deny list 静默变成"无过滤"（改动前会抛错并 fail-closed）；建议区分value == null（视为未提供）与 present-but-malformed（抛UrlSafetyError，由 handler 的 catch 转成失败结果）。 - [packages/mcp-web-fetch/src/tools.domain-filters.test.ts:46] 该用例把"畸形安全过滤器被静默忽略后继续抓取"固化为期望；随 engine 改为 fail-closed 后，应改成断言返回明确的
Invalid domain filter错误。 - [packages/mcp-web-fetch/src/tools.ts:75]
args.allowed_domains as string[] | undefined对未经校验的 MCP 入参做了不成立的断言；边界处应显式校验并拒绝非法形状，而不是靠下游normalizeHostList把它抹掉。 - [packages/mcp-web-fetch/src/url-safety.test.ts:100] 这条断言固化了"deny list 不覆盖子域"；请在 CHANGELOG 或 issue 中显式记录该语义与 Anthropic 同名参数的差异，避免调用方按直觉误用黑名单。
- [packages/core/src/llm/schemas.ts:138] 这里是
STEP_PARAMS_SCHEMA的第二份副本，且新字段 describe 措辞与 36-47 行不一致；建议直接复用STEP_PARAMS_SCHEMA而不是继续维护两份。
Karpathy 评审
- 假设: 隐含假设"到达 handler 的
allowed_domains一定是数组"——在 FrontAgent 内部由 zod 保证，但作为 MCP 公共工具不成立，而兜底策略选择了 fail-open。另一处隐含假设是新的可选字段不会影响 provider 的结构化输出约束；仓库内已有.optional()先例（code-quality-subagent.ts:160），但本 PR 未提供核心包的运行证据。 - 简洁性:
normalizeHostList与clampLimit姿态一致，没有多余抽象；集中在fetchUrl内归一化、重定向跳复用同一份结果（engine.ts:140-141,182-183）是正确做法，避免了逐跳重复清洗。 - 结构质量: 无新增分支缠绕、无薄 wrapper、无文件规模跨界（engine.ts 273 行，tools.ts 89 行）。唯一退化是
ErrorRecoveryPlanSchema的 params 副本被进一步复制（见发现 4）。 - 变更范围: 超出 PR 自述范围。core LLM 契约的改动是必要的（否则字段在 planner 侧就被 strip，特性无法端到端使用），但必须在描述与风险评估里如实反映，而不是以 "single file / LOW risk" 呈现。
- 验证: mcp-web-fetch 侧覆盖扎实且确实无网络（host 过滤在任何 fetch 之前发生，
127.0.0.1用例由isBlockedHostname提前拒绝）。core 侧新增了两个 schema 用例但缺少运行证据；重定向跳的过滤器复检（engine.ts:181-184）这一安全相关路径没有任何测试。
缺失覆盖
- 重定向场景下的域名过滤：白名单内 URL 302 跳到白名单外主机时必须被拒绝（
engine.test.ts已有fetchImpl重定向脚手架，可直接复用）。 normalizeHostList的直接单测：null/undefined/非数组/含空白与非字符串项的混合数组/全空白数组，以及（按发现 1 修复后）畸形输入应报错的断言。packages/core的实际执行证据：至少packages/core/src/llm的 vitest 与tsc --noEmit，确认新增可选字段未破坏既有规划路径。ErrorRecoveryPlanSchema侧未新增任何用例：恢复计划里的allowed_domains/blocked_domains是否同样能被解析并透传，目前完全未验证。
| * "filter disabled" rather than "deny all" (see `url-safety.ts`). | ||
| */ | ||
| export function normalizeHostList(value: unknown): string[] | undefined { | ||
| if (!Array.isArray(value)) return undefined; |
There was a problem hiding this comment.
非数组输入返回 undefined 会把调用方明确设置的 deny list 静默变成"无过滤"（改动前会抛错并 fail-closed）；建议区分 value == null（视为未提供）与 present-but-malformed（抛 UrlSafetyError，由 handler 的 catch 转成失败结果）。
| expect(result.error).toMatch(/private|blocked|loopback|safety/i); | ||
| }); | ||
|
|
||
| it('non-array filter input is normalized away instead of crashing the engine', async () => { |
There was a problem hiding this comment.
该用例把"畸形安全过滤器被静默忽略后继续抓取"固化为期望；随 engine 改为 fail-closed 后，应改成断言返回明确的 Invalid domain filter 错误。
| format: args.format as 'text' | 'html' | undefined, | ||
| timeoutMs: args.timeoutMs as number | undefined, | ||
| maxBytes: args.maxBytes as number | undefined, | ||
| allowHosts: args.allowed_domains as string[] | undefined, |
There was a problem hiding this comment.
args.allowed_domains as string[] | undefined 对未经校验的 MCP 入参做了不成立的断言；边界处应显式校验并拒绝非法形状，而不是靠下游 normalizeHostList 把它抹掉。
| it('host filters use exact hostname matching — subdomains are not implied', () => { | ||
| // denyHosts: ['example.com'] must NOT deny www.example.com (exact match only). | ||
| const allowed = parseAndValidateUrl('https://www.example.com/', { | ||
| denyHosts: ['example.com'], |
There was a problem hiding this comment.
这条断言固化了"deny list 不覆盖子域"；请在 CHANGELOG 或 issue 中显式记录该语义与 Anthropic 同名参数的差异，避免调用方按直觉误用黑名单。
| fullPage: z.boolean().describe('是否全页截图，不适用时填false'), | ||
| codeDescription: z.string().describe('要生成的代码的描述（不适用时填空字符串）'), | ||
| changeDescription: z.string().describe('要做的修改描述（不适用时填空字符串）'), | ||
| allowed_domains: z |
There was a problem hiding this comment.
这里是 STEP_PARAMS_SCHEMA 的第二份副本，且新字段 describe 措辞与 36-47 行不一致；建议直接复用 STEP_PARAMS_SCHEMA 而不是继续维护两份。
Owner
代码评审报告: feat(mcp-web-fetch): expose allowed_domains and blocked_domains on web_fetch schema (#380)风险等级: 低 级联分析
问题发现
行级发现
Karpathy 评审
缺失覆盖
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Linked Issue Or Context
Closes #380
Repo-evolver iteration 24 found that
@frontagent/mcp-web-fetchengine alreadysupports host filters via
allowHosts/denyHosts, but the MCP tool schema didnot expose them — so the agent could not pass domain restrictions that the
engine already enforces.
Summary
Add optional
allowed_domains: string[]andblocked_domains: string[]to theweb_fetchMCP tool schema, mapped to the engine's existingallowHosts/denyHostshost filters. The agent can now pass domain restrictions alignedwith Anthropic web fetch semantics, enabling least-privilege fetches without
changing SSRF defaults.
allowed_domains→ engineallowHosts(only listed domains permitted)blocked_domains→ enginedenyHosts(listed domains rejected; takesprecedence over allow list)
behavior
Impact Scope
Single file change in
packages/mcp-web-fetch:src/tools.ts— two new schema properties + two handler arg mappings (+14lines)
src/tools.domain-filters.test.ts— 4 focused no-network tests (+41 lines,new file)
CHANGELOG.md—[Unreleased]→### AddedentryNo engine, SSRF, or url-safety logic changed. No other packages touched.
GitNexus Impact Summary
packages/mcp-web- fetch(mcp-boundary), schema + handler only; no public MCP contract break —fields are additive optional.
detect_changescould not run locally (no indexedrepository in this environment). The change adds optional schema fields and a
passthrough mapping; no symbol signatures or call-graph edges are altered.
npx vitest run(48/48 pass),tsc --noEmit(clean),biome check(clean) onpackages/mcp-web-fetch.Verification
npx vitest runinpackages/mcp-web-fetch→ 7 test files, 48 tests passed(was 44; +4 new)
npx tsc --noEmit→ exit 0npx biome checkon changed files → clean, no fixes appliedNew tests (no network required — host filtering happens before any fetch):
blocked_domainsrejects the listed host (asserts/denied/i)allowed_domainsrejects hosts not in the allow list (asserts/not in allow list/i)type: arrayChecklist
pnpm quality:precommit, or explained why it could not run.(Ran equivalent focused gates manually — lint + typecheck + test on
packages/ mcp-web-fetch; pre-commit hook env has a broken pnpm/corepack 9.0.0 self-update, CI remains authoritative.)
pnpm quality:localfor critical skeleton changes, orexplained why it could not run. (Not a critical skeleton change; scoped to one
MCP adapter.)
contracts changed. (Added CHANGELOG entry + focused tests.)
with concrete results. (N/A — LOW risk, non-critical.)