sec(iac): scope Azure purchase role to onboarded subscription

[cristim]

Problem

arm/CUDly-CrossSubscription/template.json is deployed by customers into their own Azure tenants. Alongside the intended subscription-scope assignment it declared two grants that reached past the subscription being onboarded:

1. A second roleAssignment with "scope": "/providers/Microsoft.Capacity" — as an absolute path that denotes tenant-wide reservation orders.
2. /providers/Microsoft.Capacity in the custom role definition's assignableScopes — declaring the role eligible for assignment at tenant scope by anyone who can create role assignments there.

Verified live on current origin/main (887d51fd6), at template.json:64-67 and :86-100.

What the grant actually did

Worth recording, because it is narrower than the template reads. Running az deployment sub validate on the pre-fix template resolves the tenant assignment to:

/subscriptions/<subId>/providers/providers/Microsoft.Capacity/providers/Microsoft.Authorization/roleAssignments/<guid>

Note the doubled providers/providers. In a subscription-scoped deployment ARM appends the scope value beneath the subscription rather than treating it as absolute, so the likely apply-time outcome is a malformed target rather than a live tenant-wide grant. That has not been confirmed either way without applying to a real tenant, so this treats the grant as possibly live and tells operators to verify and revoke. Item (2) is a real widening regardless of how (1) resolved.

Before / after

	Before	After
assignableScopes	/subscriptions/{id} + /providers/Microsoft.Capacity	/subscriptions/{id} only
role assignments	3 subscription-scoped + 1 at /providers/Microsoft.Capacity	3 subscription-scoped

In words: the service principal was declared able to act on reservation orders across the whole Azure AD tenant, including subscriptions the customer never onboarded; it can now act only on the single subscription that az deployment sub create targeted.

Is a tenant-level grant genuinely required? No.

Evidence, all in-repo:

• iac/federation/azure-target/terraform/main.tf:77-83 — the production customer onboarding path assigns only at /subscriptions/${local.subscription_id} and works.
• terraform/modules/iam/azure/cudly-reservation-role/main.tf:58-71 — documents the tenant-root scope as deliberately removed, with include_capacity_provider_scope defaulting to false.
• known-issues.md records the origin: the tenant assignment was a workaround for the built-in Reservation Reader role not existing in every tenant. The custom role added by Azure ARM template grants role missing purchase/calculatePrice actions -- all reservation purchases 403 #731 removed that need; the assignment was left behind.
• Azure authorises a reservation purchase against the subscription named in the request body's billingScopeId, not against the tenant-level Microsoft.Capacity path.

So this was drift, not a requirement. Rather than adding an opt-in flag to the template, a tenant-wide grant is documented in known-issues.md as a separate, manual, explicitly consented az role assignment create step — never a default.

Fail-closed

There is no scope parameter at all. Every scope derives from subscription().subscriptionId, the deployment target itself. There is no caller-supplied scope string to omit, widen, or inject, so there is no validation to bypass, which is strictly stronger than validating a scope parameter. The guard now enforces exactly this, rather than merely checking the string looks subscription-shaped (see Regression guard).

Regression guard

scripts/check-azure-role-parity.sh compared only the actions lists, which is exactly why this drift stayed green in CI: both files agreed on actions while disagreeing on scope. It now also asserts scope.

Review of the first version of that guard found eight reproducible ways to reintroduce the tenant-wide grant while keeping it green. All are fixed in the second commit. This is a CI drift guard, not a security boundary (anyone who can edit the template can edit the script), so it is tuned against accidental reintroduction:

#	Bypass	Fix
1	/providers/microsoft.capacity lowercased — Azure namespaces are case-insensitive, bash =~ is not	match under shopt -s nocasematch
2	assignment hidden in a nested Microsoft.Resources/deployments, or nested in a parent resource, or type in different casing	recursive .. walk, ascii_downcase on type, and refuse nested deployments outright
3	variables.tf absent — the default-false assertion silently skipped	referenced flag whose default cannot be read is now an error
4	[concat('/subscriptions/', parameters('otherSubscriptionId'))] — passes a substring anchor, grants in another subscription	exact-match allowlist
5	$schema repointed at the management-group template, widening every scope-less assignment without touching a scope string	$schema pinned to subscriptionDeploymentTemplate

Correcting a claim in the first version of this PR. It said the guard was "allowlist-anchored (accepted only if it visibly contains /subscriptions/)". That is what the code did, and it is much weaker than it reads: case 4 above satisfies it, in a template named CrossSubscription. The allowlist is now genuinely exact-match — only the canonical expression bound to the deployment target, or a bare literal /subscriptions/<guid> for fixtures — and role assignments must carry no explicit scope at all. The token denylist is retained as defence in depth, and the in-code comment no longer overstates what is enforced.

Self-tests go from 2 cases to 9, one per bypass. Each was checked to fail for its own reason rather than incidentally:

PASS: matching lists exit 0                     PASS: other-subscription ARM exits 1
PASS: drifted ARM exits 1                       PASS: nested-deployment ARM exits 1
PASS: tenant-scope ARM exits 1                  PASS: management-group schema ARM exits 1
PASS: obfuscated tenant-scope ARM exits 1       PASS: TF flag without variables.tf exits 1
PASS: lowercase tenant-scope ARM exits 1
Results: 9 passed, 0 failed.

No workflow file was touched: the existing azure-role-parity CI job already runs both scripts.

Regression guard, round 2

A second, independent adversarial review defeated the guard above three more ways, despite its 9-case suite passing and the guard passing on the real files:

#	Bypass	Fix
F1	A bare literal /subscriptions/<guid> was accepted by GUID shape alone (never by value), so a template hard-coding a foreign subscription passed — including in the deployable shape (canonical expression retained, foreign literal appended to the same array). nocasematch was also live across that regex, so an uppercase GUID passed too.	Literals are no longer accepted at all, of any case. Only the canonical ARM expression, or its [subscription().id] equivalent, is accepted.
F3	The actions extractor matched only the top-level .resources[] array with a case-sensitive ==, while the scope walk was recursive and case-insensitive. A second role definition typed microsoft.authorization/roleDefinitions (lowercase) granting actions: ["*"] was invisible to the actions axis — the guard printed OK: ARM and TF actions lists match and correctly counted the extra role on the scope axis, proving the two axes disagreed about how many role definitions existed.	Actions extraction now uses the same recursive, case-insensitive selector as the scope walk, unioned across every matched role definition.
F4	Only permissions[0].actions was compared. ARM unions every permissions[] entry, so a second entry appended after the canonical one (e.g. {"actions": ["*"]}) passed. notActions, dataActions, and notDataActions were never compared at all — a first entry with dataActions: ["*"] also passed.	permissions[].actions[] is now flattened across the whole array, and notActions/dataActions/notDataActions are compared the same way actions is.
F2	The scope comparison was byte-exact, so whitespace, quote style (" vs '), a redundant empty-string concat argument, and the [subscription().id] equivalent all failed despite being semantically identical to the canonical expression. A pure reformat could red CI.	Both sides are normalized (whitespace stripped, quote style unified, redundant empty-string arg dropped) before comparing; the accepted spellings are printed in the error message.
F5	Microsoft.Resources/deploymentScripts and Microsoft.Resources/deploymentStacks were not refused, unlike plain deployments — despite a deployment script's runtime az/az-cli commands being able to issue role assignments this check never sees as JSON.	Both resource types are now refused alongside deployments.
F6	A role assignment's roleDefinitionId was unconstrained as long as it carried no explicit scope: a built-in Owner grant correctly inheriting the (subscription-scoped) deployment passed, since only the scope axis was checked.	roleDefinitionId is now allowlisted to the three roles this template ever assigns (the custom purchaser, Reader, Cost Management Reader).

Correcting a claim in the previous round of this PR. The code comment above LITERAL_SUBSCRIPTION_RE (and this PR's own round-1 description, item 4's fix column) said a bare literal /subscriptions/<guid> was accepted "for the test fixtures". That justification was false and was disproved directly: the fixtures were rewritten to use the canonical expression instead, the literal-acceptance branch was deleted, and the full self-test suite still passes. The literal was never structurally required by anything; accepting it just weakened the guard to GUID shape, not GUID value — the exact hole F1 exploits.

Self-tests go from 9 cases to 18, one per bypass across both rounds (nine round-2 cases: two for F1, one for F2, one for F3, two for F4, two for F5, one for F6). Each was reproduced against the pre-fix revision (fc1f5b782) first — confirmed exit 0 (or, for F2, a false-positive exit 1) — and confirmed to close (correct exit code) after the fix:

Results: 18 passed, 0 failed.

shellcheck on both scripts: exit 0 (no findings). bash -n on both scripts: exit 0. Verified against the real arm/CUDly-CrossSubscription/template.json and terraform/modules/iam/azure/cudly-reservation-role/main.tf: still exit 0.

One item deliberately left alone: several malformed-input self-test variants exit 5 (a jq error surfacing through set -e) rather than 1. Still fail-closed, not a defect, and out of scope for this round — normalizing exit codes for that path was explicitly not attempted here.

Regression guard, self-review before round 3

Before handing round 2 to another reviewer, self-checked the two highest-risk things it added — a normalizer (which exists to make different strings equal) and a newly-recursive JSON walk (which exists to see more of the template than before). Found one real issue in each, both fixed here with their own self-test case:

• normalize_scope_expr() blanket-stripped all whitespace, including inside the /subscriptions/ string literal itself, so [concat('/sub scriptions/', subscription().subscriptionId)] (a typo, not an attack) normalized to the same text as the real canonical scope and was wrongly accepted. Not exploitable for escalation — the corrupted literal fails to deploy rather than pointing anywhere else — but a normalizer that can't tell "reformatted" from "corrupted" is the wrong shape of tool. Whitespace is now collapsed only where it is immediately adjacent to structural punctuation ([ ] ( ) ,), which by construction cannot reach inside a string literal's content. Verified against every legitimate spelling from round 2 (still accepted) plus a battery of adversarial inputs — an extra trailing parameters('evil') argument after the empty-string arg, a managementGroup().id suffix, a foreign literal — none of which normalize to canonical.
• The actions extractor, the nested-deployment refusal, and the scope walk all used an unrooted .. over the whole document. A decorative object under variables or outputs that merely happened to carry a roleDefinitions-shaped "type" field — never actually deployed — was treated as a real grant, reproduced directly: a variables.decorativeDocOnly object granting actions: ["*"] at /providers/Microsoft.Capacity caused an otherwise-clean, fully-matching template to fail CI. That can only make the guard fail closed on a correct template, never accept a bad one, but a guard that reds valid input is exactly the pressure that gets a guard deleted or bypassed — which is how the original tenant-wide grant went unnoticed for as long as it did. All three walks are now rooted at .resources, which still finds a resource nested inside another resource's own resources array without ever leaving the tree of things ARM actually deploys.

Also confirmed on request: ALLOWED_ROLE_DEFINITION_IDS's Reader and Cost Management Reader entries are not fixture-driven — the real template.json genuinely assigns both (lines 91 and 103, roleDefinitionId: [variables('roles').reader] / [variables('roles').costManagementReader]) alongside the custom purchaser role, matching exactly the "three roles CUDly assigns" the allowlist claims. The comparison is confirmed exact (a probe battery of prefix/suffix/substring variants of the allowed values all correctly fail); it recognizes only the one raw-expression spelling this template actually uses, so a reformat to a bare GUID or a resourceId()/concat() expression would be rejected rather than silently accepted — a drift-guard tradeoff already called out in the code comment, not a gap.

Self-tests go from 18 to 20. Both reproduced against the prior revision (99ea68759) first — confirmed exit 0 (space-inside-literal, wrongly accepted) and exit 1 (decorative-variables, wrongly rejected) respectively — and confirmed fixed after:

Results: 20 passed, 0 failed.

Regression guard, round 4 (independent review)

Independent review of round 3 (76abcc853) found 8 hostile templates that exit 0. Two fixes close six of them cheaply; the remaining two are filed as follow-up rather than fixed here.

#	Bypass	Fix
Fix A (4 findings)	jq property-key access is case-sensitive; ARM's resource-provider JSON deserializers are documented case-insensitive. .type (a value) was already downcased for exactly this reason, but that reasoning never reached the property keys read from the same JSON. Confirmed live: a roleAssignment with "Scope" (capital S) set to /providers/Microsoft.Capacity — issue #1545 byte-for-byte apart from one capital letter — passed, along with "RoleDefinitionId" on an Owner grant, "Properties" wrapping an Owner grant, and "AssignableScopes" on a second role definition. Each made the node invisible to the check that key feeds — silence, not refusal.	Every object key in the ARM document is lowercased once into a scratch copy before any query runs; every jq field access naming a non-lowercase ARM property (roleDefinitionId, assignableScopes, notActions, dataActions, notDataActions) rewritten to match.
Fix B (2 findings)	Microsoft.Authorization/roleEligibilityScheduleRequests and roleAssignmentScheduleRequests (Azure PIM) genuinely grant a role the same way a plain roleAssignment does — confirmed by constructing one binding built-in Owner — but under a property shape the roleAssignments-only type match never saw, bypassing this PR's own roleDefinitionId allowlist by changing the resource type. Also added: the legacy Microsoft.Storage/storageAccounts/providers/roleAssignments child-type spelling, invisible for the same reason.	All three types refused outright, alongside the existing nested-deployment/deploymentScripts/deploymentStacks refusal.
Fix C	The code comment claimed this check "errs towards refusing anything it cannot reason about" — false for any resource type outside the two lists it recognizes/refuses (which is the entirety of the finding set above).	Rewritten to say what the guard actually does: a fixed recognize-list plus a fixed refuse-list, silent on everything else.

Caveat recorded honestly, per reviewer request: whether ARM itself accepts a miscased property key is Azure-side behaviour that was not independently verified against a live subscription — that would require deploying a miscased template to a real tenant, which was not done. The resource-provider JSON deserializers are documented as case-insensitive by default, which is the basis for fixing this fail-closed regardless: if ARM does accept it, this closes a real hole; if not, the guard is merely redundant with a deploy-time rejection, never wrong.

Filed rather than fixed, per reviewer guidance (the template diff in this PR is the real #1545 remediation; these are separate defense-in-depth guard gaps and shouldn't hold up the fix): #1681, covering (1) the guard constrains which role and where, but never who — a roleAssignment binding the allowed custom role at the correct scope to a hardcoded foreign principalId passes cleanly, money-spending permission handed to an arbitrary principal; and (2) the systemic fix — assert the template's exact expected set of grant tuples (4, today) rather than enumerating everything to refuse, which would close every finding above and future ones in one stroke. Also noted there as low severity: assignableScopes given as a bare string exits 5 via a raw jq error rather than a clean 1 (still fails closed).

Self-tests go from 20 to 27 (seven round-4 cases: four for Fix A, three for Fix B). Each reproduced against the prior revision (76abcc853) first — confirmed exit 0 — and confirmed fixed after:

Results: 27 passed, 0 failed.

shellcheck on both scripts: exit 0. bash -n: exit 0. Verified against the real template and TF module: still exit 0.

Live-Azure confirmation (round 4's Fix A premise): an independent reviewer ran az deployment sub what-if (validate/what-if only, nothing deployed) against a template carrying "Scope" (capital S) on a roleAssignment. ARM resolved it to /providers/Microsoft.Capacity — the miscased key was honoured, not ignored, confirming this is a live Azure behaviour and not a theoretical jq artifact. "Properties" and "AssignableScopes" capitalized were also confirmed to produce plan output byte-identical to the correctly-cased control, meaning nothing warns anyone at plan time either. Fix A closes all of these (see below for confirmation the has() trap — downcasing only the extraction expressions while leaving has("scope")/has("roleDefinitionId") case-sensitive, which would look fixed while staying open — was avoided: the whole document is downcased once via walk() before any query, including the two has() calls, so there was never a partial-fix window).

Regression guard, round 5 (explicit case-folding)

Two comparisons were already case-insensitive in practice, but only as a side effect of running inside the shopt -s nocasematch scope set for the (unrelated, already-documented) ESCAPE_TOKENS regex match: the canonical-scope comparison in normalize_scope_expr, and the ALLOWED_ROLE_DEFINITION_IDS allowlist comparison. Confirmed empirically that this dependency was real (a probe with shopt -u nocasematch correctly stopped matching). That's fragile — moving either comparison outside that scope, or narrowing what nocasematch covers, would silently drop the case-insensitivity with no visible change to the comparison line itself.

Both now fold case explicitly instead: normalize_scope_expr lowercases as an explicit final step, and the roleDefinitionId loop lowercases both sides before comparing — matching the same "ARM identifiers are case-insensitive" reasoning already applied to resource type values via ascii_downcase elsewhere in the script. Safe unconditionally: lowercasing is a 1:1 character transform that can't collapse two distinct values into one, unlike the whitespace-stripping this same function already had to be careful about (round 3).

Self-tests go from 27 to 29: an uppercase-spelled canonical scope expression (CONCAT/SUBSCRIPTION/SUBSCRIPTIONID) and an uppercase-spelled allowed roleDefinitionId ([Variables('Roles').Reader]) are both confirmed still accepted (exit 0) — re-verified the full adversarial battery from round 3 (foreign literals, evil concat args, management-group escapes, whitespace-in-literal) still correctly rejected after adding the lowercasing step.

Results: 29 passed, 0 failed.

Regression guard, round 6 (key-collision bypass)

Independent adversarial review found a live bypass of round 4's key-casing
fix, pre-existing on 99ea68759 (confirmed independently — the same four
fixtures below also exit 0 against that revision, not just the current head)
and not introduced by any change on this branch.

Round 4's fix lowercases every object key once
(with_entries(.key |= ascii_downcase)) so a correctly-recognized resource
with one miscased property key is no longer invisible. But from_entries
(which with_entries is built on) keeps the last entry when two entries
fold to the same key. An object that already carries both case-variant
spellings of a property in the same object — not one miscased key, but two —
collapses to whichever is spelled last, and the discarded value is deleted
before any selector runs, not merely unmatched. A template carrying the
hostile value first and a correctly-spelled, canonical-looking value second
passed clean:

#	Bypass
1	"AssignableScopes": ["/providers/Microsoft.Capacity"] then "assignableScopes": [<canonical>] — issue #1545 itself, reached via key collision instead of a single miscased key.
2	"RoleDefinitionId": <built-in Owner> then "roleDefinitionId": "[variables('roles').reader]" on a roleAssignment's properties object.
3	"Properties": {Owner} then "properties": {Reader} — same shape, one level up, as the two top-level keys of a roleAssignment resource.
4	"Type": "Microsoft.Resources/deployments" then "type": "...roleAssignments" — also evades REFUSED_TYPES: with the Type/deployments evidence deleted, the resource looks like an ordinary, fully-compliant roleAssignment.

Order-dependence confirms the mechanism and rules out a coincidental read of
the finding: the same collision with the two keys reversed
(benign-first, evil-last) already exits 1 today, because the surviving
(evil) value trips the pre-existing tenant-scope check. Only evil-first was
invisible.

Fix: refuse, don't pick a winner. Detects any same-object case-variant
key collision on the raw file, before normalization, and refuses
outright. Which of the two spellings ARM itself honors for a duplicate
property in one JSON object is not verified here (that would require
deploying a miscased template to a live tenant) and is not something this
script can determine from the file alone — unlike the single-miscased-key
case round 4 fixed, where ARM's documented case-insensitive deserialization
justifies treating the miscased spelling as equivalent to the correct one.
If ARM takes the first entry, a hostile value ahead of a benign one is a
live escalation this script was reporting as clean. If ARM takes the last
entry the same way jq does, this script's prior behavior was
coincidentally right rather than correctly reasoned about — and either way,
a template that is ambiguous about its own grants must not be the thing
that decides whether CI is green. The error message says so explicitly,
rather than merely reporting the refusal.

Independent re-verification of all three claims (not taken on trust):

• The bare collision-detection expression returns 0 on the real
  arm/CUDly-CrossSubscription/template.json and on every one of the 28
  pre-existing role-parity fixtures — no false positives.
• It returns 1 on all five new fixtures below.
• All four evil-first fixtures also exit 0 against 99ea68759 (checked out
  and run directly), confirming the bypass predates this branch entirely.

Five new fixtures, one per bypass shape above plus the benign-first control,
each confirmed to fail for its own reason via its printed reason line
(the error names exactly which two keys collided, e.g.
AssignableScopes / assignableScopes vs. Type / type) — not incidentally
alongside an unrelated check.

Self-tests go from 29 to 34:

Results: 34 passed, 0 failed.

shellcheck 0.11.0 on both scripts: exit 0 (no findings). bash -n on both
scripts: exit 0. Verified against the real template: still exit 0, still
OK: all 4 ARM grant scopes/roleDefinitionIds are subscription-anchored.

Sibling paths audited

Checked every path that generates, copies or embeds this grant. Only template.json was affected, so Closes is safe here:

Path	Scope	Status
iac/federation/azure-target/bicep/azure-wif.bicep + .arm.json	subscription	clean
iac/federation/azure-target/terraform/main.tf	subscription	clean
terraform/modules/compute/azure/container-apps/main.tf	subscription	clean
terraform/modules/iam/azure/cudly-reservation-role	flag defaults false	clean (now guarded)
internal/iacfiles/templates/azure-*	no role grants	clean
arm/CUDly-CrossSubscription/setup.sh	deploys template, no extra grants	clean
go:embed sets	template not embedded	n/a

Verification (exit codes)

Check	Exit
az deployment sub validate (post-fix)	0, provisioningState: Succeeded, error: null — all 4 validated resources under /subscriptions/<subId>/
az deployment sub validate (pre-fix control)	0, but produced a 5th resource at the doubled-providers path
jq empty template.json	0
shellcheck (both scripts)	0
check-azure-role-parity.sh on fixed template	0
check-azure-role-parity.sh on pre-fix template	1 (fails on the real bug)
test-azure-role-parity.sh	0 — 4 passed, 0 failed

az bicep decompile on the pre-fix template also emitted BCP036: The property "scope" expected a value of type "resource | tenant" for the tenant assignment; that error is gone post-fix. Remaining BCP034 decompiler warnings on dependsOn are pre-existing and present in both.

Not verified: whether the pre-fix assignment ever resolved to a genuine tenant-scope grant at apply time. Establishing that requires actually applying the template to a live tenant, which was not done.

Action required for existing deployments

Customers who already deployed the earlier template are not fixed by this merge. ARM deployments are incremental, so removing the resource from the template does not revoke anything already created. They must:

1. List assignments at or under /providers/Microsoft.Capacity for the CUDly SP, projecting the assignment id.
2. Delete any found by --ids, before redeploying. Not by --scope: the CLI rejects the malformed doubled-providers shape as an invalid scope, which would leave an operator with a row that is listed but undeletable.
3. Then redeploy the corrected template to narrow assignableScopes.

Order matters: Azure refuses to drop an assignable scope from a role definition while assignments still exist at it. Commands and ordering are in known-issues.md.

Severity

I'd rate this HIGH rather than CRITICAL, agreeing with the original reviewer. The escalation to CRITICAL assumed a silent tenant-wide grant; ARM in fact rewrites the scope subscription-relative.

Review strengthened this: az deployment sub validate does not preflight authorization, but az deployment sub what-if does, and it returns a hard AuthorizationFailed on exactly that resource over the malformed scope. So the doubled-providers path is not a validator artifact: ARM genuinely computes that target and preflight fails on it. Separately, per this repo's own variables.tf:12, declaring an assignable scope above your own subscription 403s for a subscription-scoped principal, so for a normal customer the pre-fix template most likely failed on its first resource. The realistic failure mode is "onboarding breaks", not "silent tenant-wide grant".

It stays high-priority: assignableScopes was genuinely widened, the outcome is unconfirmed for tenant-root-privileged deployers, and it is drift from a deliberately narrowed decision. The fix is identical under either rating. Flagging the judgement rather than relabelling the issue.

Follow-up filed: LeanerCloud/cloud-commitments-platform#143 (unused roleAssignmentGuidPrefix parameter, pre-existing, deliberately kept out of this p0 diff).

Closes #1545

Summary by CodeRabbit

• Bug Fixes

  • Removed unintended tenant-wide capacity permissions from Azure deployments.
  • Restricted custom role scopes and assignments to the target subscription.
  • Blocked broader-scope and unsafe deployment paths.

• Tests

  • Expanded validation for permission parity, scope handling, nested resources, wildcard permissions, and casing variations.
  • Added coverage for tenant, cross-subscription, management-group, and scope-escape scenarios.

• Documentation

  • Added remediation guidance for previously created tenant-wide grants.
  • Documented verification, cleanup, deployment limitations, and resolution steps.

[coderabbitai]

📝 Walkthrough

Walkthrough

The ARM template removes tenant-wide capacity access. Documentation records remediation for prior grants. The parity checker validates four permission lists, subscription-only scopes, deployment structure, role bindings, and adversarial ARM templates.

Changes

Subscription Scope Security and Parity Validation

Layer / File(s)	Summary
Remove tenant-capacity assignment arm/CUDly-CrossSubscription/template.json, known-issues.md	Restricts the custom role to the target subscription, removes the tenant-capacity assignment, and documents cleanup of existing grants.
Expand permission parity extraction scripts/check-azure-role-parity.sh	Normalizes and compares actions, not_actions, data_actions, and not_data_actions between Terraform and ARM.
Enforce deployment scope invariants scripts/check-azure-role-parity.sh	Validates canonical subscription scopes, allowed role bindings, deployment resources, collected grants, and the Terraform capacity-scope default.
Cover scope escape regressions scripts/test-azure-role-parity.sh, scripts/testdata/role-parity/*	Adds cases for tenant and foreign scopes, wildcard permissions, case variants, duplicate keys, nested resources, PIM resources, and unsupported deployment escapes.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant TerraformModule
  participant ParityCheck
  participant ARMTemplate
  participant jq
  TerraformModule->>ParityCheck: Provide permission lists and scope settings
  ARMTemplate->>jq: Provide role definitions and assignments
  jq->>ParityCheck: Return normalized permissions and scopes
  ParityCheck->>ParityCheck: Compare permissions and validate deployment scope
  ParityCheck-->>TerraformModule: Report validation result

Loading

Possibly related PRs

• LeanerCloud/CUDly#732: Modifies the same ARM custom-role scope and RBAC configuration.
• LeanerCloud/CUDly#744: Changes the same ARM role-definition and capacity-provider assignment scopes.
• LeanerCloud/CUDly#761: Introduces the parity checker and test suite extended by this change.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes scoping Azure purchase-role access to the onboarded subscription.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch sec/1545-arm-role-scope

---

_{Comment @coderabbitai help to get the list of available commands.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@known-issues.md`:
- Around line 8-17: Update the historical grant wording in known-issues.md at
lines 8-17 and 139-145 to remain conditional: state that existing deployments
may retain, or the assignment could have granted, tenant-wide reservation
access. Preserve the existing verify-and-revoke remediation guidance at both
sites.
- Around line 56-71: Update the cleanup instructions around `az role assignment
list` to query `/providers/Microsoft.Capacity` directly with `--scope`, so
tenant-level assignments are included. Remove the case-sensitive
`contains(scope, 'Microsoft.Capacity')` filter or replace it with an appropriate
path match. Preserve projecting assignment IDs and deleting listed assignments
via `az role assignment delete --ids` before redeployment.

In `@scripts/test-azure-role-parity.sh`:
- Around line 90-99: Update Case 9 in the test fixture setup to append an actual
Terraform HCL reference to include_capacity_provider_scope, rather than only a
comment. Ensure the generated main.tf contains configuration that the checker
parses as a reference while variables.tf remains absent, preserving the expected
exit status of 1.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: f5cc167c-d78b-4cda-92eb-e458b22f6bf3

📥 Commits

Reviewing files that changed from the base of the PR and between 887d51f and 99ea687.

📒 Files selected for processing (21)

• arm/CUDly-CrossSubscription/template.json
• known-issues.md
• scripts/check-azure-role-parity.sh
• scripts/test-azure-role-parity.sh
• scripts/testdata/role-parity/canonical-scope-variants-arm.json
• scripts/testdata/role-parity/dataactions-wildcard-arm.json
• scripts/testdata/role-parity/deploymentscript-scope-escape-arm.json
• scripts/testdata/role-parity/deploymentstack-scope-escape-arm.json
• scripts/testdata/role-parity/drifted-arm.json
• scripts/testdata/role-parity/foreign-subscription-literal-with-canonical-arm.json
• scripts/testdata/role-parity/lowercase-tenant-scope-arm.json
• scripts/testdata/role-parity/lowercase-type-wildcard-actions-arm.json
• scripts/testdata/role-parity/matching-arm.json
• scripts/testdata/role-parity/mgmt-group-schema-arm.json
• scripts/testdata/role-parity/nested-deployment-arm.json
• scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json
• scripts/testdata/role-parity/other-subscription-arm.json
• scripts/testdata/role-parity/second-permissions-entry-arm.json
• scripts/testdata/role-parity/tenant-scope-arm.json
• scripts/testdata/role-parity/unallowed-roledefinitionid-arm.json
• scripts/testdata/role-parity/uppercase-guid-literal-arm.json

[cristim]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 58 minutes.

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

scripts/test-azure-role-parity.sh (1)

349-352: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Case 34 no longer proves order-dependence.

The collision check in scripts/check-azure-role-parity.sh runs before scope validation and refuses both key orderings. Case 34 now exits 1 at the collision check, not at the tenant-scope check described in the comment at lines 314-318. The control therefore asserts the same code path as Case 30.

Assert the failure reason, for example by matching the checker's stderr, or restate the comment so it does not claim the tenant-scope check is what fires.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/test-azure-role-parity.sh` around lines 349 - 352, Update Case 34 in
run_case so it verifies the intended failure reason by matching the checker’s
stderr for the tenant-scope validation message, or revise its comment to
accurately describe the collision-check failure path; ensure it no longer claims
to prove order-dependence without asserting which check rejects the input.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/check-azure-role-parity.sh`:
- Around line 96-108: Update the jq collision scan in the COLLISION_DETAIL
assignment to begin at the root document object rather than only traversing
.resources, while preserving recursive inspection of nested objects and the
existing collision reporting. Ensure root-level case-variant pairs such as
Resources/resources and $Schema/$schema are detected before normalization.

---

Nitpick comments:
In `@scripts/test-azure-role-parity.sh`:
- Around line 349-352: Update Case 34 in run_case so it verifies the intended
failure reason by matching the checker’s stderr for the tenant-scope validation
message, or revise its comment to accurately describe the collision-check
failure path; ensure it no longer claims to prove order-dependence without
asserting which check rejects the input.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 12e83a2e-ff12-444d-bf9b-6978faf401d7

📥 Commits

Reviewing files that changed from the base of the PR and between 99ea687 and fe46a70.

📒 Files selected for processing (19)

• known-issues.md
• scripts/check-azure-role-parity.sh
• scripts/test-azure-role-parity.sh
• scripts/testdata/role-parity/collision-assignablescopes-benign-first-arm.json
• scripts/testdata/role-parity/collision-assignablescopes-evil-first-arm.json
• scripts/testdata/role-parity/collision-properties-evil-first-arm.json
• scripts/testdata/role-parity/collision-roledefinitionid-evil-first-arm.json
• scripts/testdata/role-parity/collision-type-evil-first-arm.json
• scripts/testdata/role-parity/decorative-variables-arm.json
• scripts/testdata/role-parity/legacy-child-roleassignment-owner-arm.json
• scripts/testdata/role-parity/miscased-assignablescopes-arm.json
• scripts/testdata/role-parity/miscased-properties-arm.json
• scripts/testdata/role-parity/miscased-roledefinitionid-arm.json
• scripts/testdata/role-parity/miscased-scope-arm.json
• scripts/testdata/role-parity/pim-roleassignment-schedule-owner-arm.json
• scripts/testdata/role-parity/pim-roleeligibility-owner-arm.json
• scripts/testdata/role-parity/space-inside-literal-arm.json
• scripts/testdata/role-parity/uppercase-allowed-roledefinitionid-arm.json
• scripts/testdata/role-parity/uppercase-canonical-scope-arm.json

🚧 Files skipped from review as they are similar to previous changes (1)

• known-issues.md

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (3)

scripts/check-azure-role-parity.sh (3)

569-581: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

A trailing comment on the default reds this check.

The gsub at line 573 strips the default = prefix and trailing whitespace only. For default = false # keep tenant scope off, CAPACITY_DEFAULT becomes false # keep tenant scope off, and the comparison at line 579 fails. The reported reason then points at the default value, not at the comment.

Strip an inline comment and any trailing punctuation before comparing.

♻️ Proposed change

       in_var && /^[[:space:]]*default[[:space:]]*=/ {
-        gsub(/^[[:space:]]*default[[:space:]]*=[[:space:]]*|[[:space:]]*$/, "")
+        sub(/^[[:space:]]*default[[:space:]]*=[[:space:]]*/, "")
+        sub(/[[:space:]]*(#|\/\/).*$/, "")
+        gsub(/^[[:space:]]+|[[:space:]]+$/, "")
         print; exit
       }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/check-azure-role-parity.sh` around lines 569 - 581, Update the
CAPACITY_DEFAULT extraction in the awk block for include_capacity_provider_scope
to remove inline comments and trailing punctuation after parsing the default
value, so values such as false # keep tenant scope off normalize to false before
the comparison. Keep the existing error reporting and default validation
unchanged.

---

383-384: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Match the schema case-insensitively for consistency.

The rest of the script folds case before comparing ARM identifiers. Line 383 matches the $schema value case-sensitively. A template that spells the URL with different casing fails this gate with the wrong reason. The direction is fail-closed, so this is not a security gap; it is a consistency gap.

♻️ Proposed change

-if ! jq -e '.["$schema"] | test("subscriptionDeploymentTemplate")' "$ARM_FILE_NORM" >/dev/null 2>&1; then
+if ! jq -e '.["$schema"] | test("subscriptionDeploymentTemplate";"i")' "$ARM_FILE_NORM" >/dev/null 2>&1; then

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/check-azure-role-parity.sh` around lines 383 - 384, Update the
`$schema` validation in the ARM template check around `ARM_FILE_NORM` to perform
a case-insensitive match for `subscriptionDeploymentTemplate`, consistent with
the script’s identifier comparisons, while preserving the existing fail-closed
behavior and `ACTUAL_SCHEMA` reporting.

---

162-175: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Handle valid inline non-empty permission lists.

extract_tf_list skips values from attr = ["a"] and can consume following permission lines, causing false parity drift. The referenced module currently uses supported multiline lists, so this is a future-format risk. Add inline-list handling or use an HCL-aware parser.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/check-azure-role-parity.sh` around lines 162 - 175, Update
extract_tf_list to parse valid inline non-empty permission lists such as attr =
["a"], emitting each value without entering multiline mode. Preserve empty-list
handling and ensure subsequent permission attributes are not consumed as list
items; retain multiline-list parsing for the existing format.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/check-azure-role-parity.sh`:
- Around line 421-433: Update the REFUSED_TYPES filtering used by the jq
expression in REFUSED_TYPE_COUNT so it rejects every type whose normalized value
ends with "/providers/roleassignments", rather than only the listed
microsoft.storage entry. Preserve the existing exact refusals for the other
deployment and scheduling types, and ensure matching remains case-insensitive
through the existing lowercase normalization.

---

Nitpick comments:
In `@scripts/check-azure-role-parity.sh`:
- Around line 569-581: Update the CAPACITY_DEFAULT extraction in the awk block
for include_capacity_provider_scope to remove inline comments and trailing
punctuation after parsing the default value, so values such as false # keep
tenant scope off normalize to false before the comparison. Keep the existing
error reporting and default validation unchanged.
- Around line 383-384: Update the `$schema` validation in the ARM template check
around `ARM_FILE_NORM` to perform a case-insensitive match for
`subscriptionDeploymentTemplate`, consistent with the script’s identifier
comparisons, while preserving the existing fail-closed behavior and
`ACTUAL_SCHEMA` reporting.
- Around line 162-175: Update extract_tf_list to parse valid inline non-empty
permission lists such as attr = ["a"], emitting each value without entering
multiline mode. Preserve empty-list handling and ensure subsequent permission
attributes are not consumed as list items; retain multiline-list parsing for the
existing format.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 3ee75540-522a-481e-aaca-b56b1410abf3

📥 Commits

Reviewing files that changed from the base of the PR and between fe46a70 and ed8e6a3.

📒 Files selected for processing (37)

• arm/CUDly-CrossSubscription/template.json
• known-issues.md
• scripts/check-azure-role-parity.sh
• scripts/test-azure-role-parity.sh
• scripts/testdata/role-parity/canonical-scope-variants-arm.json
• scripts/testdata/role-parity/collision-assignablescopes-benign-first-arm.json
• scripts/testdata/role-parity/collision-assignablescopes-evil-first-arm.json
• scripts/testdata/role-parity/collision-properties-evil-first-arm.json
• scripts/testdata/role-parity/collision-roledefinitionid-evil-first-arm.json
• scripts/testdata/role-parity/collision-type-evil-first-arm.json
• scripts/testdata/role-parity/dataactions-wildcard-arm.json
• scripts/testdata/role-parity/decorative-variables-arm.json
• scripts/testdata/role-parity/deploymentscript-scope-escape-arm.json
• scripts/testdata/role-parity/deploymentstack-scope-escape-arm.json
• scripts/testdata/role-parity/drifted-arm.json
• scripts/testdata/role-parity/foreign-subscription-literal-with-canonical-arm.json
• scripts/testdata/role-parity/legacy-child-roleassignment-owner-arm.json
• scripts/testdata/role-parity/lowercase-tenant-scope-arm.json
• scripts/testdata/role-parity/lowercase-type-wildcard-actions-arm.json
• scripts/testdata/role-parity/matching-arm.json
• scripts/testdata/role-parity/mgmt-group-schema-arm.json
• scripts/testdata/role-parity/miscased-assignablescopes-arm.json
• scripts/testdata/role-parity/miscased-properties-arm.json
• scripts/testdata/role-parity/miscased-roledefinitionid-arm.json
• scripts/testdata/role-parity/miscased-scope-arm.json
• scripts/testdata/role-parity/nested-deployment-arm.json
• scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json
• scripts/testdata/role-parity/other-subscription-arm.json
• scripts/testdata/role-parity/pim-roleassignment-schedule-owner-arm.json
• scripts/testdata/role-parity/pim-roleeligibility-owner-arm.json
• scripts/testdata/role-parity/second-permissions-entry-arm.json
• scripts/testdata/role-parity/space-inside-literal-arm.json
• scripts/testdata/role-parity/tenant-scope-arm.json
• scripts/testdata/role-parity/unallowed-roledefinitionid-arm.json
• scripts/testdata/role-parity/uppercase-allowed-roledefinitionid-arm.json
• scripts/testdata/role-parity/uppercase-canonical-scope-arm.json
• scripts/testdata/role-parity/uppercase-guid-literal-arm.json

🚧 Files skipped from review as they are similar to previous changes (33)

• scripts/testdata/role-parity/collision-assignablescopes-benign-first-arm.json
• scripts/testdata/role-parity/canonical-scope-variants-arm.json
• scripts/testdata/role-parity/dataactions-wildcard-arm.json
• scripts/testdata/role-parity/collision-properties-evil-first-arm.json
• scripts/testdata/role-parity/collision-assignablescopes-evil-first-arm.json
• scripts/testdata/role-parity/decorative-variables-arm.json
• scripts/testdata/role-parity/deploymentstack-scope-escape-arm.json
• scripts/testdata/role-parity/uppercase-guid-literal-arm.json
• scripts/testdata/role-parity/legacy-child-roleassignment-owner-arm.json
• scripts/testdata/role-parity/deploymentscript-scope-escape-arm.json
• scripts/testdata/role-parity/drifted-arm.json
• scripts/testdata/role-parity/mgmt-group-schema-arm.json
• arm/CUDly-CrossSubscription/template.json
• scripts/testdata/role-parity/nested-deployment-arm.json
• scripts/test-azure-role-parity.sh
• scripts/testdata/role-parity/lowercase-tenant-scope-arm.json
• scripts/testdata/role-parity/miscased-assignablescopes-arm.json
• scripts/testdata/role-parity/pim-roleassignment-schedule-owner-arm.json
• scripts/testdata/role-parity/other-subscription-arm.json
• scripts/testdata/role-parity/space-inside-literal-arm.json
• scripts/testdata/role-parity/uppercase-allowed-roledefinitionid-arm.json
• scripts/testdata/role-parity/matching-arm.json
• scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json
• known-issues.md
• scripts/testdata/role-parity/collision-type-evil-first-arm.json
• scripts/testdata/role-parity/miscased-roledefinitionid-arm.json
• scripts/testdata/role-parity/uppercase-canonical-scope-arm.json
• scripts/testdata/role-parity/foreign-subscription-literal-with-canonical-arm.json
• scripts/testdata/role-parity/collision-roledefinitionid-evil-first-arm.json
• scripts/testdata/role-parity/second-permissions-entry-arm.json
• scripts/testdata/role-parity/unallowed-roledefinitionid-arm.json
• scripts/testdata/role-parity/tenant-scope-arm.json
• scripts/testdata/role-parity/miscased-scope-arm.json

[cristim]

Addressed both open threads in scripts/check-azure-role-parity.sh, pushed as 34b4138b.

Thread 1 (line 108): collision scan missed root-level key collisions

Confirmed live. The collision scan was rooted at .resources, so it never treated the root document object itself as a candidate for collision. Since the normalization step just below (walk(...)) runs from the true root, a template with both "Resources" (evil, spelled first) and "resources" (benign, spelled last) at the top level folds to the benign array on normalization -- last-entry-wins, same as any other collision this script exists to catch, just one level higher in the document tree.

Fixed by widening the scanned objects from .resources // [] | .. to ., (.resources // [] | ..), so the root object's own keys are checked too. Collision detection still runs on the raw file before normalization; only what gets scanned changed, not when.

Thread 2 (line 433): legacy child-scoped roleAssignments refusal covered only microsoft.storage

Confirmed live. REFUSED_TYPES named only microsoft.storage/storageaccounts/providers/roleassignments for the legacy child-scoped role-assignment spelling. The file's own comment above it (lines 411-415) already describes this as a general pattern ("the legacy ARM spelling for a role assignment as a child resource... same grant, invisible to the same selectors for the same reason"), but the enumeration only caught one parent. A Microsoft.KeyVault/vaults/providers/roleAssignments (or any other parent) granting the same way passed unrefused.

Fixed by matching the suffix (endswith("/providers/roleassignments")) instead of enumerating parent types, the same pattern ESCAPE_TOKENS already uses for managementGroups a few lines up (matched independently of provider spelling). Removed the now-redundant storage-specific literal from the allowlist rather than keeping both -- one exact suffix match replaces an enumeration that was always going to be incomplete.

Both fixes are comparison changes only: no new modes, caps, or validation layers.

Fixture verification (pre/post)

Two fixtures, run against matching-tf.tf.fixture:

collision-root-resources-evil-first-arm.json -- "Resources" (evil, carries the issue #1545 tenant-wide grant) spelled first, "resources" (benign, canonical) spelled last at the root.

PRE-FIX : OK: ARM and TF actions/... lists match
          OK: all 1 ARM grant scopes/roleDefinitionIds are subscription-anchored.
          exit=0   <- false clean, the bug
POST-FIX: ERROR: ARM template contains an object with two case-variant spellings...
          resource type=<no type field> name=<no name field>: Resources / resources
          exit=1   <- refused

legacy-child-keyvault-roleassignment-owner-arm.json -- same legacy child-scoped shape as the existing storage-parent regression case, under Microsoft.KeyVault/vaults instead, binding built-in Owner.

PRE-FIX : OK: ARM and TF actions/... lists match
          OK: all 1 ARM grant scopes/roleDefinitionIds are subscription-anchored.
          exit=0   <- false clean, the bug
POST-FIX: ERROR: ARM template contains 1 resource(s) of a type this check cannot safely inspect...
          exit=1   <- refused

The real production template (arm/CUDly-CrossSubscription/template.json) still passes clean post-fix:

OK: ARM and TF actions/notActions/dataActions/notDataActions lists match (case-insensitive).
OK: all 4 ARM grant scopes/roleDefinitionIds are subscription-anchored.
exit=0

Both fixtures are now permanent regression fixtures under scripts/testdata/role-parity/, wired into test-azure-role-parity.sh as cases 35-36. Full suite: 36 passed, 0 failed. bash -n and shellcheck both exit 0 on both scripts.

@coderabbitai review

[coderabbitai]

@cristim: Thanks for the detailed verification. I will review commit 34b4138b, including the two new regression cases and the production-template guard behavior.

---

✏️ Learnings added

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1658

Timestamp: 2026-08-03T16:33:59.685Z
Learning: In `scripts/check-azure-role-parity.sh`, case-variant JSON key collision detection must scan the raw ARM template root object as well as objects beneath `resources` before case-folding normalization. Root-level collisions such as `"Resources"` and `"resources"` can otherwise hide a malicious resource array through last-entry-wins normalization.

---

Learnt from: cristim
URL: https://github.com/LeanerCloud/CUDly/pull/1658

Timestamp: 2026-08-03T16:33:59.685Z
Learning: In `scripts/check-azure-role-parity.sh`, legacy child-scoped ARM role assignments must be rejected generically by matching resource types that end with `/providers/roleassignments`. Do not enumerate parent resource providers, because any parent type, such as `Microsoft.KeyVault/vaults`, can use this legacy role-assignment spelling.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Correcting a clearance I gave in error: this PR has not been reviewed by CodeRabbit at its current head.

I previously treated it as reviewed because the CodeRabbit commit status read success :: Review completed at 16:39:18Z, later than the head commit 34b4138b8 at 16:32:50Z. That inference was wrong. Checking the review objects by SHA instead of by timestamp:

head                               = 34b4138b862c0d57f8bc5eda00a4bfa18e674b61
last verdict (body length 11481)   = commit ed8e6a3ffdae211278620a012df1f2a987931870

ed8e6a3ff is the previous commit on this branch, so the widening in 34b4138b8 — scanning the root object and every provider for legacy child-scoped roleAssignments — is exactly the part that has not been reviewed. The only newer CodeRabbit message is @cristim ... I will review commit 34b4138b, a statement of intent, and it follows a Fair Usage limit notice posted at 10:56:32Z.

What is independently verified and does stand: the collision-detection ordering invariant survived the widening. Confirmed on the committed blob, not the working tree — the collision scan at lines 106-119 reads the raw $ARM_FILE, ARM_FILE_NORM is not created until line 159, every downstream axis reads the normalised copy, and nothing normalises before line 108. The collision expression [., (.resources // [] | ..) | objects is a strict superset of the downstream [.resources // [] | .. | objects | select(has("type"))], so no axis can see less than another. Regression cases 35 and 36 cover both the same-object and root-level evil-first shapes.

That verification is human/agent review, not CodeRabbit review, and it does not substitute for one on a security-checker change. Not re-triggering right now given the Fair Usage state; a ping while throttled burns shared budget and tightens the adaptive limit.

[cristim]

CodeRabbit's last verdict on this PR reviewed ed8e6a3ff, the commit before 34b4138b8 widened the scan to the root object and every provider. That widening is the part most worth reviewing, and it has not been.

Requesting a full review rather than the incremental form: after the repo-wide Fair Usage throttle earlier today, an incremental review silently skips the commits that were in flight during the throttle and reports clean on a diff it never read.

@coderabbitai full review

[coderabbitai]

@cristim I will run a full review of PR #1658 at the current head. I will include commit 34b4138b8, with focus on the root-level collision scan and generic rejection of legacy child-scoped role assignments.

🐇🔎

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 36 minutes.

[cristim]

The ✅ Action performed / Full review finished reply above at 21:32:45Z did not produce a review. Recording that explicitly, because the wrapper reads like a completed pass.

Same comment, further down:

Your included review limit is currently reached under our Fair Usage Limits Policy... Your next included review will be available in 36 minutes.

And the review objects confirm it — the most recent CodeRabbit verdict with a non-empty body is still:

at=2026-08-03T13:30:47Z  bodylen=11481  commit=ed8e6a3ff

ed8e6a3ff is the commit before 34b4138b8 widened the collision scan to the root object and every provider. So the widening remains unreviewed by CodeRabbit, and the acknowledgement claiming otherwise should not be read as coverage.

Not re-triggering before the stated window elapses. The quota is per-developer per-organization and adaptive, so pinging while throttled consumes budget, tightens the limit, and returns nothing.

What does stand on this PR, independently of CodeRabbit: all checks green, zero unresolved threads, and a manual verification of the security invariant recorded earlier — collision detection at lines 106-119 reads the raw $ARM_FILE, ARM_FILE_NORM is not created until line 159, every downstream axis reads the normalised copy, and nothing normalises before line 108. The collision expression [., (.resources // [] | ..) | objects is a strict superset of the downstream [.resources // [] | .. | objects | select(has("type"))], so no axis sees less than another. Regression cases 35 and 36 cover the same-object and root-level evil-first shapes.

That is agent review, not CodeRabbit review, and on a security-checker change it is not a substitute for one.

[cristim]

Retrying after the stated cooldown. The previous full review request at 21:32:45Z replied ✅ Action performed / Full review finished while producing no review object, with a 36-minute Fair Usage cooldown noted in the same comment. That window has elapsed.

Still outstanding: the newest CodeRabbit verdict with a non-empty body is ed8e6a3ff, the commit before 34b4138b8 widened the collision scan to the root object and every provider. That widening is the part most worth reviewing on a security checker, and it remains unreviewed.

Using the full form deliberately: this branch head was pushed at 16:32:50Z, straight into a Fair Usage limit, so its automatic review never ran and is not retried retroactively. An incremental request would skip exactly those commits and report clean on a diff it never read.

@coderabbitai full review

[coderabbitai]

@cristim I will run a full review of PR #1658 at the current head. I will include 34b4138b8 and review the root-level collision scan and the generic legacy child-scoped role-assignment rejection.

🐇🔎

✅ Action performed

Full review finished.

---

Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 1 minute.

[cristim]

Requesting a full review, paced to one request per hour across this repo.

The CodeRabbit quota is per-developer per-organization and adaptive, tightening at the 95th percentile of recent review volume. Every open PR here draws on one budget, so bursting requests both consumes it faster and tightens the limit. Spacing them is what gets verdicts to land.

Full form rather than incremental: this head was pushed while the quota was exhausted, so its automatic review never ran and is not retried retroactively. An incremental request would skip exactly those commits and report clean on a diff it never read.

@coderabbitai full review

[coderabbitai]

@cristim I will run a full review at the current PR head. The review will include the root-level collision scan and the generic legacy child-scoped role-assignment rejection.

✅ Action performed

Full review finished.

[coderabbitai]

♻️ Duplicate comments (1)

known-issues.md (1)

8-17: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Keep the historical grant status conditional.

Line 8 and Line 174 state that prior deployments definitely granted tenant-wide access. Lines 30 through 37 state that this outcome is unconfirmed. Use conditional wording at both sites.

• known-issues.md#L8-L17: change the heading to state that existing deployments may retain a tenant-wide grant.
• known-issues.md#L170-L176: state that the retained template assignment could have granted tenant-wide access.

Proposed fix

-### Existing Azure deployments carry a tenant-wide reservation grant (issue `#1545`)
+### Existing Azure deployments may retain a tenant-wide reservation grant (issue `#1545`)
@@
-  that need, but the tenant-wide assignment was left behind and silently
-  granted access across the whole tenant. It has since been removed; do not
+  that need, but the tenant-wide assignment remained in the template and
+  could have granted access across the whole tenant. It has since been removed; do not

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@known-issues.md` around lines 8 - 17, Use conditional wording for the
historical tenant-wide grant status: update the heading around the existing
Azure deployments section to say deployments may retain a tenant-wide grant, and
revise the corresponding statement around the retained template assignment to
say it could have granted tenant-wide access. Apply these changes at
known-issues.md lines 8-17 and 170-176.

🧹 Nitpick comments (1)

scripts/test-azure-role-parity.sh (1)

16-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the failure reason, not only the exit status.

run_case compares exit codes only. Every refusal in check-azure-role-parity.sh exits 1. Many cases document that they must fail for one specific reason, for example the miscased-key cases 21-24 and the collision cases 30-36. The harness cannot prove that today. A fixture that starts failing on the actions axis instead of the scope axis still reports PASS.

Add an optional expected-pattern argument and match it against captured stderr.

♻️ Proposed harness change

 run_case() {
   local label="$1"
   local expected_exit="$2"
-  shift 2
+  local expect_msg="${EXPECT_MSG:-}"
+  shift 2
 
   actual_exit=0
-  "$CHECK" "$@" >/dev/null 2>&1 || actual_exit=$?
+  local output
+  output="$("$CHECK" "$@" 2>&1)" || actual_exit=$?
 
-  if [[ "$actual_exit" -eq "$expected_exit" ]]; then
+  if [[ "$actual_exit" -ne "$expected_exit" ]]; then
+    echo "FAIL: $label (expected exit $expected_exit, got $actual_exit)"
+    (( fail++ )) || true
+  elif [[ -n "$expect_msg" && ! "$output" =~ $expect_msg ]]; then
+    echo "FAIL: $label (exit matched, but message did not match /$expect_msg/)"
+    (( fail++ )) || true
+  else
     echo "PASS: $label"
     (( pass++ )) || true
-  else
-    echo "FAIL: $label (expected exit $expected_exit, got $actual_exit)"
-    (( fail++ )) || true
   fi
 }

Then set the reason per case, for example:

EXPECT_MSG='names a scope above the subscription' \
run_case "miscased 'AssignableScopes' key exits 1" 1 \
  --tf-file  "${FIXTURES}/matching-tf.tf.fixture" \
  --arm-file "${FIXTURES}/miscased-assignablescopes-arm.json"

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@scripts/test-azure-role-parity.sh` around lines 16 - 31, Update run_case to
accept an optional expected-pattern argument, capture the checker’s stderr while
preserving its exit status, and require the pattern to match stderr when
provided. Update the relevant reason-specific cases, especially miscased-key
cases 21–24 and collision cases 30–36, to pass patterns describing their
expected refusal reason while retaining existing exit-code assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@known-issues.md`:
- Around line 8-17: Use conditional wording for the historical tenant-wide grant
status: update the heading around the existing Azure deployments section to say
deployments may retain a tenant-wide grant, and revise the corresponding
statement around the retained template assignment to say it could have granted
tenant-wide access. Apply these changes at known-issues.md lines 8-17 and
170-176.

---

Nitpick comments:
In `@scripts/test-azure-role-parity.sh`:
- Around line 16-31: Update run_case to accept an optional expected-pattern
argument, capture the checker’s stderr while preserving its exit status, and
require the pattern to match stderr when provided. Update the relevant
reason-specific cases, especially miscased-key cases 21–24 and collision cases
30–36, to pass patterns describing their expected refusal reason while retaining
existing exit-code assertions.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2d546133-c026-4586-8166-b289022e7b3d

📥 Commits

Reviewing files that changed from the base of the PR and between a233ce0 and 34b4138.

📒 Files selected for processing (39)

• arm/CUDly-CrossSubscription/template.json
• known-issues.md
• scripts/check-azure-role-parity.sh
• scripts/test-azure-role-parity.sh
• scripts/testdata/role-parity/canonical-scope-variants-arm.json
• scripts/testdata/role-parity/collision-assignablescopes-benign-first-arm.json
• scripts/testdata/role-parity/collision-assignablescopes-evil-first-arm.json
• scripts/testdata/role-parity/collision-properties-evil-first-arm.json
• scripts/testdata/role-parity/collision-roledefinitionid-evil-first-arm.json
• scripts/testdata/role-parity/collision-root-resources-evil-first-arm.json
• scripts/testdata/role-parity/collision-type-evil-first-arm.json
• scripts/testdata/role-parity/dataactions-wildcard-arm.json
• scripts/testdata/role-parity/decorative-variables-arm.json
• scripts/testdata/role-parity/deploymentscript-scope-escape-arm.json
• scripts/testdata/role-parity/deploymentstack-scope-escape-arm.json
• scripts/testdata/role-parity/drifted-arm.json
• scripts/testdata/role-parity/foreign-subscription-literal-with-canonical-arm.json
• scripts/testdata/role-parity/legacy-child-keyvault-roleassignment-owner-arm.json
• scripts/testdata/role-parity/legacy-child-roleassignment-owner-arm.json
• scripts/testdata/role-parity/lowercase-tenant-scope-arm.json
• scripts/testdata/role-parity/lowercase-type-wildcard-actions-arm.json
• scripts/testdata/role-parity/matching-arm.json
• scripts/testdata/role-parity/mgmt-group-schema-arm.json
• scripts/testdata/role-parity/miscased-assignablescopes-arm.json
• scripts/testdata/role-parity/miscased-properties-arm.json
• scripts/testdata/role-parity/miscased-roledefinitionid-arm.json
• scripts/testdata/role-parity/miscased-scope-arm.json
• scripts/testdata/role-parity/nested-deployment-arm.json
• scripts/testdata/role-parity/obfuscated-tenant-scope-arm.json
• scripts/testdata/role-parity/other-subscription-arm.json
• scripts/testdata/role-parity/pim-roleassignment-schedule-owner-arm.json
• scripts/testdata/role-parity/pim-roleeligibility-owner-arm.json
• scripts/testdata/role-parity/second-permissions-entry-arm.json
• scripts/testdata/role-parity/space-inside-literal-arm.json
• scripts/testdata/role-parity/tenant-scope-arm.json
• scripts/testdata/role-parity/unallowed-roledefinitionid-arm.json
• scripts/testdata/role-parity/uppercase-allowed-roledefinitionid-arm.json
• scripts/testdata/role-parity/uppercase-canonical-scope-arm.json
• scripts/testdata/role-parity/uppercase-guid-literal-arm.json