Do not block further wipe commands on inactive existing entry (#48358)

**Related issue:** Resolves #45931 # Checklist for submitter If some of the following don't apply, delete the relevant line. - [x] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [x] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. - [x] Timeouts are implemented and retries are limited to avoid infinite loops - [x] If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes ## Testing - [x] Added/updated automated tests - [x] Where appropriate, [automated tests simulate multiple hosts and test for host isolation](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/reference/patterns-backend.md#unit-testing) (updates to one hosts's records do not affect another) - [x] QA'd all new/changed functionality manually  ## Summary by CodeRabbit * **Bug Fixes** * Improved device lock handling so only active pending lock commands are treated as valid. * Fixed stale lock state cases where an old lock reference no longer blocks a new lock request. * When a prior lock command is no longer deliverable, a new lock command is now issued and tracked correctly. * Updated coverage to verify lock status transitions and replacement behavior in these edge cases.
2026-06-26 15:58:42 -04:00
parent a764e5d595
commit b438893bc2
3 changed files with 161 additions and 3 deletions
@@ -191,6 +191,7 @@ func (s *NanoMDMStorage) GetPendingLockCommand(ctx context.Context, hostUUID str
 		LEFT JOIN nano_command_results ncr ON ncr.command_uuid = nc.command_uuid
 		INNER JOIN nano_enrollment_queue neq ON neq.command_uuid = nc.command_uuid
 		WHERE neq.id = ?
+		AND neq.active = 1
 		AND nc.request_type = 'DeviceLock'
 		AND ncr.command_uuid IS NULL
 		ORDER BY nc.created_at DESC
@@ -244,10 +245,21 @@ func (s *NanoMDMStorage) EnqueueDeviceLockCommand(
 			`SELECT lock_ref FROM host_mdm_actions WHERE host_id = ? FOR UPDATE`,
 			host.ID)

-		// If we got a row and it has a lock_ref, fail with conflict
+		// A non-null lock_ref only blocks a new lock if it still points to a
+		// deliverable command. Re-enrollment, SCEP renewal, and wipe flip the
+		// queued command to active=0 (see nanomdm ClearQueue), and an inactive
+		// command is never sent to the device, so treat it as an orphan ref and
+		// let the new lock overwrite it below.
 		if err == nil && existingLockRef != nil && *existingLockRef != "" {
-			// A lock command already exists, don't overwrite
-			return lockConflictError{hostUUID: host.UUID}
+			var active bool
+			if err := sqlx.GetContext(ctx, tx, &active,
+				`SELECT EXISTS(SELECT 1 FROM nano_enrollment_queue WHERE command_uuid = ? AND id = ? AND active = 1)`,
+				*existingLockRef, host.UUID); err != nil {
+				return ctxerr.Wrap(ctx, err, "checking if existing lock command is active")
+			}
+			if active {
+				return lockConflictError{hostUUID: host.UUID}
+			}
 		}

 		// If the row doesn't exist, that's OK, we'll insert it
@@ -28,6 +28,7 @@ func TestNanoMDMStorage(t *testing.T) {
 	}{
 		{"TestEnqueueDeviceLockCommand", testEnqueueDeviceLockCommand},
 		{"TestGetPendingLockCommand", testGetPendingLockCommand},
+		{"TestEnqueueDeviceLockReplacesOrphanRef", testEnqueueDeviceLockReplacesOrphanRef},
 		{"TestEnqueueDeviceLockCommandRaceCondition", testEnqueueDeviceLockCommandRaceCondition},
 		{"TestEnqueueDeviceUnlockCommand", testEnqueueDeviceUnlockCommand},
 		{"TestStoreAuthenticatePreservesBootstrapTokenDuringSCEPRenewal", testStoreAuthenticatePreservesBootstrapTokenDuringSCEPRenewal},
@@ -242,6 +243,73 @@ func testGetPendingLockCommand(t *testing.T, ds *Datastore) {
 	require.Empty(t, pin)
 }

+// testEnqueueDeviceLockReplacesOrphanRef verifies that a lock_ref pointing to a
+// command that is no longer deliverable (nano_enrollment_queue.active = 0, e.g.
+// after re-enrollment, SCEP renewal, or wipe) is treated as an orphan: it does
+// not count as a pending lock and does not block a fresh lock command.
+// See https://github.com/fleetdm/fleet/issues/45931
+func testEnqueueDeviceLockReplacesOrphanRef(t *testing.T, ds *Datastore) {
+	ctx := context.Background()
+	ns, err := ds.NewMDMAppleMDMStorage()
+	require.NoError(t, err)
+
+	host, err := ds.NewHost(ctx, &fleet.Host{
+		Hostname:      "orphan-relock-name",
+		OsqueryHostID: new("4242"),
+		NodeKey:       new("4242"),
+		UUID:          "orphan-relock-uuid",
+		TeamID:        nil,
+		Platform:      "darwin",
+	})
+	require.NoError(t, err)
+	nanoEnroll(t, ds, host, false)
+
+	// Enqueue an initial lock command: active=1, no result yet -> genuinely pending.
+	lockCmd := &mdm.Command{}
+	lockCmd.CommandUUID = "orphan-lock-cmd-1"
+	lockCmd.Command.RequestType = "DeviceLock"
+	lockCmd.Raw = []byte("<?xml")
+	require.NoError(t, ns.EnqueueDeviceLockCommand(ctx, host, lockCmd, "654321"))
+
+	pending, pin, err := ns.GetPendingLockCommand(ctx, host.UUID)
+	require.NoError(t, err)
+	require.NotNil(t, pending)
+	require.Equal(t, "orphan-lock-cmd-1", pending.CommandUUID)
+	require.Equal(t, "654321", pin)
+
+	// Simulate re-enrollment/SCEP renewal/wipe deactivating the queued command
+	// without a result (what nanomdm ClearQueue does on Authenticate).
+	_, err = ds.writer(ctx).ExecContext(ctx,
+		`UPDATE nano_enrollment_queue SET active = 0 WHERE id = ? AND command_uuid = ?`,
+		host.UUID, "orphan-lock-cmd-1")
+	require.NoError(t, err)
+
+	// Gate 1: the deactivated command is no longer deliverable, so it must not
+	// count as a pending lock.
+	pending, _, err = ns.GetPendingLockCommand(ctx, host.UUID)
+	require.NoError(t, err)
+	require.Nil(t, pending)
+
+	// Gate 2: the stale lock_ref must not block a fresh lock command.
+	lockCmd2 := &mdm.Command{}
+	lockCmd2.CommandUUID = "orphan-lock-cmd-2"
+	lockCmd2.Command.RequestType = "DeviceLock"
+	lockCmd2.Raw = []byte("<?xml2")
+	require.NoError(t, ns.EnqueueDeviceLockCommand(ctx, host, lockCmd2, "222222"))
+
+	// The new (active) command is now the pending lock, and lock_ref was overwritten.
+	pending, pin, err = ns.GetPendingLockCommand(ctx, host.UUID)
+	require.NoError(t, err)
+	require.NotNil(t, pending)
+	require.Equal(t, "orphan-lock-cmd-2", pending.CommandUUID)
+	require.Equal(t, "222222", pin)
+
+	var lockRef string
+	require.NoError(t, ds.writer(ctx).QueryRowContext(ctx,
+		`SELECT lock_ref FROM host_mdm_actions WHERE host_id = ?`, host.ID).Scan(&lockRef))
+	require.Equal(t, "orphan-lock-cmd-2", lockRef)
+}
+
 // testStoreAuthenticatePreservesBootstrapTokenDuringSCEPRenewal verifies that
 // StoreAuthenticate does NOT clear the bootstrap token when a SCEP renewal is
 // in progress (renew_command_uuid is set in nano_cert_auth_associations), and
@@ -204,6 +204,84 @@ func (s *integrationMDMTestSuite) TestLockUnlockWipeMacOS() {
 	require.Empty(t, lockResp.UnlockPIN)
 }

+// TestLockMacOSWithOrphanedLockRef reproduces #45931: a macOS host has a pending
+// DeviceLock whose queued command gets deactivated (nano_enrollment_queue.active
+// = 0) while its lock_ref remains — e.g. a SCEP-renewal re-checkin clears the
+// command queue without a full re-enrollment. A subsequent lock must enqueue a
+// fresh, deliverable command instead of silently reusing the orphaned ref.
+func (s *integrationMDMTestSuite) TestLockMacOSWithOrphanedLockRef() {
+	t := s.T()
+	ctx := context.Background()
+	s.setSkipWorkerJobs(t)
+	host, mdmClient := createHostThenEnrollMDM(s.ds, s.server.URL, t)
+
+	readLockRef := func() string {
+		var lockRef string
+		mysqltest.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error {
+			return sqlx.GetContext(ctx, q, &lockRef,
+				`SELECT lock_ref FROM host_mdm_actions WHERE host_id = ?`, host.ID)
+		})
+		return lockRef
+	}
+
+	// lock the host: enqueues a DeviceLock command and records lock_ref.
+	var lockResp fleet.LockHostResponse
+	s.DoJSON("POST", fmt.Sprintf("/api/latest/fleet/hosts/%d/lock", host.ID), nil, http.StatusOK, &lockResp, "view_pin", "true")
+	require.Len(t, lockResp.UnlockPIN, 6)
+
+	firstLockRef := readLockRef()
+	require.NotEmpty(t, firstLockRef)
+
+	var getHostResp getHostResponse
+	s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/hosts/%d", host.ID), nil, http.StatusOK, &getHostResp)
+	require.NotNil(t, getHostResp.Host.MDM.PendingAction)
+	require.Equal(t, "lock", *getHostResp.Host.MDM.PendingAction)
+
+	// deactivate the queued lock command without touching host_mdm_actions,
+	// leaving lock_ref pointing at a command that will never be delivered.
+	mysqltest.ExecAdhocSQL(t, s.ds, func(q sqlx.ExtContext) error {
+		_, err := q.ExecContext(ctx,
+			`UPDATE nano_enrollment_queue SET active = 0 WHERE id = ? AND command_uuid = ?`,
+			host.UUID, firstLockRef)
+		return err
+	})
+
+	// the orphaned lock_ref no longer counts as pending: the host reads back as
+	// unlocked with no pending action.
+	s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/hosts/%d", host.ID), nil, http.StatusOK, &getHostResp)
+	require.NotNil(t, getHostResp.Host.MDM.DeviceStatus)
+	require.Equal(t, "unlocked", *getHostResp.Host.MDM.DeviceStatus)
+	require.NotNil(t, getHostResp.Host.MDM.PendingAction)
+	require.Empty(t, *getHostResp.Host.MDM.PendingAction)
+
+	// locking again must enqueue a brand-new, deliverable DeviceLock command
+	// rather than silently reusing the orphaned ref.
+	lockResp = fleet.LockHostResponse{}
+	s.DoJSON("POST", fmt.Sprintf("/api/latest/fleet/hosts/%d/lock", host.ID), nil, http.StatusOK, &lockResp, "view_pin", "true")
+	require.Len(t, lockResp.UnlockPIN, 6)
+	require.Equal(t, fleet.PendingActionLock, lockResp.PendingAction)
+
+	secondLockRef := readLockRef()
+	require.NotEmpty(t, secondLockRef)
+	require.NotEqual(t, firstLockRef, secondLockRef, "lock_ref should be replaced, not reused")
+
+	// the device receives the new DeviceLock command (the bug was that nothing
+	// was delivered), and acknowledging it locks the host.
+	cmd, err := mdmClient.Idle()
+	require.NoError(t, err)
+	require.NotNil(t, cmd)
+	require.Equal(t, "DeviceLock", cmd.Command.RequestType)
+	require.Equal(t, secondLockRef, cmd.CommandUUID)
+	_, err = mdmClient.Acknowledge(cmd.CommandUUID)
+	require.NoError(t, err)
+
+	s.DoJSON("GET", fmt.Sprintf("/api/latest/fleet/hosts/%d", host.ID), nil, http.StatusOK, &getHostResp)
+	require.NotNil(t, getHostResp.Host.MDM.DeviceStatus)
+	require.Equal(t, "locked", *getHostResp.Host.MDM.DeviceStatus)
+	require.NotNil(t, getHostResp.Host.MDM.PendingAction)
+	require.Empty(t, *getHostResp.Host.MDM.PendingAction)
+}
+
 func (s *integrationMDMTestSuite) TestWipeMacOSCancelsUpcomingActivities() {
 	t := s.T()
 	s.setSkipWorkerJobs(t)