Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/rulesets/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
# Change Log - @microsoft.azure/openapi-validator-rulesets

## 2.2.3

### Patches

- [XMSSecretInResponse] Enhanced rule to check definitions properties, exclude enum properties (enum and x-ms-enum), exclude properties containing 'public' in their name, and report errors at definition level

## 2.2.2

### Patches
Expand Down
9 changes: 6 additions & 3 deletions packages/rulesets/generated/spectral/az-arm.js
Original file line number Diff line number Diff line change
Expand Up @@ -3259,10 +3259,13 @@ const XMSSecretInResponse = (properties, _opts, ctx) => {
}
else {
if (isPotentialSensitiveProperty(prpName) &&
!prpName.toLowerCase().includes("public") &&
properties[prpName] &&
properties[prpName]["x-ms-secret"] !== true &&
!keyValuePairCheck &&
properties[prpName].type === "string") {
properties[prpName].type === "string" &&
!properties[prpName].enum &&
!properties[prpName]["x-ms-enum"]) {
errors.push({
message: `Property '${prpName}' contains secret keyword and does not have 'x-ms-secret' annotation. To ensure security, must add the 'x-ms-secret' annotation to this property.`,
path: [...path, prpName],
Expand Down Expand Up @@ -3780,8 +3783,8 @@ const ruleset = {
description: `When defining the response model for an ARM PUT/GET/POST operation, any property that contains sensitive information (such as passwords, keys, tokens, credentials, or other secrets) must include the "x-ms-secret": true annotation. This ensures that secrets are properly identified and handled according to ARM security guidelines.`,
message: "{{error}}",
severity: "error",
resolved: true,
given: ["$[paths,'x-ms-paths'].*.[put,get,post].responses.*.schema.properties"],
resolved: false,
given: ["$.definitions.*.properties"],
then: {
function: XMSSecretInResponse,
},
Expand Down
4 changes: 2 additions & 2 deletions packages/rulesets/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@microsoft.azure/openapi-validator-rulesets",
"version": "2.2.2",
"version": "2.2.3",
"description": "Azure OpenAPI Validator",
"main": "dist/index.js",
"files": [
Expand Down Expand Up @@ -91,4 +91,4 @@
"^@stoplight/spectral-ruleset-bundler/(.*)$": "<rootDir>/node_modules/@stoplight/spectral-ruleset-bundler/dist/$1"
}
}
}
}
5 changes: 3 additions & 2 deletions packages/rulesets/src/spectral/az-arm.ts
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ const ruleset: any = {
// RPC Code: RPC-Async-V1-11, RPC-Async-V1-14
PostResponseCodes: {
rpcGuidelineCode: "RPC-Async-V1-11, RPC-Async-V1-14, RPC-POST-V1-02, RPC-POST-V1-03",
description: "Synchronous POST must return 200 when a response body is required or 204 when no body is needed; LRO POST must initially return 202, with the final response returning 200 if a body is expected or 204 if not.",
description:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this intended?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rush prep has adjusted the spaces, and this happens with every PR. I usually discard it, but I thought I would push it this time.

"Synchronous POST must return 200 when a response body is required or 204 when no body is needed; LRO POST must initially return 202, with the final response returning 200 if a body is expected or 204 if not.",
severity: "error",
disableForTypeSpecDataPlane: true,
disableForTypeSpecDataPlaneReason:
Expand Down Expand Up @@ -690,7 +691,7 @@ const ruleset: any = {
message: "{{error}}",
severity: "error",
resolved: true,
given: ["$[paths,'x-ms-paths'].*.[put,get,post].responses.*.schema.properties"],
given: ["$.definitions.*.properties"],
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this change is due to - check all the models irrespective of being used or not?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this ensures we don't miss any secret properties, similar to how the typespec team handled their changes.

then: {
function: XMSSecretInResponse,
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,10 +33,13 @@ export const XMSSecretInResponse = (properties: any, _opts: any, ctx: any) => {
// Add all conditions for secret detection
if (
isPotentialSensitiveProperty(prpName) && // property name matches sensitive keywords
!prpName.toLowerCase().includes("public") && // skip properties containing "public"
properties[prpName] && // property exists
properties[prpName]["x-ms-secret"] !== true && // not explicitly marked as secret
!keyValuePairCheck && // not a key-value pair key
properties[prpName].type === "string" // property type is string
properties[prpName].type === "string" && // property type is string
!properties[prpName].enum && // not a standard enum property
!properties[prpName]["x-ms-enum"] // not an x-ms-enum property
) {
errors.push({
message: `Property '${prpName}' contains secret keyword and does not have 'x-ms-secret' annotation. To ensure security, must add the 'x-ms-secret' annotation to this property.`,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import linterForRule from "./utils"
let linter: Spectral

const ERROR_MESSAGE = `Property '{prpName}' contains secret keyword and does not have 'x-ms-secret' annotation. To ensure security, must add the 'x-ms-secret' annotation to this property.`

beforeAll(async () => {
linter = await linterForRule("XMSSecretInResponse")
return linter
Expand Down Expand Up @@ -191,43 +192,25 @@ test("XMSSecretInResponse should find errors", () => {
},
}
return linter.run(oasDoc).then((results) => {
expect(results.length).toBe(18)
expect(results[0].message).toBe(ERROR_MESSAGE.replace("{prpName}", "somepassword"))
expect(results[0].path.join(".")).toBe("paths./foo.get.responses.200.schema.properties.properties.properties.somepassword")
expect(results[1].message).toBe(ERROR_MESSAGE.replace("{prpName}", "sometoken"))
expect(results[1].path.join(".")).toBe("paths./foo.get.responses.200.schema.properties.properties.properties.sometoken")
expect(results[2].message).toBe(ERROR_MESSAGE.replace("{prpName}", "someaccess"))
expect(results[2].path.join(".")).toBe("paths./foo.get.responses.200.schema.properties.someaccess")
expect(results[3].message).toBe(ERROR_MESSAGE.replace("{prpName}", "someconnection"))
expect(results[3].path.join(".")).toBe("paths./foo.get.responses.200.schema.properties.someconnection")
expect(results[4].message).toBe(ERROR_MESSAGE.replace("{prpName}", "somecredential"))
expect(results[4].path.join(".")).toBe("paths./foo.get.responses.201.schema.properties.properties.properties.somecredential")
expect(results[5].message).toBe(ERROR_MESSAGE.replace("{prpName}", "somesecret"))
expect(results[5].path.join(".")).toBe("paths./foo.get.responses.201.schema.properties.properties.properties.somesecret")
expect(results[6].message).toBe(ERROR_MESSAGE.replace("{prpName}", "somepassword"))
expect(results[6].path.join(".")).toBe("paths./foo.post.responses.200.schema.properties.properties.properties.somepassword")
expect(results[7].message).toBe(ERROR_MESSAGE.replace("{prpName}", "sometoken"))
expect(results[7].path.join(".")).toBe("paths./foo.post.responses.200.schema.properties.properties.properties.sometoken")
expect(results[8].message).toBe(ERROR_MESSAGE.replace("{prpName}", "someaccess"))
expect(results[8].path.join(".")).toBe("paths./foo.post.responses.200.schema.properties.someaccess")
expect(results[9].message).toBe(ERROR_MESSAGE.replace("{prpName}", "someconnection"))
expect(results[9].path.join(".")).toBe("paths./foo.post.responses.200.schema.properties.someconnection")
expect(results[10].message).toBe(ERROR_MESSAGE.replace("{prpName}", "somecredential"))
expect(results[10].path.join(".")).toBe("paths./foo.post.responses.201.schema.properties.properties.properties.somecredential")
expect(results[11].message).toBe(ERROR_MESSAGE.replace("{prpName}", "somesecret"))
expect(results[11].path.join(".")).toBe("paths./foo.post.responses.201.schema.properties.properties.properties.somesecret")
expect(results[12].message).toBe(ERROR_MESSAGE.replace("{prpName}", "somepassword"))
expect(results[12].path.join(".")).toBe("paths./foo.put.responses.200.schema.properties.properties.properties.somepassword")
expect(results[13].message).toBe(ERROR_MESSAGE.replace("{prpName}", "sometoken"))
expect(results[13].path.join(".")).toBe("paths./foo.put.responses.200.schema.properties.properties.properties.sometoken")
expect(results[14].message).toBe(ERROR_MESSAGE.replace("{prpName}", "someaccess"))
expect(results[14].path.join(".")).toBe("paths./foo.put.responses.200.schema.properties.someaccess")
expect(results[15].message).toBe(ERROR_MESSAGE.replace("{prpName}", "someconnection"))
expect(results[15].path.join(".")).toBe("paths./foo.put.responses.200.schema.properties.someconnection")
expect(results[16].message).toBe(ERROR_MESSAGE.replace("{prpName}", "somecredential"))
expect(results[16].path.join(".")).toBe("paths./foo.put.responses.201.schema.properties.properties.properties.somecredential")
expect(results[17].message).toBe(ERROR_MESSAGE.replace("{prpName}", "somesecret"))
expect(results[17].path.join(".")).toBe("paths./foo.put.responses.201.schema.properties.properties.properties.somesecret")
expect(results.length).toBe(10)
const errorPaths = results.map((r) => r.path.join("."))
expect(errorPaths).toContain("definitions.FooResource.properties.somepassword")
expect(errorPaths).toContain("definitions.FooResource.properties.sometoken")
expect(errorPaths).toContain("definitions.Foo.properties.somecredential")
expect(errorPaths).toContain("definitions.Foo.properties.somesecret")
expect(errorPaths).toContain("definitions.FooRule.properties.someaccess")
expect(errorPaths).toContain("definitions.FooRule.properties.someconnection")
expect(errorPaths).toContain("definitions.FooDefinition.properties.properties.properties.somecredential")
expect(errorPaths).toContain("definitions.FooDefinition.properties.properties.properties.somesecret")
expect(errorPaths).toContain("definitions.FooRule.properties.properties.properties.somepassword")
expect(errorPaths).toContain("definitions.FooRule.properties.properties.properties.sometoken")

// Verify error messages
results.forEach((result) => {
const propertyName = String(result.path[result.path.length - 1])
const expectedMessage = ERROR_MESSAGE.replace("{prpName}", propertyName)
expect(result.message).toBe(expectedMessage)
})
})
})

Expand Down Expand Up @@ -563,6 +546,30 @@ test("XMSSecretInResponse should find no errors", () => {
type: "enum",
enum: ["value1", "value2", "value3"],
},
// Test that properties with "public" in name are not flagged
publicNetworkAccess: {
type: "string",
description: "State of public network access.",
enum: ["Enabled", "Disabled"],
"x-ms-enum": {
name: "PublicNetworkAccess",
modelAsString: true,
},
},
publicKey: {
type: "string",
description: "The public key for authentication.",
},
isPublicConnection: {
type: "string",
description: "Indicates if connection is public.",
},
// Test that enum properties (without x-ms-enum) are not flagged
networkAccessEnum: {
type: "string",
description: "Network access configuration.",
enum: ["Allow", "Deny"],
},
},
required: ["properties"],
},
Expand Down
Loading