mirror of
https://github.com/MHSanaei/3x-ui.git
synced 2026-04-14 11:35:50 +00:00
fix: comprehensive bug fixes across the codebase
## Critical Fixes
### 1. DATA LOSS: 5 functions discard all other clients when updating one
Functions affected:
- SetClientTelegramUserID
- ToggleClientEnableByEmail
- ResetClientIpLimitByEmail
- ResetClientExpiryTimeByEmail
- ResetClientTrafficLimitByEmail
All five built a `newClients` slice by only appending the client
matching the target email, then replaced the entire client list.
Every other client in the inbound was silently deleted.
Fix: update client in-place with break instead of building new slice.
### 2. DATA LOSS: ResetSettings never deletes user credentials
ResetSettings() called `.Where("1 = 1").Error` instead of
`.Delete(model.User{}).Error`. The reset command did nothing to users.
### 3. SECURITY: WebSocket CheckOrigin allows cross-origin hijacking
The fallback `(originHost == "" || requestHost == "")` accepted
any origin with a missing host component. Removed the fallback and
added proper host normalization for IPv6/ports.
### 4. GRACEFUL SHUTDOWN: Server.Stop() uses cancelled context
s.cancel() was called before s.httpServer.Shutdown(s.ctx), making
the context already-done. Shutdown returned immediately (forced kill)
instead of waiting 10 seconds. Moved s.cancel() to end and used
context.WithTimeout(10s) for shutdown. Same fix applied to sub.go.
## Medium Fixes
### 5. Wrong success messages on error paths (~11 endpoints)
When validation failed, endpoints returned messages like
"inboundUpdateSuccess" alongside the error. Fixed to use
"somethingWentWrong" for all error paths.
### 6. resetAllTraffics/resetAllClientTraffics trigger restart on error
SetToNeedRestart() was called in else branch that ran even on failure.
Restructured to only call after confirming success.
### 7. disableInvalidClients has duplicate unreachable error check
Same "User %s not found" string check was nested twice.
Removed the inner duplicate.
### 8. DelInbound logs uninitialized tag variable
The else branch logged empty tag variable instead of actual inbound id.
### 9. check_cpu_usage.go index-out-of-range panic
cpu.Percent() can return empty slice. Added len(percent) > 0 guard.
### 10. Dead code: cron.Remove(entry) on never-added entry
var entry cron.EntryID defaults to 0; cron.Remove(0) is a no-op.
### 11. checkEmailExistForInbound duplicates checkEmailsExistForClients
Refactored to delegate to existing function instead of reimplementing.
This commit is contained in:
parent
38d87230d3
commit
0404fce020
7 changed files with 65 additions and 86 deletions
|
|
@ -14,6 +14,7 @@ import (
|
|||
"path/filepath"
|
||||
"strconv"
|
||||
"strings"
|
||||
"time"
|
||||
|
||||
"github.com/mhsanaei/3x-ui/v2/logger"
|
||||
"github.com/mhsanaei/3x-ui/v2/util/common"
|
||||
|
|
@ -362,16 +363,18 @@ func (s *Server) Start() (err error) {
|
|||
|
||||
// Stop gracefully shuts down the subscription server and closes the listener.
|
||||
func (s *Server) Stop() error {
|
||||
s.cancel()
|
||||
|
||||
var err1 error
|
||||
var err2 error
|
||||
if s.httpServer != nil {
|
||||
err1 = s.httpServer.Shutdown(s.ctx)
|
||||
// Use a fresh timeout context for graceful shutdown
|
||||
shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 10*time.Second)
|
||||
defer shutdownCancel()
|
||||
err1 = s.httpServer.Shutdown(shutdownCtx)
|
||||
}
|
||||
if s.listener != nil {
|
||||
err2 = s.listener.Close()
|
||||
}
|
||||
s.cancel()
|
||||
return common.Combine(err1, err2)
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -107,7 +107,7 @@ func (a *InboundController) addInbound(c *gin.Context) {
|
|||
inbound := &model.Inbound{}
|
||||
err := c.ShouldBind(inbound)
|
||||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.inboundCreateSuccess"), err)
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
}
|
||||
user := session.GetLoginUser(c)
|
||||
|
|
@ -136,7 +136,7 @@ func (a *InboundController) addInbound(c *gin.Context) {
|
|||
func (a *InboundController) delInbound(c *gin.Context) {
|
||||
id, err := strconv.Atoi(c.Param("id"))
|
||||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.inboundDeleteSuccess"), err)
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
}
|
||||
needRestart, err := a.inboundService.DelInbound(id)
|
||||
|
|
@ -158,7 +158,7 @@ func (a *InboundController) delInbound(c *gin.Context) {
|
|||
func (a *InboundController) updateInbound(c *gin.Context) {
|
||||
id, err := strconv.Atoi(c.Param("id"))
|
||||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.inboundUpdateSuccess"), err)
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
}
|
||||
inbound := &model.Inbound{
|
||||
|
|
@ -166,7 +166,7 @@ func (a *InboundController) updateInbound(c *gin.Context) {
|
|||
}
|
||||
err = c.ShouldBind(inbound)
|
||||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.inboundUpdateSuccess"), err)
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
}
|
||||
inbound, needRestart, err := a.inboundService.UpdateInbound(inbound)
|
||||
|
|
@ -234,7 +234,7 @@ func (a *InboundController) clearClientIps(c *gin.Context) {
|
|||
|
||||
err := a.inboundService.ClearClientIps(email)
|
||||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.updateSuccess"), err)
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
}
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.logCleanSuccess"), nil)
|
||||
|
|
@ -245,7 +245,7 @@ func (a *InboundController) addInboundClient(c *gin.Context) {
|
|||
data := &model.Inbound{}
|
||||
err := c.ShouldBind(data)
|
||||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.inboundUpdateSuccess"), err)
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
}
|
||||
|
||||
|
|
@ -264,7 +264,7 @@ func (a *InboundController) addInboundClient(c *gin.Context) {
|
|||
func (a *InboundController) delInboundClient(c *gin.Context) {
|
||||
id, err := strconv.Atoi(c.Param("id"))
|
||||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.inboundUpdateSuccess"), err)
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
}
|
||||
clientId := c.Param("clientId")
|
||||
|
|
@ -287,7 +287,7 @@ func (a *InboundController) updateInboundClient(c *gin.Context) {
|
|||
inbound := &model.Inbound{}
|
||||
err := c.ShouldBind(inbound)
|
||||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.inboundUpdateSuccess"), err)
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
}
|
||||
|
||||
|
|
@ -306,7 +306,7 @@ func (a *InboundController) updateInboundClient(c *gin.Context) {
|
|||
func (a *InboundController) resetClientTraffic(c *gin.Context) {
|
||||
id, err := strconv.Atoi(c.Param("id"))
|
||||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.inboundUpdateSuccess"), err)
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
}
|
||||
email := c.Param("email")
|
||||
|
|
@ -328,9 +328,8 @@ func (a *InboundController) resetAllTraffics(c *gin.Context) {
|
|||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
} else {
|
||||
a.xrayService.SetToNeedRestart()
|
||||
}
|
||||
a.xrayService.SetToNeedRestart()
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.resetAllTrafficSuccess"), nil)
|
||||
}
|
||||
|
||||
|
|
@ -338,7 +337,7 @@ func (a *InboundController) resetAllTraffics(c *gin.Context) {
|
|||
func (a *InboundController) resetAllClientTraffics(c *gin.Context) {
|
||||
id, err := strconv.Atoi(c.Param("id"))
|
||||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.inboundUpdateSuccess"), err)
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
}
|
||||
|
||||
|
|
@ -346,9 +345,8 @@ func (a *InboundController) resetAllClientTraffics(c *gin.Context) {
|
|||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
} else {
|
||||
a.xrayService.SetToNeedRestart()
|
||||
}
|
||||
a.xrayService.SetToNeedRestart()
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.resetAllClientTrafficSuccess"), nil)
|
||||
}
|
||||
|
||||
|
|
@ -386,7 +384,7 @@ func (a *InboundController) importInbound(c *gin.Context) {
|
|||
func (a *InboundController) delDepletedClients(c *gin.Context) {
|
||||
id, err := strconv.Atoi(c.Param("id"))
|
||||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.inboundUpdateSuccess"), err)
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
}
|
||||
err = a.inboundService.DelDepletedClients(id)
|
||||
|
|
@ -421,7 +419,7 @@ func (a *InboundController) updateClientTraffic(c *gin.Context) {
|
|||
var request TrafficUpdateRequest
|
||||
err := c.ShouldBindJSON(&request)
|
||||
if err != nil {
|
||||
jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.inboundUpdateSuccess"), err)
|
||||
jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err)
|
||||
return
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -33,35 +33,34 @@ var upgrader = ws.Upgrader{
|
|||
ReadBufferSize: 4096, // Increased from 1024 for better performance
|
||||
WriteBufferSize: 4096, // Increased from 1024 for better performance
|
||||
CheckOrigin: func(r *http.Request) bool {
|
||||
// Check origin for security
|
||||
origin := r.Header.Get("Origin")
|
||||
if origin == "" {
|
||||
// Allow connections without Origin header (same-origin requests)
|
||||
return true
|
||||
}
|
||||
// Get the host from the request
|
||||
host := r.Host
|
||||
// Extract scheme and host from origin
|
||||
originURL := origin
|
||||
// Simple check: origin should match the request host
|
||||
// This prevents cross-origin WebSocket hijacking
|
||||
if strings.HasPrefix(originURL, "http://") || strings.HasPrefix(originURL, "https://") {
|
||||
// Extract host from origin
|
||||
originHost := strings.TrimPrefix(strings.TrimPrefix(originURL, "http://"), "https://")
|
||||
|
||||
// Extract host from origin
|
||||
originHost := origin
|
||||
if strings.HasPrefix(originHost, "http://") || strings.HasPrefix(originHost, "https://") {
|
||||
originHost = strings.TrimPrefix(strings.TrimPrefix(originHost, "http://"), "https://")
|
||||
if idx := strings.Index(originHost, "/"); idx != -1 {
|
||||
originHost = originHost[:idx]
|
||||
}
|
||||
if idx := strings.Index(originHost, ":"); idx != -1 {
|
||||
originHost = originHost[:idx]
|
||||
}
|
||||
// Compare hosts (without port)
|
||||
requestHost := host
|
||||
if idx := strings.Index(requestHost, ":"); idx != -1 {
|
||||
requestHost = requestHost[:idx]
|
||||
}
|
||||
return originHost == requestHost || originHost == "" || requestHost == ""
|
||||
}
|
||||
return false
|
||||
|
||||
// Normalize host for comparison (strip ports and IPv6 brackets)
|
||||
normalizeHost := func(h string) string {
|
||||
h = strings.TrimPrefix(h, "[")
|
||||
if idx := strings.LastIndex(h, "]:"); idx != -1 {
|
||||
h = h[:idx+1]
|
||||
}
|
||||
if idx := strings.LastIndex(h, ":"); idx != -1 && !strings.Contains(h[:idx], "]") {
|
||||
h = h[:idx]
|
||||
}
|
||||
return h
|
||||
}
|
||||
|
||||
return normalizeHost(originHost) == normalizeHost(r.Host)
|
||||
},
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -30,7 +30,7 @@ func (j *CheckCpuJob) Run() {
|
|||
|
||||
// get latest status of server
|
||||
percent, err := cpu.Percent(1*time.Minute, false)
|
||||
if err == nil && percent[0] > float64(threshold) {
|
||||
if err == nil && len(percent) > 0 && percent[0] > float64(threshold) {
|
||||
msg := j.tgbotService.I18nBot("tgbot.messages.cpuThreshold",
|
||||
"Percent=="+strconv.FormatFloat(percent[0], 'f', 2, 64),
|
||||
"Threshold=="+strconv.Itoa(threshold))
|
||||
|
|
|
|||
|
|
@ -190,23 +190,7 @@ func (s *InboundService) checkEmailExistForInbound(inbound *model.Inbound) (stri
|
|||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
allEmails, err := s.getAllEmails()
|
||||
if err != nil {
|
||||
return "", err
|
||||
}
|
||||
var emails []string
|
||||
for _, client := range clients {
|
||||
if client.Email != "" {
|
||||
if s.contains(emails, client.Email) {
|
||||
return client.Email, nil
|
||||
}
|
||||
if s.contains(allEmails, client.Email) {
|
||||
return client.Email, nil
|
||||
}
|
||||
emails = append(emails, client.Email)
|
||||
}
|
||||
}
|
||||
return "", nil
|
||||
return s.checkEmailsExistForClients(clients)
|
||||
}
|
||||
|
||||
// AddInbound creates a new inbound configuration.
|
||||
|
|
@ -339,7 +323,7 @@ func (s *InboundService) DelInbound(id int) (bool, error) {
|
|||
}
|
||||
s.xrayApi.Close()
|
||||
} else {
|
||||
logger.Debug("No enabled inbound founded to removing by api", tag)
|
||||
logger.Debug("No enabled inbound found to remove by api for inbound id:", id)
|
||||
}
|
||||
|
||||
// Delete client traffics of inbounds
|
||||
|
|
@ -1280,12 +1264,8 @@ func (s *InboundService) disableInvalidClients(tx *gorm.DB) (bool, int64, error)
|
|||
if strings.Contains(err1.Error(), fmt.Sprintf("User %s not found.", result.Email)) {
|
||||
logger.Debug("User is already disabled. Nothing to do more...")
|
||||
} else {
|
||||
if strings.Contains(err1.Error(), fmt.Sprintf("User %s not found.", result.Email)) {
|
||||
logger.Debug("User is already disabled. Nothing to do more...")
|
||||
} else {
|
||||
logger.Debug("Error in disabling client by api:", err1)
|
||||
needRestart = true
|
||||
}
|
||||
logger.Debug("Error in disabling client by api:", err1)
|
||||
needRestart = true
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -1458,16 +1438,16 @@ func (s *InboundService) SetClientTelegramUserID(trafficId int, tgId int64) (boo
|
|||
return false, err
|
||||
}
|
||||
clients := settings["clients"].([]any)
|
||||
var newClients []any
|
||||
for client_index := range clients {
|
||||
c := clients[client_index].(map[string]any)
|
||||
if c["email"] == clientEmail {
|
||||
c["tgId"] = tgId
|
||||
c["updated_at"] = time.Now().Unix() * 1000
|
||||
newClients = append(newClients, any(c))
|
||||
clients[client_index] = c
|
||||
break
|
||||
}
|
||||
}
|
||||
settings["clients"] = newClients
|
||||
settings["clients"] = clients
|
||||
modifiedSettings, err := json.MarshalIndent(settings, "", " ")
|
||||
if err != nil {
|
||||
return false, err
|
||||
|
|
@ -1545,16 +1525,16 @@ func (s *InboundService) ToggleClientEnableByEmail(clientEmail string) (bool, bo
|
|||
return false, false, err
|
||||
}
|
||||
clients := settings["clients"].([]any)
|
||||
var newClients []any
|
||||
for client_index := range clients {
|
||||
c := clients[client_index].(map[string]any)
|
||||
if c["email"] == clientEmail {
|
||||
c["enable"] = !clientOldEnabled
|
||||
c["updated_at"] = time.Now().Unix() * 1000
|
||||
newClients = append(newClients, any(c))
|
||||
clients[client_index] = c
|
||||
break
|
||||
}
|
||||
}
|
||||
settings["clients"] = newClients
|
||||
settings["clients"] = clients
|
||||
modifiedSettings, err := json.MarshalIndent(settings, "", " ")
|
||||
if err != nil {
|
||||
return false, false, err
|
||||
|
|
@ -1625,16 +1605,16 @@ func (s *InboundService) ResetClientIpLimitByEmail(clientEmail string, count int
|
|||
return false, err
|
||||
}
|
||||
clients := settings["clients"].([]any)
|
||||
var newClients []any
|
||||
for client_index := range clients {
|
||||
c := clients[client_index].(map[string]any)
|
||||
if c["email"] == clientEmail {
|
||||
c["limitIp"] = count
|
||||
c["updated_at"] = time.Now().Unix() * 1000
|
||||
newClients = append(newClients, any(c))
|
||||
clients[client_index] = c
|
||||
break
|
||||
}
|
||||
}
|
||||
settings["clients"] = newClients
|
||||
settings["clients"] = clients
|
||||
modifiedSettings, err := json.MarshalIndent(settings, "", " ")
|
||||
if err != nil {
|
||||
return false, err
|
||||
|
|
@ -1684,16 +1664,16 @@ func (s *InboundService) ResetClientExpiryTimeByEmail(clientEmail string, expiry
|
|||
return false, err
|
||||
}
|
||||
clients := settings["clients"].([]any)
|
||||
var newClients []any
|
||||
for client_index := range clients {
|
||||
c := clients[client_index].(map[string]any)
|
||||
if c["email"] == clientEmail {
|
||||
c["expiryTime"] = expiry_time
|
||||
c["updated_at"] = time.Now().Unix() * 1000
|
||||
newClients = append(newClients, any(c))
|
||||
clients[client_index] = c
|
||||
break
|
||||
}
|
||||
}
|
||||
settings["clients"] = newClients
|
||||
settings["clients"] = clients
|
||||
modifiedSettings, err := json.MarshalIndent(settings, "", " ")
|
||||
if err != nil {
|
||||
return false, err
|
||||
|
|
@ -1746,16 +1726,16 @@ func (s *InboundService) ResetClientTrafficLimitByEmail(clientEmail string, tota
|
|||
return false, err
|
||||
}
|
||||
clients := settings["clients"].([]any)
|
||||
var newClients []any
|
||||
for client_index := range clients {
|
||||
c := clients[client_index].(map[string]any)
|
||||
if c["email"] == clientEmail {
|
||||
c["totalGB"] = totalGB * 1024 * 1024 * 1024
|
||||
c["updated_at"] = time.Now().Unix() * 1000
|
||||
newClients = append(newClients, any(c))
|
||||
clients[client_index] = c
|
||||
break
|
||||
}
|
||||
}
|
||||
settings["clients"] = newClients
|
||||
settings["clients"] = clients
|
||||
modifiedSettings, err := json.MarshalIndent(settings, "", " ")
|
||||
if err != nil {
|
||||
return false, err
|
||||
|
|
|
|||
|
|
@ -198,8 +198,7 @@ func (s *SettingService) ResetSettings() error {
|
|||
if err != nil {
|
||||
return err
|
||||
}
|
||||
return db.Model(model.User{}).
|
||||
Where("1 = 1").Error
|
||||
return db.Where("1 = 1").Delete(model.User{}).Error
|
||||
}
|
||||
|
||||
func (s *SettingService) getSetting(key string) (*model.Setting, error) {
|
||||
|
|
|
|||
10
web/web.go
10
web/web.go
|
|
@ -344,7 +344,6 @@ func (s *Server) startTask() {
|
|||
}
|
||||
|
||||
// Make a traffic condition every day, 8:30
|
||||
var entry cron.EntryID
|
||||
isTgbotenabled, err := s.settingService.GetTgbotEnabled()
|
||||
if (err == nil) && (isTgbotenabled) {
|
||||
runtime, err := s.settingService.GetTgbotRuntime()
|
||||
|
|
@ -367,8 +366,6 @@ func (s *Server) startTask() {
|
|||
if (err == nil) && (cpuThreshold > 0) {
|
||||
s.cron.AddJob("@every 10s", job.NewCheckCpuJob())
|
||||
}
|
||||
} else {
|
||||
s.cron.Remove(entry)
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -453,7 +450,6 @@ func (s *Server) Start() (err error) {
|
|||
|
||||
// Stop gracefully shuts down the web server, stops Xray, cron jobs, and Telegram bot.
|
||||
func (s *Server) Stop() error {
|
||||
s.cancel()
|
||||
s.xrayService.StopXray()
|
||||
if s.cron != nil {
|
||||
s.cron.Stop()
|
||||
|
|
@ -468,11 +464,15 @@ func (s *Server) Stop() error {
|
|||
var err1 error
|
||||
var err2 error
|
||||
if s.httpServer != nil {
|
||||
err1 = s.httpServer.Shutdown(s.ctx)
|
||||
// Use a fresh timeout context for graceful shutdown
|
||||
shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 10*time.Second)
|
||||
defer shutdownCancel()
|
||||
err1 = s.httpServer.Shutdown(shutdownCtx)
|
||||
}
|
||||
if s.listener != nil {
|
||||
err2 = s.listener.Close()
|
||||
}
|
||||
s.cancel()
|
||||
return common.Combine(err1, err2)
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue