.1616750161180714:2fb665accdd718b4eb14a70501355dff_69e6d82a59f68fc20e58829f.69e6ddf559f68fc20e5882e1.69e6ddf590f0ee6098651891:Trae CN.T(2026/4/21 10:16:21)#2932
Hidden character warning
Conversation
修改API调用失败时的日志级别以更准确反映问题严重性，同时在返回给客户端的错误信息中包含具体错误原因，便于问题排查
|
This pull request targeted The base branch has been automatically changed to |
hxf582125
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the APICall handler in api_tools.go to escalate the logging level of request failures to Error and include the raw error string in the JSON response. Review feedback suggests that using the Error level for external request failures may trigger unnecessary alerts and recommends using Warn instead. Additionally, there is a security concern regarding the exposure of sensitive system details by returning raw error messages to the client.
| if errDo != nil { | ||
| log.WithError(errDo).Debug("management APICall request failed") | ||
| c.JSON(http.StatusBadGateway, gin.H{"error": "request failed"}) | ||
| log.WithError(errDo).Error("management APICall request failed") |
There was a problem hiding this comment.
将日志级别提升至 Error 可能会导致监控系统产生大量误报。由于 APICall 是向外部 URL 发起请求，失败原因可能完全在系统控制之外（如目标服务器宕机、网络超时或用户输入了错误的 URL）。通常 Error 级别应保留给需要运维人员介入处理的系统内部故障。建议考虑使用 Warn 级别。
| log.WithError(errDo).Error("management APICall request failed") | |
| log.WithError(errDo).Warn("management APICall request failed") |
| log.WithError(errDo).Debug("management APICall request failed") | ||
| c.JSON(http.StatusBadGateway, gin.H{"error": "request failed"}) | ||
| log.WithError(errDo).Error("management APICall request failed") | ||
| c.JSON(http.StatusBadGateway, gin.H{"error": "request failed: " + errDo.Error()}) |
There was a problem hiding this comment.
直接将 errDo.Error() 的原始字符串返回给客户端存在安全风险。Go 的 http.Client 返回的错误信息可能包含敏感的底层细节，例如代理服务器的内部 IP、端口信息或特定的系统路径。建议对错误信息进行脱敏，或者仅在日志中记录详细错误，而给客户端返回更通用的错误描述。
修改测试用例以匹配apiCallTransport函数新增的布尔参数，确保测试覆盖新功能
在API调用请求中新增insecure字段，用于控制是否跳过TLS证书验证。当设置为true时，将配置HTTP客户端不验证服务器证书，适用于测试环境或自签名证书场景。同时更新相关传输层逻辑以支持此功能。
![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: c8853901b3
ℹ️ 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".
| if errDo != nil { | ||
| log.WithError(errDo).Debug("management APICall request failed") | ||
| c.JSON(http.StatusBadGateway, gin.H{"error": "request failed"}) | ||
| log.WithError(errDo).Error("management APICall request failed") |
There was a problem hiding this comment.
This now logs errDo at Error level, but http.Client.Do failures are typically *url.Error values whose string includes the full request URL. If a management caller uses signed URLs or query tokens (for example ?api_key=...) and the call fails (DNS/TLS/connect), those secrets are emitted to normal error logs; previously this path was debug-only. The repository’s root AGENTS.md explicitly says to “avoid leaking secrets/tokens in logs,” so this introduces a high-impact logging leak unless the URL/error is sanitized or the level is reduced.
Useful? React with 👍 / 👎.
luispater
left a comment
luispater
left a comment
There was a problem hiding this comment.
Summary
This PR updates the management POST /v0/management/api-call endpoint to:
- Promote request-failure logging from
DebugtoError - Include the underlying
http.Client.Doerror string in the JSON response on transport failure - Add an
insecurerequest flag to skip TLS certificate verification for that outbound request
Blocking
- Security/observability: logging
errDoatErrorand echoingerrDo.Error()back to the caller can leak sensitive details (full URL strings, internal proxy hosts) and may be overly noisy for an admin “curl-like” endpoint. Suggest using a stableerrorcode plus a separatemessagefield, and consider redacting sensitive parts and/or keeping the log level atWarn/Debug. insecurefootgun:InsecureSkipVerifyis inherently high-risk. Please documentinsecurein the endpoint’s request JSON docs (the handler comment), add an explicit warning/audit log when it’s enabled, and consider a server-side gate/allowlist (or config) so production deployments can disable it.
Non-blocking
- Add tests for
apiCallTransport(auth, insecure=true)to verify it setsTLSClientConfig.InsecureSkipVerifyfor both proxy and direct transports, and remains unset wheninsecure=false. - CI:
retargetis failing; the log indicates it tried to retarget PR #2933 and failed because a dev PR already exists for the same head branch—likely a duplicate PR / automation artifact.
Test plan
- Not run locally (reviewed via
ghdiff + checks only). Suggested:go test ./....
2 participants
修改API调用失败时的日志级别以更准确反映问题严重性，同时在返回给客户端的错误信息中包含具体错误原因，便于问题排查