diff --git a/docs/reserved-names-in-properties-bag.md b/docs/reserved-names-in-properties-bag.md new file mode 100644 index 000000000..e1a4c7377 --- /dev/null +++ b/docs/reserved-names-in-properties-bag.md @@ -0,0 +1,57 @@ +# ReservedNamesInPropertiesBag + +## Category + +ARM Error + +## Applies to + +ARM OpenAPI(swagger) specs + +## Description + +Certain property names are reserved and must not be defined in a resource's properties bag. Reserved names are matched case-insensitively and are maintained as an extensible list in the rule (currently `BillingData`); more names may be added over time. If information represented by a reserved name is required, model it under a dedicated, appropriately named property or a separate model definition instead of placing a reserved property directly in the resource properties bag. + +## How to fix the violation + +Remove the reserved property from the resource properties bag. Represent the information using a differently named property or a dedicated model definition as appropriate. + +### Valid/Good Example + +```json +"Resource": { + "properties": { + "properties": { + "provisioningState": { + "type": "string" + } + } + } +} +``` + +### Invalid/Bad Example + +```json +"Resource": { + "properties": { + "properties": { + "billingData": { + "type": "string" + } + } + } +} +``` + +```json +"Resource": { + "properties": { + "properties": { + "billingData": { + "$ref": "#/definitions/BillingData" + } + } + } +} +``` diff --git a/docs/rules.md b/docs/rules.md index 7f2703807..4e612bf53 100644 --- a/docs/rules.md +++ b/docs/rules.md @@ -1088,6 +1088,12 @@ Per [common-api-contracts](https://github.com/Azure/azure-resource-manager-rpc/b Please refer to [required-read-only-system-data.md](./required-read-only-system-data.md) for details. +### ReservedNamesInPropertiesBag + +Certain property names are reserved and must not be defined in a resource's properties bag. Reserved names are matched case-insensitively and are maintained as an extensible list in the rule (currently `BillingData`); more names may be added over time. If information represented by a reserved name is required, model it under a dedicated, appropriately named property or a separate model definition instead of placing a reserved property directly in the resource properties bag. + +Please refer to [reserved-names-in-properties-bag.md](./reserved-names-in-properties-bag.md) for details. + ### ReservedResourceNamesModelAsEnum Service-defined (reserved) resource names must be represented as an `enum` type with `modelAsString` set to `true`, not diff --git a/packages/rulesets/CHANGELOG.md b/packages/rulesets/CHANGELOG.md index cd31fd3aa..ab22e9329 100644 --- a/packages/rulesets/CHANGELOG.md +++ b/packages/rulesets/CHANGELOG.md @@ -1,5 +1,11 @@ # Change Log - @microsoft.azure/openapi-validator-rulesets +## 2.2.7 + +### Patches + +- Added rule ReservedNamesInPropertiesBag to disallow reserved property names (case-insensitive; currently 'BillingData') in a resource's properties bag + ## 2.2.6 ### Patches diff --git a/packages/rulesets/generated/spectral/az-arm.js b/packages/rulesets/generated/spectral/az-arm.js index c400a4631..690394f02 100644 --- a/packages/rulesets/generated/spectral/az-arm.js +++ b/packages/rulesets/generated/spectral/az-arm.js @@ -2741,6 +2741,54 @@ const requestBodyMustExistForPutPatch = (putPatchOperationParameters, _opts, ctx return errors; }; +const PROPERTIES$2 = "properties"; +const RESERVED_PROPERTY_NAMES = ["BillingData"]; +const reservedNameLookup = new Set(RESERVED_PROPERTY_NAMES.map((name) => name.toLowerCase())); +const errorMessage$1 = (name) => `Reserved property name '${name}' is not allowed in the resource properties bag.`; +function collectReservedNamePaths(schema, basePath, matches, visited) { + if (!_.isObject(schema) || visited.has(schema)) { + return; + } + visited.add(schema); + const s = schema; + if (_.isObject(s.properties)) { + for (const [name, propertySchema] of Object.entries(s.properties)) { + if (reservedNameLookup.has(name.toLowerCase())) { + matches.push({ path: [...basePath, PROPERTIES$2, name], name }); + } + collectReservedNamePaths(propertySchema, [...basePath, PROPERTIES$2, name], matches, visited); + } + } + for (const keyword of ["allOf", "anyOf", "oneOf"]) { + const subschemas = s[keyword]; + if (Array.isArray(subschemas)) { + subschemas.forEach((subschema, index) => { + collectReservedNamePaths(subschema, [...basePath, keyword, index], matches, visited); + }); + } + } + if (Array.isArray(s.items)) { + s.items.forEach((itemSchema, index) => { + collectReservedNamePaths(itemSchema, [...basePath, "items", index], matches, visited); + }); + } + else if (_.isObject(s.items)) { + collectReservedNamePaths(s.items, [...basePath, "items"], matches, visited); + } + if (_.isObject(s.additionalProperties)) { + collectReservedNamePaths(s.additionalProperties, [...basePath, "additionalProperties"], matches, visited); + } +} +const reservedNamesInPropertiesBag = (definition, _opts, ctx) => { + const bag = getProperties(definition); + const matches = []; + collectReservedNamePaths(bag, [], matches, new WeakSet()); + return matches.map((match) => ({ + message: errorMessage$1(match.name), + path: _.concat(ctx.path, PROPERTIES$2, match.path), + })); +}; + const ARM_ALLOWED_RESERVED_NAMES = ["operations"]; const INCLUDED_OPERATIONS = ["get", "put", "delete", "patch"]; const reservedResourceNamesModelAsEnum = (pathItem, _opts, ctx) => { @@ -4062,6 +4110,18 @@ const ruleset = { function: systemDataInPropertiesBag, }, }, + ReservedNamesInPropertiesBag: { + description: "Reserved property names are not allowed in the resource properties bag.", + message: "{{error}}", + severity: "error", + stagingOnly: true, + resolved: true, + formats: [oas2], + given: ["$.definitions.*.properties[?(@property === 'properties')]^"], + then: { + function: reservedNamesInPropertiesBag, + }, + }, ReservedResourceNamesModelAsEnum: { rpcGuidelineCode: "RPC-ConstrainedCollections-V1-04", description: "Service-defined (reserved) resource names should be represented as an enum type with modelAsString set to true, not as a static string in the path.", diff --git a/packages/rulesets/package.json b/packages/rulesets/package.json index 349e69fa5..8b142d73d 100644 --- a/packages/rulesets/package.json +++ b/packages/rulesets/package.json @@ -1,6 +1,6 @@ { "name": "@microsoft.azure/openapi-validator-rulesets", - "version": "2.2.6", + "version": "2.2.7", "description": "Azure OpenAPI Validator", "main": "dist/index.js", "files": [ diff --git a/packages/rulesets/src/spectral/az-arm.ts b/packages/rulesets/src/spectral/az-arm.ts index cab09555d..033cbb7f1 100644 --- a/packages/rulesets/src/spectral/az-arm.ts +++ b/packages/rulesets/src/spectral/az-arm.ts @@ -42,6 +42,7 @@ import { putRequestResponseScheme } from "./functions/put-request-response-schem import { PutResponseCodes } from "./functions/put-response-codes" import { queryParametersInCollectionGet } from "./functions/query-parameters-in-collection-get" import { requestBodyMustExistForPutPatch } from "./functions/request-body-must-exist-for-put-patch" +import { reservedNamesInPropertiesBag } from "./functions/reserved-names-in-properties-bag" import { reservedResourceNamesModelAsEnum } from "./functions/reserved-resource-names-model-as-enum" import resourceNameRestriction from "./functions/resource-name-restriction" import responseSchemaSpecifiedForSuccessStatusCode from "./functions/response-schema-specified-for-success-status-code" @@ -1026,6 +1027,23 @@ const ruleset: any = { }, }, + // Property names that are reserved (matched case-insensitively) must not be present in a + // resource's properties bag. The set of reserved names is defined in the + // reservedNamesInPropertiesBag function. + ReservedNamesInPropertiesBag: { + description: "Reserved property names are not allowed in the resource properties bag.", + message: "{{error}}", + severity: "error", + stagingOnly: true, + resolved: true, + formats: [oas2], + // given definitions that have the properties bag + given: ["$.definitions.*.properties[?(@property === 'properties')]^"], + then: { + function: reservedNamesInPropertiesBag, + }, + }, + /// /// ARM RPC rules for constrained resource collections /// diff --git a/packages/rulesets/src/spectral/functions/reserved-names-in-properties-bag.ts b/packages/rulesets/src/spectral/functions/reserved-names-in-properties-bag.ts new file mode 100644 index 000000000..3b09bbb77 --- /dev/null +++ b/packages/rulesets/src/spectral/functions/reserved-names-in-properties-bag.ts @@ -0,0 +1,80 @@ +// ReservedNamesInPropertiesBag +// Property names that are reserved (matched case-insensitively) must not be defined in a +// resource's properties bag. + +import _ from "lodash" +import { getProperties } from "./utils" + +const PROPERTIES = "properties" + +// Property names that are reserved and must not appear in a resource's properties bag. +// Matching is case-insensitive. Add new reserved names to this array as they are identified. +const RESERVED_PROPERTY_NAMES = ["BillingData"] + +const reservedNameLookup = new Set(RESERVED_PROPERTY_NAMES.map((name) => name.toLowerCase())) + +const errorMessage = (name: string) => `Reserved property name '${name}' is not allowed in the resource properties bag.` + +// Recursively collect the path to every property whose name is reserved (case-insensitive) and is +// defined within the given schema. Traversal is restricted to schema-structure keywords +// (properties, allOf/anyOf/oneOf, items, additionalProperties) so that only actual property +// definitions are inspected. Values inside non-structural metadata (e.g. default values, enum +// values, examples, or vendor extensions) are intentionally ignored to avoid false positives. +function collectReservedNamePaths( + schema: any, + basePath: (string | number)[], + matches: { path: (string | number)[]; name: string }[], + visited: WeakSet +): void { + if (!_.isObject(schema) || visited.has(schema)) { + return + } + visited.add(schema) + + const s = schema as { [key: string]: any } + + // properties: a map of property name -> property schema + if (_.isObject(s.properties)) { + for (const [name, propertySchema] of Object.entries(s.properties as { [key: string]: any })) { + if (reservedNameLookup.has(name.toLowerCase())) { + matches.push({ path: [...basePath, PROPERTIES, name], name }) + } + collectReservedNamePaths(propertySchema, [...basePath, PROPERTIES, name], matches, visited) + } + } + + // allOf / anyOf / oneOf: arrays of subschemas + for (const keyword of ["allOf", "anyOf", "oneOf"]) { + const subschemas = s[keyword] + if (Array.isArray(subschemas)) { + subschemas.forEach((subschema: any, index: number) => { + collectReservedNamePaths(subschema, [...basePath, keyword, index], matches, visited) + }) + } + } + + // items: a single subschema or an array of subschemas + if (Array.isArray(s.items)) { + s.items.forEach((itemSchema: any, index: number) => { + collectReservedNamePaths(itemSchema, [...basePath, "items", index], matches, visited) + }) + } else if (_.isObject(s.items)) { + collectReservedNamePaths(s.items, [...basePath, "items"], matches, visited) + } + + // additionalProperties: a subschema (when it is an object rather than a boolean) + if (_.isObject(s.additionalProperties)) { + collectReservedNamePaths(s.additionalProperties, [...basePath, "additionalProperties"], matches, visited) + } +} + +export const reservedNamesInPropertiesBag = (definition: any, _opts: any, ctx: any) => { + const bag = getProperties(definition) + const matches: { path: (string | number)[]; name: string }[] = [] + collectReservedNamePaths(bag, [], matches, new WeakSet()) + + return matches.map((match) => ({ + message: errorMessage(match.name), + path: _.concat(ctx.path, PROPERTIES, match.path), + })) +} diff --git a/packages/rulesets/src/spectral/test/reserved-names-in-properties-bag.test.ts b/packages/rulesets/src/spectral/test/reserved-names-in-properties-bag.test.ts new file mode 100644 index 000000000..b61211b5c --- /dev/null +++ b/packages/rulesets/src/spectral/test/reserved-names-in-properties-bag.test.ts @@ -0,0 +1,439 @@ +import { Spectral } from "@stoplight/spectral-core" +import linterForRule from "./utils" + +let linter: Spectral +const RULE = "ReservedNamesInPropertiesBag" +const errorMessageFor = (name: string) => `Reserved property name '${name}' is not allowed in the resource properties bag.` + +beforeAll(async () => { + linter = await linterForRule(RULE) + return linter +}) + +test(`${RULE} should find no errors when no reserved name is present in the properties bag`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + type: "object", + properties: { + provisioningState: { + type: "string", + }, + }, + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(0) + }) +}) + +test(`${RULE} should find errors when a reserved name references a model definition`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + type: "object", + properties: { + billingData: { + $ref: "#/definitions/BillingData", + }, + }, + }, + }, + }, + BillingData: { + type: "object", + properties: { + amount: { + type: "number", + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(1) + expect(results[0].path.join(".")).toBe("definitions.Resource.properties.properties.properties.billingData") + expect(results[0].message).toBe(errorMessageFor("billingData")) + }) +}) + +test(`${RULE} should find errors when a reserved name is a primitive type`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + type: "object", + properties: { + billingData: { + type: "string", + }, + }, + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(1) + expect(results[0].path.join(".")).toBe("definitions.Resource.properties.properties.properties.billingData") + expect(results[0].message).toBe(errorMessageFor("billingData")) + }) +}) + +test(`${RULE} should find errors when a reserved name is an inline model`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + type: "object", + properties: { + billingData: { + type: "object", + properties: { + amount: { + type: "number", + }, + }, + }, + }, + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(1) + expect(results[0].path.join(".")).toBe("definitions.Resource.properties.properties.properties.billingData") + expect(results[0].message).toBe(errorMessageFor("billingData")) + }) +}) + +test(`${RULE} should be case-insensitive when matching a reserved name`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + type: "object", + properties: { + BillingData: { + type: "string", + }, + BILLINGDATA: { + type: "string", + }, + }, + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(2) + expect(results[0].path.join(".")).toBe("definitions.Resource.properties.properties.properties.BillingData") + expect(results[1].path.join(".")).toBe("definitions.Resource.properties.properties.properties.BILLINGDATA") + expect(results[0].message).toBe(errorMessageFor("BillingData")) + expect(results[1].message).toBe(errorMessageFor("BILLINGDATA")) + }) +}) + +test(`${RULE} should not flag properties that only contain a reserved name as a substring`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + type: "object", + properties: { + billingDataId: { + type: "string", + }, + }, + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(0) + }) +}) + +test(`${RULE} should find errors when a reserved name is in a referenced properties definition`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + $ref: "#/definitions/ResourceProperties", + }, + }, + }, + ResourceProperties: { + type: "object", + properties: { + billingData: { + type: "string", + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(1) + expect(results[0].path.join(".")).toBe("definitions.Resource.properties.properties.properties.billingData") + expect(results[0].message).toBe(errorMessageFor("billingData")) + }) +}) + +test(`${RULE} should find errors when a reserved name is nested inside another property definition`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + type: "object", + properties: { + nested: { + type: "object", + properties: { + billingData: { + type: "string", + }, + }, + }, + }, + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(1) + expect(results[0].path.join(".")).toBe("definitions.Resource.properties.properties.properties.nested.properties.billingData") + expect(results[0].message).toBe(errorMessageFor("billingData")) + }) +}) + +test(`${RULE} should not flag a reserved name defined as a top-level property outside the properties bag`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + type: "object", + properties: { + provisioningState: { + type: "string", + }, + }, + }, + billingData: { + type: "string", + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(0) + }) +}) + +test(`${RULE} should not flag reserved names that appear inside non-structural schema metadata`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + type: "object", + properties: { + config: { + type: "object", + default: { + billingData: "someDefaultValue", + }, + enum: [ + { + billingData: 1, + }, + ], + "x-ms-metadata": { + billingData: true, + }, + }, + }, + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(0) + }) +}) + +test(`${RULE} should find errors when a reserved name is defined via allOf composition`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + allOf: [ + { + type: "object", + properties: { + billingData: { + type: "string", + }, + }, + }, + ], + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(1) + expect(results[0].path.join(".")).toBe("definitions.Resource.properties.properties.allOf.0.properties.billingData") + expect(results[0].message).toBe(errorMessageFor("billingData")) + }) +}) + +test(`${RULE} should find errors when a reserved name is defined in array items`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + type: "object", + properties: { + list: { + type: "array", + items: { + type: "object", + properties: { + billingData: { + type: "string", + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(1) + expect(results[0].path.join(".")).toBe("definitions.Resource.properties.properties.properties.list.items.properties.billingData") + expect(results[0].message).toBe(errorMessageFor("billingData")) + }) +}) + +test(`${RULE} should find errors when a reserved name is defined in tuple-style array items`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + type: "object", + properties: { + tuple: { + type: "array", + items: [ + { + type: "object", + properties: { + billingData: { + type: "string", + }, + }, + }, + ], + }, + }, + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(1) + expect(results[0].path.join(".")).toBe("definitions.Resource.properties.properties.properties.tuple.items.0.properties.billingData") + expect(results[0].message).toBe(errorMessageFor("billingData")) + }) +}) + +test(`${RULE} should find errors when a reserved name is defined in additionalProperties`, async () => { + const oasDoc = { + swagger: "2.0", + paths: {}, + definitions: { + Resource: { + properties: { + properties: { + type: "object", + properties: { + map: { + type: "object", + additionalProperties: { + type: "object", + properties: { + billingData: { + type: "string", + }, + }, + }, + }, + }, + }, + }, + }, + }, + } + return linter.run(oasDoc).then((results) => { + expect(results.length).toBe(1) + expect(results[0].path.join(".")).toBe( + "definitions.Resource.properties.properties.properties.map.additionalProperties.properties.billingData" + ) + expect(results[0].message).toBe(errorMessageFor("billingData")) + }) +})