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") + } +}