diff --git a/api/crypto_handler.go b/api/crypto_handler.go index c78022bc..9de5413f 100644 --- a/api/crypto_handler.go +++ b/api/crypto_handler.go @@ -1,7 +1,6 @@ package api import ( - "log" "net/http" "nofx/config" "nofx/crypto" @@ -53,28 +52,16 @@ func (h *CryptoHandler) HandleGetPublicKey(c *gin.Context) { }) } -// ==================== Encrypted Data Decryption Endpoint ==================== - -// HandleDecryptSensitiveData Decrypt encrypted data sent from client -func (h *CryptoHandler) HandleDecryptSensitiveData(c *gin.Context) { - var payload crypto.EncryptedPayload - if err := c.ShouldBindJSON(&payload); err != nil { - c.JSON(http.StatusBadRequest, gin.H{"error": "Invalid request"}) - return - } - - // Decrypt - decrypted, err := h.cryptoService.DecryptSensitiveData(&payload) - if err != nil { - log.Printf("❌ Decryption failed: %v", err) - c.JSON(http.StatusInternalServerError, gin.H{"error": "Decryption failed"}) - return - } - - c.JSON(http.StatusOK, map[string]string{ - "plaintext": decrypted, - }) -} +// ==================== Encrypted Data Decryption ==================== +// +// SECURITY: there is deliberately NO public decrypt endpoint. Transport +// encryption is one-directional — clients encrypt sensitive fields to the +// server's RSA public key and the authenticated config-update handlers +// (handleUpdateModelConfigs / handleUpdateExchangeConfigs / handleCreateExchange) +// decrypt them server-side via cryptoService.DecryptSensitiveData. Exposing a +// generic decrypt route would turn the server into a decryption oracle that any +// unauthenticated caller could use to recover the plaintext of a captured +// ciphertext, defeating the entire transport-encryption layer. // ==================== Audit Log Query Endpoint ==================== diff --git a/api/handler_ai_model.go b/api/handler_ai_model.go index f19b759c..a114520d 100644 --- a/api/handler_ai_model.go +++ b/api/handler_ai_model.go @@ -38,13 +38,19 @@ type SafeModelConfig struct { BalanceUSDC string `json:"balanceUsdc,omitempty"` } +// ModelConfigUpdate is a single model's update payload. It is a named type +// (rather than an inline anonymous struct) so the log-sanitizer in utils.go is +// guaranteed to stay in sync with this shape — a mismatch there is what let +// plaintext credentials reach the logs previously. +type ModelConfigUpdate struct { + Enabled bool `json:"enabled"` + APIKey string `json:"api_key"` + CustomAPIURL string `json:"custom_api_url"` + CustomModelName string `json:"custom_model_name"` +} + type UpdateModelConfigRequest struct { - Models map[string]struct { - Enabled bool `json:"enabled"` - APIKey string `json:"api_key"` - CustomAPIURL string `json:"custom_api_url"` - CustomModelName string `json:"custom_model_name"` - } `json:"models"` + Models map[string]ModelConfigUpdate `json:"models"` } // handleGetModelConfigs Get AI model configurations @@ -225,7 +231,7 @@ func (s *Server) handleUpdateModelConfigs(c *gin.Context) { // Don't return error here since model config was successfully updated to database } - logger.Infof("✓ AI model config updated: %+v", req.Models) + logger.Infof("✓ AI model config updated: %+v", SanitizeModelConfigForLog(req.Models)) c.JSON(http.StatusOK, gin.H{"message": "Model configuration updated"}) } diff --git a/api/handler_exchange.go b/api/handler_exchange.go index 2d5795c0..50208559 100644 --- a/api/handler_exchange.go +++ b/api/handler_exchange.go @@ -69,24 +69,30 @@ func safeExchangeConfigFromStore(exchange *store.Exchange) SafeExchangeConfig { } } +// ExchangeConfigUpdate is a single exchange account's update payload. It is a +// named type (rather than an inline anonymous struct) so the log-sanitizer in +// utils.go is guaranteed to cover every sensitive field — a drift between the +// two shapes is what let passphrases / private keys reach the logs previously. +type ExchangeConfigUpdate struct { + Enabled bool `json:"enabled"` + APIKey string `json:"api_key"` + SecretKey string `json:"secret_key"` + Passphrase string `json:"passphrase"` // OKX specific + Testnet bool `json:"testnet"` + HyperliquidWalletAddr string `json:"hyperliquid_wallet_addr"` + HyperliquidUnifiedAcct bool `json:"hyperliquid_unified_account"` // Unified Account mode + HyperliquidBuilderApproved *bool `json:"hyperliquid_builder_approved"` + AsterUser string `json:"aster_user"` + AsterSigner string `json:"aster_signer"` + AsterPrivateKey string `json:"aster_private_key"` + LighterWalletAddr string `json:"lighter_wallet_addr"` + LighterPrivateKey string `json:"lighter_private_key"` + LighterAPIKeyPrivateKey string `json:"lighter_api_key_private_key"` + LighterAPIKeyIndex int `json:"lighter_api_key_index"` +} + type UpdateExchangeConfigRequest struct { - Exchanges map[string]struct { - Enabled bool `json:"enabled"` - APIKey string `json:"api_key"` - SecretKey string `json:"secret_key"` - Passphrase string `json:"passphrase"` // OKX specific - Testnet bool `json:"testnet"` - HyperliquidWalletAddr string `json:"hyperliquid_wallet_addr"` - HyperliquidUnifiedAcct bool `json:"hyperliquid_unified_account"` // Unified Account mode - HyperliquidBuilderApproved *bool `json:"hyperliquid_builder_approved"` - AsterUser string `json:"aster_user"` - AsterSigner string `json:"aster_signer"` - AsterPrivateKey string `json:"aster_private_key"` - LighterWalletAddr string `json:"lighter_wallet_addr"` - LighterPrivateKey string `json:"lighter_private_key"` - LighterAPIKeyPrivateKey string `json:"lighter_api_key_private_key"` - LighterAPIKeyIndex int `json:"lighter_api_key_index"` - } `json:"exchanges"` + Exchanges map[string]ExchangeConfigUpdate `json:"exchanges"` } // CreateExchangeRequest request structure for creating a new exchange account @@ -297,7 +303,7 @@ func (s *Server) handleUpdateExchangeConfigs(c *gin.Context) { // Don't return error here since exchange config was successfully updated to database } - logger.Infof("✓ Exchange config updated: %+v", req.Exchanges) + logger.Infof("✓ Exchange config updated: %+v", SanitizeExchangeConfigForLog(req.Exchanges)) c.JSON(http.StatusOK, gin.H{"message": "Exchange configuration updated"}) } diff --git a/api/handler_user.go b/api/handler_user.go index af94ba9b..a312616a 100644 --- a/api/handler_user.go +++ b/api/handler_user.go @@ -60,7 +60,7 @@ func (s *Server) handleRegister(c *gin.Context) { var req struct { Email string `json:"email" binding:"required,email"` - Password string `json:"password" binding:"required,min=6"` + Password string `json:"password" binding:"required,min=8"` Lang string `json:"lang"` } @@ -129,6 +129,13 @@ func (s *Server) handleRegister(c *gin.Context) { }) } +// dummyPasswordHash is a valid bcrypt hash of a throwaway value. It is compared +// against when the submitted email does not exist so that login takes roughly +// the same time whether or not the account exists — closing the timing side +// channel that would otherwise let an attacker enumerate valid emails (a fast +// "no such user" vs. a slow bcrypt compare). It is not a secret. +const dummyPasswordHash = "$2a$10$0iF0bCoQLJ6Ph1bF.MXwHOW.IMTxQjeEW.w38dctRQAB2kwB6ga1q" + // handleLogin Handle user login request func (s *Server) handleLogin(c *gin.Context) { var req struct { @@ -144,6 +151,9 @@ func (s *Server) handleLogin(c *gin.Context) { // Get user information user, err := s.store.User().GetByEmail(req.Email) if err != nil { + // Perform a dummy comparison so the response time does not reveal + // whether the email exists (anti user-enumeration), then fail uniformly. + auth.CheckPassword(req.Password, dummyPasswordHash) c.JSON(http.StatusUnauthorized, gin.H{"error": "Email or password incorrect"}) return } diff --git a/api/ratelimit.go b/api/ratelimit.go new file mode 100644 index 00000000..7094a2d8 --- /dev/null +++ b/api/ratelimit.go @@ -0,0 +1,101 @@ +package api + +import ( + "math" + "net/http" + "sync" + "time" + + "github.com/gin-gonic/gin" +) + +// ipRateLimiter is a small, dependency-free token-bucket rate limiter keyed by +// client IP. It is used to throttle the unauthenticated auth endpoints +// (login / register) against online brute-force attacks. +// +// Design notes: +// - Per-IP token bucket with lazy refill (no background goroutine). +// - Idle buckets are evicted opportunistically so a flood of distinct source +// IPs (e.g. spoofed X-Forwarded-For) cannot grow the map without bound. +// - This is a throttle, not an authenticator. Behind a reverse proxy the +// effective key is whatever gin's ClientIP() resolves; operators who +// terminate TLS at a proxy should configure trusted proxies so ClientIP() +// reflects the real peer rather than a spoofable header. +type ipRateLimiter struct { + mu sync.Mutex + buckets map[string]*rlBucket + rate float64 // tokens added per second + burst float64 // maximum tokens (and initial fill) + lastGC time.Time +} + +type rlBucket struct { + tokens float64 + last time.Time +} + +// newIPRateLimiter creates a limiter that allows bursts up to `burst` requests +// and then refills at `ratePerSec` tokens/second per client IP. +func newIPRateLimiter(ratePerSec, burst float64) *ipRateLimiter { + return &ipRateLimiter{ + buckets: make(map[string]*rlBucket), + rate: ratePerSec, + burst: burst, + } +} + +// allow reports whether a request from key is permitted at time now, consuming +// one token when it is. +func (l *ipRateLimiter) allow(key string, now time.Time) bool { + l.mu.Lock() + defer l.mu.Unlock() + + // Opportunistic GC: drop buckets idle for >10 minutes. Bounds memory even + // under a spoofed-IP flood without needing a background goroutine. + if l.lastGC.IsZero() { + l.lastGC = now + } + if now.Sub(l.lastGC) > time.Minute { + for k, b := range l.buckets { + if now.Sub(b.last) > 10*time.Minute { + delete(l.buckets, k) + } + } + l.lastGC = now + } + + b, ok := l.buckets[key] + if !ok { + b = &rlBucket{tokens: l.burst, last: now} + l.buckets[key] = b + } + + // Refill based on elapsed time, capped at burst. + elapsed := now.Sub(b.last).Seconds() + if elapsed > 0 { + b.tokens = math.Min(l.burst, b.tokens+elapsed*l.rate) + b.last = now + } + + if b.tokens < 1 { + return false + } + b.tokens-- + return true +} + +// rateLimitMiddleware throttles requests per client IP, returning 429 when the +// caller exceeds the configured rate. +func rateLimitMiddleware(l *ipRateLimiter) gin.HandlerFunc { + return func(c *gin.Context) { + if !l.allow(c.ClientIP(), time.Now()) { + c.Header("Retry-After", "60") + c.JSON(http.StatusTooManyRequests, gin.H{ + "error": "Too many requests. Please slow down and try again in a minute.", + }) + c.Abort() + return + } + c.Next() + } +} diff --git a/api/ratelimit_test.go b/api/ratelimit_test.go new file mode 100644 index 00000000..e9f871ed --- /dev/null +++ b/api/ratelimit_test.go @@ -0,0 +1,54 @@ +package api + +import ( + "testing" + "time" +) + +// TestIPRateLimiterBurstThenThrottle verifies that a client gets `burst` +// immediate attempts and is then throttled until tokens refill. +func TestIPRateLimiterBurstThenThrottle(t *testing.T) { + // 1 token/sec, burst of 3. + l := newIPRateLimiter(1.0, 3) + now := time.Unix(1_700_000_000, 0) + + // First 3 requests in the same instant are allowed (the burst). + for i := 0; i < 3; i++ { + if !l.allow("1.2.3.4", now) { + t.Fatalf("request %d in burst should be allowed", i+1) + } + } + // 4th in the same instant is throttled. + if l.allow("1.2.3.4", now) { + t.Fatalf("request beyond burst should be throttled") + } + + // After 1 second, one token refills → exactly one more request allowed. + now = now.Add(time.Second) + if !l.allow("1.2.3.4", now) { + t.Fatalf("one token should have refilled after 1s") + } + if l.allow("1.2.3.4", now) { + t.Fatalf("only one token should refill per second") + } +} + +// TestIPRateLimiterIsolatesClients verifies one IP exhausting its bucket does +// not throttle a different IP. +func TestIPRateLimiterIsolatesClients(t *testing.T) { + l := newIPRateLimiter(1.0, 2) + now := time.Unix(1_700_000_000, 0) + + // Exhaust IP A. + if !l.allow("10.0.0.1", now) || !l.allow("10.0.0.1", now) { + t.Fatalf("IP A burst should be allowed") + } + if l.allow("10.0.0.1", now) { + t.Fatalf("IP A should be throttled after burst") + } + + // IP B is unaffected. + if !l.allow("10.0.0.2", now) { + t.Fatalf("IP B should be allowed regardless of IP A") + } +} diff --git a/api/server.go b/api/server.go index b8499b15..bb2f73b6 100644 --- a/api/server.go +++ b/api/server.go @@ -27,6 +27,7 @@ type Server struct { httpServer *http.Server port int telegramReloadCh chan<- struct{} // signal Telegram bot to reload + authLimiter *ipRateLimiter // per-IP throttle for login/register } // NewServer Creates API server @@ -49,6 +50,10 @@ func NewServer(traderManager *manager.TraderManager, st *store.Store, cryptoServ cryptoHandler: cryptoHandler, exchangeAccountStateCache: NewExchangeAccountStateCache(), port: port, + // Auth throttle: allow a small burst (typos / page reloads) then ~1 + // attempt every 6s (10/min) sustained per IP. Generous for a human, + // hostile to online password brute-force. + authLimiter: newIPRateLimiter(1.0/6.0, 8), } // Setup routes @@ -119,6 +124,12 @@ func corsMiddleware() gin.HandlerFunc { // setupRoutes Setup routes func (s *Server) setupRoutes() { + // Ensure the auth throttle exists even when the Server was constructed + // directly (e.g. in tests) rather than via NewServer. + if s.authLimiter == nil { + s.authLimiter = newIPRateLimiter(1.0/6.0, 8) + } + // API route group api := s.router.Group("/api") { @@ -141,10 +152,16 @@ func (s *Server) setupRoutes() { s.route(api, "GET", "/hyperliquid/account", "Get Hyperliquid account balance summary", s.handleHyperliquidAccount) s.route(api, "POST", "/hyperliquid/submit-exchange", "Submit a user-signed Hyperliquid approval action", s.handleHyperliquidSubmitExchange) - // Crypto related endpoints (no authentication required, not exposed to bot) + // Crypto related endpoints (no authentication required, not exposed to bot). + // SECURITY: only the config + public-key endpoints are exposed. Transport + // encryption is one-directional (client encrypts to the server's public key; + // the server decrypts internally on the authenticated config-update handlers). + // A public POST /crypto/decrypt would be a decryption oracle: any + // unauthenticated caller could replay a captured ciphertext and get the + // plaintext (exchange/API credentials) back. It is intentionally NOT + // registered. See crypto_handler.go. api.GET("/crypto/config", s.cryptoHandler.HandleGetCryptoConfig) api.GET("/crypto/public-key", s.cryptoHandler.HandleGetPublicKey) - api.POST("/crypto/decrypt", s.cryptoHandler.HandleDecryptSensitiveData) // Public competition data (no authentication required) s.route(api, "GET", "/traders", "Public trader list", s.handlePublicTraderList) @@ -162,9 +179,13 @@ func (s *Server) setupRoutes() { s.route(api, "GET", "/strategies/public", "Public strategy market", s.handlePublicStrategies) s.route(api, "POST", "/strategies/estimate-tokens", "Estimate token usage for a strategy config", s.handleEstimateTokens) - // 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) + // Authentication related routes (no authentication required). + // These are throttled per-IP to blunt online password brute-force; see + // ratelimit.go. Everything else in the public block is read-only or + // idempotent, so the throttle is scoped to the credential endpoints. + authRoutes := api.Group("/", rateLimitMiddleware(s.authLimiter)) + s.route(authRoutes, "POST", "/register", "Register new user", s.handleRegister) + s.route(authRoutes, "POST", "/login", "User login, returns JWT token", s.handleLogin) // SECURITY: password/account recovery is NOT exposed over HTTP. An // unauthenticated recovery endpoint is a remote auth-bypass on any // public-facing deployment (the confirm phrase is in the frontend and @@ -564,13 +585,15 @@ func isPrivateIP(ip net.IP) bool { return false } -// getTraderFromQuery resolves a trader from the ?trader_id= query parameter. +// getTraderFromQuery resolves a trader from the ?trader_id= query parameter, +// strictly scoped to the authenticated caller. // -// 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. +// Ownership is always enforced against the caller's own trader list in the +// store. We deliberately never fall back to the global in-memory trader map +// (TraderManager holds every account's traders): returning an entry from it for +// a trader the caller does not own is a cross-tenant data leak (IDOR) — a +// freshly-registered user with no traders of their own could otherwise pass any +// other account's trader_id and read its balance, positions and AI decisions. func (s *Server) getTraderFromQuery(c *gin.Context) (*manager.TraderManager, string, error) { userID := c.GetString("user_id") traderID := c.Query("trader_id") @@ -580,33 +603,27 @@ func (s *Server) getTraderFromQuery(c *gin.Context) (*manager.TraderManager, str logger.Infof("⚠️ Failed to load traders for user %s: %v", userID, err) } + // Resolve strictly from the caller's own trader list. + userTraders, err := s.store.Trader().List(userID) + if err != nil { + return nil, "", fmt.Errorf("failed to load traders for this account: %w", err) + } + if len(userTraders) == 0 { + return nil, "", fmt.Errorf("No available traders") + } + if traderID == "" { - // 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 + // No trader_id specified — default to the caller's first trader. + return s.traderManager, userTraders[0].ID, nil } - // 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 - } + // A trader_id was supplied — it must belong to the caller. + 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 + return nil, "", fmt.Errorf("trader not found for this account") } // authMiddleware JWT authentication middleware diff --git a/api/server_test.go b/api/server_test.go index 5b499ecd..172e9b25 100644 --- a/api/server_test.go +++ b/api/server_test.go @@ -2,11 +2,38 @@ package api import ( "encoding/json" + "net/http" + "net/http/httptest" "testing" "nofx/store" + + "github.com/gin-gonic/gin" ) +// TestPublicDecryptRouteNotRegistered is a security regression test: the +// unauthenticated POST /api/crypto/decrypt route was a decryption oracle and +// must never be re-registered. A built server's router must not route to it. +func TestPublicDecryptRouteNotRegistered(t *testing.T) { + gin.SetMode(gin.TestMode) + s := &Server{router: gin.New()} + s.setupRoutes() + + for _, r := range s.router.Routes() { + if r.Method == http.MethodPost && r.Path == "/api/crypto/decrypt" { + t.Fatalf("SECURITY REGRESSION: public decryption oracle POST /api/crypto/decrypt is registered") + } + } + + // Also assert at the HTTP layer that the route is not handled. + req := httptest.NewRequest(http.MethodPost, "/api/crypto/decrypt", nil) + w := httptest.NewRecorder() + s.router.ServeHTTP(w, req) + if w.Code != http.StatusNotFound { + t.Fatalf("expected 404 for POST /api/crypto/decrypt, got %d", w.Code) + } +} + // TestUpdateTraderRequest_SystemPromptTemplate Test whether SystemPromptTemplate field exists when updating trader func TestUpdateTraderRequest_SystemPromptTemplate(t *testing.T) { tests := []struct { diff --git a/api/utils.go b/api/utils.go index 147c2cf1..5dca188e 100644 --- a/api/utils.go +++ b/api/utils.go @@ -15,13 +15,10 @@ func MaskSensitiveString(s string) string { return s[:4] + "****" + s[length-4:] } -// SanitizeModelConfigForLog Sanitize model configuration for log output -func SanitizeModelConfigForLog(models map[string]struct { - Enabled bool `json:"enabled"` - APIKey string `json:"api_key"` - CustomAPIURL string `json:"custom_api_url"` - CustomModelName string `json:"custom_model_name"` -}) map[string]interface{} { +// SanitizeModelConfigForLog Sanitize model configuration for log output. +// Takes the same ModelConfigUpdate type used by the request handler so the two +// can never drift out of sync. +func SanitizeModelConfigForLog(models map[string]ModelConfigUpdate) map[string]interface{} { safe := make(map[string]interface{}) for modelID, cfg := range models { safe[modelID] = map[string]interface{}{ @@ -34,19 +31,12 @@ func SanitizeModelConfigForLog(models map[string]struct { return safe } -// SanitizeExchangeConfigForLog Sanitize exchange configuration for log output -func SanitizeExchangeConfigForLog(exchanges map[string]struct { - Enabled bool `json:"enabled"` - APIKey string `json:"api_key"` - SecretKey string `json:"secret_key"` - Testnet bool `json:"testnet"` - HyperliquidWalletAddr string `json:"hyperliquid_wallet_addr"` - AsterUser string `json:"aster_user"` - AsterSigner string `json:"aster_signer"` - AsterPrivateKey string `json:"aster_private_key"` - LighterWalletAddr string `json:"lighter_wallet_addr"` - LighterPrivateKey string `json:"lighter_private_key"` -}) map[string]interface{} { +// SanitizeExchangeConfigForLog Sanitize exchange configuration for log output. +// Takes the same ExchangeConfigUpdate type used by the request handler so every +// sensitive field is guaranteed to be masked — adding a field to the request +// type without masking it here would not compile around this helper, but more +// importantly keeps the masking exhaustive. +func SanitizeExchangeConfigForLog(exchanges map[string]ExchangeConfigUpdate) map[string]interface{} { safe := make(map[string]interface{}) for exchangeID, cfg := range exchanges { safeExchange := map[string]interface{}{ @@ -61,12 +51,18 @@ func SanitizeExchangeConfigForLog(exchanges map[string]struct { if cfg.SecretKey != "" { safeExchange["secret_key"] = MaskSensitiveString(cfg.SecretKey) } + if cfg.Passphrase != "" { + safeExchange["passphrase"] = MaskSensitiveString(cfg.Passphrase) + } if cfg.AsterPrivateKey != "" { safeExchange["aster_private_key"] = MaskSensitiveString(cfg.AsterPrivateKey) } if cfg.LighterPrivateKey != "" { safeExchange["lighter_private_key"] = MaskSensitiveString(cfg.LighterPrivateKey) } + if cfg.LighterAPIKeyPrivateKey != "" { + safeExchange["lighter_api_key_private_key"] = MaskSensitiveString(cfg.LighterAPIKeyPrivateKey) + } // Add non-sensitive fields directly if cfg.HyperliquidWalletAddr != "" { diff --git a/api/utils_test.go b/api/utils_test.go index 7e053a95..ebcacb63 100644 --- a/api/utils_test.go +++ b/api/utils_test.go @@ -1,6 +1,8 @@ package api import ( + "fmt" + "strings" "testing" ) @@ -48,12 +50,7 @@ func TestMaskSensitiveString(t *testing.T) { } func TestSanitizeModelConfigForLog(t *testing.T) { - models := map[string]struct { - Enabled bool `json:"enabled"` - APIKey string `json:"api_key"` - CustomAPIURL string `json:"custom_api_url"` - CustomModelName string `json:"custom_model_name"` - }{ + models := map[string]ModelConfigUpdate{ "deepseek": { Enabled: true, APIKey: "sk-1234567890abcdefghijklmnopqrstuvwxyz", @@ -88,32 +85,29 @@ func TestSanitizeModelConfigForLog(t *testing.T) { } func TestSanitizeExchangeConfigForLog(t *testing.T) { - exchanges := map[string]struct { - Enabled bool `json:"enabled"` - APIKey string `json:"api_key"` - SecretKey string `json:"secret_key"` - Testnet bool `json:"testnet"` - HyperliquidWalletAddr string `json:"hyperliquid_wallet_addr"` - AsterUser string `json:"aster_user"` - AsterSigner string `json:"aster_signer"` - AsterPrivateKey string `json:"aster_private_key"` - LighterWalletAddr string `json:"lighter_wallet_addr"` - LighterPrivateKey string `json:"lighter_private_key"` - }{ + exchanges := map[string]ExchangeConfigUpdate{ "binance": { Enabled: true, APIKey: "binance_api_key_1234567890abcdef", SecretKey: "binance_secret_key_1234567890abcdef", Testnet: false, - LighterWalletAddr: "", - LighterPrivateKey: "", + }, + "okx": { + Enabled: true, + APIKey: "okx_api_key_1234567890abcdef", + SecretKey: "okx_secret_key_1234567890abcdef", + Passphrase: "okx_passphrase_supersecret_value", + }, + "lighter": { + Enabled: true, + LighterWalletAddr: "0xabcdef0000000000000000000000000000000000", + LighterPrivateKey: "lighter_private_key_1234567890abcdef", + LighterAPIKeyPrivateKey: "lighter_api_key_private_key_1234567890abcdef", }, "hyperliquid": { Enabled: true, HyperliquidWalletAddr: "0x1234567890abcdef1234567890abcdef12345678", Testnet: false, - LighterWalletAddr: "", - LighterPrivateKey: "", }, } @@ -143,6 +137,32 @@ func TestSanitizeExchangeConfigForLog(t *testing.T) { t.Errorf("expected masked secret_key='bina****cdef', got %q", maskedSecretKey) } + // Check OKX passphrase is masked (regression: previously not covered) + okxConfig, ok := result["okx"].(map[string]interface{}) + if !ok { + t.Fatal("okx config not found or wrong type") + } + maskedPassphrase, ok := okxConfig["passphrase"].(string) + if !ok { + t.Fatal("okx passphrase not found or wrong type") + } + if maskedPassphrase != "okx_****alue" { + t.Errorf("expected masked passphrase='okx_****alue', got %q", maskedPassphrase) + } + + // Check Lighter API key private key is masked (regression: previously not covered) + lighterConfig, ok := result["lighter"].(map[string]interface{}) + if !ok { + t.Fatal("lighter config not found or wrong type") + } + maskedLighterAPIKey, ok := lighterConfig["lighter_api_key_private_key"].(string) + if !ok { + t.Fatal("lighter_api_key_private_key not found or wrong type") + } + if maskedLighterAPIKey != "ligh****cdef" { + t.Errorf("expected masked lighter_api_key_private_key='ligh****cdef', got %q", maskedLighterAPIKey) + } + // Check Hyperliquid configuration hlConfig, ok := result["hyperliquid"].(map[string]interface{}) if !ok { @@ -160,6 +180,41 @@ func TestSanitizeExchangeConfigForLog(t *testing.T) { } } +// TestSanitizeExchangeConfigForLog_NoPlaintextSecrets renders the sanitized log +// output exactly as the handler does (`%+v`) and asserts that no plaintext +// secret — including the passphrase and lighter API key private key that were +// historically not redacted — survives into the log line. +func TestSanitizeExchangeConfigForLog_NoPlaintextSecrets(t *testing.T) { + secrets := map[string]string{ + "api_key": "binance_api_key_1234567890abcdef", + "secret_key": "binance_secret_key_1234567890abcdef", + "passphrase": "okx_passphrase_supersecret_value", + "aster_private_key": "aster_private_key_1234567890abcdef", + "lighter_private_key": "lighter_private_key_1234567890abcdef", + "lighter_api_key_private_key": "lighter_api_key_private_key_1234567890abcdef", + } + + exchanges := map[string]ExchangeConfigUpdate{ + "okx": { + Enabled: true, + APIKey: secrets["api_key"], + SecretKey: secrets["secret_key"], + Passphrase: secrets["passphrase"], + AsterPrivateKey: secrets["aster_private_key"], + LighterPrivateKey: secrets["lighter_private_key"], + LighterAPIKeyPrivateKey: secrets["lighter_api_key_private_key"], + }, + } + + rendered := fmt.Sprintf("%+v", SanitizeExchangeConfigForLog(exchanges)) + + for field, secret := range secrets { + if strings.Contains(rendered, secret) { + t.Errorf("sanitized log leaked plaintext %s: %q present in %q", field, secret, rendered) + } + } +} + func TestMaskEmail(t *testing.T) { tests := []struct { name string diff --git a/crypto/crypto.go b/crypto/crypto.go index f2142155..f3f3261b 100644 --- a/crypto/crypto.go +++ b/crypto/crypto.go @@ -282,12 +282,16 @@ func isEncryptedStorageValue(value string) bool { } func (cs *CryptoService) DecryptPayload(payload *EncryptedPayload) ([]byte, error) { - // 1. Validate timestamp (prevent replay attacks) - if payload.TS != 0 { - elapsed := time.Since(time.Unix(payload.TS, 0)) - if elapsed > 5*time.Minute || elapsed < -1*time.Minute { - return nil, errors.New("timestamp invalid or expired") - } + // 1. Validate timestamp (prevent replay attacks). + // The timestamp is mandatory: a missing/zero ts previously skipped this check + // entirely, which let a captured ciphertext be replayed indefinitely. The + // client (web/src/lib/crypto.ts) always stamps ts, so requiring it is safe. + if payload.TS == 0 { + return nil, errors.New("missing timestamp") + } + elapsed := time.Since(time.Unix(payload.TS, 0)) + if elapsed > 5*time.Minute || elapsed < -1*time.Minute { + return nil, errors.New("timestamp invalid or expired") } // 2. Decode base64url @@ -455,8 +459,11 @@ func (es EncryptedString) Value() (driver.Value, error) { if globalCryptoService != nil { encrypted, err := globalCryptoService.EncryptForStorage(string(es)) if err != nil { - // If encryption fails, return the original value - return string(es), nil + // Fail closed: never silently persist a plaintext secret when + // encryption was expected to happen. Returning the error aborts the + // write so a misconfigured/broken crypto service cannot leak + // credentials into the database in cleartext. + return nil, fmt.Errorf("failed to encrypt sensitive field for storage: %w", err) } return encrypted, nil } diff --git a/go.mod b/go.mod index be9a705d..f8a4b187 100644 --- a/go.mod +++ b/go.mod @@ -1,6 +1,6 @@ module nofx -go 1.25.10 +go 1.25.11 require ( github.com/adshao/go-binance/v2 v2.8.9 @@ -22,6 +22,7 @@ require ( github.com/stretchr/testify v1.11.1 golang.org/x/crypto v0.51.0 golang.org/x/net v0.55.0 + golang.org/x/term v0.43.0 golang.org/x/text v0.37.0 gorm.io/driver/postgres v1.6.0 gorm.io/driver/sqlite v1.6.0 @@ -96,7 +97,6 @@ require ( golang.org/x/exp v0.0.0-20250620022241-b7579e27df2b // indirect golang.org/x/sync v0.20.0 // indirect golang.org/x/sys v0.45.0 // indirect - golang.org/x/term v0.43.0 // indirect google.golang.org/protobuf v1.36.11 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect howett.net/plist v1.0.1 // indirect diff --git a/web/src/lib/crypto.ts b/web/src/lib/crypto.ts index 4832f113..9e3d1fdb 100644 --- a/web/src/lib/crypto.ts +++ b/web/src/lib/crypto.ts @@ -179,24 +179,12 @@ export class CryptoService { return data.public_key || '' } - static async decryptSensitiveData( - payload: EncryptedPayload - ): Promise { - const response = await fetch('/api/crypto/decrypt', { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - }, - body: JSON.stringify(payload), - }) - - if (!response.ok) { - throw new Error(`Decryption failed: ${response.statusText}`) - } - - const result = await response.json() - return result.plaintext - } + // NOTE: there is intentionally no decryptSensitiveData() here. Transport + // encryption is one-directional: the client encrypts sensitive fields to the + // server's public key and the server decrypts them internally on the + // authenticated config endpoints. The server exposes no public decrypt route, + // so a client-side decrypt helper would be both useless and a security + // anti-pattern (it implied an unauthenticated decryption oracle existed). } // 生成混淆字符串（用于剪贴板混淆）