diff --git a/database/model/model.go b/database/model/model.go index 0e0eb714..5c557ab1 100644 --- a/database/model/model.go +++ b/database/model/model.go @@ -49,6 +49,26 @@ func IsSocksLike(p Protocol) bool { return p == Socks || p == Mixed } +// IsAccountBased returns true for protocols whose Xray inbound config +// stores user credentials under settings.accounts[] (an array of +// {user, pass} objects) rather than settings.clients[] (the +// Client struct with id/password/auth/email/etc.). +// +// This currently covers Socks, Mixed, and HTTP — all three inbounds +// share the same proxy/socks-style Account wire type. The panel's +// client-lifecycle code paths (AddInboundClient / UpdateInboundClient / +// DelInboundClient, depletion, traffic reset, telegram bot 'add client' +// keyboard, …) all assume settings.clients[] and would either panic +// or silently corrupt the JSON if pointed at an account-based inbound, +// so they bail out early on this helper. +// +// WireGuard is intentionally NOT in this set: its peers live under +// settings.peers[] and the panel manages them through a separate +// dedicated path, not the client lifecycle covered here. +func IsAccountBased(p Protocol) bool { + return p == Socks || p == Mixed || p == HTTP +} + // User represents a user account in the 3x-ui panel. type User struct { Id int `json:"id" gorm:"primaryKey;autoIncrement"` diff --git a/database/model/model_test.go b/database/model/model_test.go index aa7b6b40..d4d668a7 100644 --- a/database/model/model_test.go +++ b/database/model/model_test.go @@ -59,3 +59,45 @@ func TestIsSocksLike(t *testing.T) { } } } + +// TestIsAccountBased pins the set of inbounds that route credentials +// through settings.accounts[] rather than settings.clients[]. Anything +// that returns true here is OFF-LIMITS for the client-lifecycle code +// paths (AddInboundClient / UpdateInboundClient / DelInboundClient, +// depletion, traffic reset, telegram 'add client' keyboard). +// +// HTTP is intentionally included even though the panel UI doesn't +// currently surface it as a standalone protocol — the runtime API +// AddUser branch in xray/api.go handles it symmetrically with socks, +// and any future UI work plugging HTTP back in inherits the same +// safety net for free. +// +// WireGuard is intentionally EXCLUDED: its peers live under +// settings.peers[] and are managed through a separate path; treating +// it as account-based would lock out the existing wireguard UI. +func TestIsAccountBased(t *testing.T) { + cases := []struct { + in Protocol + want bool + }{ + {Socks, true}, + {Mixed, true}, + {HTTP, true}, + + {VLESS, false}, + {VMESS, false}, + {Trojan, false}, + {Shadowsocks, false}, + {WireGuard, false}, // peers, not accounts; managed separately + {Hysteria, false}, + {Hysteria2, false}, + {Tunnel, false}, + {Protocol(""), false}, + {Protocol("SOCKS"), false}, // case-sensitive lowercase invariant + } + for _, c := range cases { + if got := IsAccountBased(c.in); got != c.want { + t.Errorf("IsAccountBased(%q) = %v, want %v", c.in, got, c.want) + } + } +} diff --git a/web/service/inbound.go b/web/service/inbound.go index 16bb2528..ae0abb2e 100644 --- a/web/service/inbound.go +++ b/web/service/inbound.go @@ -289,7 +289,7 @@ func (s *InboundService) normalizeStreamSettings(inbound *model.Inbound) { model.Hysteria: true, model.Hysteria2: true, } - + if !protocolsWithStream[inbound.Protocol] { inbound.StreamSettings = "" } @@ -302,7 +302,7 @@ func (s *InboundService) normalizeStreamSettings(inbound *model.Inbound) { func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, bool, error) { // Normalize streamSettings based on protocol s.normalizeStreamSettings(inbound) - + exist, err := s.checkPortConflict(inbound, 0) if err != nil { return inbound, false, err @@ -558,7 +558,7 @@ func (s *InboundService) SetInboundEnable(id int, enable bool) (bool, error) { func (s *InboundService) UpdateInbound(inbound *model.Inbound) (*model.Inbound, bool, error) { // Normalize streamSettings based on protocol s.normalizeStreamSettings(inbound) - + exist, err := s.checkPortConflict(inbound, inbound.Id) if err != nil { return inbound, false, err @@ -840,6 +840,26 @@ func (s *InboundService) updateClientTraffics(tx *gorm.DB, oldInbound *model.Inb } func (s *InboundService) AddInboundClient(data *model.Inbound) (bool, error) { + // Account-based inbounds (Socks/Mixed/HTTP) store credentials under + // settings.accounts[] (an array of {user, pass} objects), not + // settings.clients[]. The whole client lifecycle (this method, plus + // UpdateInboundClient/DelInboundClient/CopyInboundClients, depletion, + // traffic reset, Telegram 'add client' keyboard, sub-link generation) + // assumes the latter shape. Resolving the inbound up front lets us + // reject account-based protocols with a clear error instead of + // panicking on the unchecked `settings["clients"].([]any)` cast below. + targetInbound, err := s.GetInbound(data.Id) + if err != nil { + return false, err + } + if model.IsAccountBased(targetInbound.Protocol) { + return false, common.NewError( + "client lifecycle is not supported for account-based protocol:", + string(targetInbound.Protocol), + "— update the inbound directly with settings.accounts[] instead", + ) + } + clients, err := s.GetClients(data) if err != nil { return false, err @@ -871,10 +891,8 @@ func (s *InboundService) AddInboundClient(data *model.Inbound) (bool, error) { return false, common.NewError("Duplicate email:", existEmail) } - oldInbound, err := s.GetInbound(data.Id) - if err != nil { - return false, err - } + // Already loaded above as part of the account-based guard. + oldInbound := targetInbound // Secure client ID for _, client := range clients { @@ -1091,6 +1109,28 @@ func (s *InboundService) CopyInboundClients(targetInboundID int, sourceInboundID return result, false, err } + // Account-based inbounds (Socks/Mixed/HTTP) don't participate in the + // client-copy flow: they don't store per-client metadata (sub-id, + // totalGB, expiry, …) — just plain {user, pass} accounts. Trying to + // copy clients into or out of them would either downcast a rich + // client to a bare account (losing data silently) or upcast an + // account into a client that the runtime can't actually use. Refuse + // the operation explicitly on either side. + if model.IsAccountBased(sourceInbound.Protocol) { + return result, false, common.NewError( + "copy clients: source inbound uses account-based protocol:", + string(sourceInbound.Protocol), + "— manage settings.accounts[] directly via inbound update", + ) + } + if model.IsAccountBased(targetInbound.Protocol) { + return result, false, common.NewError( + "copy clients: target inbound uses account-based protocol:", + string(targetInbound.Protocol), + "— manage settings.accounts[] directly via inbound update", + ) + } + sourceClients, err := s.GetClients(sourceInbound) if err != nil { return result, false, err @@ -1185,6 +1225,16 @@ func (s *InboundService) DelInboundClient(inboundId int, clientId string) (bool, logger.Error("Load Old Data Error") return false, err } + // See AddInboundClient: account-based inbounds (Socks/Mixed/HTTP) have + // no settings.clients[] to delete from. Refuse early with a clear error + // instead of panicking on the type assertion below. + if model.IsAccountBased(oldInbound.Protocol) { + return false, common.NewError( + "client lifecycle is not supported for account-based protocol:", + string(oldInbound.Protocol), + "— update the inbound directly with settings.accounts[] instead", + ) + } var settings map[string]any err = json.Unmarshal([]byte(oldInbound.Settings), &settings) if err != nil { @@ -1296,6 +1346,22 @@ func (s *InboundService) DelInboundClient(inboundId int, clientId string) (bool, func (s *InboundService) UpdateInboundClient(data *model.Inbound, clientId string) (bool, error) { // TODO: check if TrafficReset field is updating + + // Resolve the inbound first so we can refuse account-based protocols + // (Socks/Mixed/HTTP) before touching the settings["clients"] cast. + // See AddInboundClient for the rationale. + oldInbound, err := s.GetInbound(data.Id) + if err != nil { + return false, err + } + if model.IsAccountBased(oldInbound.Protocol) { + return false, common.NewError( + "client lifecycle is not supported for account-based protocol:", + string(oldInbound.Protocol), + "— update the inbound directly with settings.accounts[] instead", + ) + } + clients, err := s.GetClients(data) if err != nil { return false, err @@ -1309,11 +1375,6 @@ func (s *InboundService) UpdateInboundClient(data *model.Inbound, clientId strin interfaceClients := settings["clients"].([]any) - oldInbound, err := s.GetInbound(data.Id) - if err != nil { - return false, err - } - oldClients, err := s.GetClients(oldInbound) if err != nil { return false, err diff --git a/web/service/inbound_socks_guards_test.go b/web/service/inbound_socks_guards_test.go new file mode 100644 index 00000000..8f42c26f --- /dev/null +++ b/web/service/inbound_socks_guards_test.go @@ -0,0 +1,150 @@ +package service + +// Guard tests for the client-lifecycle methods on InboundService when +// applied to account-based inbounds (Socks/Mixed/HTTP). +// +// The intent here is narrow: prove that AddInboundClient, +// UpdateInboundClient, DelInboundClient and CopyInboundClients refuse +// account-based protocols cleanly *before* they ever hit the +// `settings["clients"].([]any)` cast — which would otherwise panic, +// because account-based inbounds carry settings.accounts[] instead. +// +// We deliberately don't exercise the happy path for client-based +// protocols here — that's covered elsewhere — and we don't need to +// boot xray / runtimes because the guards short-circuit at the very +// top of each method. A tiny in-memory sqlite from setupConflictDB is +// enough. + +import ( + "strings" + "testing" + + "github.com/mhsanaei/3x-ui/v3/database" + "github.com/mhsanaei/3x-ui/v3/database/model" +) + +// socksAccountsSettings is a minimal but realistic settings payload for +// a SOCKS5 inbound. The shape (accounts[]) is what makes the unguarded +// `settings["clients"].([]any)` cast panic — we keep it real so the +// regression is obvious if the guards ever get removed. +const socksAccountsSettings = `{"auth":"password","accounts":[{"user":"alice","pass":"hunter2"}],"udp":false,"ip":"127.0.0.1"}` + +// clientPayloadJSON is a stub "client add" payload. The guards must +// reject the call regardless of what the client envelope contains, so +// the actual fields here don't matter — what matters is that the call +// is made against a SOCKS inbound. +const clientPayloadJSON = `{"clients":[{"id":"00000000-0000-0000-0000-000000000000","email":"ignored@example.com","enable":true}]}` + +func seedSocksInbound(t *testing.T) *model.Inbound { + t.Helper() + seedInboundConflict(t, "socks-guard", "0.0.0.0", 1080, model.Socks, `{"network":"tcp"}`, socksAccountsSettings) + + var ib model.Inbound + if err := database.GetDB().Where("tag = ?", "socks-guard").First(&ib).Error; err != nil { + t.Fatalf("load seeded socks inbound: %v", err) + } + return &ib +} + +func seedVlessInbound(t *testing.T) *model.Inbound { + t.Helper() + const vlessSettings = `{"clients":[{"id":"11111111-1111-1111-1111-111111111111","email":"v@example.com","enable":true}],"decryption":"none"}` + seedInboundConflict(t, "vless-source", "0.0.0.0", 1443, model.VLESS, `{"network":"tcp"}`, vlessSettings) + + var ib model.Inbound + if err := database.GetDB().Where("tag = ?", "vless-source").First(&ib).Error; err != nil { + t.Fatalf("load seeded vless inbound: %v", err) + } + return &ib +} + +// expectAccountBasedError fails the test unless err is the well-known +// "client lifecycle is not supported for account-based protocol" error +// emitted by the guards. We match on substring to stay decoupled from +// the exact common.NewError formatting (which space-joins its args). +func expectAccountBasedError(t *testing.T, err error) { + t.Helper() + if err == nil { + t.Fatalf("expected account-based guard error, got nil — guard may be missing") + } + msg := err.Error() + if !strings.Contains(msg, "account-based protocol") { + t.Fatalf("expected account-based guard error, got: %v", err) + } +} + +func TestAddInboundClient_RejectsSocks(t *testing.T) { + setupConflictDB(t) + ib := seedSocksInbound(t) + + svc := &InboundService{} + _, err := svc.AddInboundClient(&model.Inbound{ + Id: ib.Id, + Settings: clientPayloadJSON, + }) + expectAccountBasedError(t, err) +} + +func TestUpdateInboundClient_RejectsSocks(t *testing.T) { + setupConflictDB(t) + ib := seedSocksInbound(t) + + svc := &InboundService{} + _, err := svc.UpdateInboundClient(&model.Inbound{ + Id: ib.Id, + Settings: clientPayloadJSON, + }, "any-client-id") + expectAccountBasedError(t, err) +} + +func TestDelInboundClient_RejectsSocks(t *testing.T) { + setupConflictDB(t) + ib := seedSocksInbound(t) + + svc := &InboundService{} + _, err := svc.DelInboundClient(ib.Id, "any-client-id") + expectAccountBasedError(t, err) +} + +// SOCKS as source and as target both have to be refused — neither +// direction has well-defined semantics (downcasting a rich client to +// {user, pass} would silently drop sub-id / totalGB / expiry; upcasting +// the other way would invent fields that the runtime can't honor). +func TestCopyInboundClients_RejectsSocksSource(t *testing.T) { + setupConflictDB(t) + socks := seedSocksInbound(t) + vless := seedVlessInbound(t) + + svc := &InboundService{} + _, _, err := svc.CopyInboundClients(vless.Id, socks.Id, nil, "") + expectAccountBasedError(t, err) +} + +func TestCopyInboundClients_RejectsSocksTarget(t *testing.T) { + setupConflictDB(t) + socks := seedSocksInbound(t) + vless := seedVlessInbound(t) + + svc := &InboundService{} + _, _, err := svc.CopyInboundClients(socks.Id, vless.Id, nil, "") + expectAccountBasedError(t, err) +} + +// Sanity check: the guards must NOT fire on client-based inbounds. +// If this ever flips, AddInboundClient is broken for everyone, not +// just SOCKS users. We don't assert success (the call may legitimately +// fail later in the pipeline because we haven't booted xray) — we just +// assert that whatever error comes back is *not* the guard error. +func TestAddInboundClient_AllowsVless(t *testing.T) { + setupConflictDB(t) + ib := seedVlessInbound(t) + + svc := &InboundService{} + _, err := svc.AddInboundClient(&model.Inbound{ + Id: ib.Id, + Settings: clientPayloadJSON, + }) + if err != nil && strings.Contains(err.Error(), "account-based protocol") { + t.Fatalf("guard should not fire on VLESS, got: %v", err) + } +}