state: install a written policy only after it is stored

Writers validated by installing the policy live, so a store failure
left a refused or unstored policy enforced. Validate on a throwaway
manager, store, then reload from the stored row.

hscontrol/api/v1/errors.go

@@ -38,7 +38,8 @@ func mapError(msg string, err error) error {
 		errors.Is(err, db.ErrUserStillHasNodes),
 		errors.Is(err, db.ErrCannotChangeOIDCUser),
 		errors.Is(err, db.ErrPreAuthKeyNotTaggedOrOwned),
-		errors.Is(err, db.ErrSingleUseAuthKeyHasBeenUsed):
+		errors.Is(err, db.ErrSingleUseAuthKeyHasBeenUsed),
+		errors.Is(err, state.ErrInvalidPolicy):
 		return huma.Error400BadRequest(msg, err)
 
 	case errors.Is(err, state.ErrNodeKeyInUse),

hscontrol/api/v1/policy.go

@@ -9,7 +9,6 @@ import (
 	"time"
 
 	"github.com/danielgtaylor/huma/v2"
-	policyv2 "github.com/juanfont/headscale/hscontrol/policy/v2"
 	"github.com/juanfont/headscale/hscontrol/types"
 	"github.com/juanfont/headscale/hscontrol/util"
 )
@@ -116,37 +115,11 @@ func registerPolicy(api huma.API, b Backend) {
 			)
 		}
 
-		p := in.Body.Policy
-
-		// Reject policy that would fail when building a map response. SSH rule
-		// validation needs a node, so a server with no nodes can't catch every
-		// case here.
-		nodes := b.State.ListNodes()
-
-		_, err := b.State.SetPolicy([]byte(p))
-		if err != nil {
-			return nil, huma.Error400BadRequest("setting policy", err)
-		}
-
-		if nodes.Len() > 0 {
-			_, err = b.State.SSHPolicy(nodes.At(0))
-			if err != nil {
-				return nil, huma.Error400BadRequest("verifying SSH rules", err)
-			}
-		}
-
-		updated, err := b.State.SetPolicyInDB(p)
-		if err != nil {
-			return nil, huma.Error500InternalServerError("setting policy", err)
-		}
-
-		// Reload even when content is unchanged: routes manually disabled before
-		// may now qualify for auto-approval, so they must be re-evaluated.
-		cs, err := b.State.ReloadPolicy()
+		updated, cs, err := b.State.UpdatePolicy([]byte(in.Body.Policy))
 		b.Change(cs...)
 
 		if err != nil {
-			return nil, huma.Error500InternalServerError("reloading policy", err)
+			return nil, mapError("setting policy", err)
 		}
 
 		out := &setPolicyOutput{}
@@ -165,23 +138,9 @@ func registerPolicy(api huma.API, b Backend) {
 		Tags:        []string{"Policy"},
 		Security:    bearerAuth,
 	}, func(ctx context.Context, in *checkPolicyInput) (*checkPolicyOutput, error) {
-		polB := []byte(in.Body.Policy)
-
-		users, err := b.State.ListAllUsers()
+		err := b.State.ValidatePolicy([]byte(in.Body.Policy))
 		if err != nil {
-			return nil, huma.Error500InternalServerError("loading users", err)
-		}
-
-		nodes := b.State.ListNodes()
-
-		pm, err := policyv2.NewPolicyManager(polB, users, nodes)
-		if err != nil {
-			return nil, huma.Error400BadRequest(err.Error(), err)
-		}
-
-		_, err = pm.SetPolicy(polB)
-		if err != nil {
-			return nil, huma.Error400BadRequest(err.Error(), err)
+			return nil, mapError(err.Error(), err)
 		}
 
 		return &checkPolicyOutput{}, nil

hscontrol/api/v2/acl.go

@@ -113,31 +113,11 @@ func registerACL(api huma.API, b Backend) {
 			}
 		}
 
-		// Mirror the v1 setPolicy flow: validate, SSH-check, persist, reload.
-		nodes := b.State.ListNodes()
-
-		_, err = b.State.SetPolicy(in.RawBody)
-		if err != nil {
-			return nil, huma.Error400BadRequest("setting policy", err)
-		}
-
-		if nodes.Len() > 0 {
-			_, err = b.State.SSHPolicy(nodes.At(0))
-			if err != nil {
-				return nil, huma.Error400BadRequest("verifying SSH rules", err)
-			}
-		}
-
-		updated, err := b.State.SetPolicyInDB(string(in.RawBody))
-		if err != nil {
-			return nil, huma.Error500InternalServerError("setting policy", err)
-		}
-
-		cs, err := b.State.ReloadPolicy()
+		updated, cs, err := b.State.UpdatePolicy(in.RawBody)
 		b.Change(cs...)
 
 		if err != nil {
-			return nil, huma.Error500InternalServerError("reloading policy", err)
+			return nil, mapError("setting policy", err)
 		}
 
 		return streamPolicy([]byte(updated.Data), aclContentType(in.Accept)), nil

hscontrol/api/v2/errors.go

@@ -79,6 +79,10 @@ func mapError(msg string, err error) error {
 		errors.Is(err, state.ErrRequestedTagsInvalidOrNotPermitted):
 		return huma.Error400BadRequest(msg, err)
 
+	// A refused policy is answered with its own reason as the message.
+	case errors.Is(err, state.ErrInvalidPolicy):
+		return huma.Error400BadRequest(err.Error(), err)
+
 	default:
 		return huma.Error500InternalServerError(msg, err)
 	}

hscontrol/apiv1_policy_test.go

@@ -97,6 +97,29 @@ func TestAPIV1PolicySet(t *testing.T) {
 		assert.NotEmpty(t, got.UpdatedAt)
 	})
 
+	t.Run("persist failure keeps live policy", func(t *testing.T) {
+		h := newAPIV1Harness(t)
+
+		body, err := json.Marshal(map[string]string{"policy": `{"acls":[]}`})
+		require.NoError(t, err)
+		require.Equal(t, http.StatusOK,
+			h.callHuma(http.MethodPut, "/api/v1/policy", body).status)
+
+		before, _ := h.app.state.Filter()
+
+		require.NoError(t, h.app.state.DB().DB.
+			Exec("ALTER TABLE policies RENAME TO policies_away").Error)
+
+		body, err = json.Marshal(map[string]string{"policy": validPolicy})
+		require.NoError(t, err)
+
+		res := h.callHuma(http.MethodPut, "/api/v1/policy", body)
+		assertStatus(t, res, http.StatusInternalServerError)
+
+		after, _ := h.app.state.Filter()
+		assert.Equal(t, before, after, "an unstored policy must not be enforced")
+	})
+
 	t.Run("empty rejected", func(t *testing.T) {
 		h := newAPIV1Harness(t)
 

hscontrol/apiv2_acl_test.go

@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"crypto/sha256"
 	"encoding/hex"
+	"encoding/json"
 	"errors"
 	"net/http"
 	"net/http/httptest"
@@ -190,9 +191,21 @@ func TestAPIv2ACLInvalidPolicyAtomicity(t *testing.T) {
 	goodETag := good.Header().Get("Etag")
 
 	// Malformed body -> 400, and the stored policy is unchanged (atomicity).
-	bad := setACL(t, api, `{ this is not valid`, "application/json")
+	const malformed = `{ this is not valid`
+
+	bad := setACL(t, api, malformed, "application/json")
 	assert.Equal(t, http.StatusBadRequest, bad.Code)
-	assert.Contains(t, bad.Body.String(), `"message"`)
+
+	var answer struct {
+		Message string `json:"message"`
+	}
+	require.NoError(t, json.Unmarshal(bad.Body.Bytes(), &answer))
+
+	reason := app.state.ValidatePolicy([]byte(malformed))
+	require.Error(t, reason)
+	assert.Equal(t, reason.Error(), answer.Message,
+		"the answer gives the parser's reason as it is")
+	assert.NotContains(t, answer.Message, "invalid policy")
 	assert.JSONEq(t, allowAllPolicy, storedPolicy(t, app))
 
 	// Wire-level: GET still serves the good policy + its ETag (no drift).
@@ -201,6 +214,25 @@ func TestAPIv2ACLInvalidPolicyAtomicity(t *testing.T) {
 	assert.JSONEq(t, allowAllPolicy, storedPolicy(t, app))
 }
 
+func TestAPIv2ACLPersistFailureKeepsLivePolicy(t *testing.T) {
+	app := createTestApp(t)
+	api := registerAPIV2(t, app)
+
+	require.Equal(t, http.StatusOK,
+		setACL(t, api, `{"acls":[]}`, "application/json").Code)
+
+	before, _ := app.state.Filter()
+
+	require.NoError(t,
+		app.state.DB().DB.Exec("ALTER TABLE policies RENAME TO policies_away").Error)
+
+	assert.Equal(t, http.StatusInternalServerError,
+		setACL(t, api, allowAllPolicy, "application/json").Code)
+
+	after, _ := app.state.Filter()
+	assert.Equal(t, before, after, "an unstored policy must not be enforced")
+}
+
 func TestAPIv2ACLNonDefaultTailnet404(t *testing.T) {
 	api := registerAPIV2(t, createTestApp(t))
 

hscontrol/state/persist_test.go

@@ -1314,6 +1314,33 @@ func TestReloadPolicyOverlappingWrites(t *testing.T) {
 	assert.Equal(t, want, got, "the policy in force must be the one stored last")
 }
 
+// TestUpdatePolicyStoreFailure refuses the write at the database and checks
+// the live policy was not changed by it.
+func TestUpdatePolicyStoreFailure(t *testing.T) {
+	_, s, _ := persistTestSetup(t)
+	t.Cleanup(func() { _ = s.Close() })
+
+	const (
+		open = `{"acls": [{"action": "accept", "src": ["*"], "dst": ["*:*"]}]}`
+		ssh  = `{"acls": [{"action": "accept", "src": ["persist-user@"], "dst": ["persist-user@:22"]}]}`
+	)
+
+	_, _, err := s.UpdatePolicy([]byte(ssh))
+	require.NoError(t, err)
+
+	before, _ := s.Filter()
+
+	require.NoError(t,
+		s.db.DB.Exec("ALTER TABLE policies RENAME TO policies_away").Error)
+
+	_, _, err = s.UpdatePolicy([]byte(open))
+	require.Error(t, err)
+	require.NotErrorIs(t, err, ErrInvalidPolicy)
+
+	after, _ := s.Filter()
+	assert.Equal(t, before, after, "an unstored policy must not be installed")
+}
+
 // TestReloadPolicyEmpty: with no policy configured a reload has nothing to
 // install; an emptied policy file is refused. Both keep the live policy.
 func TestReloadPolicyEmpty(t *testing.T) {

hscontrol/state/state.go

@@ -67,6 +67,17 @@ const (
 // ErrUnsupportedPolicyMode is returned for invalid policy modes. Valid modes are "file" and "db".
 var ErrUnsupportedPolicyMode = errors.New("unsupported policy mode")
 
+// ErrInvalidPolicy is returned by [State.UpdatePolicy] and
+// [State.ValidatePolicy] for a policy they refuse.
+var ErrInvalidPolicy = errors.New("invalid policy")
+
+// invalidPolicyError marks a refusal as [ErrInvalidPolicy] and keeps the
+// reason's own text, which API answers show as is.
+type invalidPolicyError struct{ reason error }
+
+func (e invalidPolicyError) Error() string   { return e.reason.Error() }
+func (e invalidPolicyError) Unwrap() []error { return []error{ErrInvalidPolicy, e.reason} }
+
 // ErrNodeNotFound is returned when a node cannot be found by its ID.
 var ErrNodeNotFound = errors.New("node not found")
 
@@ -1404,7 +1415,8 @@ func (s *State) NodeCanHaveTag(node types.NodeView, tag string) bool {
 	return s.polMan.NodeCanHaveTag(node, tag)
 }
 
-// SetPolicy updates the policy configuration.
+// SetPolicy installs pol without storing it. Policy writes use
+// [State.UpdatePolicy].
 func (s *State) SetPolicy(pol []byte) (bool, error) {
 	changed, err := s.polMan.SetPolicy(pol)
 	if err != nil {
@@ -1466,6 +1478,61 @@ func (s *State) SetPolicyInDB(data string) (*types.Policy, error) {
 	return s.db.SetPolicy(data)
 }
 
+// ValidatePolicy reports whether [State.UpdatePolicy] would accept pol. It
+// compiles pol against the current users and nodes on a throwaway policy
+// manager; the live policy is not touched.
+func (s *State) ValidatePolicy(pol []byte) error {
+	users, err := s.ListAllUsers()
+	if err != nil {
+		return fmt.Errorf("loading users: %w", err)
+	}
+
+	nodes := s.ListNodes()
+
+	pm, err := policy.NewPolicyManager(nil, users, nodes)
+	if err != nil {
+		return fmt.Errorf("creating policy manager: %w", err)
+	}
+
+	_, err = pm.SetPolicy(pol)
+	if err != nil {
+		return invalidPolicyError{err}
+	}
+
+	// SSH rules compile per node, so a server without nodes cannot catch
+	// every case here.
+	if nodes.Len() > 0 {
+		_, err = pm.SSHPolicy(s.cfg.ServerURL, nodes.At(0))
+		if err != nil {
+			return invalidPolicyError{fmt.Errorf("verifying SSH rules: %w", err)}
+		}
+	}
+
+	return nil
+}
+
+// UpdatePolicy validates, stores and installs pol. The live policy is only
+// ever installed from what was stored, so a refused or unstored write leaves
+// it untouched. The changes are returned alongside a reload error, as from
+// [State.ReloadPolicy].
+func (s *State) UpdatePolicy(pol []byte) (*types.Policy, []change.Change, error) {
+	err := s.ValidatePolicy(pol)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	stored, err := s.db.SetPolicy(string(pol))
+	if err != nil {
+		return nil, nil, fmt.Errorf("storing policy: %w", err)
+	}
+
+	// Reload even when content is unchanged: routes manually disabled before
+	// may now qualify for auto-approval, so they must be re-evaluated.
+	cs, err := s.ReloadPolicy()
+
+	return stored, cs, err
+}
+
 // GetNodePrimaryRoutes returns the primary routes for a node.
 func (s *State) GetNodePrimaryRoutes(nodeID types.NodeID) []netip.Prefix {
 	return s.nodeStore.PrimaryRoutesForNode(nodeID)