From ccddeceeec951a4eb08a08794c2718a37b40f181 Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Wed, 8 Apr 2026 08:42:50 +0000 Subject: [PATCH] state: fix GORM not persisting user_id=NULL on tagged node conversion GORM's struct-based Updates() silently skips nil pointer fields. When SetNodeTags sets node.UserID = nil to transfer ownership to tags, the in-memory NodeStore is correct but the database retains the old user_id value. This causes tagged nodes to remain associated with the original user in the database, preventing user deletion and risking ON DELETE CASCADE destroying tagged nodes. Add Select("*") before Omit() on all three node persistence paths to force GORM to include all fields in the UPDATE statement, including nil pointers. This is the same pattern already used in db/ip.go for IPv4/IPv6 nil handling, and is documented GORM behavior: db.Select("*").Omit("excluded").Updates(struct) The three affected paths are: - persistNodeToDB: used by SetNodeTags and MapRequest updates - applyAuthNodeUpdate: used by re-authentication with --advertise-tags - HandleNodeFromPreAuthKey: used by PAK re-registration Fixes #3161 --- hscontrol/state/state.go | 59 ++++++++++++++++++++++++++++++---------- 1 file changed, 45 insertions(+), 14 deletions(-) diff --git a/hscontrol/state/state.go b/hscontrol/state/state.go index 723a35d3..d5f43b18 100644 --- a/hscontrol/state/state.go +++ b/hscontrol/state/state.go @@ -64,6 +64,39 @@ var ErrNodeNotInNodeStore = errors.New("node no longer exists in NodeStore") // ErrNodeNameNotUnique is returned when a node name is not unique. var ErrNodeNameNotUnique = errors.New("node name is not unique") +// nodeUpdateColumns lists all Node columns that should be written +// during a struct-based GORM Updates() call. Listing them explicitly +// forces GORM to include nil/zero-value fields (e.g. UserID=nil when +// converting a user-owned node to tagged) that struct-based Updates() +// would otherwise silently skip. +// +// Excluded columns: +// - AuthKeyID, AuthKey: prevents GORM from persisting stale +// PreAuthKey references after a key has been deleted (#2862). +// - User: GORM association, not a real column. +// - IsOnline: runtime-only field (gorm:"-"). +// +// Expiry is included here but may be omitted at call sites that must +// not touch it (see persistNodeToDB). +var nodeUpdateColumns = []string{ + "MachineKey", + "NodeKey", + "DiscoKey", + "Endpoints", + "Hostinfo", + "IPv4", + "IPv6", + "Hostname", + "GivenName", + "UserID", + "RegisterMethod", + "Tags", + "Expiry", + "LastSeen", + "ApprovedRoutes", + "UpdatedAt", +} + // ErrRegistrationExpired is returned when a registration has expired. var ErrRegistrationExpired = errors.New("registration expired") @@ -449,14 +482,11 @@ func (s *State) persistNodeToDB(node types.NodeView) (types.NodeView, change.Cha nodePtr := node.AsStruct() - // Use Omit to prevent overwriting certain fields during MapRequest updates: - // - "expiry": should only be updated through explicit SetNodeExpiry calls or re-registration - // - "AuthKeyID", "AuthKey": prevents GORM from persisting stale PreAuthKey references that - // may exist in NodeStore after a PreAuthKey has been deleted. The database handles setting - // auth_key_id to NULL via ON DELETE SET NULL. Without this, Updates() would fail with a - // foreign key constraint error when trying to reference a deleted PreAuthKey. - // See also: https://github.com/juanfont/headscale/issues/2862 - err := s.db.DB.Omit("expiry", "AuthKeyID", "AuthKey").Updates(nodePtr).Error + // Explicitly select all node columns so GORM includes nil/zero-value + // fields (e.g. UserID=nil when converting a user-owned node to tagged). + // Omit "Expiry" here: expiry is only updated through explicit + // SetNodeExpiry calls or re-registration, not during MapRequest updates. + err := s.db.DB.Select(nodeUpdateColumns).Omit("Expiry").Updates(nodePtr).Error if err != nil { return types.NodeView{}, change.Change{}, fmt.Errorf("saving node: %w", err) } @@ -1458,10 +1488,11 @@ func (s *State) applyAuthNodeUpdate(params authNodeUpdateParams) (types.NodeView return types.NodeView{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, params.ExistingNode.ID()) } - // Persist to database - // Omit AuthKeyID/AuthKey to prevent stale PreAuthKey references from causing FK errors. + // Persist to database. + // Explicitly select all node columns so GORM includes nil/zero-value fields + // (see nodeUpdateColumns comment). _, err := hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) { - err := tx.Omit("AuthKeyID", "AuthKey").Updates(updatedNodeView.AsStruct()).Error + err := tx.Select(nodeUpdateColumns).Updates(updatedNodeView.AsStruct()).Error if err != nil { return nil, fmt.Errorf("saving node: %w", err) } @@ -2082,9 +2113,9 @@ func (s *State) HandleNodeFromPreAuthKey( } _, err = hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) { - // Use Updates() to preserve fields not modified by UpdateNode. - // Omit AuthKeyID/AuthKey to prevent stale PreAuthKey references from causing FK errors. - err := tx.Omit("AuthKeyID", "AuthKey").Updates(updatedNodeView.AsStruct()).Error + // Explicitly select all node columns so GORM includes nil/zero-value fields + // (see nodeUpdateColumns comment). + err := tx.Select(nodeUpdateColumns).Updates(updatedNodeView.AsStruct()).Error if err != nil { return nil, fmt.Errorf("saving node: %w", err) }