From f4d9c1c0d0be5e4af0a376dca0f2e48e08a989f4 Mon Sep 17 00:00:00 2001 From: Maxim Egorov Date: Fri, 21 Aug 2026 14:04:20 +0200 Subject: [PATCH] [client] Keep the route selection when every requested ID is unavailable A non-append SelectRoutes() wipes the current selection before applying the requested one, but it validated the requested IDs only afterwards, while already mutating. A request naming no available route at all left every route deselected and returned an error - so a typo in a route ID silently dropped the user's exit node, and the routes stayed applied while the selector claimed nothing was selected. Validate first and bail out before touching any state when nothing in the request is available. A request with at least one available route keeps applying the valid part and reporting the rest, and an empty request still deselects everything, since that is the caller asking for exactly that rather than a failed lookup. Co-Authored-By: Claude Opus 5 (1M context) --- .../internal/routeselector/routeselector.go | 28 ++++++++++---- .../routeselector/routeselector_test.go | 37 +++++++++++++++++++ 2 files changed, 57 insertions(+), 8 deletions(-) diff --git a/client/internal/routeselector/routeselector.go b/client/internal/routeselector/routeselector.go index 1254b384d..bfcf71552 100644 --- a/client/internal/routeselector/routeselector.go +++ b/client/internal/routeselector/routeselector.go @@ -32,6 +32,23 @@ func (rs *RouteSelector) SelectRoutes(routes []route.NetID, appendRoute bool, al rs.mu.Lock() defer rs.mu.Unlock() + // Validate before mutating: a non-append selection wipes the current selection first, so + // a request naming only unavailable routes would deselect everything the user had and put + // nothing in its place. Requesting no routes at all is a different thing - that's the + // caller asking to deselect all - and still goes through. + var err *multierror.Error + available := make([]route.NetID, 0, len(routes)) + for _, r := range routes { + if !slices.Contains(allRoutes, r) { + err = multierror.Append(err, fmt.Errorf("route '%s' is not available", r)) + continue + } + available = append(available, r) + } + if len(available) == 0 && err != nil { + return errors.FormatErrorOrNil(err) + } + if !appendRoute || rs.deselectAll { if rs.deselectedRoutes == nil { rs.deselectedRoutes = map[route.NetID]struct{}{} @@ -46,14 +63,9 @@ func (rs *RouteSelector) SelectRoutes(routes []route.NetID, appendRoute bool, al } } - var err *multierror.Error - for _, route := range routes { - if !slices.Contains(allRoutes, route) { - err = multierror.Append(err, fmt.Errorf("route '%s' is not available", route)) - continue - } - delete(rs.deselectedRoutes, route) - rs.selectedRoutes[route] = struct{}{} + for _, r := range available { + delete(rs.deselectedRoutes, r) + rs.selectedRoutes[r] = struct{}{} } rs.deselectAll = false diff --git a/client/internal/routeselector/routeselector_test.go b/client/internal/routeselector/routeselector_test.go index 2b1ba3fb9..3181d90d0 100644 --- a/client/internal/routeselector/routeselector_test.go +++ b/client/internal/routeselector/routeselector_test.go @@ -887,3 +887,40 @@ func TestRouteSelector_EnableExitNodeKeepsOtherRoutes(t *testing.T) { assert.True(t, rs.IsSelected("lan1"), "non-exit route must stay selected") assert.True(t, rs.IsSelected("lan2"), "non-exit route must stay selected") } + +// TestRouteSelector_SelectRoutes_AllUnavailableKeepsSelection covers the destructive case: a +// non-append selection clears the current selection before applying the requested one, so a +// request naming only unavailable routes used to leave everything deselected while still +// returning an error - a typo in a route ID silently dropped the user's exit node. A request +// with at least one available route keeps applying the valid part (see "Select non-existing +// route" above); this is only about the all-invalid case. +func TestRouteSelector_SelectRoutes_AllUnavailableKeepsSelection(t *testing.T) { + allRoutes := []route.NetID{"route1", "route2", "route3"} + + rs := routeselector.NewRouteSelector() + require.NoError(t, rs.SelectRoutes([]route.NetID{"route1"}, false, allRoutes)) + + err := rs.SelectRoutes([]route.NetID{"Route1", "route4"}, false, allRoutes) + + assert.Error(t, err, "an unavailable route ID must still be reported") + assert.True(t, rs.IsSelected("route1"), "the previous selection must survive a fully invalid request") + for _, id := range []route.NetID{"route2", "route3"} { + assert.False(t, rs.IsSelected(id), "no other route may become selected") + } +} + +// TestRouteSelector_SelectRoutes_EmptyRequestStillDeselectsAll guards the boundary of the check +// above: asking for no routes is the caller deselecting everything, not a failed request, so it +// must keep working. +func TestRouteSelector_SelectRoutes_EmptyRequestStillDeselectsAll(t *testing.T) { + allRoutes := []route.NetID{"route1", "route2", "route3"} + + rs := routeselector.NewRouteSelector() + require.NoError(t, rs.SelectRoutes([]route.NetID{"route1"}, false, allRoutes)) + + require.NoError(t, rs.SelectRoutes(nil, false, allRoutes)) + + for _, id := range allRoutes { + assert.False(t, rs.IsSelected(id), "an empty selection request must deselect everything") + } +}