fix(security): harden auth flows and lock down telegram bot tool

- config: require JWT_SECRET >=32 bytes and reject the historical default fallback; MustInit aborts startup under an insecure config - api: CORS now uses CORS_ALLOWED_ORIGINS allowlist with safe localhost defaults instead of returning Access-Control-Allow-Origin: * - api: /api/reset-password and /api/reset-account stay public so recovery still works, but require an explicit confirm phrase in the body to block accidental and drive-by triggers - api: drop adoptOrphanRecords so wiping the account no longer hands the next registrant the previous owner's wallet keys and exchange API credentials - api: getTraderFromQuery now does a soft ownership check; equity-history is restricted to traders with show_in_competition=true and GetOrderFills joins on trader_id - telegram: bot api_request tool uses a default-deny method+path allowlist so prompt injection cannot reach password, exchange key, AI provider or wallet endpoints - ci: drop @master / @main on trivy-action and trufflehog; pin to released versions with a TODO to move to SHA + Dependabot - web: reset flows send the required confirm phrase; "Forgot account" copy (en/zh/id) warns that wallet and exchange keys will be lost - docker-compose: keep ./.env mount for onboarding wallet persistence with an inline note on the tradeoff, drop the host-exposed pprof port
2026-06-06 05:51:19 +08:00 · 2026-05-29 07:51:26 +08:00
parent 70db3f5ba3
commit 99361cb085
13 changed files with 379 additions and 86 deletions
--- a/api/handler_competition.go
+++ b/api/handler_competition.go
@@ -119,12 +119,25 @@ func (s *Server) handleCompetition(c *gin.Context) {
 	c.JSON(http.StatusOK, competition)
 }

-// handleEquityHistory Return rate historical data
-// Query directly from database, not dependent on trader in memory (so historical data can be retrieved after restart)
+// handleEquityHistory returns equity history for a trader. This endpoint is
+// PUBLIC (used by the competition leaderboard), so it cannot use the
+// authenticated getTraderFromQuery helper. Instead, it validates that the
+// requested trader has explicitly opted into the public competition via
+// show_in_competition=true. Traders without that flag are not exposed.
 func (s *Server) handleEquityHistory(c *gin.Context) {
-	_, traderID, err := s.getTraderFromQuery(c)
-	if err != nil {
-		SafeBadRequest(c, "Invalid trader ID")
+	traderID := c.Query("trader_id")
+	if traderID == "" {
+		SafeBadRequest(c, "trader_id is required")
+		return
+	}
+	trader, err := s.store.Trader().GetByID(traderID)
+	if err != nil || trader == nil {
+		SafeNotFound(c, "Trader")
+		return
+	}
+	if !trader.ShowInCompetition {
+		// Do not leak that a private trader exists; report not found.
+		SafeNotFound(c, "Trader")
 		return
 	}

--- a/api/handler_order.go
+++ b/api/handler_order.go
@@ -357,8 +357,8 @@ func (s *Server) handleOrderFills(c *gin.Context) {
 		return
 	}

-	// Get fills for this order
-	fills, err := store.Order().GetOrderFills(orderID)
+	// Get fills for this order, scoped to the trader (ownership boundary).
+	fills, err := store.Order().GetOrderFills(traderID, orderID)
 	if err != nil {
 		SafeInternalError(c, "Get order fills", err)
 		return
--- a/api/handler_user.go
+++ b/api/handler_user.go
@@ -102,9 +102,11 @@ func (s *Server) handleRegister(c *gin.Context) {
 		return
 	}

-	// Adopt orphan records from previous account (e.g. after account reset)
-	// This preserves wallet keys and exchange configs so funds are not lost.
-	s.adoptOrphanRecords(userID)
+	// NOTE: Orphan record adoption was removed for security reasons. Previously,
+	// after a reset-account call, any new user would inherit the prior owner's
+	// wallet keys and exchange API credentials — a catastrophic IDOR/takeover
+	// path. Operators who need to migrate credentials across users must do so
+	// explicitly via export/import, never via implicit adoption on registration.

 	// Generate JWT token
 	token, err := auth.GenerateJWT(user.ID, user.Email)
@@ -189,53 +191,108 @@ func (s *Server) handleChangePassword(c *gin.Context) {
 	c.JSON(http.StatusOK, gin.H{"message": "Password updated"})
 }

-// handleResetPassword Reset password via email and new password
+// resetPasswordConfirmPhrase is the friction step for /api/reset-password.
+// Same security rationale as resetAccountConfirmPhrase — not a cryptographic
+// check, just a guard against accidental and drive-by triggers.
+const resetPasswordConfirmPhrase = "I_UNDERSTAND_THIS_RESETS_MY_PASSWORD"
+
+// handleResetPassword resets the password for the given email.
+//
+// SECURITY NOTE: This endpoint is intentionally callable without a JWT — it
+// IS the recovery path for "forgot password" in the single-user self-hosted
+// threat model this project targets. A logged-in user changes password via
+// PUT /api/user/password; this endpoint exists for users who can no longer
+// log in. Mitigations:
+//
+//  1. Requires the confirm phrase (blocks accidental and drive-by triggers).
+//  2. New password must be ≥ 8 chars.
+//  3. Authenticated session change is preferred (PUT /api/user/password).
+//
+// Operators exposing the API to the public internet should put a reverse-proxy
+// auth layer in front of /api/reset-password OR set up out-of-band recovery
+// (email link, OTP) instead of relying on this endpoint.
 func (s *Server) handleResetPassword(c *gin.Context) {
 	var req struct {
 		Email       string `json:"email" binding:"required,email"`
-		NewPassword string `json:"new_password" binding:"required,min=6"`
+		NewPassword string `json:"new_password" binding:"required,min=8"`
+		Confirm     string `json:"confirm"`
 	}
-
 	if err := c.ShouldBindJSON(&req); err != nil {
-		SafeBadRequest(c, "Invalid request parameters")
+		SafeBadRequest(c, "email, new_password (min 8 chars), and confirm are required")
+		return
+	}
+	if req.Confirm != resetPasswordConfirmPhrase {
+		c.JSON(http.StatusBadRequest, gin.H{
+			"error": "Confirmation phrase required",
+			"hint":  `Body must include {"confirm":"` + resetPasswordConfirmPhrase + `"}`,
+		})
 		return
 	}

-	// Query user
 	user, err := s.store.User().GetByEmail(req.Email)
 	if err != nil {
 		c.JSON(http.StatusNotFound, gin.H{"error": "Email does not exist"})
 		return
 	}

-	// Generate new password hash
 	newPasswordHash, err := auth.HashPassword(req.NewPassword)
 	if err != nil {
-		c.JSON(http.StatusInternalServerError, gin.H{"error": "Password processing failed"})
+		SafeInternalError(c, "Password processing failed", err)
+		return
+	}
+	if err := s.store.User().UpdatePassword(user.ID, newPasswordHash); err != nil {
+		SafeInternalError(c, "Password update failed", err)
 		return
 	}

-	// Update password
-	err = s.store.User().UpdatePassword(user.ID, newPasswordHash)
-	if err != nil {
-		c.JSON(http.StatusInternalServerError, gin.H{"error": "Password update failed"})
-		return
-	}
-
-	logger.Infof("✓ User %s password has been reset", user.Email)
+	logger.Infof("✓ User %s password reset via reset endpoint", user.Email)
 	c.JSON(http.StatusOK, gin.H{"message": "Password reset successful, please login with new password"})
 }

-// handleResetAccount clears user authentication data so the system returns to
-// uninitialized state for re-registration. Wallet keys (ai_models) are preserved
-// so funds are not lost — they will be adopted by the new account during onboarding.
+// resetAccountConfirmPhrase must appear in the request body for /api/reset-account.
+// This is the single intentional friction step that prevents accidental wipes
+// from drive-by scripts and crawlers. It is NOT a cryptographic check — anyone
+// who reads this source can send the phrase. The real safety comes from:
+//
+//  1. Wallet keys are NO LONGER auto-adopted by the next registrant
+//     (adoptOrphanRecords was removed). The historical takeover path was:
+//     reset → register → inherit prior wallet → drain. That path is closed.
+//  2. The destructive action is loud (logged at Warn level).
+//
+// Operators who expose the API to the public internet and want stronger
+// gating can wrap this route with a reverse-proxy auth header check.
+const resetAccountConfirmPhrase = "I_UNDERSTAND_THIS_DELETES_EVERYTHING"
+
+// handleResetAccount wipes all users + traders + strategies + AI models +
+// exchanges, returning the system to uninitialized state.
+//
+// SECURITY NOTE: For the single-user, self-hosted threat model this project
+// targets, this endpoint is intentionally callable without a JWT — the
+// frontend "forgot account" button must still work after the user forgets
+// their password. The confirm phrase blocks accidental and drive-by triggers;
+// the removal of orphan adoption blocks the post-reset takeover. A determined
+// attacker on a public-facing deployment can still grief by wiping local
+// state, but they cannot steal funds (everything is deleted, not transferred).
 func (s *Server) handleResetAccount(c *gin.Context) {
+	var req struct {
+		Confirm string `json:"confirm"`
+	}
+	_ = c.ShouldBindJSON(&req)
+	if req.Confirm != resetAccountConfirmPhrase {
+		c.JSON(http.StatusBadRequest, gin.H{
+			"error": "Confirmation phrase required",
+			"hint":  `Body must include {"confirm":"` + resetAccountConfirmPhrase + `"}`,
+		})
+		return
+	}
+
 	err := s.store.Transaction(func(tx *gorm.DB) error {
-		// Delete traders and strategies (config, not funds)
+		// Wipe ALL records — including wallet keys and exchange credentials.
+		// Preserving them across user identities is what enabled the takeover.
 		tx.Session(&gorm.Session{AllowGlobalUpdate: true}).Delete(&store.Trader{})
 		tx.Session(&gorm.Session{AllowGlobalUpdate: true}).Delete(&store.Strategy{})
-		// Delete users — ai_models and exchanges are intentionally kept
-		// so wallet private keys and exchange configs survive re-registration
+		tx.Session(&gorm.Session{AllowGlobalUpdate: true}).Delete(&store.AIModel{})
+		tx.Session(&gorm.Session{AllowGlobalUpdate: true}).Delete(&store.Exchange{})
 		if err := tx.Session(&gorm.Session{AllowGlobalUpdate: true}).Delete(&store.User{}).Error; err != nil {
 			return fmt.Errorf("failed to delete users: %w", err)
 		}
@@ -246,28 +303,10 @@ func (s *Server) handleResetAccount(c *gin.Context) {
 		return
 	}

-	logger.Infof("✓ User accounts cleared (wallets preserved) — system reset to uninitialized")
-	c.JSON(http.StatusOK, gin.H{"message": "Account reset successful, you can now register a new account"})
-}
-
-// adoptOrphanRecords re-assigns ai_models and exchanges whose user_id no longer
-// exists in the users table. This happens after account reset so the new user
-// inherits the previous wallet keys and exchange configurations.
-func (s *Server) adoptOrphanRecords(newUserID string) {
-	db := s.store.GormDB()
-	result := db.Model(&store.AIModel{}).
-		Where("user_id NOT IN (SELECT id FROM users)").
-		Update("user_id", newUserID)
-	if result.RowsAffected > 0 {
-		logger.Infof("✓ Adopted %d orphan ai_model(s) for new user %s", result.RowsAffected, newUserID)
-	}
-
-	result = db.Model(&store.Exchange{}).
-		Where("user_id NOT IN (SELECT id FROM users)").
-		Update("user_id", newUserID)
-	if result.RowsAffected > 0 {
-		logger.Infof("✓ Adopted %d orphan exchange(s) for new user %s", result.RowsAffected, newUserID)
-	}
+	logger.Warnf("⚠ Account reset performed — all users, traders, strategies, ai_models, exchanges wiped")
+	c.JSON(http.StatusOK, gin.H{
+		"message": "System wiped. All wallet keys and exchange credentials were deleted. Register a fresh account and re-import everything.",
+	})
 }

 // initUserDefaultConfigs Initialize default configs for new user
--- a/api/server.go
+++ b/api/server.go
@@ -10,6 +10,7 @@ import (
 	"nofx/logger"
 	"nofx/manager"
 	"nofx/store"
+	"os"
 	"strings"
 	"time"

@@ -56,18 +57,62 @@ func NewServer(traderManager *manager.TraderManager, st *store.Store, cryptoServ
 	return s
 }

-// corsMiddleware CORS middleware
+// corsMiddleware returns a CORS handler. Origins come from CORS_ALLOWED_ORIGINS
+// (comma-separated). The literal value "*" enables permissive mode — DO NOT use
+// in production: the JWT is sent via Authorization header so a wildcard ACAO
+// makes stolen tokens replayable from any site.
 func corsMiddleware() gin.HandlerFunc {
+	raw := strings.TrimSpace(os.Getenv("CORS_ALLOWED_ORIGINS"))
+	allowAny := raw == "*"
+	var allowlist map[string]struct{}
+	if !allowAny {
+		allowlist = make(map[string]struct{})
+		for _, o := range strings.Split(raw, ",") {
+			o = strings.TrimSpace(o)
+			if o == "" {
+				continue
+			}
+			allowlist[o] = struct{}{}
+		}
+		if len(allowlist) == 0 {
+			// Safe defaults for local development.
+			for _, o := range []string{
+				"http://localhost:3000",
+				"http://127.0.0.1:3000",
+				"http://localhost:5173",
+				"http://127.0.0.1:5173",
+			} {
+				allowlist[o] = struct{}{}
+			}
+			logger.Warnf("[CORS] CORS_ALLOWED_ORIGINS not set; defaulting to localhost dev origins only. Set this env var for production.")
+		}
+		if allowAny {
+			logger.Warnf("[CORS] CORS_ALLOWED_ORIGINS=* is INSECURE in production; restrict to your deployment origin(s).")
+		}
+	}
 	return func(c *gin.Context) {
-		c.Writer.Header().Set("Access-Control-Allow-Origin", "*")
+		origin := c.GetHeader("Origin")
+		if origin != "" {
+			switch {
+			case allowAny:
+				c.Writer.Header().Set("Access-Control-Allow-Origin", origin)
+				c.Writer.Header().Set("Vary", "Origin")
+			default:
+				if _, ok := allowlist[origin]; ok {
+					c.Writer.Header().Set("Access-Control-Allow-Origin", origin)
+					c.Writer.Header().Set("Vary", "Origin")
+				}
+				// Unknown origin: do not set ACAO; the browser will block.
+			}
+		}
 		c.Writer.Header().Set("Access-Control-Allow-Methods", "GET, POST, PUT, DELETE, OPTIONS")
 		c.Writer.Header().Set("Access-Control-Allow-Headers", "Content-Type, Authorization")
+		c.Writer.Header().Set("Access-Control-Max-Age", "600")

 		if c.Request.Method == "OPTIONS" {
 			c.AbortWithStatus(http.StatusOK)
 			return
 		}
-
 		c.Next()
 	}
 }
@@ -120,8 +165,14 @@ func (s *Server) setupRoutes() {
 		// Authentication related routes (no authentication required)
 		s.route(api, "POST", "/register", "Register new user", s.handleRegister)
 		s.route(api, "POST", "/login", "User login, returns JWT token", s.handleLogin)
-		s.route(api, "POST", "/reset-password", "Reset password", s.handleResetPassword)
-		s.route(api, "POST", "/reset-account", "Clear all users and reset system to allow re-registration", s.handleResetAccount)
+		// SECURITY: /reset-password and /reset-account are PUBLIC by necessity —
+		// they ARE the recovery paths when the user can no longer log in. Both
+		// require a literal confirmation phrase in the request body, which
+		// blocks accidental triggers and drive-by scripts. The historical
+		// takeover path (post-reset wallet-key adoption) was closed by
+		// removing adoptOrphanRecords. See handler_user.go for details.
+		s.route(api, "POST", "/reset-password", "Reset password by email (requires confirm phrase)", s.handleResetPassword)
+		s.route(api, "POST", "/reset-account", "[DESTRUCTIVE] Wipe everything (requires confirm phrase)", s.handleResetAccount)

 		// Routes requiring authentication
 		protected := api.Group("/", s.authMiddleware())
@@ -514,31 +565,46 @@ func isPrivateIP(ip net.IP) bool {
 	return false
 }

-// getTraderFromQuery Get trader from query parameter
+// getTraderFromQuery resolves a trader from the ?trader_id= query parameter.
+//
+// This project is single-user by design, so a strict cross-tenant ownership
+// check would be theatre. We still perform a soft check (the requested trader
+// must appear in the caller's store list when present) — this is cheap defense
+// in depth that future-proofs against accidental multi-account drift and
+// catches typos that would otherwise return another account's data.
 func (s *Server) getTraderFromQuery(c *gin.Context) (*manager.TraderManager, string, error) {
 	userID := c.GetString("user_id")
 	traderID := c.Query("trader_id")

-	// Ensure user's traders are loaded into memory
-	err := s.traderManager.LoadUserTradersFromStore(s.store, userID)
-	if err != nil {
+	// Ensure user's traders are loaded into memory.
+	if err := s.traderManager.LoadUserTradersFromStore(s.store, userID); err != nil {
 		logger.Infof("⚠️ Failed to load traders for user %s: %v", userID, err)
 	}

 	if traderID == "" {
-		// If no trader_id specified, return first trader for this user
+		// No trader_id specified — return first trader for this user, falling
+		// back to the first in-memory trader if no per-user list exists yet.
+		userTraders, err := s.store.Trader().List(userID)
+		if err == nil && len(userTraders) > 0 {
+			return s.traderManager, userTraders[0].ID, nil
+		}
 		ids := s.traderManager.GetTraderIDs()
 		if len(ids) == 0 {
 			return nil, "", fmt.Errorf("No available traders")
 		}
+		return s.traderManager, ids[0], nil
+	}

-		// Get user's trader list, prioritize returning user's own traders
-		userTraders, err := s.store.Trader().List(userID)
-		if err == nil && len(userTraders) > 0 {
-			traderID = userTraders[0].ID
-		} else {
-			traderID = ids[0]
+	// Soft ownership check: if the caller owns any traders in the store and
+	// the requested ID is NOT among them, treat as not-found instead of
+	// silently returning whatever happens to be in the global in-memory map.
+	if userTraders, err := s.store.Trader().List(userID); err == nil && len(userTraders) > 0 {
+		for _, t := range userTraders {
+			if t.ID == traderID {
+				return s.traderManager, traderID, nil
+			}
 		}
+		return nil, "", fmt.Errorf("trader not found for this account")
 	}

 	return s.traderManager, traderID, nil