feat: add channel/global http client TLS fingerprint support with uTLS - #3056
Closed
seefs001 wants to merge 5287 commits into
Closed
feat: add channel/global http client TLS fingerprint support with uTLS#3056seefs001 wants to merge 5287 commits into
seefs001 wants to merge 5287 commits into
Conversation
feat(gemini): map OpenAI stop to Gemini stopSequences
fix: remove disable_parallel_tool_use if tool_choice=none
fix: /v1/responses/compact default billing
* feat: 引入通用 HTTP BodyStorage/DiskCache 缓存配置与管理 - 新增 common/body_storage.go 提供 HTTP 请求体存储抽象和文件缓存能力 - 增加 common/disk_cache_config.go 支持全局磁盘缓存配置 - main.go 挂载缓存初始化流程 - 新增和补充 controller/performance.go (及 unix/windows) 用于缓存性能监控接口 - middleware/body_cleanup.go 自动清理缓存文件 - router 挂载相关接口 - 前端 settings 页面新增性能监控设置 PerformanceSetting - 优化缓存开关状态和模块热插拔能力 - 其他相关文件同步适配缓存扩展 * fix: 修复 BodyStorage 并发安全和错误处理问题 - 修复 diskStorage.Close() 竞态条件，先获取锁再执行 CAS - 为 memoryStorage 添加互斥锁和 closed 状态检查 - 修复 CreateBodyStorageFromReader 在磁盘存储失败时的回退逻辑 - 添加缓存命中统计调用 (IncrementDiskCacheHits/IncrementMemoryCacheHits) - 修复 gin.go 中 Seek 错误被忽略的问题 - 在 api-router 添加 BodyStorageCleanup 中间件 - 修复前端 formatBytes 对异常值的处理 Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
…2793) Explicitly cast Blocks, Bavail, and Bfree to uint64 for cross-platform compatibility, as these fields are int64 on FreeBSD but uint64 on Linux.
feat: Support customizing the success and cancel url of Stripe.
…8684dd7ef068e576 feat: doubao add first and last image to video
…96dc78bae181e331 feat: task pre consume modelPrice default use setting value
…98ade9b372bd6f63 feat: CodeViewer click link and auto wrap
feat(gemini): support cached token billing
fix(ui): use distinct color palette for group tags
* fix: channel affinity log styles * fix: Issue with incorrect data storage when switching key sources * feat: support not retrying after a single rule configuration fails * fix: render channel affinity tooltip as multiline content * feat: channel affinity cache hit * fix: prevent ChannelAffinityUsageCacheModal infinite loading and hide data before fetch * chore: format backend with gofmt and frontend with prettier/eslint autofix
…-take-effect fix: make channel Host override take effect
feat: /v1/responses qwen3 max && perplexity
fix: violation fee check
…assthrough fix: skip Accept-Encoding during header passthrough (#2214)
feat: move user bindings to dedicated management modal
- Introduced a new test file for StreamScannerHandler, covering various scenarios including nil inputs, empty bodies, chunk processing, order preservation, and handler failures. - Enhanced error handling and data processing logic in StreamScannerHandler to improve robustness and performance.
feat(web): add custom-model create hint and i18n translations
…uting - Introduced RouteTag middleware to set route tags for different API endpoints. - Updated logger to include route tags in log output. - Applied RouteTag middleware across various routers including API, dashboard, relay, video, and web routers for consistent logging.
fix: align Vertex content fetch flow with Gemini and handle base64
fix: vertex ai video proxy and task polling improvements
fix: show built-in user bindings from user detail API in admin modal
Contributor
WalkthroughRefactors code to replace proxy-string parameters with a typed Changes
Sequence Diagram(s)sequenceDiagram
participant Controller
participant ChannelSettings
participant Service
participant HTTPClient
rect rgba(100, 200, 0, 0.5)
Note over Controller,Service: Request flow using ChannelSettings
Controller->>ChannelSettings: settings := channel.GetSetting()
ChannelSettings-->>Controller: {Proxy, TLSFingerprint, TLSCustom}
Controller->>Service: GetHttpClientWithChannelSetting(settings)
Service->>Service: normalize options, resolve ClientHello ID
Service->>HTTPClient: buildUTLSClient or buildClassicClient (may use proxy)
HTTPClient-->>Service: *http.Client (cached)
Service-->>Controller: *http.Client
Controller->>HTTPClient: Do(request)
HTTPClient-->>Controller: Response
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
relay/channel/vertex/service_account.go (3)
125-128:⚠️ Potential issue | 🟡 MinorUse
common.DecodeJsoninstead ofjson.NewDecoder().Decode().Per coding guidelines, all JSON operations must use wrapper functions from
common/json.go. Direct use ofencoding/jsonis not permitted in business code.🔧 Proposed fix
- if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + if err := common.DecodeJson(resp.Body, &result); err != nil { return "", err }As per coding guidelines: "Do NOT directly import or call
encoding/jsonin business code; use wrapper functions fromcommon/json.goinstead."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/vertex/service_account.go` around lines 125 - 128, Replace the direct use of encoding/json decoder when parsing resp.Body into result with the project wrapper: call common.DecodeJson(resp.Body, &result) (or the exact wrapper function in common/json.go) inside the same block where result and resp are used in service_account.go; remove the direct usage/import of encoding/json from that file and ensure the common package is imported, returning the same error from common.DecodeJson if it fails.
162-165:⚠️ Potential issue | 🟡 MinorSame issue: Use
common.DecodeJsoninstead of directjson.NewDecoder().Decode().This is the second occurrence in this file where
encoding/jsonis used directly instead of the wrapper function.🔧 Proposed fix
- if err := json.NewDecoder(resp.Body).Decode(&result); err != nil { + if err := common.DecodeJson(resp.Body, &result); err != nil { return "", err }Also remove the
encoding/jsonimport from line 6 once both usages are replaced.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/vertex/service_account.go` around lines 162 - 165, Replace the direct use of json.NewDecoder(resp.Body).Decode(&result) with the wrapper common.DecodeJson to mirror the other occurrences in service_account.go (i.e., replace the call that decodes into the local variable result/from resp.Body), so call common.DecodeJson(resp.Body, &result) and handle the returned error the same way; after updating both locations in this file remove the unused encoding/json import.
60-63:⚠️ Potential issue | 🟡 MinorRemove the redundant conditional;
SetDefaultreturns a bool indicating whether the key already existed.The
asynccache.SetDefault()method returns abool(exist), not an error. The variable naming aserris misleading. More importantly, both branches of the conditional returnnewToken, nil, making the check pointless. Simplify by removing the conditional entirely.Suggested fix
- if err := Cache.SetDefault(cacheKey, newToken); err { - return newToken, nil - } + _ = Cache.SetDefault(cacheKey, newToken) return newToken, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/vertex/service_account.go` around lines 60 - 63, The code is incorrectly treating Cache.SetDefault as returning an error and uses a redundant if that returns newToken, nil in both branches; replace the conditional with a direct call to Cache.SetDefault(cacheKey, newToken) (either ignore the boolean return or capture it as exist) and then unconditionally return newToken, nil so the SetDefault result isn't misinterpreted (references: Cache.SetDefault, cacheKey, newToken).service/codex_oauth.go (1)
319-322:⚠️ Potential issue | 🟡 MinorUse
common.Unmarshalinstead ofjson.Unmarshal.Per coding guidelines, all JSON operations must use wrapper functions from
common/json.go.🔧 Proposed fix
var claims map[string]any - if err := json.Unmarshal(payloadRaw, &claims); err != nil { + if err := common.Unmarshal(payloadRaw, &claims); err != nil { return nil, false }Also remove the
encoding/jsonimport from line 8 once this is fixed.As per coding guidelines: "Use
common.Marshal(),common.Unmarshal(),common.UnmarshalJsonStr(),common.DecodeJson(), orcommon.GetJsonType()fromcommon/json.gofor all JSON marshal/unmarshal operations in business code."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/codex_oauth.go` around lines 319 - 322, Replace the direct use of json.Unmarshal for decoding the JWT payload into the local variable claims with the project's wrapper common.Unmarshal: call common.Unmarshal(payloadRaw, &claims) and handle its returned error the same way; update the code path that currently checks "if err := json.Unmarshal(payloadRaw, &claims); err != nil { return nil, false }" to use common.Unmarshal and remove the now-unused encoding/json import; keep the behavior and return values identical when the unmarshal fails.
🧹 Nitpick comments (4)
web/src/components/settings/SystemSetting.jsx (1)
317-332: Submit TLS custom/default updates sequentially to avoid partial state.These two options are coupled. Sending them through
updateOptionscan run both writes concurrently, which is brittle for dependent settings.♻️ Suggested patch
const submitDefaultTLSFingerprint = async () => { - const options = [ - { - key: 'proxy_setting.default_tls_fingerprint', - value: inputs['proxy_setting.default_tls_fingerprint'] || '', - }, - { - key: 'proxy_setting.default_tls_custom', - value: - inputs['proxy_setting.default_tls_fingerprint'] === 'custom' - ? inputs['proxy_setting.default_tls_custom'] || '' - : '', - }, - ]; - await updateOptions(options); + const fingerprint = inputs['proxy_setting.default_tls_fingerprint'] || ''; + const custom = + fingerprint === 'custom' + ? inputs['proxy_setting.default_tls_custom'] || '' + : ''; + + // 1) Persist custom spec first + await updateOptions([ + { key: 'proxy_setting.default_tls_custom', value: custom }, + ]); + // 2) Then switch fingerprint mode + await updateOptions([ + { key: 'proxy_setting.default_tls_fingerprint', value: fingerprint }, + ]); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/SystemSetting.jsx` around lines 317 - 332, The current submitDefaultTLSFingerprint bundles two coupled options into one update that may be applied concurrently; change submitDefaultTLSFingerprint so it performs the writes sequentially: first await updateOptions with the single option { key: 'proxy_setting.default_tls_fingerprint', value: ... } to persist the chosen fingerprint, then await updateOptions for { key: 'proxy_setting.default_tls_custom', value: ... } (set to the custom value only if the first value === 'custom', otherwise send an empty string) so the dependent custom value is only written after the fingerprint is saved.relay/channel/task/gemini/adaptor.go (1)
222-223: Use transport-agnostic error wording.At Line 222, the message still says
"proxy"even though this path now covers full channel settings.♻️ Suggested tweak
- return nil, fmt.Errorf("new proxy http client failed: %w", err) + return nil, fmt.Errorf("create http client failed: %w", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@relay/channel/task/gemini/adaptor.go` around lines 222 - 223, The error message "new proxy http client failed: %w" is transport-specific; update the return statement that wraps fmt.Errorf(err) (the line returning nil, fmt.Errorf("new proxy http client failed: %w", err)) to use transport-agnostic wording (e.g., "new channel http client failed" or "failed to create http client") so it no longer references "proxy" while still including the wrapped error; locate the return in adaptor.go where fmt.Errorf is used with that exact string and replace only the message text accordingly.service/http_client.go (1)
102-108: Potential infinite recursion if global default fingerprint is set incorrectly.The interaction between
getHTTPClientByOptionsandGetHttpClientis subtle:
getHTTPClientByOptionsreturns early and callsGetHttpClient()whenProxyURL == "" && TLSFingerprint == ""GetHttpClientnormalizes options and may callgetHTTPClientByOptionsif a global default fingerprint is configuredThis works correctly because the normalized options will have a non-empty fingerprint, avoiding the recursion. However, if
normalizeHTTPClientOptionsever fails or returns empty fingerprint despite global defaults, this could cause infinite recursion. Consider adding a safeguard or documenting this relationship.💡 Optional safeguard
func getHTTPClientByOptions(options httpClientOptions) (*http.Client, error) { if options.ProxyURL == "" && options.TLSFingerprint == "" { - if client := GetHttpClient(); client != nil { - return client, nil + // Direct fallback to avoid potential recursion via GetHttpClient + if httpClient != nil { + return httpClient, nil } return http.DefaultClient, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/http_client.go` around lines 102 - 108, getHTTPClientByOptions can recurse with GetHttpClient if normalizeHTTPClientOptions ever yields empty fingerprint; add a defensive fallback: when ProxyURL=="" && TLSFingerprint=="" call GetHttpClient(), and if it returns nil or if you detect it may have called back (use a simple package-level recursion sentinel like inGetHTTPClient) then return http.DefaultClient instead of recursing; also ensure GetHttpClient continues to call normalizeHTTPClientOptions as before but respects the sentinel to avoid calling getHTTPClientByOptions again.service/http_client_test.go (1)
12-26: Consider addingt.Parallel()guard or documenting non-parallel nature.Tests that mutate global state (
*proxySetting = ...) may cause race conditions if Go's test runner parallelizes tests. The current cleanup approach is correct for sequential execution. If these tests must remain sequential, consider adding a comment explaining whyt.Parallel()should not be used, or alternatively, consider using a mock/stub pattern that doesn't modify global state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@service/http_client_test.go` around lines 12 - 26, The test TestNormalizeHTTPClientOptions_UseGlobalDefaultFingerprint mutates global proxySetting (variable proxySetting) which can race with other tests; either make the test safe for parallel execution by adding t.Parallel() at the top of the test and ensuring the test uses its own isolated proxySetting instance, or explicitly document (a one-line comment) that the test must run sequentially and refactor to avoid global mutation by using a setter/mocking approach instead of directly changing *proxySetting; adjust normalizeHTTPClientOptions usage accordingly so the test no longer relies on modifying global state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@controller/option.go`:
- Around line 72-95: The handler currently accepts
"proxy_setting.default_tls_fingerprint" values without ensuring that when the
fingerprint equals the sentinel "custom" the companion field
"proxy_setting.default_tls_custom" is present and valid; update the switch case
handling for "proxy_setting.default_tls_fingerprint" (and/or add validation
after dto.NormalizeTLSFingerprint/ dto.IsValidTLSFingerprint) to check if
fingerprint == "custom" and then load and validate option.Value of
"proxy_setting.default_tls_custom" using the same JSON unmarshal/TrimSpace logic
from the "proxy_setting.default_tls_custom" case (reuse the customSpec
validation flow), returning the same HTTP error responses if the required
customSpec is missing or invalid, and only set option.Value when both
fingerprint and customSpec validations pass.
In `@relay/mjproxy_handler.go`:
- Around line 42-44: The error response in mjproxy_handler.go that returns
c.JSON(400, gin.H{"error": "proxy_url_invalid"}) is too specific; update the
handler that builds the channel HTTP client (the branch that currently emits
"proxy_url_invalid") to return a generic error key like
"http_client_config_error" (or similar) to cover proxy URL, TLS fingerprint, and
custom config failures, and ensure the JSON body still includes no sensitive
details.
In `@service/http_client.go`:
- Around line 280-290: The TLS config used for HTTPS proxy tunnels is missing
MinVersion; update the tls.Config passed to tls.Client in the block that creates
tlsConn (the code calling tls.Client(...), tlsConn.HandshakeContext and
hostWithoutPort(proxyURL.Host)) to set MinVersion to a secure minimum (e.g.,
tls.VersionTLS12 or your project-wide minimum constant), keeping
InsecureSkipVerify from common.TLSInsecureSkipVerify intact; ensure you
reference the same tls package symbol (tls.VersionTLS12) or a central constant
if one exists.
---
Outside diff comments:
In `@relay/channel/vertex/service_account.go`:
- Around line 125-128: Replace the direct use of encoding/json decoder when
parsing resp.Body into result with the project wrapper: call
common.DecodeJson(resp.Body, &result) (or the exact wrapper function in
common/json.go) inside the same block where result and resp are used in
service_account.go; remove the direct usage/import of encoding/json from that
file and ensure the common package is imported, returning the same error from
common.DecodeJson if it fails.
- Around line 162-165: Replace the direct use of
json.NewDecoder(resp.Body).Decode(&result) with the wrapper common.DecodeJson to
mirror the other occurrences in service_account.go (i.e., replace the call that
decodes into the local variable result/from resp.Body), so call
common.DecodeJson(resp.Body, &result) and handle the returned error the same
way; after updating both locations in this file remove the unused encoding/json
import.
- Around line 60-63: The code is incorrectly treating Cache.SetDefault as
returning an error and uses a redundant if that returns newToken, nil in both
branches; replace the conditional with a direct call to
Cache.SetDefault(cacheKey, newToken) (either ignore the boolean return or
capture it as exist) and then unconditionally return newToken, nil so the
SetDefault result isn't misinterpreted (references: Cache.SetDefault, cacheKey,
newToken).
In `@service/codex_oauth.go`:
- Around line 319-322: Replace the direct use of json.Unmarshal for decoding the
JWT payload into the local variable claims with the project's wrapper
common.Unmarshal: call common.Unmarshal(payloadRaw, &claims) and handle its
returned error the same way; update the code path that currently checks "if err
:= json.Unmarshal(payloadRaw, &claims); err != nil { return nil, false }" to use
common.Unmarshal and remove the now-unused encoding/json import; keep the
behavior and return values identical when the unmarshal fails.
---
Nitpick comments:
In `@relay/channel/task/gemini/adaptor.go`:
- Around line 222-223: The error message "new proxy http client failed: %w" is
transport-specific; update the return statement that wraps fmt.Errorf(err) (the
line returning nil, fmt.Errorf("new proxy http client failed: %w", err)) to use
transport-agnostic wording (e.g., "new channel http client failed" or "failed to
create http client") so it no longer references "proxy" while still including
the wrapped error; locate the return in adaptor.go where fmt.Errorf is used with
that exact string and replace only the message text accordingly.
In `@service/http_client_test.go`:
- Around line 12-26: The test
TestNormalizeHTTPClientOptions_UseGlobalDefaultFingerprint mutates global
proxySetting (variable proxySetting) which can race with other tests; either
make the test safe for parallel execution by adding t.Parallel() at the top of
the test and ensuring the test uses its own isolated proxySetting instance, or
explicitly document (a one-line comment) that the test must run sequentially and
refactor to avoid global mutation by using a setter/mocking approach instead of
directly changing *proxySetting; adjust normalizeHTTPClientOptions usage
accordingly so the test no longer relies on modifying global state.
In `@service/http_client.go`:
- Around line 102-108: getHTTPClientByOptions can recurse with GetHttpClient if
normalizeHTTPClientOptions ever yields empty fingerprint; add a defensive
fallback: when ProxyURL=="" && TLSFingerprint=="" call GetHttpClient(), and if
it returns nil or if you detect it may have called back (use a simple
package-level recursion sentinel like inGetHTTPClient) then return
http.DefaultClient instead of recursing; also ensure GetHttpClient continues to
call normalizeHTTPClientOptions as before but respects the sentinel to avoid
calling getHTTPClientByOptions again.
In `@web/src/components/settings/SystemSetting.jsx`:
- Around line 317-332: The current submitDefaultTLSFingerprint bundles two
coupled options into one update that may be applied concurrently; change
submitDefaultTLSFingerprint so it performs the writes sequentially: first await
updateOptions with the single option { key:
'proxy_setting.default_tls_fingerprint', value: ... } to persist the chosen
fingerprint, then await updateOptions for { key:
'proxy_setting.default_tls_custom', value: ... } (set to the custom value only
if the first value === 'custom', otherwise send an empty string) so the
dependent custom value is only written after the fingerprint is saved.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
controller/channel-billing.gocontroller/channel.gocontroller/codex_oauth.gocontroller/codex_usage.gocontroller/option.gocontroller/video_proxy.gocontroller/video_proxy_gemini.godto/channel_settings.gogo.modmodel/channel.gorelay/channel/adapter.gorelay/channel/api_request.gorelay/channel/aws/relay-aws.gorelay/channel/coze/relay-coze.gorelay/channel/gemini/relay-gemini.gorelay/channel/task/ali/adaptor.gorelay/channel/task/doubao/adaptor.gorelay/channel/task/gemini/adaptor.gorelay/channel/task/hailuo/adaptor.gorelay/channel/task/jimeng/adaptor.gorelay/channel/task/kling/adaptor.gorelay/channel/task/sora/adaptor.gorelay/channel/task/suno/adaptor.gorelay/channel/task/vertex/adaptor.gorelay/channel/task/vidu/adaptor.gorelay/channel/vertex/service_account.gorelay/mjproxy_handler.gorelay/relay_task.goservice/codex_credential_refresh.goservice/codex_oauth.goservice/http_client.goservice/http_client_test.goservice/task_billing_test.goservice/task_polling.gosetting/system_setting/proxy_setting.goweb/src/components/settings/SystemSetting.jsxweb/src/components/table/channels/modals/EditChannelModal.jsx
Comment on lines
+72
to
95
| case "proxy_setting.default_tls_fingerprint": | ||
| fingerprint := dto.NormalizeTLSFingerprint(option.Value.(string)) | ||
| if !dto.IsValidTLSFingerprint(fingerprint) { | ||
| c.JSON(http.StatusOK, gin.H{ | ||
| "success": false, | ||
| "message": "无效的 TLS 指纹配置", | ||
| }) | ||
| return | ||
| } | ||
| option.Value = fingerprint | ||
| case "proxy_setting.default_tls_custom": | ||
| customSpec := strings.TrimSpace(option.Value.(string)) | ||
| if customSpec != "" { | ||
| var customMap map[string]any | ||
| if err = common.Unmarshal([]byte(customSpec), &customMap); err != nil { | ||
| c.JSON(http.StatusOK, gin.H{ | ||
| "success": false, | ||
| "message": "TLS 自定义配置必须是合法 JSON", | ||
| }) | ||
| return | ||
| } | ||
| } | ||
| option.Value = customSpec | ||
| case "GitHubOAuthEnabled": |
Contributor
There was a problem hiding this comment.
Enforce cross-field invariant when fingerprint is custom.
Line 72-82 currently validates only the enum value. It should also require a valid proxy_setting.default_tls_custom; otherwise an invalid global TLS config can be saved.
🛠️ Suggested patch
case "proxy_setting.default_tls_fingerprint":
fingerprint := dto.NormalizeTLSFingerprint(option.Value.(string))
if !dto.IsValidTLSFingerprint(fingerprint) {
c.JSON(http.StatusOK, gin.H{
"success": false,
"message": "无效的 TLS 指纹配置",
})
return
}
+ if fingerprint == dto.TLSFingerprintCustom {
+ common.OptionMapRWMutex.RLock()
+ rawCustom := common.Interface2String(common.OptionMap["proxy_setting.default_tls_custom"])
+ common.OptionMapRWMutex.RUnlock()
+ if strings.TrimSpace(rawCustom) == "" {
+ c.JSON(http.StatusOK, gin.H{
+ "success": false,
+ "message": "TLS 指纹为自定义时，必须先配置默认 TLS 自定义 JSON",
+ })
+ return
+ }
+ var customMap map[string]any
+ if err = common.Unmarshal([]byte(rawCustom), &customMap); err != nil {
+ c.JSON(http.StatusOK, gin.H{
+ "success": false,
+ "message": "TLS 自定义配置必须是合法 JSON",
+ })
+ return
+ }
+ }
option.Value = fingerprint🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@controller/option.go` around lines 72 - 95, The handler currently accepts
"proxy_setting.default_tls_fingerprint" values without ensuring that when the
fingerprint equals the sentinel "custom" the companion field
"proxy_setting.default_tls_custom" is present and valid; update the switch case
handling for "proxy_setting.default_tls_fingerprint" (and/or add validation
after dto.NormalizeTLSFingerprint/ dto.IsValidTLSFingerprint) to check if
fingerprint == "custom" and then load and validate option.Value of
"proxy_setting.default_tls_custom" using the same JSON unmarshal/TrimSpace logic
from the "proxy_setting.default_tls_custom" case (reuse the customSpec
validation flow), returning the same HTTP error responses if the required
customSpec is missing or invalid, and only set option.Value when both
fingerprint and customSpec validations pass.
Comment on lines
42
to
44
| c.JSON(400, gin.H{ | ||
| "error": "proxy_url_invalid", | ||
| }) |
Contributor
There was a problem hiding this comment.
Use a generic error key for channel HTTP client config failures.
Line 42 currently returns proxy_url_invalid, but this path now also covers TLS fingerprint/custom config errors. The current message is misleading.
💡 Suggested patch
- "error": "proxy_url_invalid",
+ "error": "channel_http_client_config_invalid",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| c.JSON(400, gin.H{ | |
| "error": "proxy_url_invalid", | |
| }) | |
| c.JSON(400, gin.H{ | |
| "error": "channel_http_client_config_invalid", | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@relay/mjproxy_handler.go` around lines 42 - 44, The error response in
mjproxy_handler.go that returns c.JSON(400, gin.H{"error": "proxy_url_invalid"})
is too specific; update the handler that builds the channel HTTP client (the
branch that currently emits "proxy_url_invalid") to return a generic error key
like "http_client_config_error" (or similar) to cover proxy URL, TLS
fingerprint, and custom config failures, and ensure the JSON body still includes
no sensitive details.
Comment on lines
+280
to
+290
| if strings.EqualFold(proxyURL.Scheme, "https") { | ||
| tlsConn := tls.Client(conn, &tls.Config{ | ||
| ServerName: hostWithoutPort(proxyURL.Host), | ||
| InsecureSkipVerify: common.TLSInsecureSkipVerify, | ||
| }) | ||
| if err := tlsConn.HandshakeContext(ctx); err != nil { | ||
| _ = conn.Close() | ||
| return nil, err | ||
| } | ||
| conn = tlsConn | ||
| } |
Contributor
There was a problem hiding this comment.
Set MinVersion on TLS config for HTTPS proxy connections.
The static analysis tool correctly flags that MinVersion is missing from the TLS configuration used when connecting to HTTPS proxies. While this is for the proxy tunnel (not the destination), it's still a security best practice to enforce minimum TLS version.
🔧 Proposed fix
if strings.EqualFold(proxyURL.Scheme, "https") {
tlsConn := tls.Client(conn, &tls.Config{
ServerName: hostWithoutPort(proxyURL.Host),
InsecureSkipVerify: common.TLSInsecureSkipVerify,
+ MinVersion: tls.VersionTLS12,
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if strings.EqualFold(proxyURL.Scheme, "https") { | |
| tlsConn := tls.Client(conn, &tls.Config{ | |
| ServerName: hostWithoutPort(proxyURL.Host), | |
| InsecureSkipVerify: common.TLSInsecureSkipVerify, | |
| }) | |
| if err := tlsConn.HandshakeContext(ctx); err != nil { | |
| _ = conn.Close() | |
| return nil, err | |
| } | |
| conn = tlsConn | |
| } | |
| if strings.EqualFold(proxyURL.Scheme, "https") { | |
| tlsConn := tls.Client(conn, &tls.Config{ | |
| ServerName: hostWithoutPort(proxyURL.Host), | |
| InsecureSkipVerify: common.TLSInsecureSkipVerify, | |
| MinVersion: tls.VersionTLS12, | |
| }) | |
| if err := tlsConn.HandshakeContext(ctx); err != nil { | |
| _ = conn.Close() | |
| return nil, err | |
| } | |
| conn = tlsConn | |
| } |
🧰 Tools
🪛 ast-grep (0.41.0)
[warning] 280-283: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
ServerName: hostWithoutPort(proxyURL.Host),
InsecureSkipVerify: common.TLSInsecureSkipVerify,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@service/http_client.go` around lines 280 - 290, The TLS config used for HTTPS
proxy tunnels is missing MinVersion; update the tls.Config passed to tls.Client
in the block that creates tlsConn (the code calling tls.Client(...),
tlsConn.HandshakeContext and hostWithoutPort(proxyURL.Host)) to set MinVersion
to a secure minimum (e.g., tls.VersionTLS12 or your project-wide minimum
constant), keeping InsecureSkipVerify from common.TLSInsecureSkipVerify intact;
ensure you reference the same tls package symbol (tls.VersionTLS12) or a central
constant if one exists.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
web/src/components/settings/SystemSetting.jsx (3)
317-343: Simplify value resolution—nested object fallbacks are unnecessary.Semi UI's Form stores dot-notation field names (e.g.,
'proxy_setting.default_tls_fingerprint') as literal string keys, not as nested objects. The fallbacks toformValues?.proxy_setting?.default_tls_fingerprintwill never match and add confusion.♻️ Suggested simplification
const submitDefaultTLSFingerprint = async () => { const formValues = formApiRef.current?.getValues() || {}; const selectedFingerprint = - formValues['proxy_setting.default_tls_fingerprint'] ?? - formValues?.proxy_setting?.default_tls_fingerprint ?? - inputs['proxy_setting.default_tls_fingerprint'] ?? - inputs?.proxy_setting?.default_tls_fingerprint ?? - ''; + formValues['proxy_setting.default_tls_fingerprint'] ?? + inputs['proxy_setting.default_tls_fingerprint'] ?? + ''; const customSpec = - formValues['proxy_setting.default_tls_custom'] ?? - formValues?.proxy_setting?.default_tls_custom ?? - inputs['proxy_setting.default_tls_custom'] ?? - inputs?.proxy_setting?.default_tls_custom ?? - ''; + formValues['proxy_setting.default_tls_custom'] ?? + inputs['proxy_setting.default_tls_custom'] ?? + '';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/SystemSetting.jsx` around lines 317 - 343, In submitDefaultTLSFingerprint, simplify value resolution by removing the impossible nested-object fallbacks (formValues?.proxy_setting?.default_tls_fingerprint and inputs?.proxy_setting?.default_tls_fingerprint) and only read the flat dot-notation keys that Semi stores (e.g., formValues['proxy_setting.default_tls_fingerprint'] and inputs['proxy_setting.default_tls_fingerprint']); do the same for default_tls_custom (use formValues['proxy_setting.default_tls_custom'] and inputs['proxy_setting.default_tls_custom']), then build the options array the same way and call updateOptions(options).
851-868: Consider validating JSON before submission.The TextArea accepts custom ClientHello JSON, but there's no validation that the input is valid JSON. Invalid JSON sent to the server could cause issues or confusing error messages for users.
🛡️ Suggested validation in submit handler
const submitDefaultTLSFingerprint = async () => { const formValues = formApiRef.current?.getValues() || {}; const selectedFingerprint = formValues['proxy_setting.default_tls_fingerprint'] ?? inputs['proxy_setting.default_tls_fingerprint'] ?? ''; const customSpec = formValues['proxy_setting.default_tls_custom'] ?? inputs['proxy_setting.default_tls_custom'] ?? ''; + if (selectedFingerprint === 'custom' && customSpec) { + try { + JSON.parse(customSpec); + } catch (e) { + showError(t('自定义 TLS ClientHello 规格必须是有效的 JSON 格式')); + return; + } + } + const options = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/SystemSetting.jsx` around lines 851 - 868, The TextArea for 'proxy_setting.default_tls_custom' in SystemSetting.jsx accepts JSON but isn't validated; before submitting (in the submit handler used by this component, e.g., handleSubmit/onSubmit), attempt to JSON.parse the value from inputs['proxy_setting.default_tls_custom'] and if parsing fails set a validation error (or show a user-facing error message) and stop submission; if parsing succeeds, optionally store the parsed object or allow submission. Also consider adding inline validation on change (in the onChange that calls setInputs) to show immediate feedback.
844-850: Simplify the conditional expression.Same unnecessary nested object fallbacks as in
submitDefaultTLSFingerprint. Thevaluesobject from Semi UI Form uses the exact field name string as the key.♻️ Suggested simplification
- {(( - values?.['proxy_setting.default_tls_fingerprint'] ?? - values?.proxy_setting?.default_tls_fingerprint ?? - inputs['proxy_setting.default_tls_fingerprint'] ?? - inputs?.proxy_setting?.default_tls_fingerprint ?? - '' - ) === 'custom') && ( + {(values?.['proxy_setting.default_tls_fingerprint'] ?? + inputs['proxy_setting.default_tls_fingerprint'] ?? + '') === 'custom' && (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@web/src/components/settings/SystemSetting.jsx` around lines 844 - 850, The conditional with multiple nested fallbacks should be simplified: instead of checking both dot-key and nested object forms on values and inputs, read the form field directly by the exact key string (values['proxy_setting.default_tls_fingerprint'] ?? inputs['proxy_setting.default_tls_fingerprint'] ?? '') and compare to 'custom'; remove the unnecessary checks against values?.proxy_setting?.default_tls_fingerprint and inputs?.proxy_setting?.default_tls_fingerprint so the conditional matches the same approach used in submitDefaultTLSFingerprint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@web/src/components/settings/SystemSetting.jsx`:
- Around line 317-343: In submitDefaultTLSFingerprint, simplify value resolution
by removing the impossible nested-object fallbacks
(formValues?.proxy_setting?.default_tls_fingerprint and
inputs?.proxy_setting?.default_tls_fingerprint) and only read the flat
dot-notation keys that Semi stores (e.g.,
formValues['proxy_setting.default_tls_fingerprint'] and
inputs['proxy_setting.default_tls_fingerprint']); do the same for
default_tls_custom (use formValues['proxy_setting.default_tls_custom'] and
inputs['proxy_setting.default_tls_custom']), then build the options array the
same way and call updateOptions(options).
- Around line 851-868: The TextArea for 'proxy_setting.default_tls_custom' in
SystemSetting.jsx accepts JSON but isn't validated; before submitting (in the
submit handler used by this component, e.g., handleSubmit/onSubmit), attempt to
JSON.parse the value from inputs['proxy_setting.default_tls_custom'] and if
parsing fails set a validation error (or show a user-facing error message) and
stop submission; if parsing succeeds, optionally store the parsed object or
allow submission. Also consider adding inline validation on change (in the
onChange that calls setInputs) to show immediate feedback.
- Around line 844-850: The conditional with multiple nested fallbacks should be
simplified: instead of checking both dot-key and nested object forms on values
and inputs, read the form field directly by the exact key string
(values['proxy_setting.default_tls_fingerprint'] ??
inputs['proxy_setting.default_tls_fingerprint'] ?? '') and compare to 'custom';
remove the unnecessary checks against
values?.proxy_setting?.default_tls_fingerprint and
inputs?.proxy_setting?.default_tls_fingerprint so the conditional matches the
same approach used in submitDefaultTLSFingerprint.
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
19 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.
Summary by CodeRabbit
New Features
Improvements