fix: address review feedback for cleanup program

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.
This commit is contained in:
Reese Norris
2026-07-27 14:14:12 -04:00
parent 49229b740a
commit dd9f61a460
7 changed files with 72 additions and 11 deletions

1
.gitignore vendored
View File

@@ -32,3 +32,4 @@ apt.dat
apt.dat.zip
**/apt.dat
**/apt.dat.zip
cover.out

View File

@@ -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"`

View File

@@ -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
}

View File

@@ -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()
}

View File

@@ -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)

View File

@@ -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) {

View File

@@ -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().