From ac8cb505d1e8ac92bbe8b88d639a8192ff4c04b1 Mon Sep 17 00:00:00 2001 From: n0ctal <4c866w5fn9@privaterelay.appleid.com> Date: Mon, 15 Jun 2026 19:23:47 +0500 Subject: [PATCH] fix(subscriptions): avoid shared mutable state during generation (#5270) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(subscriptions): avoid shared mutable state during generation * fix(subscriptions): serve external-link-only subs in JSON/Clash; load remark settings per request The ForRequest refactor added an early `len(inbounds) == 0` return to GetJson/GetClash that fired before external links were fetched, so a subscription whose only entries are external links (or whose inbounds are all disabled) rendered empty in the JSON and Clash formats. Drop the premature check — the existing inbounds+externalLinks empty guard already covers the truly-empty case. Also load datepicker/emailInRemark in PrepareForRequest rather than only in getSubs, so JSON and Clash remarks honor these settings instead of seeing the zero values (emailInRemark previously depended on the shared-state leak this PR fixes). Add a regression test covering an external-link-only sub across both formats. --------- Co-authored-by: Sanaei --- internal/sub/build_urls_test.go | 21 +++++++++++ internal/sub/clash_service.go | 31 ++++++++-------- internal/sub/clash_service_test.go | 10 ++--- internal/sub/controller.go | 7 ++-- internal/sub/external_only_sub_test.go | 51 ++++++++++++++++++++++++++ internal/sub/json_service.go | 22 +++++------ internal/sub/service.go | 49 +++++++++++++++++-------- 7 files changed, 139 insertions(+), 52 deletions(-) create mode 100644 internal/sub/external_only_sub_test.go diff --git a/internal/sub/build_urls_test.go b/internal/sub/build_urls_test.go index deb7afc9f..42517b20b 100644 --- a/internal/sub/build_urls_test.go +++ b/internal/sub/build_urls_test.go @@ -70,6 +70,27 @@ func TestBuildURLs_EmptySubId(t *testing.T) { } } +func TestForRequestDoesNotMutateSharedService(t *testing.T) { + initSubDB(t) + base := &SubService{} + + first := base.ForRequest("first.example.com") + second := base.ForRequest("second.example.com") + + if base.address != "" || base.nodesByID != nil { + t.Fatalf("ForRequest mutated the shared service: address=%q nodes=%v", base.address, base.nodesByID) + } + + firstURL, _, _ := first.BuildURLs("/sub/", "/json/", "/clash/", "ABC") + secondURL, _, _ := second.BuildURLs("/sub/", "/json/", "/clash/", "ABC") + if !strings.Contains(firstURL, "first.example.com") { + t.Fatalf("first request URL = %q, want first.example.com", firstURL) + } + if !strings.Contains(secondURL, "second.example.com") { + t.Fatalf("second request URL = %q, want second.example.com", secondURL) + } +} + // A subscriber arriving via a reverse proxy (subURI configured with full // HTTPS URL) must see the same scheme+host in the JSON and Clash Copy // URLs as in the main subURL — not the raw sub-server port 2096. diff --git a/internal/sub/clash_service.go b/internal/sub/clash_service.go index fabd894dc..29fdaca22 100644 --- a/internal/sub/clash_service.go +++ b/internal/sub/clash_service.go @@ -22,13 +22,12 @@ func NewSubClashService(enableRouting bool, clashRules string, subService *SubSe } func (s *SubClashService) GetClash(subId string, host string) (string, string, error) { - // Set per-request state so resolveInboundAddress sees the node map. - s.SubService.PrepareForRequest(host) - inbounds, err := s.SubService.getInboundsBySubId(subId) + subReq := s.SubService.ForRequest(host) + inbounds, err := subReq.getInboundsBySubId(subId) if err != nil { return "", "", err } - externalLinks, err := s.SubService.getClientExternalLinksBySubId(subId) + externalLinks, err := subReq.getClientExternalLinksBySubId(subId) if err != nil { return "", "", err } @@ -40,14 +39,14 @@ func (s *SubClashService) GetClash(subId string, host string) (string, string, e seenEmails := make(map[string]struct{}) for _, inbound := range inbounds { - clients := s.SubService.matchingClients(inbound, subId) + clients := subReq.matchingClients(inbound, subId) if len(clients) == 0 { continue } - s.SubService.projectThroughFallbackMaster(inbound) + subReq.projectThroughFallbackMaster(inbound) for _, client := range clients { seenEmails[client.Email] = struct{}{} - proxies = append(proxies, s.getProxies(inbound, client, host)...) + proxies = append(proxies, s.getProxies(subReq, inbound, client, host)...) } } for _, ext := range externalLinks { @@ -73,7 +72,7 @@ func (s *SubClashService) GetClash(subId string, host string) (string, string, e for e := range seenEmails { emails = append(emails, e) } - traffic, _ := s.SubService.AggregateTrafficByEmails(emails) + traffic, _ := subReq.AggregateTrafficByEmails(emails) proxyNames := make([]string, 0, len(proxies)+1) for _, proxy := range proxies { @@ -140,12 +139,12 @@ func fallbackProxyName(proxy map[string]any, idx int) string { return fmt.Sprintf("proxy-%d", idx+1) } -func (s *SubClashService) getProxies(inbound *model.Inbound, client model.Client, host string) []map[string]any { +func (s *SubClashService) getProxies(subReq *SubService, inbound *model.Inbound, client model.Client, host string) []map[string]any { stream := s.streamData(inbound.StreamSettings) // For node-managed inbounds the Clash proxy "server" must be the // node's address, not the request host. resolveInboundAddress handles // the node→subscriber-host fallback chain. - defaultDest := s.SubService.resolveInboundAddress(inbound) + defaultDest := subReq.resolveInboundAddress(inbound) if defaultDest == "" { defaultDest = host } @@ -187,7 +186,7 @@ func (s *SubClashService) getProxies(inbound *model.Inbound, client model.Client applyExternalProxyTLSToStream(extPrxy, workingStream, security) } - proxy := s.buildProxy(&workingInbound, client, workingStream, extPrxy["remark"].(string)) + proxy := s.buildProxy(subReq, &workingInbound, client, workingStream, extPrxy["remark"].(string)) if len(proxy) > 0 { proxies = append(proxies, proxy) } @@ -195,15 +194,15 @@ func (s *SubClashService) getProxies(inbound *model.Inbound, client model.Client return proxies } -func (s *SubClashService) buildProxy(inbound *model.Inbound, client model.Client, stream map[string]any, extraRemark string) map[string]any { +func (s *SubClashService) buildProxy(subReq *SubService, inbound *model.Inbound, client model.Client, stream map[string]any, extraRemark string) map[string]any { // Hysteria has its own transport + TLS model, applyTransport / // applySecurity don't fit. if inbound.Protocol == model.Hysteria { - return s.buildHysteriaProxy(inbound, client, extraRemark) + return s.buildHysteriaProxy(subReq, inbound, client, extraRemark) } proxy := map[string]any{ - "name": s.SubService.genRemark(inbound, client.Email, extraRemark), + "name": subReq.genRemark(inbound, client.Email, extraRemark), "server": inbound.Listen, "port": inbound.Port, "udp": true, @@ -274,7 +273,7 @@ func (s *SubClashService) buildProxy(inbound *model.Inbound, client model.Client // directly instead of going through streamData/tlsData, because those // helpers prune fields (like `allowInsecure` / the salamander obfs // block) that the hysteria proxy wants preserved. -func (s *SubClashService) buildHysteriaProxy(inbound *model.Inbound, client model.Client, extraRemark string) map[string]any { +func (s *SubClashService) buildHysteriaProxy(subReq *SubService, inbound *model.Inbound, client model.Client, extraRemark string) map[string]any { var inboundSettings map[string]any _ = json.Unmarshal([]byte(inbound.Settings), &inboundSettings) @@ -286,7 +285,7 @@ func (s *SubClashService) buildHysteriaProxy(inbound *model.Inbound, client mode } proxy := map[string]any{ - "name": s.SubService.genRemark(inbound, client.Email, extraRemark), + "name": subReq.genRemark(inbound, client.Email, extraRemark), "type": proxyType, "server": inbound.Listen, "port": inbound.Port, diff --git a/internal/sub/clash_service_test.go b/internal/sub/clash_service_test.go index 450aea0c1..f38572d90 100644 --- a/internal/sub/clash_service_test.go +++ b/internal/sub/clash_service_test.go @@ -56,7 +56,7 @@ func TestBuildProxy_VLESSRealityFieldsForClash(t *testing.T) { "realitySettings": map[string]any{"serverName": "reality.example.com", "publicKey": "PBKvalue", "shortId": "ab12", "fingerprint": "chrome"}, } - proxy := svc.buildProxy(inbound, client, stream, "") + proxy := svc.buildProxy(svc.SubService, inbound, client, stream, "") if proxy == nil { t.Fatal("buildProxy returned nil for a valid reality stream") } @@ -199,7 +199,7 @@ func TestBuildProxy_VLESSPostQuantumEncryptionUsesMihomoEncryptionField(t *testi }, } - proxy := svc.buildProxy(inbound, client, stream, "") + proxy := svc.buildProxy(svc.SubService, inbound, client, stream, "") if proxy["encryption"] != encryption { t.Fatalf("encryption = %v, want %q", proxy["encryption"], encryption) @@ -231,7 +231,7 @@ func TestBuildProxy_VLESSFlowXhttpRealityVlessenc(t *testing.T) { }, } - proxy := svc.buildProxy(inbound, client, stream, "") + proxy := svc.buildProxy(svc.SubService, inbound, client, stream, "") if proxy["flow"] != "xtls-rprx-vision" { t.Fatalf("xhttp+reality+vlessenc Clash proxy must carry the vision flow (#5232): %#v", proxy) @@ -256,7 +256,7 @@ func TestBuildProxy_VLESSFlowDroppedWithoutVisionSupport(t *testing.T) { }, } - proxy := svc.buildProxy(inbound, client, stream, "") + proxy := svc.buildProxy(svc.SubService, inbound, client, stream, "") if _, ok := proxy["flow"]; ok { t.Fatalf("tcp without tls/reality must not carry a flow: %#v", proxy) @@ -281,7 +281,7 @@ func TestBuildProxy_VLESSNoneEncryptionOmittedForClash(t *testing.T) { }, } - proxy := svc.buildProxy(inbound, client, stream, "") + proxy := svc.buildProxy(svc.SubService, inbound, client, stream, "") if _, ok := proxy["encryption"]; ok { t.Fatalf("plain vless encryption should be omitted for mihomo: %#v", proxy) diff --git a/internal/sub/controller.go b/internal/sub/controller.go index 7ffe93ead..95d93bd38 100644 --- a/internal/sub/controller.go +++ b/internal/sub/controller.go @@ -137,7 +137,8 @@ func (a *SUBController) initRouter(g *gin.RouterGroup) { func (a *SUBController) subs(c *gin.Context) { subId := c.Param("subid") scheme, host, hostWithPort, hostHeader := a.subService.ResolveRequest(c) - subs, emails, lastOnline, traffic, err := a.subService.GetSubs(subId, host) + subReq := a.subService.ForRequest(host) + subs, emails, lastOnline, traffic, err := subReq.getSubs(subId) if err != nil || len(subs) == 0 { writeSubError(c, err) } else { @@ -149,7 +150,7 @@ func (a *SUBController) subs(c *gin.Context) { // If the request expects HTML (e.g., browser) or explicitly asked (?html=1 or ?view=html), render the info page here accept := c.GetHeader("Accept") if strings.Contains(strings.ToLower(accept), "text/html") || c.Query("html") == "1" || strings.EqualFold(c.Query("view"), "html") { - subURL, subJsonURL, subClashURL := a.subService.BuildURLs(a.subPath, a.subJsonPath, a.subClashPath, subId) + subURL, subJsonURL, subClashURL := subReq.BuildURLs(a.subPath, a.subJsonPath, a.subClashPath, subId) if !a.jsonEnabled { subJsonURL = "" } @@ -161,7 +162,7 @@ func (a *SUBController) subs(c *gin.Context) { basePath = "/" } basePathStr := basePath.(string) - page := a.subService.BuildPageData(subId, hostHeader, traffic, lastOnline, subs, emails, subURL, subJsonURL, subClashURL, basePathStr, a.subTitle, a.subSupportUrl) + page := subReq.BuildPageData(subId, hostHeader, traffic, lastOnline, subs, emails, subURL, subJsonURL, subClashURL, basePathStr, a.subTitle, a.subSupportUrl) a.serveSubPage(c, basePathStr, page) return } diff --git a/internal/sub/external_only_sub_test.go b/internal/sub/external_only_sub_test.go new file mode 100644 index 000000000..49c1b008c --- /dev/null +++ b/internal/sub/external_only_sub_test.go @@ -0,0 +1,51 @@ +package sub + +import ( + "strings" + "testing" + + "github.com/mhsanaei/3x-ui/v3/internal/database" + "github.com/mhsanaei/3x-ui/v3/internal/database/model" +) + +// A subscription whose only entries are external links — no enabled standard +// inbound — must still render in the JSON and Clash formats, not just the raw +// one. Regression guard for the premature len(inbounds)==0 early return that +// short-circuited GetJson/GetClash before external links were ever fetched. +func TestJsonAndClashServeExternalLinkOnlySub(t *testing.T) { + initSubDB(t) + db := database.GetDB() + + rec := &model.ClientRecord{Email: "ext@x", SubID: "ext-only", UUID: "ext-uuid", Enable: true} + if err := db.Create(rec).Error; err != nil { + t.Fatalf("seed client: %v", err) + } + link := "vless://11111111-1111-1111-1111-111111111111@example.com:443?type=tcp&security=reality&pbk=abc&sid=12&fp=chrome#orig" + if err := db.Create(&model.ClientExternalLink{ClientId: rec.Id, Kind: model.ExternalLinkKindLink, Value: link, Remark: "DE-Provider", SortIndex: 1}).Error; err != nil { + t.Fatalf("seed external link: %v", err) + } + + base := NewSubService(false, "-io") + + jsonOut, _, err := NewSubJsonService("", "", "", base).GetJson("ext-only", "sub.example.com") + if err != nil { + t.Fatalf("GetJson err = %v", err) + } + if jsonOut == "" { + t.Fatal("GetJson returned empty for an external-link-only sub") + } + if !strings.Contains(jsonOut, "DE-Provider") { + t.Fatalf("GetJson missing external remark: %s", jsonOut) + } + + clashOut, _, err := NewSubClashService(false, "", base).GetClash("ext-only", "sub.example.com") + if err != nil { + t.Fatalf("GetClash err = %v", err) + } + if clashOut == "" { + t.Fatal("GetClash returned empty for an external-link-only sub") + } + if !strings.Contains(clashOut, "DE-Provider") { + t.Fatalf("GetClash missing external proxy: %s", clashOut) + } +} diff --git a/internal/sub/json_service.go b/internal/sub/json_service.go index 4fa6f127f..6389885b9 100644 --- a/internal/sub/json_service.go +++ b/internal/sub/json_service.go @@ -58,14 +58,12 @@ func NewSubJsonService(mux string, rules string, finalMask string, subService *S // GetJson generates a JSON subscription configuration for the given subscription ID and host. func (s *SubJsonService) GetJson(subId string, host string) (string, string, error) { - // Set per-request state on the shared SubService so any - // resolveInboundAddress call inside picks node-aware host values. - s.SubService.PrepareForRequest(host) - inbounds, err := s.SubService.getInboundsBySubId(subId) + subReq := s.SubService.ForRequest(host) + inbounds, err := subReq.getInboundsBySubId(subId) if err != nil { return "", "", err } - externalLinks, err := s.SubService.getClientExternalLinksBySubId(subId) + externalLinks, err := subReq.getClientExternalLinksBySubId(subId) if err != nil { return "", "", err } @@ -79,15 +77,15 @@ func (s *SubJsonService) GetJson(subId string, host string) (string, string, err seenEmails := make(map[string]struct{}) // Prepare Inbounds for _, inbound := range inbounds { - clients := s.SubService.matchingClients(inbound, subId) + clients := subReq.matchingClients(inbound, subId) if len(clients) == 0 { continue } - s.SubService.projectThroughFallbackMaster(inbound) + subReq.projectThroughFallbackMaster(inbound) for _, client := range clients { seenEmails[client.Email] = struct{}{} - configArray = append(configArray, s.getConfig(inbound, client, host)...) + configArray = append(configArray, s.getConfig(subReq, inbound, client, host)...) } } for _, ext := range externalLinks { @@ -120,7 +118,7 @@ func (s *SubJsonService) GetJson(subId string, host string) (string, string, err for e := range seenEmails { emails = append(emails, e) } - traffic, _ := s.SubService.AggregateTrafficByEmails(emails) + traffic, _ := subReq.AggregateTrafficByEmails(emails) // Combile outbounds var finalJson []byte @@ -134,7 +132,7 @@ func (s *SubJsonService) GetJson(subId string, host string) (string, string, err return string(finalJson), header, nil } -func (s *SubJsonService) getConfig(inbound *model.Inbound, client model.Client, host string) []json_util.RawMessage { +func (s *SubJsonService) getConfig(subReq *SubService, inbound *model.Inbound, client model.Client, host string) []json_util.RawMessage { var newJsonArray []json_util.RawMessage stream := s.streamData(inbound.StreamSettings) @@ -143,7 +141,7 @@ func (s *SubJsonService) getConfig(inbound *model.Inbound, client model.Client, // For node-managed inbounds we want the node's address — request // host won't reach the right xray. resolveInboundAddress already // implements the node→subscriber-host fallback chain. - defaultDest := s.SubService.resolveInboundAddress(inbound) + defaultDest := subReq.resolveInboundAddress(inbound) if defaultDest == "" { defaultDest = host } @@ -204,7 +202,7 @@ func (s *SubJsonService) getConfig(inbound *model.Inbound, client model.Client, maps.Copy(newConfigJson, s.configJson) newConfigJson["outbounds"] = newOutbounds - newConfigJson["remarks"] = s.SubService.genRemark(inbound, client.Email, extPrxy["remark"].(string)) + newConfigJson["remarks"] = subReq.genRemark(inbound, client.Email, extPrxy["remark"].(string)) newConfig, _ := json.MarshalIndent(newConfigJson, "", " ") newJsonArray = append(newJsonArray, newConfig) diff --git a/internal/sub/service.go b/internal/sub/service.go index 2afacfb02..60b637d13 100644 --- a/internal/sub/service.go +++ b/internal/sub/service.go @@ -49,11 +49,18 @@ func NewSubService(showInfo bool, remarkModel string) *SubService { } } -// PrepareForRequest sets per-request state (host + nodes map) on the -// shared SubService. Called by every entry point — GetSubs, GetJson, -// GetClash — so resolveInboundAddress sees the right host and the -// freshly-loaded node map regardless of which sub flavour the client -// hit. +// ForRequest returns a shallow copy with request-scoped state populated. +// Subscription controllers share one base SubService, so request-specific +// fields such as address and nodesByID must live on a per-request copy. +func (s *SubService) ForRequest(host string) *SubService { + req := *s + req.PrepareForRequest(host) + return &req +} + +// PrepareForRequest sets per-request state (host + nodes map) on this +// SubService instance. HTTP handlers should call ForRequest instead so the +// controller's shared base service is never mutated by concurrent requests. func (s *SubService) PrepareForRequest(host string) { if !isRoutableHost(host) { if d := s.configuredPublicHost(); d != "" { @@ -64,6 +71,23 @@ func (s *SubService) PrepareForRequest(host string) { } s.address = host s.loadNodes() + s.loadRemarkSettings() +} + +// loadRemarkSettings populates the per-request remark formatting state so +// every subscription format — raw, JSON, Clash — renders remarks the same +// way. genRemark reads emailInRemark and the date formatter reads datepicker; +// loading these only in getSubs left JSON/Clash with the zero values. +func (s *SubService) loadRemarkSettings() { + var err error + s.datepicker, err = s.settingService.GetDatepicker() + if err != nil { + s.datepicker = "gregorian" + } + s.emailInRemark, err = s.settingService.GetSubEmailInRemark() + if err != nil { + s.emailInRemark = true + } } func (s *SubService) configuredPublicHost() string { @@ -139,7 +163,10 @@ func (s *SubService) matchingClients(inbound *model.Inbound, subId string) []mod // GetSubs retrieves subscription links for a given subscription ID and host. func (s *SubService) GetSubs(subId string, host string) ([]string, []string, int64, xray.ClientTraffic, error) { - s.PrepareForRequest(host) + return s.ForRequest(host).getSubs(subId) +} + +func (s *SubService) getSubs(subId string) ([]string, []string, int64, xray.ClientTraffic, error) { var result []string var emails []string var traffic xray.ClientTraffic @@ -157,16 +184,6 @@ func (s *SubService) GetSubs(subId string, host string) ([]string, []string, int return nil, nil, 0, traffic, nil } - s.datepicker, err = s.settingService.GetDatepicker() - if err != nil { - s.datepicker = "gregorian" - } - - s.emailInRemark, err = s.settingService.GetSubEmailInRemark() - if err != nil { - s.emailInRemark = true - } - seenEmails := make(map[string]struct{}) for _, inbound := range inbounds { clients := s.matchingClients(inbound, subId)