refactor: 删除 max-loop 空概念并修复权限默认放行与 webfetch SSRF#338
Conversation
…n/webfetch security Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
|
@xgopilot 修复comment提到的问题,然后解决冲突后推送 |
处理结果已按 review comment 修复 本次修改
冲突处理
验证
推送信息
|
Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: Cai-Tang-www <106404101+Cai-Tang-www@users.noreply.github.com>
|
/code 修复编译错误 |
处理结果已修复本 PR 分支的编译错误并推送。 变更说明
验证
推送信息
|
Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
/review |
There was a problem hiding this comment.
Found 1 actionable code issue and 1 documentation follow-up.
- Actionable:
webfetchcurrently resolves DNS twice for hostname targets (validateFetchTargetand thenresolveDialAddress), adding avoidable network latency per request. - Follow-up: behavior for blocking localhost/private/link-local targets appears changed; please ensure user-facing docs clearly describe this restriction and the testing-only bypass behavior.
| return net.JoinHostPort(ip.String(), port), nil | ||
| } | ||
|
|
||
| lookupCtx, cancel := context.WithTimeout(ctx, time.Second) |
There was a problem hiding this comment.
Execute already calls validateFetchTarget, which resolves hostnames via LookupIPAddr, and this dial path resolves the hostname again in resolveDialAddress. This duplicates DNS work on every request and can add noticeable latency under load. Consider consolidating validation + final-address selection into one resolution step (or caching/share the first lookup result for the request).
|
/simplify |
|
|
||
| const bypassTargetValidationKey validationContextKey = "webfetch_bypass_target_validation" | ||
|
|
||
| // WithUnsafeBypassTargetValidation 返回跳过目标地址安全校验的上下文,仅用于受控测试场景。 |
There was a problem hiding this comment.
WithUnsafeBypassTargetValidation 作为导出 API 存在于生产代码路径中,任何仓内调用方都可以显式关闭 SSRF 校验(包含 dial-time 校验)。这会把“仅测试用途”的假设变成可被误用的安全后门。建议将 bypass 能力收敛到 _test.go(测试辅助)或通过仅测试构建约束暴露,避免运行时代码可调用。
| if err != nil { | ||
| return "", fmt.Errorf("%s: resolve host: %w", toolName, err) | ||
| } | ||
| for _, record := range records { |
There was a problem hiding this comment.
这里在 DNS 结果中命中第一个非阻断 IP 就立即返回,等价于把原本的多地址拨号退化为“单 IP 一次尝试”。当该 IP 短暂不可达但同名其他记录可用时会直接失败,属于可用性回归。建议保留并尝试全部允许的解析结果(至少按顺序回退),而不是只选首个地址。
|
/code 修复最近两个comment提到的问题 |
处理结果已修复“最近两个 comment”指出的问题,并推送到当前 PR 分支。 变更说明
验证
推送信息
未执行项
|
Generated with [codeagent](https://github.com/qbox/codeagent) Co-authored-by: phantom5099 <245659304+phantom5099@users.noreply.github.com>
Requested by @Cai-Tang-www
Summary
max loop相关的空概念残留(定义、分支、测试)。tools.NewManager的engine=nil默认放行改为 fail-closed,避免权限系统被静默绕过。webfetch增加目标主机安全校验,阻断localhost、回环、链路本地、私网等地址访问,修复 SSRF 风险。Verification
Notes
webfetch.WithUnsafeBypassTargetValidation(ctx),仅供受控测试场景使用。Closes #334