From 8f5e4055326ddb7edd84a8245000b0aa6886926a Mon Sep 17 00:00:00 2001 From: Sisi Date: Sat, 4 Apr 2026 19:56:20 +0200 Subject: [PATCH] fix connection handlers, group ops, and various HTTP handler bugs --- buglist.md | 35 ++++++++++ database.go | 27 ++++++- http.go | 198 ++++++++++++++++++++++++++++++++++++++++++++++++++-- main.go | 2 +- 4 files changed, 254 insertions(+), 8 deletions(-) create mode 100644 buglist.md diff --git a/buglist.md b/buglist.md new file mode 100644 index 0000000..3555029 --- /dev/null +++ b/buglist.md @@ -0,0 +1,35 @@ +# Bug List + +## Open + +### Bug 1 — `WsSendToUser`: sends message to sender, not recipient +**File:** `wsServer.go:86` +`to *User` parameter is never used — message is sent back to `from` instead. +```go +sendMessageCloseIfTimeout(from, &msg) // should be `to` +``` + +### Bug 2 — `ServeWsConnection`: `ignoreCache` captured by value in defer +**File:** `wsServer.go:30` +`ignoreCache` is `false` at defer registration; setting it to `true` later has no effect. +The user is always evicted from cache on disconnect. +```go +defer closeConnection(&user, ignoreCache) // wrong +defer func() { closeConnection(&user, ignoreCache) }() // correct +``` + +### Bug 3 — Deadlock in `sendToAllMessageCloseIfTimeout` +**File:** `wsServer.go:72`, `cache.go:52` +`sendToAllMessageCloseIfTimeout` holds `mu.RLock()`, calls `sendMessageCloseIfTimeout`, +which on timeout calls `closeConnection` → `CacheDeleteUser` → `mu.Lock()`. +Upgrading RLock to write Lock deadlocks. + +### Bug 4 — `closeConnection`: nil `WsConn` not guarded +**File:** `wsServer.go:179` +`user.WsConn.CloseNow()` panics if `WsConn` is nil (e.g. unauthenticated user). +`sendMessageCloseIfTimeout` guards against this but `closeConnection` does not. + +### Bug 5 — `HttpHandleUserAcceptConnection`: potential nil dereference +**File:** `http.go:410` +`target.Connections[user.Id].IsAccepted = true` panics if the entry is missing +(DB inconsistency). The sender side is guarded but the recipient side is not. diff --git a/database.go b/database.go index b5b6cd0..d9008b8 100644 --- a/database.go +++ b/database.go @@ -38,9 +38,13 @@ func DbInit(ctx context.Context) { requestor_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, recipient_id INTEGER NOT NULL REFERENCES users(id) ON DELETE CASCADE, is_accepted BOOLEAN NOT NULL DEFAULT FALSE, - created_at TIMESTAMP NOT NULL DEFAULT NOW() + created_at TIMESTAMP NOT NULL DEFAULT NOW(), + PRIMARY KEY (requestor_id, recipient_id) ) `) + if err != nil { + panic(err) + } _, err = dbConn.Exec(ctx, ` CREATE TABLE IF NOT EXISTS chat_groups ( @@ -184,6 +188,27 @@ func DbGroupSetColor(ctx context.Context, group *Group) error { return err } +func DbConnectionSave(ctx context.Context, creationDate time.Time, requestorId uint32, recipientId uint32, isAccepted bool) error { + _, err := dbConn.Exec(ctx, ` + INSERT INTO user_connections (created_at, requestor_id, recipient_id, is_accepted) VALUES ($1, $2, $3, $4) + `, creationDate, requestorId, recipientId, isAccepted) + return err +} + +func DbConnectionAccept(ctx context.Context, requestorId uint32, recipientId uint32) error { + _, err := dbConn.Exec(ctx, ` + UPDATE user_connections SET is_accepted = true WHERE requestor_id = $1 AND recipient_id = $2 + `, requestorId, recipientId) + return err +} + +func DbConnectionDelete(ctx context.Context, requestorId uint32, recipientId uint32) error { + _, err := dbConn.Exec(ctx, ` + DELETE FROM user_connections WHERE requestor_id = $1 AND recipient_id = $2 + `, requestorId, recipientId) + return err +} + func DbGroupSave(ctx context.Context, group *Group) error { err := dbConn.QueryRow(ctx, ` INSERT INTO chat_groups (name, creator_id, owner_id, enable_client_colors, color_red, color_green, color_blue, created_at) diff --git a/http.go b/http.go index 0dacf69..2a60684 100644 --- a/http.go +++ b/http.go @@ -3,6 +3,7 @@ package main import ( "context" json2 "encoding/json" + "errors" "fmt" "maps" "net/http" @@ -89,7 +90,7 @@ func getIfOwnerUserAndGroup(ctx context.Context, response *http.ResponseWriter, if !isOwner(user, group) { http.Error(*response, "no such group", http.StatusUnauthorized) - return nil, nil, err + return nil, nil, errors.New("not an owner") } return user, group, nil } @@ -153,6 +154,7 @@ func HttpHandleUserDelete(response http.ResponseWriter, request *http.Request) { err = DbUserDelete(ctx, userId) if err != nil { http.Error(response, "internal server error", http.StatusInternalServerError) + return } CacheDeleteUser(userId) @@ -192,7 +194,7 @@ func HttpHandleUserModifyAbout(response http.ResponseWriter, request *http.Reque } pronouns := request.FormValue("pronouns") - if len(pronouns) > 25 && len(pronouns) < 2 { + if len(pronouns) > 25 || len(pronouns) < 2 { http.Error(response, "invalid pronouns", http.StatusBadRequest) return } @@ -226,10 +228,16 @@ func HttpHandleUserMessage(response http.ResponseWriter, request *http.Request) err = DbUserGetById(ctx, target) if err != nil { http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + err = DbUserGetConnections(ctx, target) + if err != nil { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return } } - if user.Connections[target.Id] == nil { + if user.Connections[target.Id] == nil || !user.Connections[targetId].IsAccepted { http.Error(response, "invalid recipient id", http.StatusBadRequest) return } @@ -243,7 +251,176 @@ func HttpHandleUserMessage(response http.ResponseWriter, request *http.Request) WsSendToUser(user, target, message) } -func HttpHandleNewToken(response http.ResponseWriter, request *http.Request) { +func HttpHandleUserNewConnection(response http.ResponseWriter, request *http.Request) { + if !isMethodAllowed(&response, request) { + return + } + + ctx := request.Context() + + user, err := getUser(ctx, request.FormValue("token")) + if err != nil { + http.Error(response, "invalid token", http.StatusUnauthorized) + return + } + + targetId, err := ConvertStringUint32(request.FormValue("recipientid")) + if err != nil { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + + if user.Id == targetId { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + + target, err := CacheGetUserById(targetId) + if err != nil { + target = &User{Id: targetId} + err = DbUserGetById(ctx, target) + if err != nil { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + err = DbUserGetConnections(ctx, target) + if err != nil { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + } + if user.Connections[target.Id] != nil { + http.Error(response, "already sent/connected", http.StatusConflict) + return + } + + timeNow := time.Now() + + err = DbConnectionSave(ctx, timeNow, user.Id, targetId, false) + if err != nil { + http.Error(response, "internal server error", http.StatusInternalServerError) + return + } + + user.Connections[target.Id] = &Connection{ + CreatedAt: timeNow, + With: targetId, + IsFromUser: true, + IsAccepted: false, + } + if target.Connections == nil { + target.Connections = make(map[uint32]*Connection) + } + target.Connections[user.Id] = &Connection{ + CreatedAt: timeNow, + With: user.Id, + IsFromUser: false, + IsAccepted: false, + } + + response.WriteHeader(http.StatusCreated) +} + +func HttpHandleUserDeleteConnection(response http.ResponseWriter, request *http.Request) { + ctx := request.Context() + + user, err := getUser(ctx, request.FormValue("token")) + if err != nil { + http.Error(response, "invalid token", http.StatusUnauthorized) + return + } + + targetId, err := ConvertStringUint32(request.FormValue("connectedid")) + if err != nil { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + + target, err := CacheGetUserById(targetId) + if err != nil { + target = &User{Id: targetId} + err = DbUserGetById(ctx, target) + if err != nil { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + err = DbUserGetConnections(ctx, target) + if err != nil { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + } + if user.Connections[targetId] == nil { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + + if user.Connections[targetId].IsFromUser { + err = DbConnectionDelete(ctx, user.Id, targetId) + } else { + err = DbConnectionDelete(ctx, targetId, user.Id) + } + if err != nil { + http.Error(response, "internal server error", http.StatusInternalServerError) + return + } + + delete(user.Connections, targetId) + delete(target.Connections, user.Id) + + response.WriteHeader(http.StatusAccepted) +} + +func HttpHandleUserAcceptConnection(response http.ResponseWriter, request *http.Request) { + ctx := request.Context() + + user, err := getUser(ctx, request.FormValue("token")) + if err != nil { + http.Error(response, "invalid token", http.StatusUnauthorized) + return + } + + targetId, err := ConvertStringUint32(request.FormValue("connectedid")) + if err != nil { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + + target, err := CacheGetUserById(targetId) + if err != nil { + target = &User{Id: targetId} + err = DbUserGetById(ctx, target) + if err != nil { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + err = DbUserGetConnections(ctx, target) + if err != nil { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + } + if user.Connections[targetId] == nil { + http.Error(response, "invalid recipient id", http.StatusBadRequest) + return + } + if user.Connections[targetId].IsFromUser { + http.Error(response, "cant accept own request", http.StatusConflict) + return + } + + user.Connections[targetId].IsAccepted = true + target.Connections[user.Id].IsAccepted = true + + err = DbConnectionAccept(ctx, targetId, user.Id) + if err != nil { + http.Error(response, "internal server error", http.StatusInternalServerError) + return + } + response.WriteHeader(http.StatusAccepted) +} + +func HttpHandleTokenNew(response http.ResponseWriter, request *http.Request) { if !isMethodAllowed(&response, request) { return } @@ -321,6 +498,10 @@ func HttpHandeGroupCreate(response http.ResponseWriter, request *http.Request) { colorString := request.FormValue("color") color, err := ConvertStringToRgb(colorString) + if err != nil { + http.Error(response, "invalid color", http.StatusBadRequest) + return + } group := Group{ Name: name, @@ -503,7 +684,7 @@ func HttpHandleGroupChangeOwner(response http.ResponseWriter, request *http.Requ } ctx := request.Context() - user, group, err := getIfOwnerUserAndGroup(ctx, &response, request) + _, group, err := getIfOwnerUserAndGroup(ctx, &response, request) if err != nil { return } @@ -519,7 +700,7 @@ func HttpHandleGroupChangeOwner(response http.ResponseWriter, request *http.Requ return } - CacheSaveUser(user) + CacheSaveUser(newOwner) } _, ok := group.Users[newOwner.Id] @@ -558,6 +739,10 @@ func HttpHandleGroupMessage(response http.ResponseWriter, request *http.Request) } group, err := getGroup(ctx, groupId) + if err != nil { + http.Error(response, "no such group", http.StatusUnauthorized) + return + } content := request.FormValue("content") if content == "" { @@ -568,6 +753,7 @@ func HttpHandleGroupMessage(response http.ResponseWriter, request *http.Request) _, ok := group.Users[user.Id] if !ok { http.Error(response, "no such group", http.StatusUnauthorized) + return } err = WsSendToGroup(group, user, content) diff --git a/main.go b/main.go index a511fdf..486ed83 100644 --- a/main.go +++ b/main.go @@ -18,7 +18,7 @@ func main() { DbInit(ctx) http.HandleFunc("/new/user", withCORS(HttpHandleUserNew)) - http.HandleFunc("/new/token", withCORS(HttpHandleNewToken)) + http.HandleFunc("/new/token", withCORS(HttpHandleTokenNew)) http.HandleFunc("/new/group", withCORS(HttpHandeGroupCreate)) http.HandleFunc("/mod/user/appearence", withCORS(HttpHandleUserModifyAppearance))