Skip to content

Commit a713711

Browse files
Copilotravimeda
andauthored
ARM API Reviewer agent: detect and flag denylist regex patterns in pattern constraints (OAPI-PATTERN-ALLOWLIST) (#43796)
* Initial plan * Add OAPI-PATTERN-ALLOWLIST rule: denylist regex detection in pattern constraints - Add pattern-validation.md reference with rule OAPI-PATTERN-ALLOWLIST - Update openapi-review.instructions.md: Section 4 rule + checklist item - Update typespec-review.instructions.md: Section 2.2 rule + checklist item - Update arm-api-review.instructions.md: Section 21.4 CNA-004 + checklist items - Update SKILL.md: add pattern-validation.md to reference table - Update linter-rule-coverage.md: Pattern Constraint Rules section + missing rule note - Add denylist-pattern-new.json fixture (new spec, blocking severity) - Add version-pairs/denylist-pattern/ fixtures (existing vs new, warning vs blocking) - Add denylist-pattern-violations.tsp TypeSpec fixture - Add eval-pattern-validation.yaml with 3 stimuli (17xxxx) - Update README.md and fixtures/README.md counts (16→17 evals, 51→54 stimuli, 36→39 fixtures) Co-authored-by: ravimeda <12752450+ravimeda@users.noreply.github.com> * Fix Prettier formatting in linter-rule-coverage.md and pattern-validation.md Co-authored-by: ravimeda <12752450+ravimeda@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: ravimeda <12752450+ravimeda@users.noreply.github.com> Co-authored-by: Ravi Eda <raeda@microsoft.com>
1 parent a100cb4 commit a713711

13 files changed

Lines changed: 1264 additions & 17 deletions

File tree

.github/instructions/arm-api-review.instructions.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -909,6 +909,7 @@ When reviewing resources that support availability zones, verify: `zones` is a t
909909

910910
- The `name` property in the `checkNameAvailability` request **MUST** have `pattern` and `maxLength` constraints matching the target resource type's name validation rules.
911911
- At minimum, the `name` field **SHOULD** be constrained to valid ARM resource name characters (alphanumeric, hyphens, underscores, periods). Example: `"pattern": "^[a-zA-Z0-9][a-zA-Z0-9-_.]{0,259}$"`, `"maxLength": 260`.
912+
- The `pattern` constraint **MUST** use allowlist (positive character class) syntax. Denylist (negated character class `[^...]`) syntax **MUST NOT** be used as the primary character-matching construct (`OAPI-PATTERN-ALLOWLIST`). A denylist such as `^[^<>%&:\\?/]+$` permits emoji, control characters, and non-Latin scripts that the service almost certainly rejects. Severity: **Blocking** for new properties/parameters; **Warning** for existing ones. See [`.github/skills/azure-api-review/references/pattern-validation.md`](../skills/azure-api-review/references/pattern-validation.md) for detection guidance, the severity matrix, and fix examples.
912913
- If a spec defines a custom `checkNameAvailability` request model with a bare `string` `name` field (no `pattern`, no `maxLength`), flag it as a **security concern** and instruct the author to add constraints.
913914
- **Why:** Unconstrained string inputs to backend queries are a SQL/NoSQL injection risk. Input validation in the spec provides client-side SDK validation and signals to implementers that the field requires server-side sanitization.
914915

@@ -1151,6 +1152,8 @@ When reviewing ARM resource-manager swagger files, verify:
11511152

11521153
-`checkNameAvailability` uses common-types `CheckNameAvailabilityRequest` / `CheckNameAvailabilityResponse` — not custom request/response models (CNA-003)
11531154
-`name` field has `pattern` and `maxLength` constraints — not a bare unconstrained string (CNA-004)
1155+
-`name` field `pattern` uses allowlist syntax (positive character class) — not denylist (`[^...]`) (OAPI-PATTERN-ALLOWLIST, CNA-004)
1156+
- ✅ All `pattern` constraints across path parameters, query parameters, and body properties use allowlist syntax — no denylist (`[^...]`) as the primary construct (`OAPI-PATTERN-ALLOWLIST`): Blocking for new, Warning for existing
11541157

11551158
### Tenant-Level APIs
11561159

.github/instructions/openapi-review.instructions.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,7 @@ A finding "passes" the rule means the rule does not appear in the findings list
157157
- Prevention of leading special characters (e.g., `^(?![.-])`) — resource names should not start with `.` or `-`.
158158
- Allowed character set (e.g., `[A-Za-z0-9_.-]`) that matches the service's actual validation.
159159
- Example: `"pattern": "^(?![.-])[A-Za-z0-9_.-]{1,128}$"`
160+
- `pattern` constraints **MUST** use allowlist (positive character class) syntax. Denylist (negated character class `[^...]`) syntax **MUST NOT** be used as the primary character-matching construct (`OAPI-PATTERN-ALLOWLIST`). A negative lookahead (`(?!...)`) used alongside a positive class is acceptable (e.g., `^(?![.-])[A-Za-z0-9_.-]{1,128}$`). Severity: **Blocking** for new properties/parameters; **Warning** for existing ones carried forward from a prior API version. See [`.github/skills/azure-api-review/references/pattern-validation.md`](../skills/azure-api-review/references/pattern-validation.md) for detection guidance, the severity matrix, fix examples, and regression-risk notes.
160161
- For action operations on a resource, the URL pattern **SHOULD** be: `/<resource-collection>/<resource-id>:<action>`.
161162
- For action operations on a collection: `/<resource-collection>:<action>`.
162163
- **DO NOT** use action URLs for standard CRUD operations that map naturally to GET/PUT/PATCH/DELETE.
@@ -644,6 +645,7 @@ When reviewing, systematically check:
644645
- ✅ No unused/orphaned definitions in `definitions` section (SCHEMA-UNUSED-DEF)
645646
- ✅ Example files use string placeholders for secrets, SSH keys, and credentials — no realistic-looking sensitive values (EX-SECRET-PLACEHOLDER)
646647
- ✅ Resource name path parameters include `pattern` constraints with length limits and character restrictions
648+
- ✅ All `pattern` constraints use allowlist (positive character class) syntax — no denylist (`[^...]`) as the primary construct (`OAPI-PATTERN-ALLOWLIST`): Blocking for new properties/parameters, Warning for existing ones
647649
- ✅ PUT operation descriptions use idempotent language ("Creates or updates..." not "Creates a new...")
648650
- ✅ Operations API display object uses camelCase property names (`provider`, `resource`, `operation`, `description`)
649651

.github/instructions/typespec-review.instructions.md

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,17 @@ See the canonical contract in [`.github/skills/azure-api-review/references/revie
130130
set, and prevention of leading special characters.
131131
Example: `@pattern("^(?![.-])[A-Za-z0-9_.-]{1,128}$")`
132132
(Also enforced by: `@azure-tools/typespec-azure-resource-manager/arm-resource-name-pattern`)
133+
- `@pattern` decorator values and `NamePattern` constraints **MUST**
134+
use allowlist (positive character class) syntax. Denylist (negated
135+
character class `[^...]`) syntax **MUST NOT** be used as the primary
136+
character-matching construct (`OAPI-PATTERN-ALLOWLIST`). A negative
137+
lookahead (`(?!...)`) used alongside a positive class is acceptable
138+
(e.g., `@pattern("^(?![.-])[A-Za-z0-9_.-]{1,128}$")`). Severity:
139+
**Blocking** for new properties/parameters; **Warning** for existing
140+
ones carried forward from a prior API version. See
141+
[`.github/skills/azure-api-review/references/pattern-validation.md`](../skills/azure-api-review/references/pattern-validation.md)
142+
for detection guidance, severity matrix, fix examples, and
143+
regression-risk notes.
133144
- Properties representing UTC timestamps **SHOULD** include a `Utc`
134145
suffix in the name (e.g., `lastModifiedTimeUtc`).
135146
- Properties named `<something>Id` **MUST** be specific about what kind
@@ -444,3 +455,4 @@ When reviewing TypeSpec files, verify:
444455
- ✅ No bearer/OAuth tokens passed in ARM request bodies -- use managed identity or Key Vault
445456
- ✅ Generated OpenAPI files match `tsp compile .` output
446457
- ✅ Example files present for all operations, with realistic descriptive values (EX-DESCRIPTIVE-VALUES)
458+
- ✅ All `@pattern` and `NamePattern` constraints use allowlist (positive character class) syntax — no denylist (`[^...]`) as the primary construct (`OAPI-PATTERN-ALLOWLIST`): Blocking for new properties/parameters, Warning for existing ones

.github/skills/azure-api-review/SKILL.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,7 @@ Each reference file covers one cross-cutting rule area with:
4343
| [linter-rule-coverage.md](references/linter-rule-coverage.md) | Linter rule ID → instruction file mapping (130+ rules) | -- |
4444
| [example-quality.md](references/example-quality.md) | Example file quality: orphan detection, coverage, descriptive values | EX-ORPHAN, EX-COVERAGE, EX-DESCRIPTIVE-VALUES |
4545
| [design-decisions.md](references/design-decisions.md) | Grey-area design trade-off frameworks (10 decision matrices) | DD-001–DD-010 |
46+
| [pattern-validation.md](references/pattern-validation.md) | Allowlist vs. denylist `pattern` constraints; Unicode bypass risk; severity matrix | OAPI-PATTERN-ALLOWLIST |
4647

4748
## Authoritative External Sources
4849

.github/skills/azure-api-review/references/linter-rule-coverage.md

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -260,6 +260,21 @@ instruction files.
260260
| -- | ResourceNameRestriction | arm-api-review §15.7 (PREFLIGHT-005) | ✅ Covered |
261261
| -- | TenantLevelAPIsNotAllowed | arm-api-review §12A (RPC-Uri-V1-11) | ✅ Annotated |
262262

263+
## Pattern Constraint Rules
264+
265+
| Linter Rule ID | Name | Instruction Coverage | Status |
266+
| -------------- | ------------------------ | ----------------------------------------------------------------- | -------------------- |
267+
| _(none)_ | DenylistPatternDetection | openapi-review §4 (OAPI-PATTERN-ALLOWLIST); pattern-validation.md | ❌ NO AUTOMATED RULE |
268+
269+
> **No automated linter rule** currently exists for `OAPI-PATTERN-ALLOWLIST`
270+
> in `@microsoft.azure/openapi-validator`, `@azure-tools/typespec-azure-core`,
271+
> or `@azure-tools/typespec-azure-resource-manager`. This check is enforced
272+
> exclusively by the ARM API Reviewer agent during code review. A follow-up
273+
> issue should be filed to add an automated rule to the OpenAPI validator and
274+
> TypeSpec linter packages. The existing `ResourceNameRestriction` rule
275+
> verifies that a `pattern` _exists_ on resource name parameters but does not
276+
> verify whether the pattern is an allowlist or a denylist.
277+
263278
---
264279

265280
## Coverage Summary
@@ -278,3 +293,9 @@ instruction files.
278293
| R2063 | OperationIdNounConflictingModelNames | openapi-review §7.1 |
279294
| R4006 | DeprecatedXmsCodeGenerationSetting | openapi-review §18 |
280295
| R2001 | AvoidNestedProperties | arm-api-review §8.2 |
296+
297+
### Missing Automated Linter Rules
298+
299+
| Rule ID | Name | Status | Follow-Up |
300+
| -------- | ------------------------ | -------------------- | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
301+
| _(none)_ | DenylistPatternDetection | ❌ No automated rule | File a follow-up issue to add `OAPI-PATTERN-ALLOWLIST` to `@azure-tools/typespec-azure-core` and `@microsoft.azure/openapi-validator`. Agent-level coverage: `openapi-review.instructions.md §4`, `typespec-review.instructions.md §2.2`, `arm-api-review.instructions.md §21.4`, and [`pattern-validation.md`](pattern-validation.md). |

0 commit comments

Comments
 (0)