From ee9a5c2e20d003ba6134978c26a59eaf4f0e8e64 Mon Sep 17 00:00:00 2001
From: riccardom <riccardomanfrin@gmail.com>
Date: Wed, 2 Sep 2026 14:35:05 +0200
Subject: [PATCH] [client] Re-take the update-settings decision under the
 config lock
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Login checks twice on purpose: the first check refuses the ordinary case
early, and authorizeAndPrepareLogin re-takes the authoritative one under
guardedConfigMu because the first is unsynchronized against a concurrent
privileged request. The update-settings decision is now equally
value-dependent — it compares the request against the stored config — but it
was taken only in the first, unlocked check.

So a login that was a no-op when it was checked could be written after a
concurrent writer had repointed the profile, which is exactly the window the
lock exists to close. The decision is now re-taken alongside the privilege one,
which also makes it the last read before persistLoginOverrides writes.

The test drives that interleaving through the existing afterLoginPreCheck seam
and fails without the re-check.
---
 client/server/server.go                    |  9 ++++
 client/server/update_settings_gate_test.go | 49 ++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/client/server/server.go b/client/server/server.go
index d1dafa518..de6870ff2 100644
--- a/client/server/server.go
+++ b/client/server/server.go
@@ -2634,6 +2634,15 @@ func (s *Server) authorizeAndPrepareLogin(callerCtx context.Context, msg *proto.
 		return nil, nil, err
 	}
 
+	// The update-settings decision is re-taken here for the same reason as the
+	// privilege one: Login's earlier check ran outside this lock, so the stored
+	// config it compared against could have moved since. This one is the
+	// authoritative check, and it is the last read before persistLoginOverrides
+	// writes.
+	if s.checkUpdateSettingsDisabled() && configChangeRequested(stored, loginOverridesInput(msg)) {
+		return nil, nil, gstatus.Errorf(codes.Unavailable, errUpdateSettingsDisabled)
+	}
+
 	s.mutex.Lock()
 	if s.actCancel != nil {
 		s.actCancel()
diff --git a/client/server/update_settings_gate_test.go b/client/server/update_settings_gate_test.go
index 2972ec415..d9dc41b13 100644
--- a/client/server/update_settings_gate_test.go
+++ b/client/server/update_settings_gate_test.go
@@ -292,3 +292,52 @@ func TestLogin_RestatingTheStoredConfigPassesTheGate(t *testing.T) {
 			"the gate refused a login that changes nothing: %v", err)
 	}
 }
+
+// The value-aware decision has the same synchronization problem as the
+// privileged-change one: Login's first check runs outside guardedConfigMu, so
+// the stored config it compared against can move before the write. A login that
+// was a no-op when it was checked must not be written once it has become a
+// change.
+func TestLogin_ChangeThatAppearsMidRequestIsRefused(t *testing.T) {
+	s, _, _, username, _ := setupServerWithProfile(t)
+	s.updateSettingsDisabled = true
+	s.rootCtx = internal.CtxInitState(context.Background())
+
+	target := "moved-under-us"
+	targetPath := filepath.Join(profilemanager.DefaultConfigPathDir, target+".json")
+	_, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{
+		ConfigPath:    targetPath,
+		ManagementURL: storedManagementURL,
+	})
+	require.NoError(t, err)
+
+	cancelled := false
+	s.actCancel = func() { cancelled = true }
+
+	// Stand in for a concurrent writer that repoints the profile between the two
+	// checks, which is the interleaving the lock has to make safe. The login
+	// restates the URL the profile held when it was checked, so the first check
+	// sees a no-op and lets it through.
+	afterLoginPreCheck = func() {
+		_, err := profilemanager.UpdateOrCreateConfig(profilemanager.ConfigInput{
+			ConfigPath:    targetPath,
+			ManagementURL: "https://mgmt.elsewhere.example:443",
+		})
+		require.NoError(t, err)
+	}
+	t.Cleanup(func() { afterLoginPreCheck = nil })
+
+	_, err = s.Login(userCtx(), &proto.LoginRequest{
+		ProfileName:   &target,
+		Username:      &username,
+		ManagementUrl: storedManagementURL,
+	})
+	require.Error(t, err, "the login became a settings change before it was written")
+	require.Equal(t, codes.Unavailable, gstatus.Code(err), "want the update-settings refusal, got %v", err)
+	require.False(t, cancelled, "the refused login cancelled the login already in progress")
+
+	stored, err := profilemanager.PeekConfig(targetPath)
+	require.NoError(t, err)
+	require.Equal(t, "https://mgmt.elsewhere.example:443", stored.ManagementURL.String(),
+		"the refused login wrote the management URL it was asked for")
+}