fix: 11 bugs including critical data-loss and security issues#3974
fix: 11 bugs including critical data-loss and security issues#3974mishrarenu066-beep wants to merge 1 commit intoMHSanaei:mainfrom
Conversation
## 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.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0404fce020
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
| settings["clients"] = newClients | ||
| settings["clients"] = clients |
There was a problem hiding this comment.
This change now sends the entire clients array to UpdateInboundClient, but that method still applies only clients[0]/interfaceClients[0] when replacing the matched client entry (see UpdateInboundClient logic around clients[0]). When the target email is not the first client in the inbound, calls like SetClientTelegramUserID will overwrite the target slot with the first client's data, causing duplicate/lost client records and updating the wrong user. The same pattern is repeated in the other updated helpers (ToggleClientEnableByEmail, ResetClientIpLimitByEmail, ResetClientExpiryTimeByEmail, ResetClientTrafficLimitByEmail).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR addresses a set of correctness and security bugs across the web panel and subscription server, focusing on preventing client data-loss during updates, tightening WebSocket origin checks, and improving graceful shutdown behavior.
Changes:
- Fixes multiple inbound client update paths that previously overwrote the entire client list (data-loss).
- Tightens WebSocket
CheckOriginlogic and improves shutdown flow by using a fresh timeout context. - Fixes several controller/service issues (reset settings deletion, CPU usage panic guard, error message correctness).
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
web/web.go |
Improves shutdown sequencing and removes dead cron removal logic. |
web/service/setting.go |
Fixes ResetSettings() to actually delete users on reset. |
web/service/inbound.go |
Prevents inbound client list data-loss; refactors duplicate email check; log message fix; removes unreachable duplicate error check. |
web/job/check_cpu_usage.go |
Prevents potential index-out-of-range panic when CPU percent slice is empty. |
web/controller/websocket.go |
Updates WebSocket origin validation logic to prevent cross-origin hijacking. |
web/controller/inbound.go |
Corrects error-path toast keys and restart-trigger control flow. |
sub/sub.go |
Improves subscription server graceful shutdown using a fresh timeout context. |
| h = strings.TrimPrefix(h, "[") | ||
| if idx := strings.LastIndex(h, "]:"); idx != -1 { | ||
| h = h[:idx+1] | ||
| } | ||
| // Compare hosts (without port) | ||
| requestHost := host | ||
| if idx := strings.Index(requestHost, ":"); idx != -1 { | ||
| requestHost = requestHost[:idx] | ||
| if idx := strings.LastIndex(h, ":"); idx != -1 && !strings.Contains(h[:idx], "]") { |
There was a problem hiding this comment.
CheckOrigin host normalization is incorrect for IPv6 addresses (e.g. [2001:db8::1]:443): the LastIndex(":") logic can truncate the IPv6 host, which can cause false matches between different origins and weaken the cross-origin protection. Prefer parsing Origin with url.Parse and comparing u.Hostname() to the request host extracted via net.SplitHostPort/r.Host (and lowercased).
| if s.listener != nil { | ||
| err2 = s.listener.Close() | ||
| } | ||
| s.cancel() | ||
| return common.Combine(err1, err2) |
There was a problem hiding this comment.
After switching to httpServer.Shutdown(...) with a fresh context, Shutdown will typically close the tracked listener created by Serve(listener). Calling s.listener.Close() afterwards can return net.ErrClosed/"use of closed network connection" and make Stop() fail even though shutdown succeeded. Consider removing the explicit listener.Close() when Shutdown is invoked, or ignore net.ErrClosed when combining errors.
| err := db.Where("1 = 1").Delete(model.Setting{}).Error | ||
| if err != nil { | ||
| return err | ||
| } | ||
| return db.Model(model.User{}). | ||
| Where("1 = 1").Error | ||
| return db.Where("1 = 1").Delete(model.User{}).Error |
There was a problem hiding this comment.
ResetSettings() now performs two destructive deletes (settings then users) without a transaction. If the second delete fails, the panel can end up in a partially-reset state. Consider wrapping both deletes in a single DB transaction so reset is atomic (commit only if both succeed).
| if s.listener != nil { | ||
| err2 = s.listener.Close() | ||
| } | ||
| s.cancel() | ||
| return common.Combine(err1, err2) |
There was a problem hiding this comment.
Same as web server: after httpServer.Shutdown(...) (when serving with Serve(listener)), the listener is usually already closed. The subsequent s.listener.Close() can return net.ErrClosed and bubble up via common.Combine, turning a clean shutdown into an error. Consider skipping the extra close or ignoring net.ErrClosed.
Summary
This PR fixes 11 bugs found through a comprehensive deep scan of the entire codebase, including 4 critical issues that cause data loss or security vulnerabilities.
🔴 Critical Fixes
1. DATA LOSS: 5 functions silently delete all other clients when updating one
Files:
web/service/inbound.goFunctions:
SetClientTelegramUserID,ToggleClientEnableByEmail,ResetClientIpLimitByEmail,ResetClientExpiryTimeByEmail,ResetClientTrafficLimitByEmailAll five functions had the same copy-paste bug: they built a
newClientsslice by only appending the client matching the target email, then replaced the entire client list with just that one client. Every other client in the inbound was silently deleted.2. DATA LOSS:
ResetSettings()never deletes user credentialsFile:
web/service/setting.goThe function called
.Where("1 = 1").Errorinstead of.Delete(model.User{}).Error. This only reads the error from a query that does nothing — users were never deleted on reset.3. SECURITY: WebSocket
CheckOriginallows cross-origin hijackingFile:
web/controller/websocket.goThe fallback
|| originHost == "" || requestHost == ""accepted any origin with a missing host component, allowing WebSocket hijacking from malicious sites. Removed the fallback and added proper host normalization.4. GRACEFUL SHUTDOWN:
Server.Stop()uses cancelled contextFiles:
web/web.go,sub/sub.gos.cancel()was called befores.httpServer.Shutdown(s.ctx), making the context already-done.Shutdownreturned immediately (forced kill) instead of gracefully draining connections. Moveds.cancel()to end and usedcontext.WithTimeout(10s).🟡 Medium Fixes
5. Wrong success messages on error paths (~11 endpoints)
File:
web/controller/inbound.goWhen validation failed, endpoints returned messages like
"inboundUpdateSuccess"alongside the error. Fixed to use"somethingWentWrong"for all error paths.6.
resetAllTraffics/resetAllClientTrafficstrigger restart on errorSetToNeedRestart()was in anelseblock that ran even on failure. Restructured to only call after confirming success.7.
disableInvalidClientshas duplicate unreachable error checkSame
"User %s not found"string check was nested twice. Removed the inner duplicate.8.
DelInboundlogs uninitializedtagvariableThe
elsebranch logged the emptytagvariable instead of the actual inboundid.9.
check_cpu_usage.goindex-out-of-range paniccpu.Percent()can return an empty slice. Addedlen(percent) > 0guard.10. Dead code:
cron.Remove(entry)on never-added entryvar entry cron.EntryIDdefaults to 0;cron.Remove(0)is a no-op.11.
checkEmailExistForInboundduplicatescheckEmailsExistForClientsRefactored to delegate to existing function instead of reimplementing.
Files Changed
web/controller/inbound.go— error messages + flow fixesweb/controller/websocket.go— CheckOrigin security fixweb/service/inbound.go— 5 client-discard fixes + refactorweb/service/setting.go— ResetSettings fixweb/web.go— graceful shutdown + dead code removalsub/sub.go— graceful shutdown fixweb/job/check_cpu_usage.go— panic guard