Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default behavior for additionalProperties has changed in OAS3.1 #26

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

philsturgeon
Copy link
Contributor

the functionality in the repo was contradicting itself, saying it had to be false but also another rule allowed it to be an object. OAS3.0 is additionalPorperties: false by default, and OAS3.1 is additionalProperties: true by default. It seems like we should address that, by making OAS3.1 users use additionalProperties, and saying it either needs to be false or an object with maxProperties set?

Motivation and Context

#InsertIssueNumberHere

Description

How Has This Been Tested?

Screenshot(s)/recordings(s)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

@philsturgeon
Copy link
Contributor Author

@DavidBiesack I'd be interested in your thoughts on this, as the default behavior for additional properties has changed in OAS 3.1 to make it more permissive. The rules you wrote for #50 are the same as the old rules: they'll look for this keyword and if used will restrict it to false, or an object with maxProperties, but perhaps 3.1 needs a new approach?

@philsturgeon philsturgeon changed the title what to do about additionalProperties Default behavior for additionalProperties has changed in OAS3.1 Jan 21, 2024
the functionality in the repo was contradicting itself, saying it had to be false but also another rule allowed it to be an object. OAS3.0 is additionalPorperties: false by default, and OAS3.1 is additionalProperties: true by default. It seems like we should address that, by making OAS3.1 users use additionalProperties, and saying it either needs to be false or an object with maxProperties set?
@philsturgeon philsturgeon force-pushed the feat/what-about-additionalProps branch from 2c74abf to f8c5e7b Compare January 21, 2024 10:48
@DavidBiesack
Copy link
Contributor

@philsturgeon
Re "the default behavior for additionalProperties has changed in OAS 3.1 to make it more permissive."
Can you provide a citation for that? https://spec.openapis.org/oas/v3.1.0 does not specify any behavior or changes (only uses additionalProperties in some examples) and the JSON Schema 2020/12 core changelog does not help either (only mentions "Comment on ambiguity around annotations and "additionalProperties")

@philsturgeon
Copy link
Contributor Author

JSON Schema has always allowed additional properties by default, that hasn't changed.

What did change in OpenAPI v3.1 was schema becoming a proper JSON Schema Vocabulary instead of something vaguely based off the concept of JSON Schema. One of these changes included a change that means extra properties are welcome without needing to necessarily start with x-.

In v3.0.3 the Schema Object section of the spec says:

Additional properties defined by the JSON Schema specification that are not mentioned here are strictly unsupported.

In v3.1.1 the Schema Object section of the spec says:

In addition to the JSON Schema properties comprising the OAS dialect, the Schema Object supports keywords from any other vocabularies, or entirely arbitrary properties.

Another change is from this in 3.0.x

This object MAY be extended with Specification Extensions.

To this in 3.1.x:

This object MAY be extended with Specification Extensions, though as noted, additional properties MAY omit the x- prefix within this object.

tl;dr: Y'all can do what you want now.

@DavidBiesack
Copy link
Contributor

@philsturgeon That 3.0.3 vs. 3.1.0 language is not talking about the JSON schema additionalProperties keyword (i.e. whether a JSON value can have properties not listed in the schema properties) but whether a schema object can have additional fields not defined by OAS or JSON Schema.
For OWASP, it is too strict to warn if a schema does not have additionalProperties (as i may use unevaluatedProperties which is more flexible when used with a schema that is composed from other schemas. I think the rule should advise one or the other, but there are situations when they should not be used, i.e. for a schema that is referenced/used to compose other schemas.

@@ -698,15 +698,29 @@ export default {
"owasp:api6:2019-constrained-additionalProperties": {
message: "Objects should not allow unconstrained additionalProperties.",
description:
"By default JSON Schema allows additional properties, which can potentially lead to mass assignment issues, where unspecified fields are passed to the API without validation. Disable them with `additionalProperties: false` or add `maxProperties`",
"Additional properties are enabled by default in modern OpenAPI and JSON Schema as it helps keep your API forwards compatible, but it can potentially lead to mass assignment issues, where unspecified fields are passed to the API without validation. Disable additional properties explicitly with `additionalProperties: false`, or constrain the additional properties by providing a schema for their validation: `additionalProperties: { type: ... } }`.",
Copy link
Contributor

Choose a reason for hiding this comment

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

or additionalProperties: { '$ref: .... }

@@ -698,15 +698,29 @@ export default {
"owasp:api6:2019-constrained-additionalProperties": {
message: "Objects should not allow unconstrained additionalProperties.",
description:
"By default JSON Schema allows additional properties, which can potentially lead to mass assignment issues, where unspecified fields are passed to the API without validation. Disable them with `additionalProperties: false` or add `maxProperties`",
"Additional properties are enabled by default in modern OpenAPI and JSON Schema as it helps keep your API forwards compatible, but it can potentially lead to mass assignment issues, where unspecified fields are passed to the API without validation. Disable additional properties explicitly with `additionalProperties: false`, or constrain the additional properties by providing a schema for their validation: `additionalProperties: { type: ... } }`.",
Copy link
Contributor

Choose a reason for hiding this comment

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

double } } should be just } at the end of the sentence

@philsturgeon
Copy link
Contributor Author

@DavidBiesack

That 3.0.3 vs. 3.1.0 language is not talking about the JSON schema additionalProperties keyword (i.e. whether a JSON value can have properties not listed in the schema properties) but whether a schema object can have additional fields not defined by OAS or JSON Schema.

Well exactly. OWASP does not have opinions about how OpenAPI or JSON Schema is written. OWASP is concerned about whether additional keywords can be sent than you're expecting.

In OpenAPI v3.0 the assumption was default off, and you would have to say "yeah thats fine" with additionalProperties: true, so we'd look for that and say "dont do that".

In OpenAPI v3.1 the assumption is that you can, so it feels like we should tell everyone to turn that off. Right?

It feels like its a bigger change than just changing additionalProperties to unevaluatedProperties, it's a change in the default behavior which means a change in how we approach the rules.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants