mirror of
https://github.com/MHSanaei/3x-ui.git
synced 2026-06-05 12:44:22 +00:00
feat(socks): guard client-lifecycle paths against account-based inbounds
Account-based inbounds (Socks/Mixed/HTTP) keep their credentials in
`settings.accounts[]` — an array of plain {user, pass} objects — while
every other inbound (vless/vmess/trojan/shadowsocks/hysteria/…) keeps
them in `settings.clients[]`, the rich Client struct with id, email,
sub-id, totalGB, expiry, traffic-reset cadence, etc.
The whole client lifecycle on InboundService (AddInboundClient,
UpdateInboundClient, DelInboundClient, CopyInboundClients) was written
against the latter shape, and several of those methods do an unchecked
`settings["clients"].([]any)` cast on the way in. If anything ever
managed to call them against a SOCKS5 inbound the panel would panic
straight out of the goroutine.
In practice the UI itself can't get there — `dbinbound.isMultiUser()`
returns false for SOCKS, which already gates the ClientRowTable,
"add client" menu, copy-clients menu, etc. — but the HTTP API is
addressable directly, the Telegram bot path is independent, and a
future feature could easily plug into one of those entry points and
hit the cast. Defense in depth is cheap here.
Backend
-------
* Add `model.IsAccountBased(p Protocol) bool` covering Socks, Mixed
and HTTP. WireGuard is *not* in the set — its peers live under
`settings.peers[]` and are managed through a separate code path
that already knows about them.
* AddInboundClient / UpdateInboundClient / DelInboundClient now load
the target inbound up front and bail out with a clear, actionable
error when the protocol is account-based, instead of falling into
the unchecked clients cast. The error message points the caller at
the right escape hatch ("update the inbound directly with
settings.accounts[] instead").
* CopyInboundClients refuses account-based inbounds on either side
of the copy — neither direction has well-defined semantics
(downcasting a rich client to {user, pass} silently drops
sub-id/totalGB/expiry; upcasting the other way invents fields the
runtime can't honor).
Tests
-----
* TestIsAccountBased pins the protocol set, including the explicit
WireGuard-excluded and lowercase-invariant cases.
* TestAddInboundClient_RejectsSocks, TestUpdateInboundClient_RejectsSocks,
TestDelInboundClient_RejectsSocks: the three guards must fire on a
SOCKS inbound seeded with a realistic settings.accounts[] payload.
* TestCopyInboundClients_RejectsSocksSource and ...Target: both
directions are refused.
* TestAddInboundClient_AllowsVless: sanity check that the guard does
not fire on a client-based protocol — if this ever flipped the
feature would be broken for everyone, not just SOCKS users.
Other scenarios reviewed (no code changes needed):
* Routing rules — keyed off inbound tag, protocol-agnostic.
* Balancers — outbound-tag based, untouched by inbound protocol.
* Outbound side — frontend already exposes SOCKS as an outbound
with user/pass through the existing OutboundFormModal.
* Depletion / traffic reset / disable-invalid-clients — driven by
SQL queries on the client_traffics table, which is naturally empty
for account-based inbounds (they never create rows there).
* SetInboundEnable — operates on the inbound table directly, no
per-client surgery, safe for SOCKS.
* Sub-link generators (sub/subService, subJsonService, subClashService)
— already return empty for SOCKS/Mixed/HTTP/Tunnel/WireGuard.
* Frontend client modals (ClientFormModal, ClientRowTable,
ClientBulkModal, CopyClientsModal) — gated upstream by
`dbInbound.isMultiUser()`, which is false for SOCKS.
This commit is contained in:
parent
2562e2eb82
commit
6a5cac385d
4 changed files with 285 additions and 12 deletions
|
|
@ -49,6 +49,26 @@ func IsSocksLike(p Protocol) bool {
|
||||||
return p == Socks || p == Mixed
|
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.
|
// User represents a user account in the 3x-ui panel.
|
||||||
type User struct {
|
type User struct {
|
||||||
Id int `json:"id" gorm:"primaryKey;autoIncrement"`
|
Id int `json:"id" gorm:"primaryKey;autoIncrement"`
|
||||||
|
|
|
||||||
|
|
@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
|
||||||
|
|
@ -840,6 +840,26 @@ func (s *InboundService) updateClientTraffics(tx *gorm.DB, oldInbound *model.Inb
|
||||||
}
|
}
|
||||||
|
|
||||||
func (s *InboundService) AddInboundClient(data *model.Inbound) (bool, error) {
|
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)
|
clients, err := s.GetClients(data)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return false, err
|
return false, err
|
||||||
|
|
@ -871,10 +891,8 @@ func (s *InboundService) AddInboundClient(data *model.Inbound) (bool, error) {
|
||||||
return false, common.NewError("Duplicate email:", existEmail)
|
return false, common.NewError("Duplicate email:", existEmail)
|
||||||
}
|
}
|
||||||
|
|
||||||
oldInbound, err := s.GetInbound(data.Id)
|
// Already loaded above as part of the account-based guard.
|
||||||
if err != nil {
|
oldInbound := targetInbound
|
||||||
return false, err
|
|
||||||
}
|
|
||||||
|
|
||||||
// Secure client ID
|
// Secure client ID
|
||||||
for _, client := range clients {
|
for _, client := range clients {
|
||||||
|
|
@ -1091,6 +1109,28 @@ func (s *InboundService) CopyInboundClients(targetInboundID int, sourceInboundID
|
||||||
return result, false, err
|
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)
|
sourceClients, err := s.GetClients(sourceInbound)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return result, false, err
|
return result, false, err
|
||||||
|
|
@ -1185,6 +1225,16 @@ func (s *InboundService) DelInboundClient(inboundId int, clientId string) (bool,
|
||||||
logger.Error("Load Old Data Error")
|
logger.Error("Load Old Data Error")
|
||||||
return false, err
|
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
|
var settings map[string]any
|
||||||
err = json.Unmarshal([]byte(oldInbound.Settings), &settings)
|
err = json.Unmarshal([]byte(oldInbound.Settings), &settings)
|
||||||
if err != nil {
|
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) {
|
func (s *InboundService) UpdateInboundClient(data *model.Inbound, clientId string) (bool, error) {
|
||||||
// TODO: check if TrafficReset field is updating
|
// 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)
|
clients, err := s.GetClients(data)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return false, err
|
return false, err
|
||||||
|
|
@ -1309,11 +1375,6 @@ func (s *InboundService) UpdateInboundClient(data *model.Inbound, clientId strin
|
||||||
|
|
||||||
interfaceClients := settings["clients"].([]any)
|
interfaceClients := settings["clients"].([]any)
|
||||||
|
|
||||||
oldInbound, err := s.GetInbound(data.Id)
|
|
||||||
if err != nil {
|
|
||||||
return false, err
|
|
||||||
}
|
|
||||||
|
|
||||||
oldClients, err := s.GetClients(oldInbound)
|
oldClients, err := s.GetClients(oldInbound)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return false, err
|
return false, err
|
||||||
|
|
|
||||||
150
web/service/inbound_socks_guards_test.go
Normal file
150
web/service/inbound_socks_guards_test.go
Normal file
|
|
@ -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)
|
||||||
|
}
|
||||||
|
}
|
||||||
Loading…
Reference in a new issue