From c93a3f23d5973829bacad298b85582224e8556fc Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Thu, 23 Jul 2026 15:24:05 +0530 Subject: [PATCH 1/2] fix(reconcile): validate the permission namespace charset at plan time The permission slug is service_resource_verb joined with "_", but the namespace charset was never checked, only the verb. So a namespace with an underscore in a part (resource_order/item) flattened to the same slug as a differently-spelled one (resource/order_item) and was silently treated as already present: the diff planned zero ops while the desired state differed (rule 2), and nothing ever detected it (rule 4). Add IsValidPermissionNamespace (each of service/resource lowercase alphanumeric, no underscore) and call it from validatePermissionSpec, so an ambiguous namespace fails the plan up front. Reconcile-side only; the rule sits inside SpiceDB's object-type grammar, so it never rejects a namespace the server could store, except the ambiguous underscore ones the fix targets. --- internal/bootstrap/schema/schema.go | 24 +++++++++++++ internal/bootstrap/schema/schema_test.go | 30 ++++++++++++++++ internal/reconcile/permission.go | 7 ++++ .../reconcile/permission_reconciler_test.go | 12 +++++++ internal/reconcile/permission_test.go | 36 ++++++++++++++----- 5 files changed, 101 insertions(+), 8 deletions(-) diff --git a/internal/bootstrap/schema/schema.go b/internal/bootstrap/schema/schema.go index 60b52fd0b..df14b0f5b 100644 --- a/internal/bootstrap/schema/schema.go +++ b/internal/bootstrap/schema/schema.go @@ -311,6 +311,30 @@ func IsValidPermissionName(name string) bool { return true } +// IsValidPermissionNamespace checks that a custom permission namespace is in +// service/resource form with each part lowercase alphanumeric. An underscore is +// forbidden inside a part: FQPermissionNameFromNamespace joins service, resource, +// and the verb with "_", so an underscore in a part would let two different +// namespaces flatten to the same slug. Uppercase is forbidden because SpiceDB +// object type names are lowercase, so it could never be stored anyway. +func IsValidPermissionNamespace(namespace string) bool { + parts := strings.Split(namespace, "/") + if len(parts) != 2 { + return false + } + for _, part := range parts { + if part == "" { + return false + } + for _, r := range part { + if !((r >= 'a' && r <= 'z') || (r >= '0' && r <= '9')) { + return false + } + } + } + return true +} + func IsPlatformPermission(name string) bool { name = strings.ToLower(name) return name == PlatformSudoPermission || name == PlatformCheckPermission diff --git a/internal/bootstrap/schema/schema_test.go b/internal/bootstrap/schema/schema_test.go index deeb980d9..6b75057cd 100644 --- a/internal/bootstrap/schema/schema_test.go +++ b/internal/bootstrap/schema/schema_test.go @@ -50,6 +50,36 @@ func TestFQPermissionNameFromNamespace(t *testing.T) { } } +func TestIsValidPermissionNamespace(t *testing.T) { + tests := []struct { + ns string + want bool + }{ + {"resource/aoi", true}, + {"user/project", true}, + {"org/user", true}, + {"compute/disk", true}, + {"a1/b2", true}, + {"resource_order/item", false}, // underscore in a part collides on the slug + {"resource/order_item", false}, + {"Compute/order", false}, // uppercase is not a SpiceDB object type + {"compute/Order", false}, + {"compute", false}, // one part + {"a/b/c", false}, // three parts + {"/x", false}, // empty part + {"x/", false}, // empty part + {"", false}, // empty + {"comp-ute/x", false}, // hyphen is not alphanumeric + } + for _, tt := range tests { + t.Run(tt.ns, func(t *testing.T) { + if got := schema.IsValidPermissionNamespace(tt.ns); got != tt.want { + t.Errorf("IsValidPermissionNamespace(%q) = %v, want %v", tt.ns, got, tt.want) + } + }) + } +} + func TestIsBootstrapServiceUser(t *testing.T) { tests := []struct { name string diff --git a/internal/reconcile/permission.go b/internal/reconcile/permission.go index 33616efa9..bbbafe9b6 100644 --- a/internal/reconcile/permission.go +++ b/internal/reconcile/permission.go @@ -48,6 +48,13 @@ func validatePermissionSpec(s PermissionSpec) error { if parts := strings.Split(s.Namespace, "/"); len(parts) != 2 || parts[0] == "" || parts[1] == "" { return fmt.Errorf("namespace %q must be in service/resource form", s.Namespace) } + // The slug joins service, resource, and verb with "_", so an underscore inside + // a namespace part would make two different namespaces flatten to the same slug + // and be silently treated as one. Require each part to be lowercase alphanumeric + // so the slug is one-to-one and a plan cannot mistake one namespace for another. + if !schema.IsValidPermissionNamespace(s.Namespace) { + return fmt.Errorf("invalid namespace %q (each of service/resource must be lowercase alphanumeric, no underscore)", s.Namespace) + } return nil } diff --git a/internal/reconcile/permission_reconciler_test.go b/internal/reconcile/permission_reconciler_test.go index 1724b5d01..7e53c8928 100644 --- a/internal/reconcile/permission_reconciler_test.go +++ b/internal/reconcile/permission_reconciler_test.go @@ -77,6 +77,18 @@ func TestPermissionReconciler(t *testing.T) { assert.Empty(t, api.created) }) + t.Run("an ambiguous namespace fails Validate before any server call", func(t *testing.T) { + // The charset check is server-free, so a namespace that would collide on the + // slug must fail the whole file up front (rule 3), not at apply. + api := &fakePermissionAPI{} + spec := []byte("- {namespace: resource_order/item, name: get}\n") + + err := NewPermissionReconciler(api, "").Validate(spec) + + assert.ErrorContains(t, err, "resource_order/item") + assert.Empty(t, api.created) + }) + t.Run("reconciling an exported document plans no changes", func(t *testing.T) { api := &fakePermissionAPI{perms: []*frontierv1beta1.Permission{ permissionPB("b1", "app/project", "get"), diff --git a/internal/reconcile/permission_test.go b/internal/reconcile/permission_test.go index ade1237e6..3c442e35c 100644 --- a/internal/reconcile/permission_test.go +++ b/internal/reconcile/permission_test.go @@ -63,14 +63,34 @@ func TestDiffPermissions(t *testing.T) { assert.ErrorContains(t, err, "listed both with and without delete") }) - t.Run("distinct entries that flatten to the same slug fail", func(t *testing.T) { - _, err := diffPermissions([]PermissionSpec{ - {Namespace: "compute/order", Name: "get"}, - {Namespace: "compute/order", Name: "legacy"}, - {Namespace: "resource/order_item", Name: "get"}, - {Namespace: "resource_order/item", Name: "get"}, - }, current) - assert.ErrorContains(t, err, `collide on the same slug "resource_order_item_get"`) + t.Run("a file namespace that would collide with a server slug is rejected, not absorbed", func(t *testing.T) { + // The server stores resource/order_item; the file lists a genuinely different + // namespace resource_order/item that flattens to the same slug. The diff used + // to treat it as already present and plan zero ops (the rule 2 gap). The + // ambiguous namespace is now rejected at validation, so it cannot be absorbed. + server := []currentPermission{{ID: "p1", Namespace: "resource/order_item", Name: "get"}} + _, err := diffPermissions([]PermissionSpec{{Namespace: "resource_order/item", Name: "get"}}, server) + assert.ErrorContains(t, err, "resource_order/item") + }) + + t.Run("rejects a namespace with an underscore or uppercase in a part", func(t *testing.T) { + // The slug joins service, resource, and verb with "_", so an underscore in a + // part makes two namespaces flatten to one slug; uppercase cannot be a + // SpiceDB object type. Both are rejected so the slug stays one-to-one. + for _, ns := range []string{"resource_order/item", "resource/order_item", "Compute/order", "compute/Order"} { + _, err := diffPermissions([]PermissionSpec{{Namespace: ns, Name: "get"}}, nil) + if assert.Error(t, err, ns) { + assert.ErrorContains(t, err, "namespace") + } + } + }) + + t.Run("accepts valid custom namespaces", func(t *testing.T) { + for _, ns := range []string{"resource/aoi", "user/project", "org/user", "compute/disk"} { + ops, err := diffPermissions([]PermissionSpec{{Namespace: ns, Name: "get"}}, nil) + assert.NoError(t, err, ns) + assert.Len(t, ops, 1) // a valid new permission plans an add + } }) t.Run("rejects base-schema namespaces and bad shapes", func(t *testing.T) { From 319ad0f22c3acde5638dcb865ff20ae8a43de4c1 Mon Sep 17 00:00:00 2001 From: Rohil Surana Date: Thu, 23 Jul 2026 15:32:50 +0530 Subject: [PATCH 2/2] chore(reconcile): use a regex for the namespace check and shorten the error Per review: IsValidPermissionNamespace is now a single regex (^[a-z0-9]+/[a-z0-9]+$) instead of a split plus rune loop, and the plan-time error drops the redundant "no underscore" (lowercase alphanumeric already excludes it). --- internal/bootstrap/schema/schema.go | 33 +++++++++++------------------ internal/reconcile/permission.go | 2 +- 2 files changed, 13 insertions(+), 22 deletions(-) diff --git a/internal/bootstrap/schema/schema.go b/internal/bootstrap/schema/schema.go index df14b0f5b..475065085 100644 --- a/internal/bootstrap/schema/schema.go +++ b/internal/bootstrap/schema/schema.go @@ -4,6 +4,7 @@ import ( _ "embed" "errors" "fmt" + "regexp" "strings" "github.com/google/uuid" @@ -311,28 +312,18 @@ func IsValidPermissionName(name string) bool { return true } -// IsValidPermissionNamespace checks that a custom permission namespace is in -// service/resource form with each part lowercase alphanumeric. An underscore is -// forbidden inside a part: FQPermissionNameFromNamespace joins service, resource, -// and the verb with "_", so an underscore in a part would let two different -// namespaces flatten to the same slug. Uppercase is forbidden because SpiceDB -// object type names are lowercase, so it could never be stored anyway. +// permissionNamespaceRe matches a custom permission namespace: service/resource +// with each part one or more lowercase alphanumeric characters. It forbids the +// underscore inside a part, because FQPermissionNameFromNamespace joins service, +// resource, and the verb with "_", so an underscore in a part would let two +// different namespaces flatten to the same slug. Uppercase is forbidden because +// SpiceDB object type names are lowercase, so it could never be stored anyway. +var permissionNamespaceRe = regexp.MustCompile(`^[a-z0-9]+/[a-z0-9]+$`) + +// IsValidPermissionNamespace reports whether namespace is in service/resource +// form with each part lowercase alphanumeric. func IsValidPermissionNamespace(namespace string) bool { - parts := strings.Split(namespace, "/") - if len(parts) != 2 { - return false - } - for _, part := range parts { - if part == "" { - return false - } - for _, r := range part { - if !((r >= 'a' && r <= 'z') || (r >= '0' && r <= '9')) { - return false - } - } - } - return true + return permissionNamespaceRe.MatchString(namespace) } func IsPlatformPermission(name string) bool { diff --git a/internal/reconcile/permission.go b/internal/reconcile/permission.go index bbbafe9b6..bce219234 100644 --- a/internal/reconcile/permission.go +++ b/internal/reconcile/permission.go @@ -53,7 +53,7 @@ func validatePermissionSpec(s PermissionSpec) error { // and be silently treated as one. Require each part to be lowercase alphanumeric // so the slug is one-to-one and a plan cannot mistake one namespace for another. if !schema.IsValidPermissionNamespace(s.Namespace) { - return fmt.Errorf("invalid namespace %q (each of service/resource must be lowercase alphanumeric, no underscore)", s.Namespace) + return fmt.Errorf("invalid namespace %q (each of service/resource must be lowercase alphanumeric)", s.Namespace) } return nil }