Merge branch 'main' into embedded-vnc

client/internal/auth/auth.go

@@ -369,6 +369,7 @@ func (a *Auth) setSystemInfoFlags(info *system.Info) {
 		a.config.EnableSSHLocalPortForwarding,
 		a.config.EnableSSHRemotePortForwarding,
 		a.config.DisableSSHAuth,
+		a.config.RemoteJobsAllowed,
 	)
 }
 

client/internal/auth/device_flow.go

@@ -304,6 +304,16 @@ func (d *DeviceAuthorizationFlow) WaitToken(ctx context.Context, info AuthFlowIn
 				return TokenInfo{}, fmt.Errorf("validate access token failed with error: %v", err)
 			}
 
+			// Same as the PKCE flow: the account the token belongs to is what
+			// callers store to send back as the login_hint. Without it a client
+			// driven through the device flow — Android TV and tvOS — never binds
+			// an account to its profile and every later login goes out blind.
+			if email, err := parseEmailFromIDToken(tokenInfo.IDToken); err != nil {
+				log.Warnf("failed to parse email from ID token: %v", err)
+			} else {
+				tokenInfo.Email = email
+			}
+
 			log.Infof("device flow: user authorization confirmed after %d polls in %s", polls, time.Since(start).Round(time.Second))
 			return tokenInfo, err
 		}

client/internal/connect.go

@@ -242,7 +242,7 @@ func (c *ConnectClient) run(mobileDependency MobileDependency, runningChan chan
 	wrapErr := state.Wrap
 	myPrivateKey, err := wgtypes.ParseKey(c.config.PrivateKey)
 	if err != nil {
-		log.Errorf("failed parsing Wireguard key %s: [%s]", c.config.PrivateKey, err.Error())
+		log.Errorf("failed parsing Wireguard key: %s", err)
 		return wrapErr(err)
 	}
 
@@ -652,6 +652,7 @@ func createEngineConfig(key wgtypes.Key, config *profilemanager.Config, peerConf
 		RosenpassEnabled:              config.RosenpassEnabled,
 		RosenpassPermissive:           config.RosenpassPermissive,
 		ServerSSHAllowed:              util.ReturnBoolWithDefaultTrue(config.ServerSSHAllowed),
+		RemoteJobsAllowed:             util.ReturnBoolWithDefaultFalse(config.RemoteJobsAllowed),
 		ServerVNCAllowed:              config.ServerVNCAllowed != nil && *config.ServerVNCAllowed,
 		DisableVNCApproval:            config.DisableVNCApproval,
 		EnableSSHRoot:                 config.EnableSSHRoot,
@@ -752,6 +753,7 @@ func loginToManagement(ctx context.Context, client mgm.Client, pubSSHKey []byte,
 		config.EnableSSHLocalPortForwarding,
 		config.EnableSSHRemotePortForwarding,
 		config.DisableSSHAuth,
+		config.RemoteJobsAllowed,
 	)
 	return client.Login(sysInfo, pubSSHKey, config.DNSLabels)
 }

client/internal/debug/debug.go

@@ -711,6 +711,9 @@ func (g *BundleGenerator) addCommonConfigFields(configContent *strings.Builder)
 	if g.internalConfig.ServerSSHAllowed != nil {
 		configContent.WriteString(fmt.Sprintf("ServerSSHAllowed: %v\n", *g.internalConfig.ServerSSHAllowed))
 	}
+	if g.internalConfig.RemoteJobsAllowed != nil {
+		configContent.WriteString(fmt.Sprintf("RemoteJobsAllowed: %v\n", *g.internalConfig.RemoteJobsAllowed))
+	}
 	if g.internalConfig.EnableSSHRoot != nil {
 		configContent.WriteString(fmt.Sprintf("EnableSSHRoot: %v\n", *g.internalConfig.EnableSSHRoot))
 	}

client/internal/debug/debug_test.go

@@ -839,12 +839,13 @@ COMMIT`
 // the excluded set with a justification.
 func TestAddConfig_AllFieldsCovered(t *testing.T) {
 	excluded := map[string]string{
-		"PrivateKey":        "sensitive: WireGuard private key",
-		"PreSharedKey":      "sensitive: WireGuard pre-shared key",
-		"SSHKey":            "sensitive: SSH private key",
-		"ClientCertKeyPair": "non-config: parsed cert pair, not serialized",
-		"Name":              "non-config: profile name is not needed for debug purposes",
-		"policy":            "non-config: in-memory MDM policy snapshot, surfaced via Config.Policy() / GetConfigResponse.MDMManagedFields",
+		"PrivateKey":           "sensitive: WireGuard private key",
+		"PreSharedKey":         "sensitive: WireGuard pre-shared key",
+		"SSHKey":               "sensitive: SSH private key",
+		"ClientCertKeyPair":    "non-config: parsed cert pair, not serialized",
+		"Name":                 "non-config: profile name is not needed for debug purposes",
+		"policy":               "non-config: in-memory MDM policy snapshot, surfaced via Config.Policy() / GetConfigResponse.MDMManagedFields",
+		"DebugBundleUploadURL": "sensitive: MDM-provided upload URL may carry credentials or query tokens; kept out of the shared bundle",
 	}
 
 	mURL, _ := url.Parse("https://api.example.com:443")
@@ -864,6 +865,7 @@ func TestAddConfig_AllFieldsCovered(t *testing.T) {
 		RosenpassEnabled:              true,
 		RosenpassPermissive:           true,
 		ServerSSHAllowed:              &bTrue,
+		RemoteJobsAllowed:             &bTrue,
 		ServerVNCAllowed:              &bTrue,
 		DisableVNCApproval:            &bTrue,
 		EnableSSHRoot:                 &bTrue,
@@ -888,6 +890,7 @@ func TestAddConfig_AllFieldsCovered(t *testing.T) {
 		ClientCertPath:                "/tmp/cert",
 		ClientCertKeyPath:             "/tmp/key",
 		LazyConnection:                "on",
+		DebugBundleUploadURL:          "https://upload.example.test/bundle?token=secret",
 		MTU:                           1280,
 		DisableIPv6:                   true,
 		SyncMessageVersion:            func(v int) *int { return &v }(1),
@@ -905,6 +908,13 @@ func TestAddConfig_AllFieldsCovered(t *testing.T) {
 			g.addCommonConfigFields(&sb)
 			rendered := sb.String() + renderAddConfigSpecific(g)
 
+			// DebugBundleUploadURL is an MDM-provided value that can carry
+			// credentials or signed query tokens. It is deliberately excluded
+			// above; assert it never reaches the rendered bundle — neither the
+			// field name nor the token — in either anonymize mode.
+			assert.NotContains(t, rendered, "DebugBundleUploadURL:", "MDM upload URL field must not be serialized into the debug bundle")
+			assert.NotContains(t, rendered, "token=secret", "MDM upload URL value must not leak into the debug bundle")
+
 			val := reflect.ValueOf(cfg).Elem()
 			typ := val.Type()
 			var missing []string

client/internal/engine.go

@@ -138,6 +138,7 @@ type EngineConfig struct {
 	RosenpassPermissive bool
 
 	ServerSSHAllowed              bool
+	RemoteJobsAllowed             bool
 	ServerVNCAllowed              bool
 	DisableVNCApproval            *bool
 	EnableSSHRoot                 *bool
@@ -1270,6 +1271,7 @@ func (e *Engine) applyInfoFlags(info *system.Info) {
 		e.config.EnableSSHLocalPortForwarding,
 		e.config.EnableSSHRemotePortForwarding,
 		e.config.DisableSSHAuth,
+		&e.config.RemoteJobsAllowed,
 	)
 }
 
@@ -1359,6 +1361,13 @@ func (e *Engine) receiveJobEvents() {
 				ID:     msg.ID,
 				Status: mgmProto.JobStatus_failed,
 			}
+			// Remote jobs are an explicit opt-in. When not enabled on this
+			// peer, every job is refused before any work is done.
+			if !e.config.RemoteJobsAllowed {
+				log.Warnf("refusing remote job: remote jobs are not enabled on this peer (enable with --allow-remote-jobs)")
+				resp.Reason = []byte("remote jobs are not enabled on this peer")
+				return &resp
+			}
 			switch params := msg.WorkloadParameters.(type) {
 			case *mgmProto.JobRequest_Bundle:
 				bundleResult, err := e.handleBundle(params.Bundle)
@@ -1388,7 +1397,25 @@ func (e *Engine) receiveJobEvents() {
 }
 
 func (e *Engine) handleBundle(params *mgmProto.BundleParameters) (*mgmProto.JobResponse_Bundle, error) {
-	log.Infof("handle remote debug bundle request: %s", params.String())
+	// The upload URL can carry a host, credentials, or query tokens, so it is
+	// kept out of the info-level line; the full parameters stay available at
+	// debug level for troubleshooting.
+	log.Infof("handle remote debug bundle request: anonymize=%v anonymize_level=%q log_file_count=%d bundle_for=%v bundle_for_time=%d",
+		params.GetAnonymize(), params.GetAnonymizeLevel(), params.GetLogFileCount(), params.GetBundleFor(), params.GetBundleForTime())
+	log.Debugf("remote debug bundle request parameters: %s", params.String())
+
+	// Resolve the upload destination: an MDM override, when set, takes
+	// precedence over the management-supplied URL. Both are validated the same
+	// way; an empty result falls back to the default upload server downstream.
+	uploadURL := params.GetUploadUrl()
+	if override := e.config.ProfileConfig.DebugBundleUploadURL; override != "" {
+		log.Infof("using MDM debug bundle upload URL override instead of the management-supplied value")
+		uploadURL = override
+	}
+	if err := validateBundleUploadURL(uploadURL); err != nil {
+		return nil, err
+	}
+
 	syncResponse, err := e.GetLatestSyncResponse()
 	if err != nil {
 		log.Warnf("get latest sync response: %v", err)
@@ -1416,7 +1443,7 @@ func (e *Engine) handleBundle(params *mgmProto.BundleParameters) (*mgmProto.JobR
 
 	waitFor := time.Duration(params.BundleForTime) * time.Minute
 
-	uploadKey, err := e.jobExecutor.BundleJob(e.ctx, bundleDeps, bundleJobParams, waitFor, e.config.ProfileConfig.ManagementURL.String())
+	uploadKey, err := e.jobExecutor.BundleJob(e.ctx, bundleDeps, bundleJobParams, waitFor, e.config.ProfileConfig.ManagementURL.String(), uploadURL)
 	if err != nil {
 		return nil, err
 	}
@@ -1429,6 +1456,16 @@ func (e *Engine) handleBundle(params *mgmProto.BundleParameters) (*mgmProto.JobR
 	return response, nil
 }
 
+// validateBundleUploadURL sanity-checks a management-supplied upload URL for a
+// remote debug bundle job. It delegates to profilemanager.ValidateBundleUploadURL
+// so the executor and the MDM policy override share one definition of the rule
+// (empty accepted; otherwise a well-formed https URL with a host) and cannot
+// drift. The host is deliberately left unconstrained pending a decision on
+// management-directed uploads.
+func validateBundleUploadURL(raw string) error {
+	return profilemanager.ValidateBundleUploadURL(raw)
+}
+
 // receiveManagementEvents connects to the Management Service event stream to receive updates from the management service
 // E.g. when a new peer has been registered and we are allowed to connect to it.
 func (e *Engine) receiveManagementEvents() {

client/internal/engine_bundle_test.go

@@ -0,0 +1,36 @@
+package internal
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// TestValidateBundleUploadURL covers the sanity check applied to a
+// management-supplied upload URL before a remote debug bundle is generated.
+func TestValidateBundleUploadURL(t *testing.T) {
+	for _, tc := range []struct {
+		name    string
+		raw     string
+		wantErr bool
+	}{
+		{name: "empty falls back to default", raw: ""},
+		{name: "https with host", raw: "https://upload.debug.netbird.io/upload"},
+		{name: "https self-hosted host", raw: "https://upload.example.com"},
+		{name: "plaintext rejected", raw: "http://upload.example.com", wantErr: true},
+		{name: "missing host rejected", raw: "https:///upload", wantErr: true},
+		{name: "port-only authority rejected", raw: "https://:443", wantErr: true},
+		{name: "non-url scheme rejected", raw: "ftp://upload.example.com", wantErr: true},
+		{name: "garbage rejected", raw: "://not a url", wantErr: true},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			err := validateBundleUploadURL(tc.raw)
+			if tc.wantErr {
+				require.Error(t, err, "an invalid upload URL must be rejected")
+				return
+			}
+			assert.NoError(t, err, "a valid or empty upload URL must be accepted")
+		})
+	}
+}

client/internal/ipcauth/identity.go

@@ -91,6 +91,19 @@ func (i Identity) IsPrivileged() bool {
 	return slices.Contains(i.Groups, sidAdministrators)
 }
 
+// SameUser reports whether two identities are the same local principal. Only
+// the account is compared: the group set and the elevation flag describe what a
+// token may do, not who it belongs to. A SID on either side decides the
+// comparison, so a Windows principal never matches a Unix one on the UID both
+// happen to leave at zero. The zero Identity carries uid 0, so callers must
+// establish that both identities are real before the answer means anything.
+func (i Identity) SameUser(other Identity) bool {
+	if i.SID != "" || other.SID != "" {
+		return i.SID == other.SID
+	}
+	return i.UID == other.UID
+}
+
 // String renders the identity for audit logs and denial messages.
 func (i Identity) String() string {
 	if i.IsWindows() {

client/internal/ipcauth/identity_sameuser_test.go

@@ -0,0 +1,66 @@
+package ipcauth
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestIdentitySameUser(t *testing.T) {
+	tests := []struct {
+		name string
+		a    Identity
+		b    Identity
+		want bool
+	}{
+		{
+			name: "same uid",
+			a:    Identity{UID: 1000, GID: 1000},
+			b:    Identity{UID: 1000, GID: 1000},
+			want: true,
+		},
+		{
+			name: "same uid, different gid and pid still the same user",
+			a:    Identity{UID: 1000, GID: 1000, PID: 11},
+			b:    Identity{UID: 1000, GID: 27, PID: 22},
+			want: true,
+		},
+		{
+			name: "different uid",
+			a:    Identity{UID: 1000},
+			b:    Identity{UID: 1001},
+			want: false,
+		},
+		{
+			name: "same sid",
+			a:    Identity{SID: "S-1-5-21-1-2-3-1001"},
+			b:    Identity{SID: "S-1-5-21-1-2-3-1001"},
+			want: true,
+		},
+		{
+			name: "same sid, elevation and groups differ",
+			a:    Identity{SID: "S-1-5-21-1-2-3-1001", Elevated: true, Groups: []string{sidAdministrators}},
+			b:    Identity{SID: "S-1-5-21-1-2-3-1001"},
+			want: true,
+		},
+		{
+			name: "different sid",
+			a:    Identity{SID: "S-1-5-21-1-2-3-1001"},
+			b:    Identity{SID: "S-1-5-21-1-2-3-1002"},
+			want: false,
+		},
+		{
+			name: "a windows principal is never a unix one",
+			a:    Identity{SID: "S-1-5-18"},
+			b:    Identity{UID: 0},
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, tt.a.SameUser(tt.b))
+			assert.Equal(t, tt.want, tt.b.SameUser(tt.a), "SameUser must be symmetric")
+		})
+	}
+}

client/internal/metrics/infra/README.md

@@ -32,13 +32,24 @@ Clients do not talk to InfluxDB directly. An ingest server sits between clients
 ```text
 Client ──POST──▶ Ingest Server (:8087) ──▶ InfluxDB (internal)
                   │
+                  ├─ Checks the X-Peer-ID header format
                   ├─ Validates line protocol
                   ├─ Allowlists measurements, fields, and tags
                   ├─ Rejects out-of-bound values
                   └─ Serves remote config at /config
 ```
 
-- **No secret/token-based client auth** — the ingest server holds the InfluxDB token server-side. Clients must send a hashed peer ID via `X-Peer-ID` header.
+- **Intentionally unauthenticated** — the endpoint receives obfuscated telemetry from
+  the peers of both cloud and self-hosted deployments. For a self-hosted peer there is
+  no shared trust anchor with this server, so there is nothing to authenticate against.
+- **`X-Peer-ID` is a correlation tag, not a credential** — it carries the obfuscated
+  peer identifier so samples from one peer can be grouped. The server only checks that
+  the header is well-formed (16 hex chars); a malformed value is rejected with
+  `400 Bad Request`, not `401`. Any well-formed value is accepted by design, and the
+  header must not be relied on for access control. The header itself is not forwarded
+  to InfluxDB — the stored `peer_id` tag comes from the request body and is constrained
+  only by the tag allowlist and the maximum tag value length.
+- **The InfluxDB token stays server-side** — clients never hold a write credential.
 - **InfluxDB is not exposed** — only accessible within the docker network
 - Source: `ingest/main.go`
 
@@ -61,7 +72,7 @@ Tags:
 - `version`: NetBird version string
 - `os`: Operating system (linux, darwin, windows, android, ios, etc.)
 - `arch`: CPU architecture (amd64, arm64, etc.)
-- `peer_id`: anonymised peer identifier (truncated SHA-256 of the WireGuard public key)
+- `peer_id`: obfuscated peer identifier (truncated SHA-256 of the WireGuard public key)
 - `connection_pair_id`: deterministic identifier for the peer pair, identical on both sides
 
 **Note:** `SignalingReceived` is set when the first offer or answer arrives from the remote peer (in both initial and reconnection paths). It excludes the potentially unbounded wait for the remote peer to come online.
@@ -195,7 +206,7 @@ docker compose up -d
 ```
 
 This starts:
-- **Ingest server** on http://localhost:8087 — accepts client metrics (requires `X-Peer-ID` header, no secret/token auth)
+- **Ingest server** on http://localhost:8087 — accepts client metrics (unauthenticated by design; expects a well-formed `X-Peer-ID` correlation tag)
 - **InfluxDB** — internal only, not exposed to host
 - **Grafana** on http://localhost:3001
 

client/internal/metrics/infra/ingest/main.go

@@ -22,6 +22,11 @@ const (
 	maxDurationSeconds = 86400.0          // reject any duration field > 24 hours
 	peerIDLength       = 16               // truncated SHA-256: 8 bytes = 16 hex chars
 	maxTagValueLength  = 64               // reject tag values longer than this
+	readTimeout        = 30 * time.Second // must fit reading a compressed body up to maxBodySize
+	writeTimeout       = 60 * time.Second // must exceed the upstream client timeout below
+	idleTimeout        = 120 * time.Second
+	readHeaderTimeout  = 10 * time.Second
+	maxHeaderBytes     = 1 << 20 // 1 MB
 )
 
 type measurementSpec struct {
@@ -144,8 +149,17 @@ func main() {
 		fmt.Fprint(w, "ok") //nolint:errcheck
 	})
 
+	srv := &http.Server{
+		Addr:              listenAddr,
+		ReadTimeout:       readTimeout,
+		ReadHeaderTimeout: readHeaderTimeout,
+		WriteTimeout:      writeTimeout,
+		IdleTimeout:       idleTimeout,
+		MaxHeaderBytes:    maxHeaderBytes,
+	}
+
 	log.Printf("ingest server listening on %s, forwarding to %s", listenAddr, influxURL)
-	if err := http.ListenAndServe(listenAddr, nil); err != nil { //nolint:gosec
+	if err := srv.ListenAndServe(); err != nil {
 		log.Fatal(err)
 	}
 }
@@ -157,8 +171,8 @@ func handleIngest(client *http.Client, influxURL, influxToken string) http.Handl
 			return
 		}
 
-		if err := validateAuth(r); err != nil {
-			http.Error(w, err.Error(), http.StatusUnauthorized)
+		if err := validatePeerIDFormat(r); err != nil {
+			http.Error(w, err.Error(), http.StatusBadRequest)
 			return
 		}
 
@@ -207,8 +221,13 @@ func forwardToInflux(w http.ResponseWriter, r *http.Request, client *http.Client
 	io.Copy(w, resp.Body) //nolint:errcheck
 }
 
-// validateAuth checks that the X-Peer-ID header contains a valid hashed peer ID.
-func validateAuth(r *http.Request) error {
+// validatePeerIDFormat checks the shape of the X-Peer-ID header. The header is a
+// correlation tag, not a credential: this endpoint is intentionally
+// unauthenticated so that peers of self-hosted deployments, for which no shared
+// trust anchor exists, can report obfuscated telemetry. The header is not forwarded
+// to InfluxDB, so this check does not bound the stored peer_id tag; it only rejects
+// a malformed header as a bad request rather than an auth failure.
+func validatePeerIDFormat(r *http.Request) error {
 	peerID := r.Header.Get("X-Peer-ID")
 	if peerID == "" {
 		return fmt.Errorf("missing X-Peer-ID header")

client/internal/metrics/infra/ingest/main_test.go

@@ -94,7 +94,7 @@ func TestValidateLineProtocol_RejectsOnBadLine(t *testing.T) {
 	require.Error(t, err)
 }
 
-func TestValidateAuth(t *testing.T) {
+func TestValidatePeerIDFormat(t *testing.T) {
 	tests := []struct {
 		name    string
 		peerID  string
@@ -113,7 +113,7 @@ func TestValidateAuth(t *testing.T) {
 			if tt.peerID != "" {
 				r.Header.Set("X-Peer-ID", tt.peerID)
 			}
-			err := validateAuth(r)
+			err := validatePeerIDFormat(r)
 			if tt.wantErr {
 				require.Error(t, err)
 			} else {

client/internal/peer/worker_ice.go

@@ -64,6 +64,9 @@ type WorkerICE struct {
 
 	// portForwardAttempted tracks if we've already tried port forwarding this session
 	portForwardAttempted bool
+
+	// dialFunc, when non-nil, replaces agentDial in connect(). Only for tests.
+	dialFunc func(ctx context.Context, agent *icemaker.ThreadSafeAgent, remoteOfferAnswer *OfferAnswer) (net.Conn, error)
 }
 
 func NewWorkerICE(ctx context.Context, log *log.Entry, config ConnConfig, conn *Conn, signaler *Signaler, ifaceDiscover stdnet.ExternalIFaceDiscover, statusRecorder *Status, hasRelayOnLocally bool) (*WorkerICE, error) {
@@ -123,7 +126,7 @@ func (w *WorkerICE) OnNewOffer(remoteOfferAnswer *OfferAnswer) {
 			w.log.Errorf("failed to create new session ID: %s", err)
 		}
 		w.sessionID = sessionID
-		w.agent = nil
+		w.abandonNegotiation()
 	}
 
 	var preferredCandidateTypes []ice.CandidateType
@@ -151,7 +154,9 @@ func (w *WorkerICE) OnNewOffer(remoteOfferAnswer *OfferAnswer) {
 		w.remoteSessionID = ""
 	}
 
-	go w.connect(dialerCtx, agent, remoteOfferAnswer)
+	// Capture the cancel func at spawn time: connect reads it from the argument
+	// instead of the field, which a newer OnNewOffer may already have replaced.
+	go w.connect(dialerCtx, dialerCancel, agent, remoteOfferAnswer)
 }
 
 // OnRemoteCandidate Handles ICE connection Candidate provided by the remote peer.
@@ -200,16 +205,16 @@ func (w *WorkerICE) Close() {
 	w.muxAgent.Lock()
 	defer w.muxAgent.Unlock()
 
-	if w.agent == nil {
-		return
+	if w.agent != nil {
+		w.agentDialerCancel()
+		if err := w.agent.Close(); err != nil {
+			w.log.Warnf("failed to close ICE agent: %s", err)
+		}
 	}
-
-	w.agentDialerCancel()
-	if err := w.agent.Close(); err != nil {
-		w.log.Warnf("failed to close ICE agent: %s", err)
-	}
-
-	w.agent = nil
+	// Unconditional: a dial goroutine racing this Close skips its own cleanup
+	// (closeAgent finds a nil agent), so the flags must be dropped here too or
+	// the reconnection guard reads the stale state as Connected forever.
+	w.abandonNegotiation()
 }
 
 func (w *WorkerICE) reCreateAgent(dialerCancel context.CancelFunc, candidates []ice.CandidateType) (*icemaker.ThreadSafeAgent, error) {
@@ -247,31 +252,52 @@ func (w *WorkerICE) SessionID() ICESessionID {
 // will block until connection succeeded
 // but it won't release if ICE Agent went into Disconnected or Failed state,
 // so we have to cancel it with the provided context once agent detected a broken connection
-func (w *WorkerICE) connect(ctx context.Context, agent *icemaker.ThreadSafeAgent, remoteOfferAnswer *OfferAnswer) {
+func (w *WorkerICE) connect(ctx context.Context, dialerCancel context.CancelFunc, agent *icemaker.ThreadSafeAgent, remoteOfferAnswer *OfferAnswer) {
 	w.log.Debugf("gather candidates")
 	if err := agent.GatherCandidates(); err != nil {
 		w.log.Warnf("failed to gather candidates: %s", err)
-		w.closeAgent(agent, w.agentDialerCancel)
+		w.closeAgent(agent, dialerCancel)
 		return
 	}
 
 	w.log.Debugf("agent dial")
-	remoteConn, err := w.agentDial(ctx, agent, remoteOfferAnswer)
+	dial := func(ctx context.Context, agent *icemaker.ThreadSafeAgent, remoteOfferAnswer *OfferAnswer) (net.Conn, error) {
+		return w.agentDial(ctx, agent, remoteOfferAnswer)
+	}
+	if w.dialFunc != nil {
+		dial = w.dialFunc
+	}
+	remoteConn, err := dial(ctx, agent, remoteOfferAnswer)
 	if err != nil {
 		w.log.Debugf("failed to dial the remote peer: %s", err)
-		w.closeAgent(agent, w.agentDialerCancel)
+		w.closeAgent(agent, dialerCancel)
 		return
 	}
 	w.log.Debugf("agent dial succeeded")
 
+	// A newer negotiation may have replaced our agent while agentDial was
+	// blocked. Drop the dead connection before running pair retrieval, port
+	// punching or candidate work against a closed agent. The commit-point
+	// check below still guards a replacement arriving after this point.
+	w.muxAgent.Lock()
+	stale := w.agent != agent
+	w.muxAgent.Unlock()
+	if stale {
+		if err := remoteConn.Close(); err != nil {
+			w.log.Warnf("failed to close stale ICE connection: %s", err)
+		}
+		w.log.Warnf("discarding connection from a stale ICE negotiation")
+		return
+	}
+
 	pair, err := agent.GetSelectedCandidatePair()
 	if err != nil {
-		w.closeAgent(agent, w.agentDialerCancel)
+		w.closeAgent(agent, dialerCancel)
 		return
 	}
 	if pair == nil {
 		w.log.Warnf("selected candidate pair is nil, cannot proceed")
-		w.closeAgent(agent, w.agentDialerCancel)
+		w.closeAgent(agent, dialerCancel)
 		return
 	}
 
@@ -301,11 +327,27 @@ func (w *WorkerICE) connect(ctx context.Context, agent *icemaker.ThreadSafeAgent
 
 	w.log.Infof("connection succeeded with offer session: %s", remoteOfferAnswer.SessionIDString())
 	w.muxAgent.Lock()
+	// Authoritative ownership guard: a negotiation that lost w.agent to a newer
+	// one between the post-dial check and the commit must not clear agentConnecting,
+	// record lastSuccess or report the connection, so the state commit has to be
+	// atomic with the check.
+	if w.agent != agent {
+		w.muxAgent.Unlock()
+		if err := remoteConn.Close(); err != nil {
+			w.log.Warnf("failed to close stale ICE connection: %s", err)
+		}
+		w.log.Warnf("discarding connection from a stale ICE negotiation")
+		return
+	}
 	w.agentConnecting = false
 	w.lastSuccess = time.Now()
 	w.muxAgent.Unlock()
 
 	// todo: the potential problem is a race between the onConnectionStateChange
+	// and the delivery below: after this unlock, a newer offer can replace
+	// w.agent before onICEConnectionIsReady runs, delivering this (now stale)
+	// connection. The newer negotiation overwrites it with its own delivery,
+	// so the window only ever downgrades an endpoint transiently.
 	w.conn.onICEConnectionIsReady(selectedPriority(pair), ci)
 }
 
@@ -321,20 +363,32 @@ func (w *WorkerICE) closeAgent(agent *icemaker.ThreadSafeAgent, cancel context.C
 	sessionChanged := w.remoteSessionChanged
 	w.remoteSessionChanged = false
 
+	// Only the owner of the current session may reset its state: a stale dial
+	// goroutine waking after a newer attempt must not clobber it.
 	if w.agent == agent {
-		// consider to remove from here and move to the OnNewOffer
 		sessionID, err := NewICESessionID()
 		if err != nil {
 			w.log.Errorf("failed to create new session ID: %s", err)
 		}
 		w.sessionID = sessionID
-		w.agent = nil
-		w.agentConnecting = false
-		w.remoteSessionID = ""
+		w.abandonNegotiation()
 	}
 	return sessionChanged
 }
 
+// abandonNegotiation drops all recorded ICE session state so the worker treats the
+// next offer as a fresh start instead of a duplicate of a dead negotiation. The
+// agent and agentConnecting flags must change together: leaving one stale wedges
+// the reconnection guard into reporting Connected forever. It neither cancels an
+// in-flight dial nor closes an agent — callers dispose of those themselves first,
+// so a stale goroutine can never cancel another session's dial through this path.
+// Caller must hold muxAgent.
+func (w *WorkerICE) abandonNegotiation() {
+	w.agent = nil
+	w.agentConnecting = false
+	w.remoteSessionID = ""
+}
+
 func (w *WorkerICE) punchRemoteWGPort(pair *ice.CandidatePair, remoteWgPort int) {
 	// wait local endpoint configuration
 	time.Sleep(time.Second)

client/internal/peer/worker_ice_close_test.go

@@ -0,0 +1,257 @@
+package peer
+
+import (
+	"context"
+	"net"
+	"sync/atomic"
+	"testing"
+	"time"
+
+	log "github.com/sirupsen/logrus"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	"golang.zx2c4.com/wireguard/wgctrl/wgtypes"
+
+	icemaker "github.com/netbirdio/netbird/client/internal/peer/ice"
+	signal "github.com/netbirdio/netbird/shared/signal/client"
+	sProto "github.com/netbirdio/netbird/shared/signal/proto"
+)
+
+// stubSignalClient satisfies signal.Client as a no-op so the candidate
+// goroutine spawned by a real GatherCandidates never dereferences a nil
+// signaler in tests.
+type stubSignalClient struct{}
+
+func (stubSignalClient) Close() error                                               { return nil }
+func (stubSignalClient) StreamConnected() bool                                      { return false }
+func (stubSignalClient) GetStatus() signal.Status                                   { return signal.StreamDisconnected }
+func (stubSignalClient) Receive(context.Context, func(*sProto.Message) error) error { return nil }
+func (stubSignalClient) Ready() bool                                                { return false }
+func (stubSignalClient) IsHealthy() bool                                            { return false }
+func (stubSignalClient) WaitStreamConnected(context.Context)                        {}
+func (stubSignalClient) SendToStream(*sProto.EncryptedMessage) error                { return nil }
+func (stubSignalClient) Send(*sProto.Message) error                                 { return nil }
+func (stubSignalClient) SetOnReconnectedListener(func())                            {}
+
+// newTestWorkerICE builds a worker with real pion plumbing and no-op signaling.
+func newTestWorkerICE(t *testing.T) *WorkerICE {
+	t.Helper()
+
+	config := connConf
+	stunTurn := &icemaker.StunTurn{}
+	stunTurn.Store(nil)
+	config.ICEConfig.StunTurn = stunTurn
+
+	w, err := NewWorkerICE(context.Background(), log.WithField("test", t.Name()), config, nil,
+		NewSignaler(stubSignalClient{}, wgtypes.Key{}), nil, nil, false)
+	require.NoError(t, err, "worker setup must succeed")
+	return w
+}
+
+// TestWorkerICE_CloseDuringDial_ClearsConnectingFlag drives the teardown race
+// through the real dial goroutine instead of simulating its cleanup.
+//
+// The real-world sequence this models:
+//  1. OnNewOffer starts a negotiation: agent set, agentConnecting = true,
+//     go connect()
+//  2. The network dies and connect() stays blocked inside GatherCandidates/Dial
+//  3. A WG handshake timeout calls Close(): the agent is released and the dial
+//     context cancelled, but agentConnecting is not reset
+//  4. The real goroutine wakes with an error and runs its own cleanup
+//     (closeAgent), where `w.agent == agent` is now false, so the flag reset
+//     is skipped
+//
+// There is no remote responder, so Dial can never succeed: whatever point the
+// goroutine is at, closing first forces it down the error path. Before the fix
+// the flag stays true forever and the deadline below expires.
+func TestWorkerICE_CloseDuringDial_ClearsConnectingFlag(t *testing.T) {
+	w := newTestWorkerICE(t)
+
+	sid := ICESessionID("test-session-id")
+	w.OnNewOffer(&OfferAnswer{
+		IceCredentials: IceCredentials{
+			UFrag: "testufrag",
+			Pwd:   "testpwdtestpwdtestpwd12",
+		},
+		SessionID: &sid,
+	})
+	require.True(t, w.InProgress(), "OnNewOffer must mark the negotiation as in progress")
+
+	// Teardown wins the race while connect() is still running.
+	w.Close()
+
+	// Close drops the flags synchronously, so the assertion below does not
+	// converge on the goroutine: the deadline only absorbs the dial goroutine
+	// waking up in the background, proving nothing re-wedges it afterwards.
+	require.Eventually(t, func() bool {
+		return !w.InProgress()
+	}, 10*time.Second, 50*time.Millisecond,
+		"Close must leave the negotiation idle even while the dial goroutine is still winding down")
+
+	// abandonNegotiation owns these three fields together; the worker is idle
+	// only when all of them are dropped.
+	w.muxAgent.Lock()
+	defer w.muxAgent.Unlock()
+	assert.Nil(t, w.agent, "no agent may survive the teardown")
+	assert.False(t, w.agentConnecting, "the connecting flag must match the nil agent")
+	assert.Empty(t, w.remoteSessionID, "a dead session's remote ID must not linger")
+}
+
+// TestWorkerICE_CloseClearsResidualConnectingState covers Close on a worker whose
+// agent is already gone but whose flag is stuck on true, e.g. after an aborted
+// recreate in OnNewOffer or after a first Close raced a dial goroutine.
+func TestWorkerICE_CloseClearsResidualConnectingState(t *testing.T) {
+	w := newTestWorkerICE(t)
+
+	w.muxAgent.Lock()
+	w.agentConnecting = true
+	w.muxAgent.Unlock()
+
+	w.Close()
+
+	assert.False(t, w.InProgress(), "Close must drop residual connecting state even without a live agent")
+
+	w.muxAgent.Lock()
+	defer w.muxAgent.Unlock()
+	assert.Nil(t, w.agent)
+	assert.False(t, w.agentConnecting)
+	assert.Empty(t, w.remoteSessionID)
+}
+
+// TestWorkerICE_StaleCloseAgentKeepsCurrentSession pins the ownership guard in
+// closeAgent: a late-waking dial goroutine from an older session must not reset
+// the state of a newer negotiation that reused the worker. The newer session
+// must survive wholesale - agent, flag and remote session identity alike.
+func TestWorkerICE_StaleCloseAgentKeepsCurrentSession(t *testing.T) {
+	w := newTestWorkerICE(t)
+	t.Cleanup(w.Close)
+
+	sidA := ICESessionID("session-a")
+	w.OnNewOffer(&OfferAnswer{
+		IceCredentials: IceCredentials{UFrag: "ufragaaaa", Pwd: "pwdpwdpwdpwdpwdpwdpwdp1"},
+		SessionID:      &sidA,
+	})
+	w.muxAgent.Lock()
+	oldAgent := w.agent
+	oldCancel := w.agentDialerCancel
+	w.muxAgent.Unlock()
+	require.NotNil(t, oldAgent, "OnNewOffer must have created an ICE agent")
+
+	w.Close()
+
+	sidB := ICESessionID("session-b")
+	w.OnNewOffer(&OfferAnswer{
+		IceCredentials: IceCredentials{UFrag: "ufragbbbb", Pwd: "pwdpwdpwdpwdpwdpwdpwdp2"},
+		SessionID:      &sidB,
+	})
+	require.True(t, w.InProgress(), "the second negotiation must be in flight")
+
+	w.muxAgent.Lock()
+	newAgent := w.agent
+	w.muxAgent.Unlock()
+
+	// The old dial goroutine finally wakes and cleans up its captured agent.
+	w.closeAgent(oldAgent, oldCancel)
+
+	w.muxAgent.Lock()
+	defer w.muxAgent.Unlock()
+	assert.Same(t, newAgent, w.agent, "the current agent must be untouched by the stale cleanup")
+	assert.True(t, w.agentConnecting, "the current negotiation must stay in flight")
+	// Read live under the lock: a snapshot captured before the stale cleanup
+	// would pass even if the cleanup wiped current state.
+	assert.Equal(t, sidB, w.remoteSessionID, "the remote session identity must be preserved")
+}
+
+// closeTrackConn records Close calls so a test can assert that a discarded
+// connection was actually released.
+type closeTrackConn struct {
+	net.Conn
+	closed atomic.Bool
+}
+
+func (c *closeTrackConn) Close() error {
+	c.closed.Store(true)
+	return c.Conn.Close()
+}
+
+// TestWorkerICE_StaleDialSuccessKeepsNewerNegotiation pins the ownership guard
+// in connect()'s success path: a dial that came back after a newer negotiation
+// replaced the agent must discard its connection and leave the newer session's
+// state - agent, agentConnecting, remoteSessionID, lastSuccess - intact.
+//
+// The dial hook holds session A's goroutine open until session B is installed,
+// then returns a live connection, mimicking the vendored pion dial which hands
+// out a live *ice.Conn when a pair is selected without checking afterwards
+// whether the agent was replaced meanwhile. Releasing A's dial therefore
+// exercises the stale-success commit path deterministically instead of racing
+// real ICE.
+func TestWorkerICE_StaleDialSuccessKeepsNewerNegotiation(t *testing.T) {
+	w := newTestWorkerICE(t)
+	t.Cleanup(w.Close)
+
+	dialStarted := make(chan struct{})
+	releaseDial := make(chan struct{})
+	staleConn := &closeTrackConn{}
+
+	var calls atomic.Int32
+	w.dialFunc = func(ctx context.Context, _ *icemaker.ThreadSafeAgent, _ *OfferAnswer) (net.Conn, error) {
+		if calls.Add(1) == 1 {
+			// Session A: hold the goroutine open until session B is installed,
+			// then return a live connection, mimicking the vendored pion dial
+			// which hands out a live *ice.Conn once a pair is selected without
+			// re-checking whether the agent was replaced meanwhile. Releasing
+			// the dial therefore exercises the stale-success commit path
+			// deterministically instead of racing real ICE.
+			close(dialStarted)
+			<-releaseDial
+			client, _ := net.Pipe()
+			staleConn.Conn = client
+			return staleConn, nil
+		}
+		// A newer negotiation parks on its dialer context, cancelled by the
+		// t.Cleanup Close at test end.
+		<-ctx.Done()
+		return nil, ctx.Err()
+	}
+
+	sidA := ICESessionID("session-a")
+	w.OnNewOffer(&OfferAnswer{
+		IceCredentials: IceCredentials{UFrag: "ufragaaaa", Pwd: "pwdpwdpwdpwdpwdpwdpwdp1"},
+		SessionID:      &sidA,
+	})
+	require.True(t, w.InProgress(), "session A must be in flight")
+
+	// Session A's goroutine is now parked in the dial hook.
+	<-dialStarted
+
+	sidB := ICESessionID("session-b")
+	w.OnNewOffer(&OfferAnswer{
+		IceCredentials: IceCredentials{UFrag: "ufragbbbb", Pwd: "pwdpwdpwdpwdpwdpwdpwdp2"},
+		SessionID:      &sidB,
+	})
+
+	w.muxAgent.Lock()
+	agentB := w.agent
+	w.lastSuccess = time.Time{}
+	w.muxAgent.Unlock()
+	require.NotNil(t, agentB, "session B must have created an ICE agent")
+	require.True(t, w.InProgress(), "session B must be in flight")
+
+	// Release session A's dial: it must be recognized as stale and discarded.
+	close(releaseDial)
+	require.Eventually(t, func() bool {
+		return staleConn.closed.Load()
+	}, 10*time.Second, 10*time.Millisecond,
+		"the stale connection must be closed by the ownership guard")
+
+	w.muxAgent.Lock()
+	defer w.muxAgent.Unlock()
+	assert.Same(t, agentB, w.agent, "session A must not uninstall session B's agent")
+	assert.True(t, w.agentConnecting, "session A must not clear session B's connecting flag")
+	assert.Equal(t, sidB, w.remoteSessionID, "session A must not clear session B's remote session identity")
+	assert.True(t, w.lastSuccess.IsZero(), "session A must not record a success for session B")
+	// The commit block guards agentConnecting, lastSuccess and
+	// onICEConnectionIsReady together, so the state assertions above imply the
+	// callback never ran for session A; the nil conn would have panicked the
+	// stale goroutine on any invocation.
+}

client/internal/profilemanager/config.go

@@ -70,6 +70,7 @@ type ConfigInput struct {
 	StateFilePath                 string
 	PreSharedKey                  *string
 	ServerSSHAllowed              *bool
+	RemoteJobsAllowed             *bool
 	ServerVNCAllowed              *bool
 	DisableVNCApproval            *bool
 	EnableSSHRoot                 *bool
@@ -129,6 +130,7 @@ type Config struct {
 	RosenpassEnabled              bool
 	RosenpassPermissive           bool
 	ServerSSHAllowed              *bool
+	RemoteJobsAllowed             *bool
 	ServerVNCAllowed              *bool
 	DisableVNCApproval            *bool
 	EnableSSHRoot                 *bool
@@ -196,6 +198,12 @@ type Config struct {
 	// Runtime-only: re-derived from MDM policy on each load, never persisted.
 	LazyConnection string `json:"-"`
 
+	// DebugBundleUploadURL is the MDM-managed debug-bundle upload URL override.
+	// When set, it takes precedence over the management-supplied upload URL for
+	// remote debug bundle jobs. Runtime-only: re-derived from MDM policy on each
+	// load, never persisted.
+	DebugBundleUploadURL string `json:"-"`
+
 	MTU uint16
 
 	// policy is the MDM policy that produced the currently-set values for
@@ -229,6 +237,12 @@ func getConfigDir() (string, error) {
 	}
 
 	configDir := filepath.Join(base, "netbird")
+	// Under sudo this is the invoking user's directory and strictly read-only:
+	// anything root creates in it would be root-owned and break the user's own
+	// runs. Reads of a missing directory fall through to defaults.
+	if sudoActive() {
+		return configDir, nil
+	}
 	if err := os.MkdirAll(configDir, 0o755); err != nil {
 		return "", err
 	}
@@ -236,6 +250,16 @@ func getConfigDir() (string, error) {
 }
 
 func baseConfigDir() (string, error) {
+	if u, ok := sudoInvokingUser(); ok {
+		return userBaseConfigDir(u)
+	}
+	// Fail closed instead of falling through to root's own config directory:
+	// reading root's active-profile and email state for what is actually the
+	// invoking user's invocation is the very confusion this resolution exists
+	// to prevent.
+	if sudoActive() {
+		return "", fmt.Errorf("resolve sudo invoking user %q: refusing to fall back to root's config directory", os.Getenv(envSudoUser))
+	}
 	if runtime.GOOS == "darwin" {
 		if u, err := user.Current(); err == nil && u.HomeDir != "" {
 			return filepath.Join(u.HomeDir, "Library", "Application Support"), nil
@@ -277,7 +301,10 @@ func createNewConfig(input ConfigInput) (*Config, error) {
 	config := &Config{
 		// defaults to false only for new (post 0.26) configurations
 		ServerSSHAllowed: util.False(),
-		WgPort:           iface.DefaultWgPort,
+		// Remote jobs are an explicit opt-in and default off, including for
+		// legacy configs (a nil value materializes to false at connect time).
+		RemoteJobsAllowed: util.False(),
+		WgPort:            iface.DefaultWgPort,
 	}
 
 	if _, err := config.apply(input); err != nil {
@@ -507,6 +534,21 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
 		}
 	}
 
+	if input.RemoteJobsAllowed != nil && (config.RemoteJobsAllowed == nil || *input.RemoteJobsAllowed != *config.RemoteJobsAllowed) {
+		if *input.RemoteJobsAllowed {
+			log.Infof("enabling remote jobs")
+		} else {
+			log.Infof("disabling remote jobs")
+		}
+		config.RemoteJobsAllowed = input.RemoteJobsAllowed
+		updated = true
+	} else if config.RemoteJobsAllowed == nil {
+		// Remote jobs are an explicit opt-in: unlike SSH, a pre-existing config
+		// with no value defaults to disabled rather than being turned on.
+		config.RemoteJobsAllowed = util.False()
+		updated = true
+	}
+
 	if input.EnableSSHRoot != nil && (config.EnableSSHRoot == nil || *input.EnableSSHRoot != *config.EnableSSHRoot) {
 		if *input.EnableSSHRoot {
 			log.Infof("enabling SSH root login")
@@ -716,6 +758,14 @@ func (config *Config) apply(input ConfigInput) (updated bool, err error) {
 // for the key, so per-field rejection of user writes still applies).
 func (config *Config) applyMDMPolicy(policy *mdm.Policy) {
 	config.policy = policy
+
+	// DebugBundleUploadURL is a runtime-only override re-derived from MDM on
+	// every apply. Resolve it unconditionally (before the IsEmpty early return)
+	// so a policy that drops the key, becomes empty, or carries an invalid
+	// value can never leave a previously-enforced upload target active on a
+	// reused Config instance.
+	config.DebugBundleUploadURL = mdmDebugBundleUploadURL(policy)
+
 	if policy.IsEmpty() {
 		return
 	}
@@ -763,6 +813,7 @@ func (config *Config) applyMDMPolicy(policy *mdm.Policy) {
 	}
 
 	applyBool(mdm.KeyAllowServerSSH, func(v bool) { bv := v; config.ServerSSHAllowed = &bv })
+	applyBool(mdm.KeyRemoteJobsAllowed, func(v bool) { bv := v; config.RemoteJobsAllowed = &bv })
 	applyBool(mdm.KeyAllowServerVNC, func(v bool) { bv := v; config.ServerVNCAllowed = &bv })
 	applyBool(mdm.KeyDisableVNCApproval, func(v bool) { bv := v; config.DisableVNCApproval = &bv })
 	applyBool(mdm.KeyDisableClientRoutes, func(v bool) { config.DisableClientRoutes = v })
@@ -798,6 +849,52 @@ func (config *Config) applyMDMPolicy(policy *mdm.Policy) {
 		config.LazyConnection = state
 		logApplied(mdm.KeyLazyConnection, state)
 	}
+
+}
+
+// ValidateBundleUploadURL sanity-checks a debug-bundle upload URL. An empty
+// value is accepted — the executor falls back to the default upload service. A
+// non-empty value must be a well-formed https URL with a host; a malformed
+// value or a plaintext scheme is rejected. It deliberately does not constrain
+// which host may receive the bundle. This is the single source of truth for the
+// rule, shared by the remote-job executor (client/internal) and the MDM policy
+// override below so the two validation paths cannot drift.
+func ValidateBundleUploadURL(raw string) error {
+	if raw == "" {
+		return nil
+	}
+	parsed, err := url.Parse(raw)
+	if err != nil {
+		return fmt.Errorf("parse upload URL: %w", err)
+	}
+	// Hostname(), not Host: an authority like ":443" is non-empty but has no
+	// host, and would fail the actual upload.
+	if parsed.Scheme != "https" || parsed.Hostname() == "" {
+		return fmt.Errorf("upload URL must be an https URL with a host")
+	}
+	return nil
+}
+
+// mdmDebugBundleUploadURL resolves the MDM-enforced debug-bundle upload URL
+// override from the policy, returning the empty string when the policy does
+// not carry a valid KeyBundleUploadURL. An absent or invalid value fails
+// closed to "" so it falls back to the management-supplied or default upload
+// target rather than a previously-enforced one. The URL is never logged: it
+// can embed credentials or signed query tokens (KeyBundleUploadURL is in
+// mdm.SecretKeys).
+func mdmDebugBundleUploadURL(policy *mdm.Policy) string {
+	v, ok := policy.GetString(mdm.KeyBundleUploadURL)
+	if !ok || v == "" {
+		return ""
+	}
+	// Must be a well-formed https URL with a host, matching the client's
+	// remote-job upload-URL validation (shared validator, single source of truth).
+	if err := ValidateBundleUploadURL(v); err != nil {
+		log.Warnf("MDM debug bundle upload URL is invalid (must be an https URL with a host); ignoring the override")
+		return ""
+	}
+	log.Infof("MDM override %s = ********** (secret)", mdm.KeyBundleUploadURL)
+	return v
 }
 
 // parseURL parses and validates the URL for the named service. The URL

client/internal/profilemanager/config_test.go

@@ -14,6 +14,7 @@ import (
 
 	"github.com/netbirdio/netbird/client/iface"
 	"github.com/netbirdio/netbird/client/internal/routemanager/dynamic"
+	"github.com/netbirdio/netbird/client/mdm"
 	"github.com/netbirdio/netbird/util"
 )
 
@@ -271,6 +272,83 @@ func TestUpdateConfigServerSSHAllowedNotSet(t *testing.T) {
 	}
 }
 
+func TestUpdateConfigRemoteJobsAllowed(t *testing.T) {
+	// Unlike SSH (which defaults on for legacy configs), remote jobs are an
+	// explicit opt-in: a pre-existing config with no value materializes to off.
+	t.Run("legacy config defaults off", func(t *testing.T) {
+		configPath := filepath.Join(t.TempDir(), "config.json")
+		require.NoError(t, os.WriteFile(configPath, []byte("{}"), 0600))
+
+		config, err := UpdateConfig(ConfigInput{ConfigPath: configPath})
+		require.NoError(t, err)
+		require.NotNil(t, config.RemoteJobsAllowed, "RemoteJobsAllowed should be materialized")
+		assert.False(t, *config.RemoteJobsAllowed, "remote jobs must default off")
+	})
+
+	for _, tt := range []struct {
+		name  string
+		input *bool
+		want  bool
+	}{
+		{"enable", util.True(), true},
+		{"disable", util.False(), false},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			configPath := filepath.Join(t.TempDir(), "config.json")
+			require.NoError(t, os.WriteFile(configPath, []byte("{}"), 0600))
+
+			config, err := UpdateConfig(ConfigInput{ConfigPath: configPath, RemoteJobsAllowed: tt.input})
+			require.NoError(t, err)
+			require.NotNil(t, config.RemoteJobsAllowed)
+			assert.Equal(t, tt.want, *config.RemoteJobsAllowed)
+		})
+	}
+}
+
+func TestApplyMDMPolicyRemoteJobs(t *testing.T) {
+	t.Run("enables remote jobs and sets the upload URL override", func(t *testing.T) {
+		cfg := &Config{}
+		cfg.applyMDMPolicy(mdm.NewPolicy(map[string]any{
+			mdm.KeyRemoteJobsAllowed: true,
+			mdm.KeyBundleUploadURL:   "https://upload.example.com",
+		}))
+		require.NotNil(t, cfg.RemoteJobsAllowed)
+		assert.True(t, *cfg.RemoteJobsAllowed, "MDM allowRemoteJobs must enable the flag")
+		assert.Equal(t, "https://upload.example.com", cfg.DebugBundleUploadURL, "MDM upload URL override must be applied")
+	})
+
+	t.Run("a non-https upload URL is rejected", func(t *testing.T) {
+		cfg := &Config{}
+		cfg.applyMDMPolicy(mdm.NewPolicy(map[string]any{
+			mdm.KeyBundleUploadURL: "http://insecure.example.com",
+		}))
+		assert.Empty(t, cfg.DebugBundleUploadURL, "a non-https upload URL must be skipped")
+	})
+
+	t.Run("dropping the key clears a previously-applied override", func(t *testing.T) {
+		cfg := &Config{DebugBundleUploadURL: "https://old.example.com"}
+		// A replacement policy that no longer carries the key must not leave
+		// the old upload target directing bundles.
+		cfg.applyMDMPolicy(mdm.NewPolicy(map[string]any{mdm.KeyRemoteJobsAllowed: true}))
+		assert.Empty(t, cfg.DebugBundleUploadURL, "the stale upload URL override must be cleared")
+	})
+
+	t.Run("an empty replacement policy clears a previously-applied override", func(t *testing.T) {
+		cfg := &Config{DebugBundleUploadURL: "https://old.example.com"}
+		// A policy that becomes empty entirely hits the IsEmpty early return;
+		// the override must still be cleared rather than surviving on the
+		// reused Config instance.
+		cfg.applyMDMPolicy(mdm.NewPolicy(map[string]any{}))
+		assert.Empty(t, cfg.DebugBundleUploadURL, "the stale upload URL override must be cleared when the policy empties")
+	})
+
+	t.Run("an invalid upload URL clears a previously-applied override (fail closed)", func(t *testing.T) {
+		cfg := &Config{DebugBundleUploadURL: "https://old.example.com"}
+		cfg.applyMDMPolicy(mdm.NewPolicy(map[string]any{mdm.KeyBundleUploadURL: "not-a-url"}))
+		assert.Empty(t, cfg.DebugBundleUploadURL, "an invalid override must fail closed, not keep the stale target")
+	})
+}
+
 func TestUpdateOldManagementURL(t *testing.T) {
 	origProber := newMgmProber
 	newMgmProber = func(_ context.Context, _ string, _ wgtypes.Key, _ bool) (mgmProber, error) {

client/internal/profilemanager/invoking_user.go

@@ -0,0 +1,100 @@
+package profilemanager
+
+import (
+	"fmt"
+	"os"
+	"os/user"
+	"path/filepath"
+	"runtime"
+
+	log "github.com/sirupsen/logrus"
+)
+
+const envSudoUser = "SUDO_USER"
+
+var (
+	geteuid    = os.Geteuid
+	lookupUser = user.Lookup
+)
+
+// InvokingUser returns the user a CLI invocation acts for. Under sudo that is
+// the user who ran sudo, not root: privileged flags force commands through
+// sudo, and resolving profiles as root would silently switch the daemon to
+// root's (default) profile instead of the invoking user's. Privilege decisions
+// are not made here — those stay on the kernel credentials of the daemon
+// connection, which SUDO_USER (a plain environment variable) can never
+// influence; a forged value only selects a profile root could select anyway.
+func InvokingUser() (*user.User, error) {
+	if u, ok := sudoInvokingUser(); ok {
+		return u, nil
+	}
+	// Fail closed instead of falling through to root: every caller feeds this
+	// username into profile-path resolution, so a lookup failure would resolve
+	// (and create) a root-owned profile namespace and switch the daemon onto it
+	// behind the invoking user's back.
+	if sudoActive() {
+		return nil, fmt.Errorf("resolve sudo invoking user %q: refusing to fall back to root", os.Getenv(envSudoUser))
+	}
+	return user.Current()
+}
+
+// IsPlainRoot reports that the process runs as root with no usable sudo
+// context: there is no invoking user to act for, so per-user resolution falls
+// back to root's own (empty) state. Callers use it to refuse ambiguous
+// operations instead of silently acting on the wrong profile.
+func IsPlainRoot() bool {
+	if geteuid() != 0 {
+		return false
+	}
+	_, ok := sudoInvokingUser()
+	return !ok
+}
+
+// MirrorIsAuthoritative reports whether the invoking user's local
+// active-profile mirror can be trusted as the profile selector. It cannot under
+// sudo (writes to it are skipped, so it goes stale) or as plain root (there is
+// no invoking user, so it falls back to root's own default). Callers use it to
+// decide whether to read the profile from the mirror or from the daemon.
+func MirrorIsAuthoritative() bool {
+	return !sudoActive() && !IsPlainRoot()
+}
+
+// sudoInvokingUser resolves SUDO_USER when the process runs as root under
+// sudo. Returns false whenever the sudo context is absent or unusable, in
+// which case callers fall back to the process user.
+func sudoInvokingUser() (*user.User, bool) {
+	if !sudoActive() {
+		return nil, false
+	}
+	name := os.Getenv(envSudoUser)
+	u, err := lookupUser(name)
+	if err != nil {
+		log.Warnf("sudo invoking user %q lookup: %v", name, err)
+		return nil, false
+	}
+	return u, true
+}
+
+// sudoActive reports a sudo context from the environment alone: write-skip
+// decisions key off it so a transient user lookup failure can never flip a
+// run from read-only to writing root-owned files into the user's directory.
+func sudoActive() bool {
+	if geteuid() != 0 {
+		return false
+	}
+	name := os.Getenv(envSudoUser)
+	return name != "" && name != "root"
+}
+
+// userBaseConfigDir mirrors os.UserConfigDir for a user other than the process
+// owner. Environment overrides (XDG_CONFIG_HOME) cannot be honoured here: under
+// sudo the environment is root's, not the invoking user's.
+func userBaseConfigDir(u *user.User) (string, error) {
+	if u.HomeDir == "" {
+		return "", fmt.Errorf("user %s has no home directory", u.Username)
+	}
+	if runtime.GOOS == "darwin" {
+		return filepath.Join(u.HomeDir, "Library", "Application Support"), nil
+	}
+	return filepath.Join(u.HomeDir, ".config"), nil
+}

client/internal/profilemanager/invoking_user_test.go

@@ -0,0 +1,230 @@
+package profilemanager
+
+import (
+	"errors"
+	"io/fs"
+	"os"
+	"os/user"
+	"path/filepath"
+	"runtime"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestInvokingUserFallsBackToProcessUser(t *testing.T) {
+	t.Setenv(envSudoUser, "")
+
+	got, err := InvokingUser()
+	require.NoError(t, err)
+
+	current, err := user.Current()
+	require.NoError(t, err)
+	assert.Equal(t, current.Username, got.Username)
+}
+
+func TestSudoInvokingUserInactiveWithoutSudoContext(t *testing.T) {
+	t.Setenv(envSudoUser, "")
+	_, ok := sudoInvokingUser()
+	assert.False(t, ok)
+}
+
+func TestSudoInvokingUserIgnoresRoot(t *testing.T) {
+	t.Setenv(envSudoUser, "root")
+	origEuid := geteuid
+	geteuid = func() int { return 0 }
+	t.Cleanup(func() { geteuid = origEuid })
+
+	_, ok := sudoInvokingUser()
+	assert.False(t, ok, "sudo from a root shell must not redirect anything")
+	assert.False(t, sudoActive())
+	assert.True(t, IsPlainRoot())
+}
+
+func TestSudoInvokingUserResolvesInvokingUser(t *testing.T) {
+	fakeSudo(t, filepath.Join("/home", "misha"))
+
+	u, ok := sudoInvokingUser()
+	require.True(t, ok)
+	assert.Equal(t, "misha", u.Username)
+
+	got, err := InvokingUser()
+	require.NoError(t, err)
+	assert.Equal(t, "misha", got.Username)
+
+	assert.False(t, IsPlainRoot())
+}
+
+func TestInvokingUserFailsClosedWhenSudoLookupFails(t *testing.T) {
+	fakeSudo(t, filepath.Join("/home", "misha"))
+	lookupUser = func(string) (*user.User, error) { return nil, errors.New("nss unavailable") }
+
+	got, err := InvokingUser()
+	require.Error(t, err)
+	assert.Nil(t, got, "must not resolve to the root process user")
+}
+
+func TestProfileFilePathFailsClosedWhenSudoLookupFails(t *testing.T) {
+	profilesRoot := t.TempDir()
+	fakeSudo(t, filepath.Join("/home", "misha"))
+	lookupUser = func(string) (*user.User, error) { return nil, errors.New("nss unavailable") }
+
+	origDir := DefaultConfigPathDir
+	DefaultConfigPathDir = profilesRoot
+	t.Cleanup(func() { DefaultConfigPathDir = origDir })
+
+	p := &Profile{ID: "0123456789abcdef0123456789abcdef"}
+	_, err := p.FilePath()
+	require.Error(t, err)
+	assertNoEntries(t, profilesRoot)
+}
+
+func TestSudoActiveSurvivesLookupFailure(t *testing.T) {
+	fakeSudo(t, filepath.Join("/home", "misha"))
+	lookupUser = func(string) (*user.User, error) { return nil, errors.New("nss unavailable") }
+
+	_, ok := sudoInvokingUser()
+	assert.False(t, ok)
+	assert.True(t, sudoActive())
+	assert.True(t, IsPlainRoot())
+}
+
+func TestGetConfigDirUnderSudoIsReadOnly(t *testing.T) {
+	home := t.TempDir()
+	fakeSudo(t, home)
+
+	base, err := baseConfigDir()
+	require.NoError(t, err)
+	if runtime.GOOS == "darwin" {
+		assert.Equal(t, filepath.Join(home, "Library", "Application Support"), base)
+	} else {
+		assert.Equal(t, filepath.Join(home, ".config"), base)
+	}
+
+	dir, err := getConfigDir()
+	require.NoError(t, err)
+	assert.Equal(t, filepath.Join(base, "netbird"), dir)
+	assert.NoDirExists(t, dir)
+}
+
+func TestBaseConfigDirFailsClosedWhenSudoLookupFails(t *testing.T) {
+	fakeSudo(t, filepath.Join("/home", "misha"))
+	lookupUser = func(string) (*user.User, error) { return nil, errors.New("nss unavailable") }
+
+	_, err := baseConfigDir()
+	require.Error(t, err)
+
+	_, err = getConfigDir()
+	require.Error(t, err)
+}
+
+func TestSwitchProfileSkipsStateWriteUnderSudo(t *testing.T) {
+	home := t.TempDir()
+	fakeSudo(t, home)
+
+	pm := NewProfileManager()
+	require.NoError(t, pm.SwitchProfile(defaultProfileName))
+	assertNoEntries(t, home)
+}
+
+func TestSetProfileStateSkipsWriteUnderSudo(t *testing.T) {
+	home := t.TempDir()
+	fakeSudo(t, home)
+
+	pm := NewProfileManager()
+	require.NoError(t, pm.SetProfileState(defaultProfileName, &ProfileState{Email: "misha@example.com"}))
+	assertNoEntries(t, home)
+}
+
+func TestRemoveProfileStateSkipsRemoveUnderSudo(t *testing.T) {
+	home := t.TempDir()
+	stateDir := filepath.Join(home, ".config", "netbird")
+	if runtime.GOOS == "darwin" {
+		stateDir = filepath.Join(home, "Library", "Application Support", "netbird")
+	}
+	require.NoError(t, os.MkdirAll(stateDir, 0o700))
+	stateFile := filepath.Join(stateDir, "default.state.json")
+	require.NoError(t, os.WriteFile(stateFile, []byte(`{"email":"misha@example.com"}`), 0o600))
+
+	fakeSudo(t, home)
+	pm := NewProfileManager()
+	require.NoError(t, pm.RemoveProfileState("default"))
+	assert.FileExists(t, stateFile)
+}
+
+func TestUserBaseConfigDir(t *testing.T) {
+	u := &user.User{Username: "misha", HomeDir: filepath.Join("/home", "misha")}
+	dir, err := userBaseConfigDir(u)
+	require.NoError(t, err)
+	if runtime.GOOS == "darwin" {
+		assert.Equal(t, filepath.Join(u.HomeDir, "Library", "Application Support"), dir)
+	} else {
+		assert.Equal(t, filepath.Join(u.HomeDir, ".config"), dir)
+	}
+
+	_, err = userBaseConfigDir(&user.User{Username: "nohome"})
+	require.Error(t, err)
+}
+
+func TestIsPlainRoot(t *testing.T) {
+	t.Setenv(envSudoUser, "")
+	origEuid := geteuid
+	t.Cleanup(func() { geteuid = origEuid })
+
+	geteuid = func() int { return 1000 }
+	assert.False(t, IsPlainRoot())
+
+	geteuid = func() int { return 0 }
+	assert.True(t, IsPlainRoot())
+}
+
+func TestMirrorIsAuthoritative(t *testing.T) {
+	t.Setenv(envSudoUser, "")
+	origEuid := geteuid
+	t.Cleanup(func() { geteuid = origEuid })
+
+	geteuid = func() int { return 1000 }
+	assert.True(t, MirrorIsAuthoritative(), "a normal user's own mirror is authoritative")
+
+	geteuid = func() int { return 0 }
+	assert.False(t, MirrorIsAuthoritative(), "plain root has no authoritative mirror")
+}
+
+func TestMirrorIsAuthoritativeFalseUnderSudo(t *testing.T) {
+	fakeSudo(t, filepath.Join("/home", "misha"))
+	assert.False(t, MirrorIsAuthoritative(), "the sudo mirror is frozen, so it is not authoritative")
+}
+
+func fakeSudo(t *testing.T, home string) {
+	t.Helper()
+	t.Setenv(envSudoUser, "misha")
+
+	origEuid := geteuid
+	origLookup := lookupUser
+	origOverride := ConfigDirOverride
+	geteuid = func() int { return 0 }
+	lookupUser = func(name string) (*user.User, error) {
+		return &user.User{Username: name, Uid: "1234", Gid: "1234", HomeDir: home}, nil
+	}
+	ConfigDirOverride = ""
+	t.Cleanup(func() {
+		geteuid = origEuid
+		lookupUser = origLookup
+		ConfigDirOverride = origOverride
+	})
+}
+
+func assertNoEntries(t *testing.T, root string) {
+	t.Helper()
+	err := filepath.WalkDir(root, func(path string, _ fs.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		if path != root {
+			t.Errorf("unexpected entry created under %s: %s", root, path)
+		}
+		return nil
+	})
+	require.NoError(t, err)
+}

client/internal/profilemanager/profilemanager.go

@@ -3,7 +3,6 @@ package profilemanager
 import (
 	"fmt"
 	"os"
-	"os/user"
 	"path/filepath"
 	"strings"
 	"sync"
@@ -54,7 +53,7 @@ func (p *Profile) FilePath() (string, error) {
 		return "", fmt.Errorf("invalid profile ID: %q", id)
 	}
 
-	username, err := user.Current()
+	username, err := InvokingUser()
 	if err != nil {
 		return "", fmt.Errorf("failed to get current user: %w", err)
 	}
@@ -130,7 +129,7 @@ func (pm *ProfileManager) getActiveProfileState() ID {
 	if err != nil {
 		if !os.IsNotExist(err) {
 			log.Warnf("failed to read active profile state: %v", err)
-		} else {
+		} else if !sudoActive() {
 			if err := pm.setActiveProfileState(defaultProfileName); err != nil {
 				log.Warnf("failed to set default profile state: %v", err)
 			}
@@ -148,6 +147,13 @@ func (pm *ProfileManager) getActiveProfileState() ID {
 }
 
 func (pm *ProfileManager) setActiveProfileState(id ID) error {
+	// The invoking user's state is read-only under sudo — a root-owned file in
+	// the user's directory would break their own runs. The daemon still records
+	// the switch on its side; only the user-local bookkeeping is skipped.
+	if sudoActive() {
+		log.Infof("running under sudo: not persisting active profile %q for user %s", id, os.Getenv(envSudoUser))
+		return nil
+	}
 
 	configDir, err := getConfigDir()
 	if err != nil {

client/internal/profilemanager/state.go

@@ -7,6 +7,8 @@ import (
 	"os"
 	"path/filepath"
 
+	log "github.com/sirupsen/logrus"
+
 	"github.com/netbirdio/netbird/util"
 )
 
@@ -63,6 +65,15 @@ func (pm *ProfileManager) SetProfileState(id ID, state *ProfileState) error {
 		return fmt.Errorf("invalid profile ID: %q", id)
 	}
 
+	// The invoking user's state is read-only under sudo. The file only carries
+	// the account email for the login hint and display, so skipping the write
+	// costs at most one extra account prompt later — a root-owned file in the
+	// user's directory would cost every later update instead.
+	if sudoActive() {
+		log.Debugf("running under sudo: not persisting profile state for user %s", os.Getenv(envSudoUser))
+		return nil
+	}
+
 	stateFile := filepath.Join(configDir, id.String()+".state.json")
 	if err := util.WriteJsonWithRestrictedPermission(context.Background(), stateFile, state); err != nil {
 		return fmt.Errorf("write profile state: %w", err)
@@ -92,6 +103,11 @@ func (pm *ProfileManager) SetActiveProfileState(state *ProfileState) error {
 // equivalent to clearing it; the next SSO login recreates it. A missing file
 // is not an error.
 func (pm *ProfileManager) RemoveProfileState(profileName string) error {
+	if sudoActive() {
+		log.Debugf("running under sudo: not removing profile state for user %s", os.Getenv(envSudoUser))
+		return nil
+	}
+
 	configDir, err := getConfigDir()
 	if err != nil {
 		return fmt.Errorf("get config directory: %w", err)

client/internal/routemanager/selection.go

@@ -17,23 +17,30 @@ import (
 // are mutually exclusive: if the selection activates an exit node, every other
 // available exit node is deselected so two can't be active at once. With
 // appendRoute=false the previous selection is replaced instead of extended.
+// A partial failure (e.g. an unknown ID mixed with valid ones) still applies
+// the valid IDs to the routing table; the unknown ones are reported in the
+// returned error.
 func (m *DefaultManager) SelectRoutes(ids []route.NetID, appendRoute bool) error {
-	if err := m.selectRoutes(ids, appendRoute); err != nil {
-		return err
-	}
+	err := m.selectRoutes(ids, appendRoute)
+	// Apply regardless of err: selectRoutes already selects the valid part of a
+	// partial request, and skipping this on error would leave those routes
+	// selected in the selector but never installed in the routing table.
 	m.TriggerSelection(m.GetClientRoutes())
-	return nil
+	return err
 }
 
 // DeselectRoutes removes the routes with the given network IDs from the
 // selection and applies the change. V4/v6 exit-node pairs are expanded
-// automatically.
+// automatically. A partial failure (e.g. an unknown ID mixed with valid ones)
+// still applies the valid IDs to the routing table; the unknown ones are
+// reported in the returned error.
 func (m *DefaultManager) DeselectRoutes(ids []route.NetID) error {
-	if err := m.deselectRoutes(ids); err != nil {
-		return err
-	}
+	err := m.deselectRoutes(ids)
+	// Apply regardless of err: deselectRoutes already deselects the valid part
+	// of a partial request, and skipping this on error would leave those routes
+	// installed in the routing table despite being marked deselected.
 	m.TriggerSelection(m.GetClientRoutes())
-	return nil
+	return err
 }
 
 func (m *DefaultManager) deselectRoutes(ids []route.NetID) error {

client/internal/routemanager/selection_test.go

@@ -1,12 +1,17 @@
 package routemanager
 
 import (
+	"context"
 	"net/netip"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"golang.org/x/exp/maps"
 
+	"github.com/netbirdio/netbird/client/internal/peer"
+	"github.com/netbirdio/netbird/client/internal/routemanager/client"
+	"github.com/netbirdio/netbird/client/internal/routemanager/notifier"
 	"github.com/netbirdio/netbird/client/internal/routeselector"
 	"github.com/netbirdio/netbird/route"
 )
@@ -112,6 +117,75 @@ func TestSelectRoutes_UnknownRoute(t *testing.T) {
 	assert.Error(t, m.deselectRoutes([]route.NetID{"missing"}), "deselecting an unavailable route must fail")
 }
 
+// newPartialFailureTestManager exercises the real install/remove path without
+// touching the system: the noop refcounter absorbs the route changes, and every
+// route already has a watcher, so none is started.
+func newPartialFailureTestManager() *DefaultManager {
+	ctx := context.Background()
+
+	m := &DefaultManager{
+		ctx: ctx,
+		clientRoutes: route.HAMap{
+			"lan|192.168.1.0/24": {{NetID: "lan", Network: netip.MustParsePrefix("192.168.1.0/24"), Peer: "p1"}},
+			"other|10.1.2.0/24":  {{NetID: "other", Network: netip.MustParsePrefix("10.1.2.0/24"), Peer: "p2"}},
+		},
+		routeSelector:  routeselector.NewRouteSelector(),
+		notifier:       notifier.NewNotifier(),
+		statusRecorder: peer.NewRecorder("https://mgm"),
+		activeRoutes:   make(map[route.HAUniqueID]client.RouteHandler),
+		clientNetworks: map[route.HAUniqueID]*client.Watcher{
+			"lan|192.168.1.0/24": client.NewWatcher(client.WatcherConfig{Context: ctx}),
+			"other|10.1.2.0/24":  client.NewWatcher(client.WatcherConfig{Context: ctx}),
+		},
+	}
+	m.setupRefCounters(true)
+	return m
+}
+
+// Regression for the reported symptom: a partial failure returned before
+// TriggerSelection ran, so the valid route was marked selected while never
+// reaching the routing table (activeRoutes/ip route).
+func TestSelectRoutes_PartialFailureStillInstallsValidRoute(t *testing.T) {
+	m := newPartialFailureTestManager()
+
+	err := m.SelectRoutes([]route.NetID{"missing", "lan"}, false)
+
+	assert.Error(t, err, "the unknown id must still be reported")
+	assert.Contains(t, m.activeRoutes, route.HAUniqueID("lan|192.168.1.0/24"), "the valid route must be installed despite the error")
+	assert.NotContains(t, m.activeRoutes, route.HAUniqueID("other|10.1.2.0/24"), "the deselected route must not be installed")
+}
+
+// Mirror of the case above: a partial failure must remove the valid route from
+// the routing table, not just mark it deselected in the selector.
+func TestDeselectRoutes_PartialFailureStillRemovesValidRoute(t *testing.T) {
+	m := newPartialFailureTestManager()
+
+	require.NoError(t, m.SelectRoutes([]route.NetID{"lan", "other"}, false))
+	require.Contains(t, m.activeRoutes, route.HAUniqueID("lan|192.168.1.0/24"))
+	require.Contains(t, m.activeRoutes, route.HAUniqueID("other|10.1.2.0/24"))
+
+	err := m.DeselectRoutes([]route.NetID{"missing", "other"})
+
+	assert.Error(t, err, "the unknown id must still be reported")
+	assert.NotContains(t, m.activeRoutes, route.HAUniqueID("other|10.1.2.0/24"), "the deselected route must be removed")
+	assert.Contains(t, m.activeRoutes, route.HAUniqueID("lan|192.168.1.0/24"), "the untouched route stays installed")
+}
+
+// The selection now runs on every request, including one where no ID is known
+// and the selector stays untouched. Nothing may be torn down or reinstalled on
+// that path.
+func TestSelectRoutes_TotalFailureLeavesInstalledRoutesAlone(t *testing.T) {
+	m := newPartialFailureTestManager()
+
+	require.NoError(t, m.SelectRoutes([]route.NetID{"lan", "other"}, false))
+	installed := maps.Keys(m.activeRoutes)
+
+	err := m.SelectRoutes([]route.NetID{"missing"}, false)
+
+	assert.Error(t, err, "the unknown id must still be reported")
+	assert.ElementsMatch(t, installed, maps.Keys(m.activeRoutes), "a fully invalid request must not disturb the routing table")
+}
+
 func TestExitNodeSelectionHelpers(t *testing.T) {
 	routesMap := map[route.NetID][]*route.Route{
 		"exitA": {{Network: netip.MustParsePrefix("0.0.0.0/0")}},

client/internal/routeselector/routeselector.go

@@ -32,6 +32,22 @@ 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 of only unavailable routes would deselect everything and
+	// put nothing back. An empty request means deselect all, so it 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 +62,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

client/internal/routeselector/routeselector_test.go

@@ -887,3 +887,70 @@ 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")
 }
+
+// A non-append selection clears the current selection before applying the requested
+// one, so an all-unavailable request used to leave nothing selected while returning
+// an error. Requests with at least one available route are unaffected.
+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")
+	}
+}
+
+// Boundary of the check above: an empty request is the caller deselecting everything,
+// not a failed lookup, 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")
+	}
+}
+
+// Mobile clients always call SelectRoutes with append=true. On that path an
+// all-unavailable request was never destructive to begin with (append skips the
+// wipe regardless of the guard above), but the behavior has no coverage yet.
+func TestRouteSelector_SelectRoutes_AppendAllUnavailableKeepsSelection(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{"missing"}, true, 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")
+	}
+}
+
+// The early return for an all-unavailable request must not clear deselectAll,
+// or a typo'd network ID would silently drop the "nothing selected, including
+// future networks" policy.
+func TestRouteSelector_SelectRoutes_AllUnavailableAfterDeselectAllKeepsPolicy(t *testing.T) {
+	allRoutes := []route.NetID{"route1", "route2"}
+
+	rs := routeselector.NewRouteSelector()
+	rs.DeselectAllRoutes()
+
+	err := rs.SelectRoutes([]route.NetID{"missing"}, false, allRoutes)
+
+	assert.Error(t, err, "an unavailable route ID must still be reported")
+	assert.True(t, rs.IsDeselectAll(), "deselect-all policy must survive a fully invalid request")
+	assert.False(t, rs.IsSelected("route3"), "deselect-all must still cover networks not present in allRoutes yet")
+}