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
This commit is contained in:
Kristoffer Dalby
2026-04-08 08:42:50 +00:00
parent 580dcad683
commit ccddeceeec

View File

@@ -64,6 +64,39 @@ var ErrNodeNotInNodeStore = errors.New("node no longer exists in NodeStore")
// ErrNodeNameNotUnique is returned when a node name is not unique. // ErrNodeNameNotUnique is returned when a node name is not unique.
var ErrNodeNameNotUnique = errors.New("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. // ErrRegistrationExpired is returned when a registration has expired.
var ErrRegistrationExpired = errors.New("registration 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() nodePtr := node.AsStruct()
// Use Omit to prevent overwriting certain fields during MapRequest updates: // Explicitly select all node columns so GORM includes nil/zero-value
// - "expiry": should only be updated through explicit SetNodeExpiry calls or re-registration // fields (e.g. UserID=nil when converting a user-owned node to tagged).
// - "AuthKeyID", "AuthKey": prevents GORM from persisting stale PreAuthKey references that // Omit "Expiry" here: expiry is only updated through explicit
// may exist in NodeStore after a PreAuthKey has been deleted. The database handles setting // SetNodeExpiry calls or re-registration, not during MapRequest updates.
// auth_key_id to NULL via ON DELETE SET NULL. Without this, Updates() would fail with a err := s.db.DB.Select(nodeUpdateColumns).Omit("Expiry").Updates(nodePtr).Error
// 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
if err != nil { if err != nil {
return types.NodeView{}, change.Change{}, fmt.Errorf("saving node: %w", err) 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()) return types.NodeView{}, fmt.Errorf("%w: %d", ErrNodeNotInNodeStore, params.ExistingNode.ID())
} }
// Persist to database // Persist to database.
// Omit AuthKeyID/AuthKey to prevent stale PreAuthKey references from causing FK errors. // 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 := 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 { if err != nil {
return nil, fmt.Errorf("saving node: %w", err) 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) { _, err = hsdb.Write(s.db.DB, func(tx *gorm.DB) (*types.Node, error) {
// Use Updates() to preserve fields not modified by UpdateNode. // Explicitly select all node columns so GORM includes nil/zero-value fields
// Omit AuthKeyID/AuthKey to prevent stale PreAuthKey references from causing FK errors. // (see nodeUpdateColumns comment).
err := tx.Omit("AuthKeyID", "AuthKey").Updates(updatedNodeView.AsStruct()).Error err := tx.Select(nodeUpdateColumns).Updates(updatedNodeView.AsStruct()).Error
if err != nil { if err != nil {
return nil, fmt.Errorf("saving node: %w", err) return nil, fmt.Errorf("saving node: %w", err)
} }