From dd9f61a460aa3449fdba620297ba18e84a150441 Mon Sep 17 00:00:00 2001 From: Reese Norris Date: Mon, 27 Jul 2026 14:14:12 -0400 Subject: [PATCH] fix: address review feedback for cleanup program MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scrub residual classic dual-path godoc; assert PilotRating sticky on JWT and PPL-success auth paths (incl. invalid rating → 0); log coalesce outbound constructor errors before gnet.Close. --- .gitignore | 1 + internal/server/config.go | 2 +- internal/server/gnet_fsd.go | 5 +- internal/server/handler_admin.go | 2 +- internal/server/http_service.go | 2 +- internal/server/security_handlers_test.go | 62 ++++++++++++++++++++++- internal/session/session.go | 9 ++-- 7 files changed, 72 insertions(+), 11 deletions(-) diff --git a/.gitignore b/.gitignore index 68ca81a..6e78a6e 100644 --- a/.gitignore +++ b/.gitignore @@ -32,3 +32,4 @@ apt.dat apt.dat.zip **/apt.dat **/apt.dat.zip +cover.out diff --git a/internal/server/config.go b/internal/server/config.go index 7558c9b..45f717e 100644 --- a/internal/server/config.go +++ b/internal/server/config.go @@ -40,7 +40,7 @@ type Config struct { FsdMaxConnectionsPerIP int `env:"FSD_MAX_CONNECTIONS_PER_IP, default=50"` FsdMaxSessionsPerCID int `env:"FSD_MAX_SESSIONS_PER_CID, default=5"` - // Timeouts (0 = disabled). Applied on classic and gnet FSD planes. + // Timeouts (0 = disabled). Applied on the gnet FSD plane. FsdLoginTimeout time.Duration `env:"FSD_LOGIN_TIMEOUT, default=30s"` FsdIdleTimeout time.Duration `env:"FSD_IDLE_TIMEOUT, default=120s"` diff --git a/internal/server/gnet_fsd.go b/internal/server/gnet_fsd.go index cbd746c..62d8248 100644 --- a/internal/server/gnet_fsd.go +++ b/internal/server/gnet_fsd.go @@ -299,6 +299,7 @@ func (e *fsdEngine) finishLogin(c gnet.Conn, cc *fsdConnCtx, idPacket, addPacket ) if outErr != nil { // Defensive: writeAsync is non-nil above; should never fire in production. + e.srv.logger.Error("coalesce outbound", "err", outErr) return gnet.Close } client.SetOutbound(out) @@ -347,7 +348,7 @@ func (e *fsdEngine) attemptAuthGnet(c gnet.Conn, client *session.Session, token adapter := &gnetLoginConn{gc: c, remote: c.RemoteAddr(), local: c.LocalAddr()} client.Conn = adapter err := e.srv.attemptAuthentication(client, token) - // Clear classic Conn so post-login path cannot Conn.Write. + // Clear temporary login adapter so post-login path cannot Conn.Write. client.Conn = nil return err } @@ -422,7 +423,7 @@ func toGnetAddr(addr string) string { if strings.Contains(addr, "://") { return addr } - // Default FSD to IPv4 TCP (matches classic listenLoop "tcp4"). + // Default FSD to IPv4 TCP (historical tcp4 bind preference). if strings.HasPrefix(addr, ":") { return "tcp4://0.0.0.0" + addr } diff --git a/internal/server/handler_admin.go b/internal/server/handler_admin.go index 339fc1a..40cc5d0 100644 --- a/internal/server/handler_admin.go +++ b/internal/server/handler_admin.go @@ -22,6 +22,6 @@ func (s *Server) handleKillRequest(client *session.Session, packet []byte) { return } - // Closing the context + transport forces disconnect on classic and gnet. + // Closing the context + transport forces disconnect (gnet outbound / synthetic channel path). victim.Disconnect() } diff --git a/internal/server/http_service.go b/internal/server/http_service.go index fe2821f..6a03a38 100644 --- a/internal/server/http_service.go +++ b/internal/server/http_service.go @@ -198,7 +198,7 @@ func (s *Server) handleKickUser(c *gin.Context) { return } - // Disconnect cancels context and closes gnet/classic transports. + // Disconnect cancels context and closes gnet outbound / channel-path transports. client.Disconnect() c.AbortWithStatus(http.StatusNoContent) diff --git a/internal/server/security_handlers_test.go b/internal/server/security_handlers_test.go index 1cfa4bd..98b89a3 100644 --- a/internal/server/security_handlers_test.go +++ b/internal/server/security_handlers_test.go @@ -473,9 +473,12 @@ func TestAttemptAuth_SuccessPassword(t *testing.T) { } func TestAttemptAuth_JWT(t *testing.T) { - // JWT path loads the user for pilot_rating checks (REQUIRE_PILOT_PPL). + // JWT path loads the user for pilot_rating checks (REQUIRE_PILOT_PPL) and + // must stick PilotRating on LoginData for online_users / datafeed. srv := newAuthTestServer(t, &fakeUserStore{byCID: map[int]*db.User{ 42: {CID: 42, NetworkRating: int(protocol.NetworkRatingStudent1), PilotRating: int(protocol.PilotRatingNone)}, + 43: {CID: 43, NetworkRating: int(protocol.NetworkRatingStudent1), PilotRating: int(protocol.PilotRatingCMEL)}, + 44: {CID: 44, NetworkRating: int(protocol.NetworkRatingStudent1), PilotRating: 2}, // invalid wire ID }}, nil) tok, err := auth.MakeJwtToken(&auth.CustomFields{ TokenType: "fsd", @@ -498,6 +501,58 @@ func TestAttemptAuth_JWT(t *testing.T) { if err := srv.attemptAuthentication(client, signed); err != nil { t.Fatalf("jwt auth: %v", err) } + if client.PilotRating != int(protocol.PilotRatingNone) { + t.Fatalf("JWT PilotRating=%d want P0(%d)", client.PilotRating, protocol.PilotRatingNone) + } + + // Non-zero certificate rating must stick on the JWT branch. + tokCMEL, err := auth.MakeJwtToken(&auth.CustomFields{ + TokenType: "fsd", + CID: 43, + NetworkRating: protocol.NetworkRatingStudent1, + }, time.Minute) + if err != nil { + t.Fatal(err) + } + signedCMEL, err := tokCMEL.SignedString([]byte(TestJWTSecret)) + if err != nil { + t.Fatal(err) + } + clientCMEL := session.New(context.Background(), &discardConn{}, nil, session.LoginData{ + Callsign: "N43", CID: 43, NetworkRating: protocol.NetworkRatingObserver, + }) + clientCMEL.Auth = &auth.AuthState{} + if err := srv.attemptAuthentication(clientCMEL, signedCMEL); err != nil { + t.Fatalf("jwt CMEL auth: %v", err) + } + if clientCMEL.PilotRating != int(protocol.PilotRatingCMEL) { + t.Fatalf("JWT PilotRating=%d want CMEL(%d)", clientCMEL.PilotRating, protocol.PilotRatingCMEL) + } + + // Invalid pilot_rating from DB → session 0. + tokBad, err := auth.MakeJwtToken(&auth.CustomFields{ + TokenType: "fsd", + CID: 44, + NetworkRating: protocol.NetworkRatingStudent1, + }, time.Minute) + if err != nil { + t.Fatal(err) + } + signedBad, err := tokBad.SignedString([]byte(TestJWTSecret)) + if err != nil { + t.Fatal(err) + } + clientBad := session.New(context.Background(), &discardConn{}, nil, session.LoginData{ + Callsign: "N44", CID: 44, NetworkRating: protocol.NetworkRatingObserver, + }) + clientBad.Auth = &auth.AuthState{} + if err := srv.attemptAuthentication(clientBad, signedBad); err != nil { + t.Fatalf("jwt invalid rating auth: %v", err) + } + if clientBad.PilotRating != 0 { + t.Fatalf("invalid PilotRating must store 0, got %d", clientBad.PilotRating) + } + // Wrong token type tok2, _ := auth.MakeJwtToken(&auth.CustomFields{ TokenType: "access", @@ -553,7 +608,7 @@ func TestAttemptAuth_RequirePilotPPL(t *testing.T) { t.Fatalf("ATC must not be gated by pilot PPL: %v", err) } - // Raise pilot rating to PPL → pilot OK. + // Raise pilot rating to PPL → pilot OK; rating must stick on session. f.byCID[9].PilotRating = int(protocol.PilotRatingPPL) pilot2 := session.New(context.Background(), &discardConn{}, nil, session.LoginData{ Callsign: "N9B", CID: 9, NetworkRating: protocol.NetworkRatingObserver, IsAtc: false, @@ -562,6 +617,9 @@ func TestAttemptAuth_RequirePilotPPL(t *testing.T) { if err := srv.attemptAuthentication(pilot2, "secret"); err != nil { t.Fatalf("PPL pilot should connect: %v", err) } + if pilot2.PilotRating != int(protocol.PilotRatingPPL) { + t.Fatalf("PPL success PilotRating=%d want %d", pilot2.PilotRating, protocol.PilotRatingPPL) + } } func TestAttemptAuth_RatingTooHigh(t *testing.T) { diff --git a/internal/session/session.go b/internal/session/session.go index 4c59295..f59b37e 100644 --- a/internal/session/session.go +++ b/internal/session/session.go @@ -55,7 +55,7 @@ type LatLon struct { // // # Field ownership // -// Writer: owning read loop only (eventLoop / packet handlers on this connection). +// Writer: owning read loop only (gnet dispatch / packet handlers on this connection). // Concurrent readers without extra sync are allowed for the fields below where // noted; they may observe a torn value — acceptable for snapshot/privilege checks: // - SendFastEnabled, ClosestVelocityClientDistance — read-loop only; not read @@ -88,8 +88,8 @@ type LatLon struct { // // After login, all packet writes to the client MUST go through Send / SendPosition. // Two sinks are supported: -// 1. Channel path: Send → sendChan → SenderWorker → Conn.Write (classic / synthetic). -// 2. Outbound path: Send → Outbound (coalesced AsyncWrite; no per-conn writer goroutine). +// 1. Channel path: Send → sendChan → SenderWorker → Conn.Write (synthetic / tests). +// 2. Outbound path: Send → Outbound (coalesced AsyncWrite; gnet; no per-conn writer). // // Direct Conn.Write outside SenderWorker is forbidden post-login on the channel path // (login-phase errors may still use protocol.WriteError on the raw connection @@ -100,7 +100,8 @@ type LatLon struct { // skip them as recipients; direct registry.Send still enqueues and relies on // SenderWorker drain (no network write when Conn is nil). type Session struct { - // Conn is the underlying network connection (classic net path). + // Conn is the underlying network connection when a net.Conn is used + // (channel-path synthetics/tests). Gnet sets Conn nil and uses Outbound. // Exported for RemoteAddr and login-phase protocol.WriteError only. // Post-login packet writes MUST use Send, not Conn.Write. // May be nil for synthetic / unit-test / gnet sessions; use RemoteIP().