From e6c9952ad4f1e969ba235fa2558e5bdb7f66d419 Mon Sep 17 00:00:00 2001 From: ginuerzh Date: Thu, 4 Jun 2026 15:09:38 +0800 Subject: [PATCH] test(handler/router): fix review findings - error matching, version test, cache assertion, entrypoint sync --- handler/router/associate_test.go | 27 +++++++++++------ handler/router/entrypoint_test.go | 16 ++++++----- handler/router/handler_test.go | 48 +++++++++++++++++++++++++++++-- 3 files changed, 73 insertions(+), 18 deletions(-) diff --git a/handler/router/associate_test.go b/handler/router/associate_test.go index 006841df..b3aaec4e 100644 --- a/handler/router/associate_test.go +++ b/handler/router/associate_test.go @@ -3,6 +3,7 @@ package router import ( "bytes" "context" + "io" "net" "testing" "time" @@ -27,9 +28,12 @@ func TestHandleAssociate_NoIngress(t *testing.T) { conn := &fakeConn{buf: reqData} err := h.handleAssociate(context.Background(), conn, "ip", "10.0.0.1", rid, &testLogger{}) - // The error comes from the packet read loop when the fake conn has no - // more data — it is not an associate failure. - if err != nil && err.Error() != "unexpected EOF" { + // handleAssociate returns io.EOF when the packet read loop exhausts the + // fake connection's data — that signals normal completion, not a failure. + // packetConn.Read uses io.ReadFull, which returns io.ErrUnexpectedEOF + // when the underlying connection has been exhausted mid-frame, rather + // than the plain io.EOF that the loop checks for. + if err != nil && err != io.EOF && err != io.ErrUnexpectedEOF { t.Fatalf("handleAssociate: %v", err) } @@ -58,9 +62,7 @@ func TestHandleAssociate_IngressMatch(t *testing.T) { reqData := buildRelayAssociateRequest(t, "10.0.0.1:0", rid, "ip") conn := &fakeConn{buf: reqData} err := h.handleAssociate(context.Background(), conn, "ip", "10.0.0.1", rid, &testLogger{}) - // The error comes from the packet read loop when the fake conn has no - // more data — it is not an associate failure. - if err != nil && err.Error() != "unexpected EOF" { + if err != nil && err != io.EOF && err != io.ErrUnexpectedEOF { t.Fatalf("handleAssociate: %v", err) } @@ -249,10 +251,19 @@ func TestGetRoute_CacheDisabled(t *testing.T) { h := newInitdHandler(t) h.md.routerCacheEnabled = false - // With no registry router and no fallback, should return nil. + // With cache disabled, getRoute always performs a lookup. + // Since cache is disabled, a cached value should NOT be returned even if one exists. + h.routeCache.Set("10.0.0.1", cache.NewItem(&router.Route{ + Dst: "10.0.0.0/24", + Gateway: "10.0.0.254", + }, time.Minute)) + route := h.getRoute(context.Background(), "test-rid", "10.0.0.1") + // With cache disabled, the cached route must NOT be returned. + // Since there is no registry router registered for "test-rid" and no + // fallback router, the result should be nil. if route != nil { - t.Log("route is not nil (may have registry router)") + t.Errorf("getRoute returned %+v, want nil (cache disabled, no fallback)", route) } } diff --git a/handler/router/entrypoint_test.go b/handler/router/entrypoint_test.go index c6ee0ac2..0c13d1f8 100644 --- a/handler/router/entrypoint_test.go +++ b/handler/router/entrypoint_test.go @@ -72,9 +72,10 @@ func TestHandleEntrypoint_NonAssociateCmd(t *testing.T) { h.epConn = fpc h.pool.Add(rid, c) - errCh := make(chan error, 1) + doneCh := make(chan struct{}) go func() { - errCh <- h.handleEntrypoint(&testLogger{}) + h.handleEntrypoint(&testLogger{}) + close(doneCh) }() // Send request with non-Associate cmd (CmdConnect) @@ -93,7 +94,7 @@ func TestHandleEntrypoint_NonAssociateCmd(t *testing.T) { // Close to stop the loop, then wait for goroutine to exit. fpc.Close() - <-errCh + <-doneCh if connBuf.Len() > 0 { t.Error("data was forwarded to connector despite non-Associate cmd") @@ -110,9 +111,10 @@ func TestHandleEntrypoint_NoMatchingConnector(t *testing.T) { h.epConn = fpc // No connector added to pool - errCh := make(chan error, 1) + doneCh := make(chan struct{}) go func() { - errCh <- h.handleEntrypoint(&testLogger{}) + h.handleEntrypoint(&testLogger{}) + close(doneCh) }() req := relay.Request{ @@ -132,9 +134,9 @@ func TestHandleEntrypoint_NoMatchingConnector(t *testing.T) { fpc.dataCh <- buf.Bytes() fpc.addrCh <- laddr - // Should not panic or error, just silently drop. + // Signal the loop to stop by sending EOF via the pipe concurrency pattern. fpc.Close() - <-errCh + <-doneCh } func TestHandleEntrypoint_ReadError(t *testing.T) { diff --git a/handler/router/handler_test.go b/handler/router/handler_test.go index e6a26225..0c74e5ad 100644 --- a/handler/router/handler_test.go +++ b/handler/router/handler_test.go @@ -126,8 +126,11 @@ func TestInit_WithLimiter(t *testing.T) { func TestHandle_BadVersion(t *testing.T) { h := newInitdHandler(t) + // Version2 causes relay.Request.ReadFrom to return an error because the + // relay protocol only supports Version1. The Handle method returns the + // read error directly — it never reaches the version check. req := relay.Request{ - Version: 0x02, // not Version1 — triggers error in ReadFrom + Version: 0x02, Cmd: relay.CmdAssociate, } var buf bytes.Buffer @@ -139,8 +142,47 @@ func TestHandle_BadVersion(t *testing.T) { t.Fatal("expected error for bad version") } - // The response is not written when ReadFrom fails, so we only verify - // that the connection was closed (defer conn.Close in Handle). + // Connection must be closed via defer. + if !conn.closed { + t.Error("connection was not closed") + } +} + +// TestHandle_BadRequestVersion verifies that Handle rejects requests where +// the version byte has been tampered with. relay.Request.ReadFrom checks the +// version byte before the handler does — the handler's own version check +// (req.Version != relay.Version1) is a defensive secondary guard. Since +// ReadFrom catches bad versions first, Handle returns relay.ErrBadVersion +// from ReadFrom without writing a response. +func TestHandle_BadRequestVersion(t *testing.T) { + h := newInitdHandler(t) + + // Build a valid relay request with Version1, then override the version byte. + rid := relay.NewTunnelID([]byte("0123456789abcdef")) + req := relay.Request{ + Version: relay.Version1, + Cmd: relay.CmdAssociate, + Features: []relay.Feature{ + &relay.TunnelFeature{ID: rid}, + &relay.NetworkFeature{Network: relay.NetworkIP}, + }, + } + var buf bytes.Buffer + req.WriteTo(&buf) + + // Override the version byte to something that is not Version1. + b := buf.Bytes() + if len(b) > 0 { + b[0] = 0x02 + } + + conn := &fakeConn{buf: b} + err := h.Handle(context.Background(), conn) + if err == nil { + t.Fatal("expected error for bad request version") + } + + // Connection must be closed via defer. if !conn.closed { t.Error("connection was not closed") }