From c15f8510fbd5ccf9a122f88d6e975d1ef69e00b3 Mon Sep 17 00:00:00 2001 From: Jeff Halter <868228+jhalter@users.noreply.github.com> Date: Fri, 12 Jun 2026 08:47:54 -0700 Subject: Fix data races on ClientConn state, banner reload, and rate limiter growth ClientConn's mutable session state (Flags, UserName, Icon, IdleTime, AutoReply) was guarded inconsistently: two mutexes (FlagsMU and mu) covered some paths while others mutated or read the fields with no locking at all, including HandleSetClientUserInfo, HandleUpdateUser (which writes other clients' admin flag), the login flow, the HTTP API handlers, and the keepalive loop. Consolidate on a single mutex with accessor methods (SetFlag/IsFlagSet/FlagBytes, SetUserName/GetUserName, and so on) used by all production code; direct field access remains for test construction. The idle/away logic moves into incrementIdleTime and clearIdleAndAway helpers that report whether a notification is needed, so SendAll is no longer called while holding the lock. HandleRejectChatInvite also no longer appends to the username slice, which could write past its length into the backing buffer. The server banner is now behind Banner/SetBanner with an RWMutex: the SIGHUP reload previously reassigned the field while banner download goroutines read it, and nilled it when the file read failed. Reload now keeps the previous banner on failure. Per-IP rate limiter entries now record a last-seen time, and the keepalive ticker evicts entries idle for over seven days, so the map no longer grows unboundedly with each unique client IP. --- cmd/mobius-hotline-server/main.go | 9 +- hotline/client_conn.go | 140 ++++++++++++++++++++++++----- hotline/client_conn_test.go | 57 ++++++++++++ hotline/server.go | 103 ++++++++++++++------- hotline/server_test.go | 42 +++++++++ internal/mobius/api.go | 4 +- internal/mobius/handlers_accounts.go | 10 +-- internal/mobius/handlers_chat.go | 57 ++++++------ internal/mobius/handlers_news.go | 4 +- internal/mobius/handlers_session.go | 63 +++++++------ internal/mobius/handlers_transfers.go | 2 +- internal/mobius/handlers_transfers_test.go | 10 ++- 12 files changed, 368 insertions(+), 133 deletions(-) diff --git a/cmd/mobius-hotline-server/main.go b/cmd/mobius-hotline-server/main.go index 1c298a2..117cb2a 100644 --- a/cmd/mobius-hotline-server/main.go +++ b/cmd/mobius-hotline-server/main.go @@ -159,11 +159,12 @@ func main() { } bannerPath := path.Join(*configDir, config.BannerFile) - srv.Banner, err = os.ReadFile(bannerPath) + banner, err := os.ReadFile(bannerPath) if err != nil { slogger.Error("Error loading banner", "err", err) os.Exit(1) } + srv.SetBanner(banner) reloadFunc := func() { if err := srv.MessageBoard.(*mobius.FlatNews).Reload(); err != nil { @@ -185,11 +186,13 @@ func main() { slogger.Error("Error reloading agreement", "err", err) } - // Let's try to reload the banner + // Let's try to reload the banner. On failure, keep serving the previous banner. bannerPath := path.Join(*configDir, config.BannerFile) - srv.Banner, err = os.ReadFile(bannerPath) + banner, err := os.ReadFile(bannerPath) if err != nil { slogger.Error("Error reloading banner", "err", err) + } else { + srv.SetBanner(banner) } } diff --git a/hotline/client_conn.go b/hotline/client_conn.go index 363376e..3419e0a 100644 --- a/hotline/client_conn.go +++ b/hotline/client_conn.go @@ -30,23 +30,26 @@ type ClientConn struct { Connection io.ReadWriteCloser RemoteAddr string ID ClientID - Icon []byte // TODO: make fixed size of 2 Version []byte // TODO: make fixed size of 2 - FlagsMU sync.Mutex // TODO: move into UserFlags struct - Flags UserFlags + Account *Account + Server *Server // TODO: consider adding methods to interact with server + // The following fields hold mutable session state guarded by mu. They are read and written + // by multiple goroutines (the client's own transaction loop, other clients' handlers, and the + // server keepalive loop), so production code must use the accessor methods below. Direct + // field access is only safe before the connection is shared, e.g. in tests. + Flags UserFlags UserName []byte - Account *Account + Icon []byte // TODO: make fixed size of 2 IdleTime int - Server *Server // TODO: consider adding methods to interact with server AutoReply []byte ClientFileTransferMgr ClientFileTransferMgr Logger *slog.Logger - mu sync.RWMutex + mu sync.RWMutex // guards the mutable session state fields above sendCh chan Transaction sendInit sync.Once @@ -120,6 +123,109 @@ func (cc *ClientConn) writeLoop() { func (cc *ClientConn) TextDecoder() *encoding.Decoder { return cc.Server.TextDecoder } func (cc *ClientConn) TextEncoder() *encoding.Encoder { return cc.Server.TextEncoder } +// SetFlag sets the user flag at position flag to v. +func (cc *ClientConn) SetFlag(flag int, v uint) { + cc.mu.Lock() + defer cc.mu.Unlock() + + cc.Flags.Set(flag, v) +} + +// IsFlagSet reports whether the user flag at position flag is set. +func (cc *ClientConn) IsFlagSet(flag int) bool { + cc.mu.RLock() + defer cc.mu.RUnlock() + + return cc.Flags.IsSet(flag) +} + +// FlagBytes returns a copy of the user flags bitmap, suitable for use as a transaction field. +func (cc *ClientConn) FlagBytes() []byte { + cc.mu.RLock() + defer cc.mu.RUnlock() + + flags := cc.Flags + return flags[:] +} + +// SetUserName sets the client's display name. +func (cc *ClientConn) SetUserName(name []byte) { + cc.mu.Lock() + defer cc.mu.Unlock() + + cc.UserName = name +} + +// GetUserName returns the client's display name. Callers must not modify the returned slice. +func (cc *ClientConn) GetUserName() []byte { + cc.mu.RLock() + defer cc.mu.RUnlock() + + return cc.UserName +} + +// SetIcon sets the client's icon ID bytes. +func (cc *ClientConn) SetIcon(icon []byte) { + cc.mu.Lock() + defer cc.mu.Unlock() + + cc.Icon = icon +} + +// GetIcon returns the client's icon ID bytes. Callers must not modify the returned slice. +func (cc *ClientConn) GetIcon() []byte { + cc.mu.RLock() + defer cc.mu.RUnlock() + + return cc.Icon +} + +// SetAutoReply sets the client's away auto-reply message. +func (cc *ClientConn) SetAutoReply(msg []byte) { + cc.mu.Lock() + defer cc.mu.Unlock() + + cc.AutoReply = msg +} + +// GetAutoReply returns the client's away auto-reply message. Callers must not modify the +// returned slice. +func (cc *ClientConn) GetAutoReply() []byte { + cc.mu.RLock() + defer cc.mu.RUnlock() + + return cc.AutoReply +} + +// incrementIdleTime adds interval seconds to the client's idle time. It returns true if this +// crossed the idle threshold and marked the client as away, in which case the caller should +// notify other clients of the change. +func (cc *ClientConn) incrementIdleTime(interval int) bool { + cc.mu.Lock() + defer cc.mu.Unlock() + + cc.IdleTime += interval + if cc.IdleTime > userIdleSeconds && !cc.Flags.IsSet(UserFlagAway) { + cc.Flags.Set(UserFlagAway, 1) + return true + } + return false +} + +// clearIdleAndAway resets the client's idle timer. It returns true if the client was marked as +// away and is no longer, in which case the caller should notify other clients of the change. +func (cc *ClientConn) clearIdleAndAway() bool { + cc.mu.Lock() + defer cc.mu.Unlock() + + cc.IdleTime = 0 + if cc.Flags.IsSet(UserFlagAway) { + cc.Flags.Set(UserFlagAway, 0) + return true + } + return false +} + func (cc *ClientConn) FileRoot() string { if cc.Account.FileRoot != "" { return cc.Account.FileRoot @@ -201,23 +307,15 @@ func (cc *ClientConn) handleTransaction(transaction Transaction) { } if transaction.Type != TranKeepAlive { - cc.mu.Lock() - defer cc.mu.Unlock() - - // reset the user idle timer - cc.IdleTime = 0 - - // if user was previously idle, mark as not idle and notify other connected clients that - // the user is no longer away - if cc.Flags.IsSet(UserFlagAway) { - cc.Flags.Set(UserFlagAway, 0) - + // Reset the user idle timer. If the user was previously marked as away, notify other + // connected clients that the user is no longer away. + if cc.clearIdleAndAway() { cc.SendAll( TranNotifyChangeUser, NewField(FieldUserID, cc.ID[:]), - NewField(FieldUserFlags, cc.Flags[:]), - NewField(FieldUserName, cc.UserName), - NewField(FieldUserIconID, cc.Icon), + NewField(FieldUserFlags, cc.FlagBytes()), + NewField(FieldUserName, cc.GetUserName()), + NewField(FieldUserIconID, cc.GetIcon()), ) } } @@ -328,7 +426,7 @@ func formatDownloadList(fts []FileTransfer) (s string) { func (cc *ClientConn) String() string { template := fmt.Sprintf( userInfoTemplate, - cc.UserName, + cc.GetUserName(), cc.Account.Name, cc.Account.Login, cc.RemoteAddr, diff --git a/hotline/client_conn_test.go b/hotline/client_conn_test.go index 5c5463d..3312954 100644 --- a/hotline/client_conn_test.go +++ b/hotline/client_conn_test.go @@ -660,3 +660,60 @@ func TestClientConn_SendDisconnectRace(t *testing.T) { wg.Wait() } + +func TestClientConn_incrementIdleTime(t *testing.T) { + cc := &ClientConn{} + + // Increment until just below the idle threshold: not yet away. + for i := 0; i < userIdleSeconds/idleCheckInterval; i++ { + assert.False(t, cc.incrementIdleTime(idleCheckInterval)) + } + assert.False(t, cc.IsFlagSet(UserFlagAway)) + + // The increment that crosses the threshold marks the client away exactly once. + assert.True(t, cc.incrementIdleTime(idleCheckInterval)) + assert.True(t, cc.IsFlagSet(UserFlagAway)) + assert.False(t, cc.incrementIdleTime(idleCheckInterval), "already-away client should not be marked away again") +} + +func TestClientConn_clearIdleAndAway(t *testing.T) { + cc := &ClientConn{IdleTime: 500} + + // Not away: idle timer resets, no notification needed. + assert.False(t, cc.clearIdleAndAway()) + assert.Equal(t, 0, cc.IdleTime) + + // Away: flag clears and the caller is told to notify. + cc.SetFlag(UserFlagAway, 1) + assert.True(t, cc.clearIdleAndAway()) + assert.False(t, cc.IsFlagSet(UserFlagAway)) + assert.False(t, cc.clearIdleAndAway(), "second clear should report no change") +} + +// TestClientConn_sessionStateRace exercises concurrent access to the mutable session state through +// the accessor methods. Run with -race. +func TestClientConn_sessionStateRace(t *testing.T) { + cc := &ClientConn{} + + var wg sync.WaitGroup + for range 4 { + wg.Add(1) + go func() { + defer wg.Done() + for i := range 100 { + cc.SetUserName(fmt.Appendf(nil, "user-%d", i)) + _ = cc.GetUserName() + cc.SetIcon([]byte{0, byte(i)}) + _ = cc.GetIcon() + cc.SetAutoReply([]byte("brb")) + _ = cc.GetAutoReply() + cc.SetFlag(UserFlagRefusePM, uint(i%2)) + _ = cc.IsFlagSet(UserFlagRefusePM) + _ = cc.FlagBytes() + _ = cc.incrementIdleTime(idleCheckInterval) + _ = cc.clearIdleAndAway() + } + }() + } + wg.Wait() +} diff --git a/hotline/server.go b/hotline/server.go index 3459346..8a530c2 100644 --- a/hotline/server.go +++ b/hotline/server.go @@ -34,7 +34,7 @@ type Server struct { NetInterface string Port int - rateLimiters map[string]*rate.Limiter + rateLimiters map[string]*rateLimiterEntry rateLimitersMu sync.Mutex handlers map[TranType]HandlerFunc @@ -49,7 +49,9 @@ type Server struct { FS FileStore // Storage backend to use for File storage Agreement io.ReadSeeker - Banner []byte + + banner []byte // server banner image; guarded by bannerMu as it is replaced on config reload + bannerMu sync.RWMutex FileTransferMgr FileTransferMgr ChatMgr ChatManager @@ -80,6 +82,22 @@ func (s *Server) initShutdownCh() { s.shutdownInit.Do(func() { s.shutdownCh = make(chan struct{}) }) } +// Banner returns the server banner image. Callers must not modify the returned slice. +func (s *Server) Banner() []byte { + s.bannerMu.RLock() + defer s.bannerMu.RUnlock() + + return s.banner +} + +// SetBanner replaces the server banner image. +func (s *Server) SetBanner(banner []byte) { + s.bannerMu.Lock() + defer s.bannerMu.Unlock() + + s.banner = banner +} + type Option = func(s *Server) func WithConfig(config Config) func(s *Server) { @@ -129,7 +147,7 @@ type ServerConfig struct { func NewServer(options ...Option) (*Server, error) { server := Server{ handlers: make(map[TranType]HandlerFunc), - rateLimiters: make(map[string]*rate.Limiter), + rateLimiters: make(map[string]*rateLimiterEntry), FS: &OSFileStore{}, ChatMgr: NewMemChatManager(), ClientMgr: NewMemClientMgr(), @@ -274,6 +292,29 @@ func (s *Server) Send(t Transaction) { // 0.5 = 1 connection every 2 seconds const perIPRateLimit = rate.Limit(0.5) +// rateLimiterTTL is how long the rate limiter for an idle IP address is retained before eviction. +const rateLimiterTTL = 7 * 24 * time.Hour + +type rateLimiterEntry struct { + limiter *rate.Limiter + lastSeen time.Time +} + +// sweepRateLimiters evicts rate limiters for IP addresses that have not connected recently, so +// the rate limiter map does not grow unbounded over the lifetime of the server. +func (s *Server) sweepRateLimiters() { + cutoff := time.Now().Add(-rateLimiterTTL) + + s.rateLimitersMu.Lock() + defer s.rateLimitersMu.Unlock() + + for ip, entry := range s.rateLimiters { + if entry.lastSeen.Before(cutoff) { + delete(s.rateLimiters, ip) + } + } +} + func (s *Server) Serve(ctx context.Context, ln net.Listener) error { for { conn, err := ln.Accept() @@ -302,11 +343,13 @@ func (s *Server) Serve(ctx context.Context, ln net.Listener) error { // Check if we have an existing rate limit for the IP and create one if we do not. s.rateLimitersMu.Lock() - rl, ok := s.rateLimiters[ipAddr] + entry, ok := s.rateLimiters[ipAddr] if !ok { - rl = rate.NewLimiter(perIPRateLimit, 1) - s.rateLimiters[ipAddr] = rl + entry = &rateLimiterEntry{limiter: rate.NewLimiter(perIPRateLimit, 1)} + s.rateLimiters[ipAddr] = entry } + entry.lastSeen = time.Now() + rl := entry.limiter s.rateLimitersMu.Unlock() // Check if the rate limit is exceeded and close the connection if so. @@ -417,6 +460,7 @@ const ( // keepaliveHandler runs every idleCheckInterval seconds and increments a user's idle time by idleCheckInterval seconds. // If the updated idle time exceeds userIdleSeconds and the user was not previously idle, we notify all connected clients // that the user has gone idle. For most clients, this turns the user grey in the user list. +// It also sweeps stale per-IP rate limiters on each tick. func (s *Server) keepaliveHandler(ctx context.Context) { ticker := time.NewTicker(idleCheckInterval * time.Second) defer ticker.Stop() @@ -427,23 +471,18 @@ func (s *Server) keepaliveHandler(ctx context.Context) { return case <-ticker.C: for _, c := range s.ClientMgr.List() { - c.mu.Lock() - c.IdleTime += idleCheckInterval - - // Check if the user - if c.IdleTime > userIdleSeconds && !c.Flags.IsSet(UserFlagAway) { - c.Flags.Set(UserFlagAway, 1) - + if c.incrementIdleTime(idleCheckInterval) { c.SendAll( TranNotifyChangeUser, NewField(FieldUserID, c.ID[:]), - NewField(FieldUserFlags, c.Flags[:]), - NewField(FieldUserName, c.UserName), - NewField(FieldUserIconID, c.Icon), + NewField(FieldUserFlags, c.FlagBytes()), + NewField(FieldUserName, c.GetUserName()), + NewField(FieldUserIconID, c.GetIcon()), ) } - c.mu.Unlock() } + + s.sweepRateLimiters() } } } @@ -547,8 +586,8 @@ func (s *Server) handleNewConnection(ctx context.Context, rwc io.ReadWriteCloser defer func() { if s.Redis != nil { s.Redis.SRem(context.Background(), RedisKeyOnline, login+"::"+ipAddr) - if len(c.UserName) != 0 { - s.Redis.SRem(context.Background(), RedisKeyOnline, login+":"+string(c.UserName)+":"+ipAddr) + if userName := c.GetUserName(); len(userName) != 0 { + s.Redis.SRem(context.Background(), RedisKeyOnline, login+":"+string(userName)+":"+ipAddr) } } c.Disconnect() @@ -574,7 +613,7 @@ func (s *Server) handleNewConnection(ctx context.Context, rwc io.ReadWriteCloser } if clientLogin.GetField(FieldUserIconID).Data != nil { - c.Icon = clientLogin.GetField(FieldUserIconID).Data + c.SetIcon(clientLogin.GetField(FieldUserIconID).Data) } c.Account = c.Server.AccountManager.Get(login) @@ -584,14 +623,14 @@ func (s *Server) handleNewConnection(ctx context.Context, rwc io.ReadWriteCloser if clientLogin.GetField(FieldUserName).Data != nil { if c.Authorize(AccessAnyName) { - c.UserName = clientLogin.GetField(FieldUserName).Data + c.SetUserName(clientLogin.GetField(FieldUserName).Data) } else { - c.UserName = []byte(c.Account.Name) + c.SetUserName([]byte(c.Account.Name)) } } if c.Authorize(AccessDisconUser) { - c.Flags.Set(UserFlagAdmin, 1) + c.SetFlag(UserFlagAdmin, 1) } c.Send(c.NewReply(&clientLogin, @@ -620,18 +659,18 @@ func (s *Server) handleNewConnection(ctx context.Context, rwc io.ReadWriteCloser // If the client has provided a username as part of the login, we can infer that it is using the 1.2.3 login // flow and not the 1.5+ flow. - if len(c.UserName) != 0 { + if userName := c.GetUserName(); len(userName) != 0 { // Add the client username to the logger. For 1.5+ clients, we don't have this information yet as it comes as // part of TranAgreed - c.Logger = c.Logger.With("name", string(c.UserName)) + c.Logger = c.Logger.With("name", string(userName)) c.Logger.Info("Login successful") // Update the Redis set with the new information - if s.Redis != nil && len(c.UserName) != 0 { + if s.Redis != nil { // Remove old entry (login::ip) s.Redis.SRem(context.Background(), RedisKeyOnline, login+"::"+ipAddr) // Add new entry with login, nickname, ip - s.Redis.SAdd(context.Background(), RedisKeyOnline, login+":"+string(c.UserName)+":"+ipAddr) + s.Redis.SAdd(context.Background(), RedisKeyOnline, login+":"+string(userName)+":"+ipAddr) } // Notify other clients on the server that the new user has logged in. For 1.5+ clients we don't have this @@ -639,10 +678,10 @@ func (s *Server) handleNewConnection(ctx context.Context, rwc io.ReadWriteCloser for _, t := range c.NotifyOthers( NewTransaction( TranNotifyChangeUser, [2]byte{0, 0}, - NewField(FieldUserName, c.UserName), + NewField(FieldUserName, userName), NewField(FieldUserID, c.ID[:]), - NewField(FieldUserIconID, c.Icon), - NewField(FieldUserFlags, c.Flags[:]), + NewField(FieldUserIconID, c.GetIcon()), + NewField(FieldUserFlags, c.FlagBytes()), ), ) { c.Server.Send(t) @@ -701,7 +740,7 @@ func (s *Server) handleFileTransfer(ctx context.Context, rwc io.ReadWriter) erro rLogger := s.Logger.With( "remoteAddr", remoteAddr, "login", fileTransfer.ClientConn.Account.Login, - "Name", string(fileTransfer.ClientConn.UserName), + "Name", string(fileTransfer.ClientConn.GetUserName()), ) fullPath, err := ReadPath(fileTransfer.FileRoot, fileTransfer.FilePath, fileTransfer.FileName, s.TextDecoder) @@ -711,7 +750,7 @@ func (s *Server) handleFileTransfer(ctx context.Context, rwc io.ReadWriter) erro switch fileTransfer.Type { case BannerDownload: - if _, err := io.Copy(rwc, bytes.NewBuffer(s.Banner)); err != nil { + if _, err := io.Copy(rwc, bytes.NewReader(s.Banner())); err != nil { return fmt.Errorf("banner download: %w", err) } case FileDownload: diff --git a/hotline/server_test.go b/hotline/server_test.go index a6c5e19..ebcd073 100644 --- a/hotline/server_test.go +++ b/hotline/server_test.go @@ -10,6 +10,7 @@ import ( "net" "os" "strings" + "sync" "testing" "time" @@ -18,6 +19,7 @@ import ( "github.com/stretchr/testify/require" "golang.org/x/text/encoding" "golang.org/x/text/encoding/charmap" + "golang.org/x/time/rate" ) type mockReadWriter struct { @@ -1036,3 +1038,43 @@ func TestServer_Shutdown_stopsListenAndServe(t *testing.T) { t.Fatal("ListenAndServe did not return after Shutdown") } } + +func TestServer_Banner(t *testing.T) { + srv := &Server{} + assert.Nil(t, srv.Banner()) + + srv.SetBanner([]byte("banner-data")) + assert.Equal(t, []byte("banner-data"), srv.Banner()) + + // Concurrent reads and writes; run with -race. + var wg sync.WaitGroup + for range 4 { + wg.Add(1) + go func() { + defer wg.Done() + for i := range 100 { + srv.SetBanner([]byte{byte(i)}) + _ = srv.Banner() + } + }() + } + wg.Wait() +} + +func TestServer_sweepRateLimiters(t *testing.T) { + srv := &Server{rateLimiters: make(map[string]*rateLimiterEntry)} + + srv.rateLimiters["10.0.0.1"] = &rateLimiterEntry{ + limiter: rate.NewLimiter(perIPRateLimit, 1), + lastSeen: time.Now().Add(-rateLimiterTTL - time.Minute), // stale + } + srv.rateLimiters["10.0.0.2"] = &rateLimiterEntry{ + limiter: rate.NewLimiter(perIPRateLimit, 1), + lastSeen: time.Now(), // fresh + } + + srv.sweepRateLimiters() + + assert.NotContains(t, srv.rateLimiters, "10.0.0.1", "stale rate limiter should be evicted") + assert.Contains(t, srv.rateLimiters, "10.0.0.2", "fresh rate limiter should be retained") +} diff --git a/internal/mobius/api.go b/internal/mobius/api.go index a574bf8..cf0a46f 100644 --- a/internal/mobius/api.go +++ b/internal/mobius/api.go @@ -115,7 +115,7 @@ func (srv *APIServer) OnlineHandler(w http.ResponseWriter, r *http.Request) { for _, c := range srv.hlServer.ClientMgr.List() { users = append(users, map[string]string{ "login": string(c.Account.Login), - "nickname": string(c.UserName), + "nickname": string(c.GetUserName()), "ip": c.RemoteAddr, }) } @@ -169,7 +169,7 @@ func (srv *APIServer) BanHandler(w http.ResponseWriter, r *http.Request) { // Disconnect user if online for _, c := range srv.hlServer.ClientMgr.List() { if (req.Username != "" && c.Account.Login == req.Username) || - (req.Nickname != "" && string(c.UserName) == req.Nickname) || + (req.Nickname != "" && string(c.GetUserName()) == req.Nickname) || (req.IP != "" && c.IP() == req.IP) { c.Disconnect() } diff --git a/internal/mobius/handlers_accounts.go b/internal/mobius/handlers_accounts.go index 871e632..02b7c77 100644 --- a/internal/mobius/handlers_accounts.go +++ b/internal/mobius/handlers_accounts.go @@ -60,9 +60,9 @@ func HandleSetUser(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotlin res = append(res, newT) if c.Authorize(hotline.AccessDisconUser) { - c.Flags.Set(hotline.UserFlagAdmin, 1) + c.SetFlag(hotline.UserFlagAdmin, 1) } else { - c.Flags.Set(hotline.UserFlagAdmin, 0) + c.SetFlag(hotline.UserFlagAdmin, 0) } c.Account.Access = account.Access @@ -70,9 +70,9 @@ func HandleSetUser(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotlin cc.SendAll( hotline.TranNotifyChangeUser, hotline.NewField(hotline.FieldUserID, c.ID[:]), - hotline.NewField(hotline.FieldUserFlags, c.Flags[:]), - hotline.NewField(hotline.FieldUserName, c.UserName), - hotline.NewField(hotline.FieldUserIconID, c.Icon), + hotline.NewField(hotline.FieldUserFlags, c.FlagBytes()), + hotline.NewField(hotline.FieldUserName, c.GetUserName()), + hotline.NewField(hotline.FieldUserIconID, c.GetIcon()), ) } } diff --git a/internal/mobius/handlers_chat.go b/internal/mobius/handlers_chat.go index bda4e58..b81cfa5 100644 --- a/internal/mobius/handlers_chat.go +++ b/internal/mobius/handlers_chat.go @@ -2,10 +2,8 @@ package mobius import ( "bytes" - "encoding/binary" "fmt" "io" - "math/big" "github.com/jhalter/mobius/hotline" ) @@ -28,14 +26,14 @@ func HandleChatSend(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotli // Truncate long usernames // %13.13s: This means a string that is right-aligned in a field of 13 characters. // If the string is longer than 13 characters, it will be truncated to 13 characters. - formattedMsg := fmt.Sprintf("\r%13.13s: %s", cc.UserName, t.GetField(hotline.FieldData).Data) + formattedMsg := fmt.Sprintf("\r%13.13s: %s", cc.GetUserName(), t.GetField(hotline.FieldData).Data) // By holding the option key, Hotline chat allows users to send /me formatted messages like: // *** Halcyon does stuff // This is indicated by the presence of the optional field FieldChatOptions set to a value of 1. // Most clients do not send this option for normal chat messages. if t.GetField(hotline.FieldChatOptions).Data != nil && bytes.Equal(t.GetField(hotline.FieldChatOptions).Data, []byte{0, 1}) { - formattedMsg = fmt.Sprintf("\r*** %s %s", cc.UserName, t.GetField(hotline.FieldData).Data) + formattedMsg = fmt.Sprintf("\r*** %s %s", cc.GetUserName(), t.GetField(hotline.FieldData).Data) } // Truncate the message to the limit. This does not handle the edge case of a string ending on multibyte character. @@ -103,7 +101,7 @@ func HandleSendInstantMsg(cc *hotline.ClientConn, t *hotline.Transaction) (res [ hotline.TranServerMsg, targetID, hotline.NewField(hotline.FieldData, msg.Data), - hotline.NewField(hotline.FieldUserName, cc.UserName), + hotline.NewField(hotline.FieldUserName, cc.GetUserName()), hotline.NewField(hotline.FieldUserID, cc.ID[:]), hotline.NewField(hotline.FieldOptions, []byte{0, 1}), ) @@ -122,13 +120,13 @@ func HandleSendInstantMsg(cc *hotline.ClientConn, t *hotline.Transaction) (res [ } // Check if target user has "Refuse private messages" flag - if otherClient.Flags.IsSet(hotline.UserFlagRefusePM) { + if otherClient.IsFlagSet(hotline.UserFlagRefusePM) { res = append(res, hotline.NewTransaction( hotline.TranServerMsg, cc.ID, - hotline.NewField(hotline.FieldData, []byte(fmt.Sprintf(ErrMsgDoesNotAcceptTemplate, string(otherClient.UserName), "private messages"))), - hotline.NewField(hotline.FieldUserName, otherClient.UserName), + hotline.NewField(hotline.FieldData, []byte(fmt.Sprintf(ErrMsgDoesNotAcceptTemplate, string(otherClient.GetUserName()), "private messages"))), + hotline.NewField(hotline.FieldUserName, otherClient.GetUserName()), hotline.NewField(hotline.FieldUserID, otherClient.ID[:]), hotline.NewField(hotline.FieldOptions, []byte{0, 2}), ), @@ -138,13 +136,13 @@ func HandleSendInstantMsg(cc *hotline.ClientConn, t *hotline.Transaction) (res [ } // Respond with auto reply if other client has it enabled - if len(otherClient.AutoReply) > 0 { + if len(otherClient.GetAutoReply()) > 0 { res = append(res, hotline.NewTransaction( hotline.TranServerMsg, cc.ID, - hotline.NewField(hotline.FieldData, otherClient.AutoReply), - hotline.NewField(hotline.FieldUserName, otherClient.UserName), + hotline.NewField(hotline.FieldData, otherClient.GetAutoReply()), + hotline.NewField(hotline.FieldUserName, otherClient.GetUserName()), hotline.NewField(hotline.FieldUserID, otherClient.ID[:]), hotline.NewField(hotline.FieldOptions, []byte{0, 1}), ), @@ -185,14 +183,13 @@ func HandleInviteNewChat(cc *hotline.ClientConn, t *hotline.Transaction) (res [] // Create a new chat with self as initial member. newChatID := cc.Server.ChatMgr.New(cc) - flagBitmap := big.NewInt(int64(binary.BigEndian.Uint16(targetClient.Flags[:]))) - if flagBitmap.Bit(hotline.UserFlagRefusePChat) == 1 { + if targetClient.IsFlagSet(hotline.UserFlagRefusePChat) { res = append(res, hotline.NewTransaction( hotline.TranServerMsg, cc.ID, - hotline.NewField(hotline.FieldData, []byte(fmt.Sprintf(ErrMsgDoesNotAcceptTemplate, string(targetClient.UserName), "private chats"))), - hotline.NewField(hotline.FieldUserName, targetClient.UserName), + hotline.NewField(hotline.FieldData, []byte(fmt.Sprintf(ErrMsgDoesNotAcceptTemplate, string(targetClient.GetUserName()), "private chats"))), + hotline.NewField(hotline.FieldUserName, targetClient.GetUserName()), hotline.NewField(hotline.FieldUserID, targetClient.ID[:]), hotline.NewField(hotline.FieldOptions, []byte{0, 2}), ), @@ -203,7 +200,7 @@ func HandleInviteNewChat(cc *hotline.ClientConn, t *hotline.Transaction) (res [] hotline.TranInviteToChat, targetID, hotline.NewField(hotline.FieldChatID, newChatID[:]), - hotline.NewField(hotline.FieldUserName, cc.UserName), + hotline.NewField(hotline.FieldUserName, cc.GetUserName()), hotline.NewField(hotline.FieldUserID, cc.ID[:]), ), ) @@ -213,10 +210,10 @@ func HandleInviteNewChat(cc *hotline.ClientConn, t *hotline.Transaction) (res [] res, cc.NewReply(t, hotline.NewField(hotline.FieldChatID, newChatID[:]), - hotline.NewField(hotline.FieldUserName, cc.UserName), + hotline.NewField(hotline.FieldUserName, cc.GetUserName()), hotline.NewField(hotline.FieldUserID, cc.ID[:]), - hotline.NewField(hotline.FieldUserIconID, cc.Icon), - hotline.NewField(hotline.FieldUserFlags, cc.Flags[:]), + hotline.NewField(hotline.FieldUserIconID, cc.GetIcon()), + hotline.NewField(hotline.FieldUserFlags, cc.FlagBytes()), ), ) } @@ -245,16 +242,16 @@ func HandleInviteToChat(cc *hotline.ClientConn, t *hotline.Transaction) (res []h hotline.TranInviteToChat, targetID, hotline.NewField(hotline.FieldChatID, chatID), - hotline.NewField(hotline.FieldUserName, cc.UserName), + hotline.NewField(hotline.FieldUserName, cc.GetUserName()), hotline.NewField(hotline.FieldUserID, cc.ID[:]), ), cc.NewReply( t, hotline.NewField(hotline.FieldChatID, chatID), - hotline.NewField(hotline.FieldUserName, cc.UserName), + hotline.NewField(hotline.FieldUserName, cc.GetUserName()), hotline.NewField(hotline.FieldUserID, cc.ID[:]), - hotline.NewField(hotline.FieldUserIconID, cc.Icon), - hotline.NewField(hotline.FieldUserFlags, cc.Flags[:]), + hotline.NewField(hotline.FieldUserIconID, cc.GetIcon()), + hotline.NewField(hotline.FieldUserFlags, cc.FlagBytes()), ), } } @@ -278,7 +275,7 @@ func HandleRejectChatInvite(cc *hotline.ClientConn, t *hotline.Transaction) (res hotline.TranChatMsg, c.ID, hotline.NewField(hotline.FieldChatID, chatID[:]), - hotline.NewField(hotline.FieldData, append(cc.UserName, []byte(" declined invitation to chat")...)), + hotline.NewField(hotline.FieldData, fmt.Appendf(nil, "%s declined invitation to chat", cc.GetUserName())), ), ) } @@ -307,10 +304,10 @@ func HandleJoinChat(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotli hotline.TranNotifyChatChangeUser, c.ID, hotline.NewField(hotline.FieldChatID, chatID[:]), - hotline.NewField(hotline.FieldUserName, cc.UserName), + hotline.NewField(hotline.FieldUserName, cc.GetUserName()), hotline.NewField(hotline.FieldUserID, cc.ID[:]), - hotline.NewField(hotline.FieldUserIconID, cc.Icon), - hotline.NewField(hotline.FieldUserFlags, cc.Flags[:]), + hotline.NewField(hotline.FieldUserIconID, cc.GetIcon()), + hotline.NewField(hotline.FieldUserFlags, cc.FlagBytes()), ), ) } @@ -323,9 +320,9 @@ func HandleJoinChat(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotli for _, c := range cc.Server.ChatMgr.Members(chatID) { b, err := io.ReadAll(&hotline.User{ ID: c.ID, - Icon: c.Icon, - Flags: c.Flags[:], - Name: string(c.UserName), + Icon: c.GetIcon(), + Flags: c.FlagBytes(), + Name: string(c.GetUserName()), }) if err != nil { cc.Logger.Error("join chat: read member info", "err", err) diff --git a/internal/mobius/handlers_news.go b/internal/mobius/handlers_news.go index 5946fa5..29b0ae2 100644 --- a/internal/mobius/handlers_news.go +++ b/internal/mobius/handlers_news.go @@ -33,7 +33,7 @@ func HandleTranOldPostNews(cc *hotline.ClientConn, t *hotline.Transaction) (res newsTemplate = cc.Server.Config.NewsDelimiter } - newsPost := fmt.Sprintf(newsTemplate+"\r", cc.UserName, time.Now().Format(newsDateTemplate), t.GetField(hotline.FieldData).Data) + newsPost := fmt.Sprintf(newsTemplate+"\r", cc.GetUserName(), time.Now().Format(newsDateTemplate), t.GetField(hotline.FieldData).Data) newsPost = strings.ReplaceAll(newsPost, "\n", "\r") _, err := cc.Server.MessageBoard.Write([]byte(newsPost)) @@ -341,7 +341,7 @@ func HandlePostNewsArt(cc *hotline.ClientConn, t *hotline.Transaction) (res []ho uint32(parentArticleID), hotline.NewsArtData{ Title: string(t.GetField(hotline.FieldNewsArtTitle).Data), - Poster: string(cc.UserName), + Poster: string(cc.GetUserName()), Date: hotline.NewTime(time.Now()), DataFlav: hotline.NewsFlavor, Data: string(t.GetField(hotline.FieldNewsArtData).Data), diff --git a/internal/mobius/handlers_session.go b/internal/mobius/handlers_session.go index ad6773b..f17ffa3 100644 --- a/internal/mobius/handlers_session.go +++ b/internal/mobius/handlers_session.go @@ -59,7 +59,7 @@ func HandleGetClientInfoText(cc *hotline.ClientConn, t *hotline.Transaction) (re return append(res, cc.NewReply(t, hotline.NewField(hotline.FieldData, []byte(clientConn.String())), - hotline.NewField(hotline.FieldUserName, clientConn.UserName), + hotline.NewField(hotline.FieldUserName, clientConn.GetUserName()), )) } @@ -74,9 +74,9 @@ func HandleGetUserNameList(cc *hotline.ClientConn, t *hotline.Transaction) (res for _, c := range cc.Server.ClientMgr.List() { b, err := io.ReadAll(&hotline.User{ ID: c.ID, - Icon: c.Icon, - Flags: c.Flags[:], - Name: string(c.UserName), + Icon: c.GetIcon(), + Flags: c.FlagBytes(), + Name: string(c.GetUserName()), }) if err != nil { cc.Logger.Error("get user name list: read user info", "err", err) @@ -101,9 +101,9 @@ func HandleGetUserNameList(cc *hotline.ClientConn, t *hotline.Transaction) (res func HandleTranAgreed(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotline.Transaction) { if t.GetField(hotline.FieldUserName).Data != nil { if cc.Authorize(hotline.AccessAnyName) { - cc.UserName = t.GetField(hotline.FieldUserName).Data + cc.SetUserName(t.GetField(hotline.FieldUserName).Data) } else { - cc.UserName = []byte(cc.Account.Name) + cc.SetUserName([]byte(cc.Account.Name)) } } @@ -114,15 +114,15 @@ func HandleTranAgreed(cc *hotline.ClientConn, t *hotline.Transaction) (res []hot // Remove old entry (login::ip) cc.Server.Redis.SRem(context.Background(), hotline.RedisKeyOnline, login+"::"+ip) // Add new entry with login, nickname, ip - cc.Server.Redis.SAdd(context.Background(), hotline.RedisKeyOnline, login+":"+string(cc.UserName)+":"+ip) + cc.Server.Redis.SAdd(context.Background(), hotline.RedisKeyOnline, login+":"+string(cc.GetUserName())+":"+ip) } // Ban check for nickname - if cc.Server.BanList != nil && cc.Server.BanList.IsNicknameBanned(string(cc.UserName)) { + if cc.Server.BanList != nil && cc.Server.BanList.IsNicknameBanned(string(cc.GetUserName())) { if cc.Server.Redis != nil { // Remove all possible online entries for this login and IP cc.Server.Redis.SRem(context.Background(), hotline.RedisKeyOnline, login+"::"+ip) - cc.Server.Redis.SRem(context.Background(), hotline.RedisKeyOnline, login+":"+string(cc.UserName)+":"+ip) + cc.Server.Redis.SRem(context.Background(), hotline.RedisKeyOnline, login+":"+string(cc.GetUserName())+":"+ip) } if err := cc.Server.BanList.Add(ip, nil); err != nil { cc.Logger.Error("Failed to ban IP for banned nickname", "ip", ip, "err", err) @@ -132,35 +132,32 @@ func HandleTranAgreed(cc *hotline.ClientConn, t *hotline.Transaction) (res []hot return res } - cc.Icon = t.GetField(hotline.FieldUserIconID).Data + cc.SetIcon(t.GetField(hotline.FieldUserIconID).Data) - cc.Logger = cc.Logger.With("Name", string(cc.UserName)) + cc.Logger = cc.Logger.With("Name", string(cc.GetUserName())) cc.Logger.Info("Login successful") options := t.GetField(hotline.FieldOptions).Data optBitmap := big.NewInt(int64(binary.BigEndian.Uint16(options))) // Check refuse private PM option - - cc.FlagsMU.Lock() - defer cc.FlagsMU.Unlock() - cc.Flags.Set(hotline.UserFlagRefusePM, optBitmap.Bit(hotline.UserOptRefusePM)) + cc.SetFlag(hotline.UserFlagRefusePM, optBitmap.Bit(hotline.UserOptRefusePM)) // Check refuse private chat option - cc.Flags.Set(hotline.UserFlagRefusePChat, optBitmap.Bit(hotline.UserOptRefuseChat)) + cc.SetFlag(hotline.UserFlagRefusePChat, optBitmap.Bit(hotline.UserOptRefuseChat)) // Check auto response if optBitmap.Bit(hotline.UserOptAutoResponse) == 1 { - cc.AutoReply = t.GetField(hotline.FieldAutomaticResponse).Data + cc.SetAutoReply(t.GetField(hotline.FieldAutomaticResponse).Data) } trans := cc.NotifyOthers( hotline.NewTransaction( hotline.TranNotifyChangeUser, [2]byte{0, 0}, - hotline.NewField(hotline.FieldUserName, cc.UserName), + hotline.NewField(hotline.FieldUserName, cc.GetUserName()), hotline.NewField(hotline.FieldUserID, cc.ID[:]), - hotline.NewField(hotline.FieldUserIconID, cc.Icon), - hotline.NewField(hotline.FieldUserFlags, cc.Flags[:]), + hotline.NewField(hotline.FieldUserIconID, cc.GetIcon()), + hotline.NewField(hotline.FieldUserFlags, cc.FlagBytes()), ), ) res = append(res, trans...) @@ -210,7 +207,7 @@ func HandleDisconnectUser(cc *hotline.ClientConn, t *hotline.Transaction) (res [ switch options[1] { case 1: // send message: "You are temporarily banned on this server" - cc.Logger.Info("Disconnect & temporarily ban user", "username", string(clientConn.UserName)) + cc.Logger.Info("Disconnect & temporarily ban user", "username", string(clientConn.GetUserName())) res = append(res, hotline.NewTransaction( hotline.TranServerMsg, @@ -229,7 +226,7 @@ func HandleDisconnectUser(cc *hotline.ClientConn, t *hotline.Transaction) (res [ } case 2: // send message: "You are permanently banned on this server" - cc.Logger.Info("Disconnect & ban user", "username", string(clientConn.UserName)) + cc.Logger.Info("Disconnect & ban user", "username", string(clientConn.GetUserName())) res = append(res, hotline.NewTransaction( hotline.TranServerMsg, @@ -266,14 +263,14 @@ func HandleDisconnectUser(cc *hotline.ClientConn, t *hotline.Transaction) (res [ // Reply is not expected. func HandleSetClientUserInfo(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotline.Transaction) { if len(t.GetField(hotline.FieldUserIconID).Data) == 4 { - cc.Icon = t.GetField(hotline.FieldUserIconID).Data[2:] + cc.SetIcon(t.GetField(hotline.FieldUserIconID).Data[2:]) } else { - cc.Icon = t.GetField(hotline.FieldUserIconID).Data + cc.SetIcon(t.GetField(hotline.FieldUserIconID).Data) } if cc.Authorize(hotline.AccessAnyName) { - oldNickname := string(cc.UserName) + oldNickname := string(cc.GetUserName()) newNickname := string(t.GetField(hotline.FieldUserName).Data) - cc.UserName = t.GetField(hotline.FieldUserName).Data + cc.SetUserName(t.GetField(hotline.FieldUserName).Data) login := cc.Account.Login ip := cc.IP() @@ -312,14 +309,14 @@ func HandleSetClientUserInfo(cc *hotline.ClientConn, t *hotline.Transaction) (re if options != nil { optBitmap := big.NewInt(int64(binary.BigEndian.Uint16(options))) - cc.Flags.Set(hotline.UserFlagRefusePM, optBitmap.Bit(hotline.UserOptRefusePM)) - cc.Flags.Set(hotline.UserFlagRefusePChat, optBitmap.Bit(hotline.UserOptRefuseChat)) + cc.SetFlag(hotline.UserFlagRefusePM, optBitmap.Bit(hotline.UserOptRefusePM)) + cc.SetFlag(hotline.UserFlagRefusePChat, optBitmap.Bit(hotline.UserOptRefuseChat)) // Check auto response if optBitmap.Bit(hotline.UserOptAutoResponse) == 1 { - cc.AutoReply = t.GetField(hotline.FieldAutomaticResponse).Data + cc.SetAutoReply(t.GetField(hotline.FieldAutomaticResponse).Data) } else { - cc.AutoReply = []byte{} + cc.SetAutoReply([]byte{}) } } @@ -328,9 +325,9 @@ func HandleSetClientUserInfo(cc *hotline.ClientConn, t *hotline.Transaction) (re hotline.TranNotifyChangeUser, c.ID, hotline.NewField(hotline.FieldUserID, cc.ID[:]), - hotline.NewField(hotline.FieldUserIconID, cc.Icon), - hotline.NewField(hotline.FieldUserFlags, cc.Flags[:]), - hotline.NewField(hotline.FieldUserName, cc.UserName), + hotline.NewField(hotline.FieldUserIconID, cc.GetIcon()), + hotline.NewField(hotline.FieldUserFlags, cc.FlagBytes()), + hotline.NewField(hotline.FieldUserName, cc.GetUserName()), )) } diff --git a/internal/mobius/handlers_transfers.go b/internal/mobius/handlers_transfers.go index dfb2389..315135e 100644 --- a/internal/mobius/handlers_transfers.go +++ b/internal/mobius/handlers_transfers.go @@ -277,7 +277,7 @@ func HandleUploadFile(cc *hotline.ClientConn, t *hotline.Transaction) (res []hot // - 108 Transfer size Size of data to be downloaded func HandleDownloadBanner(cc *hotline.ClientConn, t *hotline.Transaction) (res []hotline.Transaction) { ft := cc.NewFileTransfer(hotline.BannerDownload, "", []byte{}, []byte{}, make([]byte, 4)) - binary.BigEndian.PutUint32(ft.TransferSize, uint32(len(cc.Server.Banner))) + binary.BigEndian.PutUint32(ft.TransferSize, uint32(len(cc.Server.Banner()))) return append(res, cc.NewReply(t, hotline.NewField(hotline.FieldRefNum, ft.RefNum[:]), diff --git a/internal/mobius/handlers_transfers_test.go b/internal/mobius/handlers_transfers_test.go index 90eea2d..28c2768 100644 --- a/internal/mobius/handlers_transfers_test.go +++ b/internal/mobius/handlers_transfers_test.go @@ -318,12 +318,14 @@ func TestHandleDownloadFile(t *testing.T) { func TestHandleDownloadBanner(t *testing.T) { t.Run("returns banner transfer info", func(t *testing.T) { + srv := &hotline.Server{ + FileTransferMgr: hotline.NewMemFileTransferMgr(), + } + srv.SetBanner([]byte("test-banner-data")) + cc := &hotline.ClientConn{ ClientFileTransferMgr: hotline.NewClientFileTransferMgr(), - Server: &hotline.Server{ - Banner: []byte("test-banner-data"), - FileTransferMgr: hotline.NewMemFileTransferMgr(), - }, + Server: srv, } tran := hotline.NewTransaction(hotline.TranDownloadBanner, [2]byte{0, 1}) -- cgit