From 288635adfcd9ff3eb1c80a9aadc0880e0fb7cadb Mon Sep 17 00:00:00 2001 From: root Date: Fri, 24 Apr 2026 14:35:28 +0800 Subject: [PATCH] fix: Clash YAML injection, path validation, and default template - Quote all YAML string values with %q to prevent injection - Remove unused host parameter from GetClash - Add backend path normalization for SubClashPath - Log StreamSettings JSON unmarshal errors - Expand template panel by default and provide default template --- config/version | 2 +- .../2026-04-24-fix-clash-yaml-code-quality.md | 18 ++++++ sub/subClashService.go | 60 ++++++++++--------- sub/subController.go | 4 +- web/entity/entity.go | 7 +++ .../settings/panel/subscription/clash.html | 2 +- web/service/setting.go | 21 ++++++- 7 files changed, 80 insertions(+), 34 deletions(-) create mode 100644 docs/Tasktracking/2026-04-24-fix-clash-yaml-code-quality.md diff --git a/config/version b/config/version index 89ab8a23..9ddafeb7 100644 --- a/config/version +++ b/config/version @@ -1 +1 @@ -v1.5.2-beta \ No newline at end of file +v1.5.3-beta \ No newline at end of file diff --git a/docs/Tasktracking/2026-04-24-fix-clash-yaml-code-quality.md b/docs/Tasktracking/2026-04-24-fix-clash-yaml-code-quality.md new file mode 100644 index 00000000..2e88b262 --- /dev/null +++ b/docs/Tasktracking/2026-04-24-fix-clash-yaml-code-quality.md @@ -0,0 +1,18 @@ +# Fix Clash YAML Subscription Code Quality Issues + +**Date:** 2026-04-24 +**Type:** bugfix / code-quality +**Version:** v1.5.3-beta + +## Issues Fixed + +- **YAML injection (C2):** All string values in Clash YAML generation changed from `%s` to `%q` for proper quoting +- **Unused parameter (I1):** Removed unused `host` parameter from `GetClash` method +- **Path validation (I3):** Added backend normalization for `SubClashPath` (auto-add leading/trailing `/`) +- **Silent JSON error (I4):** Added warning log when `StreamSettings` JSON unmarshal fails +- **Template visibility:** Clash YAML template panel now expanded by default +- **Default template:** Added sensible default Clash YAML template in settings defaults + +## Note + +C1 from review (nil `inboundService` panic) was a false alarm — `InboundService` is a value type, not a pointer. diff --git a/sub/subClashService.go b/sub/subClashService.go index bb58836a..bc5f46f4 100644 --- a/sub/subClashService.go +++ b/sub/subClashService.go @@ -26,8 +26,8 @@ func NewSubClashService(template string, subService *SubService) *SubClashServic } } -// GetClash generates a Clash YAML configuration for the given subscription ID and host. -func (s *SubClashService) GetClash(subId string, host string) (string, string, error) { +// GetClash generates a Clash YAML configuration for the given subscription ID. +func (s *SubClashService) GetClash(subId string) (string, string, error) { if s.template == "" { return "", "", fmt.Errorf("clash template is empty") } @@ -112,7 +112,9 @@ func (s *SubClashService) GetClash(subId string, host string) (string, string, e func (s *SubClashService) getProxy(inbound *model.Inbound, client model.Client) []string { var proxies []string var stream map[string]any - json.Unmarshal([]byte(inbound.StreamSettings), &stream) + if err := json.Unmarshal([]byte(inbound.StreamSettings), &stream); err != nil { + logger.Warning("SubClashService - failed to parse StreamSettings for inbound", inbound.Tag, ":", err) + } // Resolve address var address string @@ -184,34 +186,34 @@ func (s *SubClashService) buildProxyEntry(inbound *model.Inbound, client model.C switch inbound.Protocol { case model.VMESS: parts = append(parts, "type: vmess") - parts = append(parts, fmt.Sprintf("server: %s", address)) + parts = append(parts, fmt.Sprintf("server: %q", address)) parts = append(parts, fmt.Sprintf("port: %d", port)) - parts = append(parts, fmt.Sprintf("uuid: %s", client.ID)) + parts = append(parts, fmt.Sprintf("uuid: %q", client.ID)) parts = append(parts, "alterId: 0") parts = append(parts, "cipher: auto") case model.VLESS: parts = append(parts, "type: vless") - parts = append(parts, fmt.Sprintf("server: %s", address)) + parts = append(parts, fmt.Sprintf("server: %q", address)) parts = append(parts, fmt.Sprintf("port: %d", port)) - parts = append(parts, fmt.Sprintf("uuid: %s", client.ID)) + parts = append(parts, fmt.Sprintf("uuid: %q", client.ID)) if client.Flow != "" { - parts = append(parts, fmt.Sprintf("flow: %s", client.Flow)) + parts = append(parts, fmt.Sprintf("flow: %q", client.Flow)) } case model.Trojan: parts = append(parts, "type: trojan") - parts = append(parts, fmt.Sprintf("server: %s", address)) + parts = append(parts, fmt.Sprintf("server: %q", address)) parts = append(parts, fmt.Sprintf("port: %d", port)) - parts = append(parts, fmt.Sprintf("password: %s", client.Password)) + parts = append(parts, fmt.Sprintf("password: %q", client.Password)) case model.Shadowsocks: parts = append(parts, "type: ss") - parts = append(parts, fmt.Sprintf("server: %s", address)) + parts = append(parts, fmt.Sprintf("server: %q", address)) parts = append(parts, fmt.Sprintf("port: %d", port)) cipher, password := s.parseShadowsocksSettings(client) - parts = append(parts, fmt.Sprintf("cipher: %s", cipher)) - parts = append(parts, fmt.Sprintf("password: %s", password)) + parts = append(parts, fmt.Sprintf("cipher: %q", cipher)) + parts = append(parts, fmt.Sprintf("password: %q", password)) parts = append(parts, "udp: true") return strings.Join(parts, "\n ") } @@ -222,9 +224,9 @@ func (s *SubClashService) buildProxyEntry(inbound *model.Inbound, client model.C if security == "reality" { realitySetting, _ := stream["realitySettings"].(map[string]any) if publicKey, ok := realitySetting["publicKey"].(string); ok && publicKey != "" { - realityOpts := fmt.Sprintf("reality-opts:\n public-key: %s", publicKey) + realityOpts := fmt.Sprintf("reality-opts:\n public-key: %q", publicKey) if shortId, ok := realitySetting["shortId"].(string); ok && shortId != "" { - realityOpts += fmt.Sprintf("\n short-id: %s", shortId) + realityOpts += fmt.Sprintf("\n short-id: %q", shortId) } parts = append(parts, realityOpts) } @@ -232,13 +234,13 @@ func (s *SubClashService) buildProxyEntry(inbound *model.Inbound, client model.C serverNames, _ := realitySetting["serverNames"].([]any) if len(serverNames) > 0 { sni := fmt.Sprintf("%v", serverNames[0]) - parts = append(parts, fmt.Sprintf("sni: %s", sni)) + parts = append(parts, fmt.Sprintf("sni: %q", sni)) } } else { // TLS settings tlsSetting, _ := stream["tlsSettings"].(map[string]any) if serverName, ok := tlsSetting["serverName"].(string); ok && serverName != "" { - parts = append(parts, fmt.Sprintf("sni: %s", serverName)) + parts = append(parts, fmt.Sprintf("sni: %q", serverName)) } if alpn, ok := tlsSetting["alpn"].([]any); ok && len(alpn) > 0 { alpnStrs := make([]string, len(alpn)) @@ -250,7 +252,7 @@ func (s *SubClashService) buildProxyEntry(inbound *model.Inbound, client model.C } // Fingerprint if fp, ok := stream["fingerprint"].(string); ok && fp != "" { - parts = append(parts, fmt.Sprintf("client-fingerprint: %s", fp)) + parts = append(parts, fmt.Sprintf("client-fingerprint: %q", fp)) } } else { parts = append(parts, "tls: false") @@ -263,13 +265,13 @@ func (s *SubClashService) buildProxyEntry(inbound *model.Inbound, client model.C case "ws": ws, _ := stream["wsSettings"].(map[string]any) if path, ok := ws["path"].(string); ok && path != "" { - wsOpts := fmt.Sprintf("ws-opts:\n path: %s", path) + wsOpts := fmt.Sprintf("ws-opts:\n path: %q", path) if host, ok := ws["host"].(string); ok && host != "" { - wsOpts += fmt.Sprintf("\n headers:\n Host: %s", host) + wsOpts += fmt.Sprintf("\n headers:\n Host: %q", host) } else { headers, _ := ws["headers"].(map[string]any) if h, ok := headers["Host"].(string); ok && h != "" { - wsOpts += fmt.Sprintf("\n headers:\n Host: %s", h) + wsOpts += fmt.Sprintf("\n headers:\n Host: %q", h) } } parts = append(parts, wsOpts) @@ -278,17 +280,17 @@ func (s *SubClashService) buildProxyEntry(inbound *model.Inbound, client model.C case "grpc": grpc, _ := stream["grpcSettings"].(map[string]any) if serviceName, ok := grpc["serviceName"].(string); ok && serviceName != "" { - parts = append(parts, fmt.Sprintf("grpc-opts:\n grpc-service-name: %s", serviceName)) + parts = append(parts, fmt.Sprintf("grpc-opts:\n grpc-service-name: %q", serviceName)) } case "h2": h2, _ := stream["h2Settings"].(map[string]any) if path, ok := h2["path"].(string); ok && path != "" { - h2Opts := fmt.Sprintf("h2-opts:\n path: %s", path) + h2Opts := fmt.Sprintf("h2-opts:\n path: %q", path) if host, ok := h2["host"].([]any); ok && len(host) > 0 { hostStrs := make([]string, len(host)) for i, h := range host { - hostStrs[i] = fmt.Sprintf("%v", h) + hostStrs[i] = fmt.Sprintf("%q", fmt.Sprintf("%v", h)) } h2Opts += fmt.Sprintf("\n host: [%s]", strings.Join(hostStrs, ", ")) } @@ -302,13 +304,13 @@ func (s *SubClashService) buildProxyEntry(inbound *model.Inbound, client model.C request, _ := header["request"].(map[string]any) httpOpts := "http-opts:" if path, ok := request["path"].([]any); ok && len(path) > 0 { - httpOpts += fmt.Sprintf("\n path:\n - %v", path[0]) + httpOpts += fmt.Sprintf("\n path:\n - %q", fmt.Sprintf("%v", path[0])) } if headers, ok := request["headers"].(map[string]any); ok && len(headers) > 0 { httpOpts += "\n headers:" for k, v := range headers { if vals, ok := v.([]any); ok && len(vals) > 0 { - httpOpts += fmt.Sprintf("\n %s:\n - %v", k, vals[0]) + httpOpts += fmt.Sprintf("\n %s:\n - %q", k, fmt.Sprintf("%v", vals[0])) } } } @@ -318,13 +320,13 @@ func (s *SubClashService) buildProxyEntry(inbound *model.Inbound, client model.C case "httpupgrade": hu, _ := stream["httpupgradeSettings"].(map[string]any) if path, ok := hu["path"].(string); ok && path != "" { - huOpts := fmt.Sprintf("httpupgrade-opts:\n path: %s", path) + huOpts := fmt.Sprintf("httpupgrade-opts:\n path: %q", path) if host, ok := hu["host"].(string); ok && host != "" { - huOpts += fmt.Sprintf("\n host: %s", host) + huOpts += fmt.Sprintf("\n host: %q", host) } else { headers, _ := hu["headers"].(map[string]any) if h, ok := headers["Host"].(string); ok && h != "" { - huOpts += fmt.Sprintf("\n host: %s", h) + huOpts += fmt.Sprintf("\n host: %q", h) } } parts = append(parts, huOpts) diff --git a/sub/subController.go b/sub/subController.go index 38648af8..94c26a60 100644 --- a/sub/subController.go +++ b/sub/subController.go @@ -199,8 +199,8 @@ func (a *SUBController) subJsons(c *gin.Context) { // clashSubs handles HTTP requests for Clash YAML subscription configurations. func (a *SUBController) clashSubs(c *gin.Context) { subId := c.Param("subid") - scheme, host, hostWithPort, _ := a.subService.ResolveRequest(c) - clashYaml, header, err := a.subClashService.GetClash(subId, host) + scheme, _, hostWithPort, _ := a.subService.ResolveRequest(c) + clashYaml, header, err := a.subClashService.GetClash(subId) if err != nil || len(clashYaml) == 0 { c.String(400, "Error!") } else { diff --git a/web/entity/entity.go b/web/entity/entity.go index f0214e92..9a3541e0 100644 --- a/web/entity/entity.go +++ b/web/entity/entity.go @@ -185,6 +185,13 @@ func (s *AllSetting) CheckValid() error { s.SubJsonPath += "/" } + if !strings.HasPrefix(s.SubClashPath, "/") { + s.SubClashPath = "/" + s.SubClashPath + } + if !strings.HasSuffix(s.SubClashPath, "/") { + s.SubClashPath += "/" + } + _, err := time.LoadLocation(s.TimeLocation) if err != nil { return common.NewError("time location not exist:", s.TimeLocation) diff --git a/web/html/settings/panel/subscription/clash.html b/web/html/settings/panel/subscription/clash.html index 12a260af..1c28a644 100644 --- a/web/html/settings/panel/subscription/clash.html +++ b/web/html/settings/panel/subscription/clash.html @@ -1,5 +1,5 @@ {{define "settings/panel/subscription/clash"}} - + diff --git a/web/service/setting.go b/web/service/setting.go index 4b64851e..208496e6 100644 --- a/web/service/setting.go +++ b/web/service/setting.go @@ -81,7 +81,26 @@ var defaultValueMap = map[string]string{ "subClashEnable": "false", "subClashPath": "/clash/", "subClashURI": "", - "subClashTemplate": "", + "subClashTemplate": `port: 7890 +socks-port: 7891 +allow-lan: false +mode: rule +log-level: info +proxies: [] +proxy-groups: + - name: Proxy + type: select + proxies: + - DIRECT +dns: + enable: true + enhanced-mode: fake-ip + nameserver: + - 8.8.8.8 + - 1.1.1.1 +rules: + - GEOIP,LAN,DIRECT + - MATCH,Proxy`, "datepicker": "gregorian", "warp": "", "externalTrafficInformEnable": "false",