fix(reconcile): validate the permission namespace charset at plan time#1797
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds strict validation for custom permission namespaces, requiring exactly two lowercase alphanumeric components separated by ChangesPermission namespace validation
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 29997795583Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage increased (+0.008%) to 46.682%Details
Uncovered ChangesNo uncovered changes found. Coverage Regressions2 previously-covered lines in 1 file lost coverage.
Coverage Stats
💛 - Coveralls |
… 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).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/reconcile/permission_reconciler_test.go (1)
80-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise the orchestration path for the side-effect guarantee.
Calling
Validatedirectly guarantees that no API method is reached, soapi.createddoes not prove the advertised “before any server call” behavior. ExerciseRunor the full reconciliation path with fake API call counters and assert that invalid input causes zero server calls.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 60bcd133-1920-413d-af7b-f7ffed07d43f
📒 Files selected for processing (5)
internal/bootstrap/schema/schema.gointernal/bootstrap/schema/schema_test.gointernal/reconcile/permission.gointernal/reconcile/permission_reconciler_test.gointernal/reconcile/permission_test.go
2 participants
What
Closes the Permission kind's Rule 2 and Rule 4 gaps on the rules-v2 audit. Reconcile-side only, one commit.
The gap
A permission's slug is
service_resource_verb, built by joining the three parts with_(schema.FQPermissionNameFromNamespace). The verb was charset-checked but the namespace was not, and_is legal inside a namespace part. So the map from{namespace, name}to slug was not one-to-one:resource_order/item+getandresource/order_item+getboth flatten toresource_order_item_get.The diff keys on that slug. So a file entry whose namespace differs from a stored one but collides on the slug was treated as already present: the plan showed zero ops while the desired state actually differed (Rule 2), and because nothing ever detected the mismatch it never self-corrected on re-run (Rule 4).
The fix
schema.IsValidPermissionNamespace: the namespace must beservice/resourcewith each part lowercase alphanumeric, no underscore.validatePermissionSpec(server-free), so it runs in bothValidate(whole file, before any apply) and the diff. An ambiguous namespace now fails the plan up front with a clear message.Forbidding underscore-in-a-part makes the slug one-to-one, which closes both cells at once. The rule sits inside SpiceDB's own object-type grammar (lowercase, and although SpiceDB allows
_, Frontier's slug scheme cannot represent it unambiguously), so it never rejects a namespace the server could actually store, apart from the ambiguous underscore ones the fix targets.Scope decisions
CreatePermissionstill has no namespace charset check, so a direct API call could create an ambiguous namespace outside the reconcile flow; adding the same check there is a reasonable follow-up but is out of scope here.Exportwill emit it and the newValidatewill reject re-reconciling it. That is intended: the loud failure names the offending namespace so an operator renames it (rename = delete + create, per Rule 1). No silent absorption.Testing
resource/aoi,user/project,org/user,compute/disk) still pass, the check fails inValidatebefore any server call, and a direct unit test forIsValidPermissionNamespace.go build,go vet,go test ./internal/reconcile/... ./internal/bootstrap/schema/...,gofmt, andgolangci-lintall pass.Status
Closes the last two audit cells (Permission Rules 2 and 4). Targets
main.