From a5bde25463eec3b95e2aa10da79b124da708d3bd Mon Sep 17 00:00:00 2001 From: Stephen Finucane Date: Fri, 29 Aug 2025 11:44:43 +0100 Subject: [PATCH] api: Add runtime check for general additionalProperties Change-Id: I959afd6e6fa89f0656c10599e50ecb179c87d354 Signed-off-by: Stephen Finucane --- .../openstack/compute/schemas/hypervisors.py | 1 + nova/api/openstack/compute/schemas/ips.py | 2 + nova/api/openstack/compute/schemas/volumes.py | 7 ++- nova/api/validation/__init__.py | 47 +++++++++++++++++++ nova/tests/unit/test_api_validation.py | 31 ++++++++++-- 5 files changed, 83 insertions(+), 5 deletions(-) diff --git a/nova/api/openstack/compute/schemas/hypervisors.py b/nova/api/openstack/compute/schemas/hypervisors.py index bced27500d..37c965184e 100644 --- a/nova/api/openstack/compute/schemas/hypervisors.py +++ b/nova/api/openstack/compute/schemas/hypervisors.py @@ -130,6 +130,7 @@ _hypervisor_detail_response = { 'id': {'type': 'integer'}, }, 'required': ['disabled_reason', 'host', 'id'], + 'additionalProperties': False, }, 'state': {'enum': ['up', 'down']}, 'status': {'enum': ['enabled', 'disabled']}, diff --git a/nova/api/openstack/compute/schemas/ips.py b/nova/api/openstack/compute/schemas/ips.py index c6e468868b..a8ac4ec7f8 100644 --- a/nova/api/openstack/compute/schemas/ips.py +++ b/nova/api/openstack/compute/schemas/ips.py @@ -57,6 +57,7 @@ index_response = { 'items': copy.deepcopy(_ip_address), }, }, + 'additionalProperties': False, }, }, 'required': ['addresses'], @@ -73,4 +74,5 @@ show_response = { 'items': copy.deepcopy(_ip_address), }, }, + 'additionalProperties': False, } diff --git a/nova/api/openstack/compute/schemas/volumes.py b/nova/api/openstack/compute/schemas/volumes.py index 2e82e5051b..e823f8331f 100644 --- a/nova/api/openstack/compute/schemas/volumes.py +++ b/nova/api/openstack/compute/schemas/volumes.py @@ -22,7 +22,12 @@ create = { 'type': 'object', 'properties': { 'volume_type': {'type': 'string'}, - 'metadata': {'type': 'object'}, + # This could probably be stricter but it's a deprecated API... + 'metadata': { + 'type': 'object', + 'properties': {}, + 'additionalProperties': True, + }, 'snapshot_id': {'type': 'string'}, 'size': { 'type': ['integer', 'string'], diff --git a/nova/api/validation/__init__.py b/nova/api/validation/__init__.py index 6bf804f267..691de67e29 100644 --- a/nova/api/validation/__init__.py +++ b/nova/api/validation/__init__.py @@ -74,11 +74,58 @@ class Schemas: self.validate_schemas() + @classmethod + def _validate_schema(cls, schema: ty.Any) -> None: + # we should only be given dicts (JSON objects) here + assert isinstance(schema, dict) + + # some schemas are empty, hence .get + if schema.get('type') != 'object': + return + + # if we have oneOf then there are subschemas: fake these to look like + # complete schema + # NOTE(stephenfin): we may need to extend this for anyOf/allOf one day + if 'oneOf' in schema: + for sub_schema in schema['oneOf']: + cls._validate_schema({'type': 'object', **sub_schema}) + return + + # if we have an object-type additionalProperties value then this + # contains a schema (we use this to allow arbitrary keys and validated + # values) + if ( + 'additionalProperties' in schema and + isinstance(schema['additionalProperties'], dict) + ): + cls._validate_schema(schema['additionalProperties']) + return + + if 'properties' in schema: + properties = schema['properties'] + elif 'patternProperties' in schema: + properties = schema['patternProperties'] + else: + raise RuntimeError( + f'no properties/patternProperties key in {schema}' + ) + + # if we have an object with defined properties, then we insist that + # 'additionalProperties' be set (though we don't care what value it is + # set to) + if 'additionalProperties' not in schema: + raise RuntimeError(f'no additionalProperties key in {schema}') + + for value in properties.values(): + cls._validate_schema(value) + def validate_schemas(self) -> None: """Ensure there are no overlapping schemas.""" prev_max_version: api_version_request.APIVersionRequest | None = None for schema, min_version, max_version in self._schemas: + self._validate_schema(schema) + if prev_max_version: # it doesn't make sense to have multiple schemas if one of them # is unversioned (i.e. applies to everything) diff --git a/nova/tests/unit/test_api_validation.py b/nova/tests/unit/test_api_validation.py index 9ce72f195d..0ffcb921ec 100644 --- a/nova/tests/unit/test_api_validation.py +++ b/nova/tests/unit/test_api_validation.py @@ -173,7 +173,8 @@ class MicroversionsSchemaTestCase(APIValidationTestCase): 'foo': { 'type': 'integer', } - } + }, + 'additionalProperties': False, } schema_v20_str = copy.deepcopy(schema_v21_int) schema_v20_str['properties']['foo'] = {'type': 'string'} @@ -221,7 +222,8 @@ class MicroversionsSchemaTestCase(APIValidationTestCase): 'foo': { 'type': 'integer' } - } + }, + 'additionalProperties': False, } @validation.schema(schema_none) @@ -318,6 +320,8 @@ class RequiredDisableTestCase(APIValidationTestCase): 'type': 'integer', }, }, + 'required': [], + 'additionalProperties': True, } def test_validate_required_disable(self): @@ -334,7 +338,8 @@ class RequiredEnableTestCase(APIValidationTestCase): 'type': 'integer', }, }, - 'required': ['foo'] + 'required': ['foo'], + 'additionalProperties': False, } def test_validate_required_enable(self): @@ -356,6 +361,7 @@ class AdditionalPropertiesEnableTestCase(APIValidationTestCase): }, }, 'required': ['foo'], + 'additionalProperties': True, } def test_validate_additionalProperties_enable(self): @@ -440,6 +446,7 @@ class StringTestCase(APIValidationTestCase): 'type': 'string', }, }, + 'additionalProperties': False, } def test_validate_string(self): @@ -475,6 +482,7 @@ class StringLengthTestCase(APIValidationTestCase): 'maxLength': 10, }, }, + 'additionalProperties': False, } def test_validate_string_length(self): @@ -507,6 +515,7 @@ class IntegerTestCase(APIValidationTestCase): 'pattern': '^[0-9]+$', }, }, + 'additionalProperties': False, } def test_validate_integer(self): @@ -553,6 +562,7 @@ class IntegerRangeTestCase(APIValidationTestCase): 'maximum': 10, }, }, + 'additionalProperties': False, } def test_validate_integer_range(self): @@ -589,6 +599,7 @@ class BooleanTestCase(APIValidationTestCase): 'properties': { 'foo': parameter_types.boolean, }, + 'additionalProperties': False, } def test_validate_boolean(self): @@ -623,6 +634,7 @@ class FQDNTestCase(APIValidationTestCase): 'properties': { 'foo': parameter_types.fqdn, }, + 'additionalProperties': False, } def test_validate_fqdn(self): @@ -655,6 +667,7 @@ class NameTestCase(APIValidationTestCase): 'properties': { 'foo': parameter_types.name, }, + 'additionalProperties': False, } def test_validate_name(self): @@ -695,6 +708,7 @@ class NameWithLeadingTrailingSpacesTestCase(APIValidationTestCase): 'properties': { 'foo': parameter_types.name_with_leading_trailing_spaces, }, + 'additionalProperties': False, } def test_validate_name(self): @@ -737,7 +751,8 @@ class NameOrNoneTestCase(APIValidationTestCase): 'type': 'object', 'properties': { 'foo': parameter_types.name_or_none - } + }, + 'additionalProperties': False, } def test_valid(self): @@ -774,6 +789,7 @@ class CidrFormatTestCase(APIValidationTestCase): 'format': 'cidr', }, }, + 'additionalProperties': False, } def test_validate_cidr(self): @@ -817,6 +833,7 @@ class DatetimeTestCase(APIValidationTestCase): 'format': 'date-time', }, }, + 'additionalProperties': False, } def test_validate_datetime(self): @@ -854,6 +871,7 @@ class UuidTestCase(APIValidationTestCase): 'format': 'uuid', }, }, + 'additionalProperties': False, } def test_validate_uuid(self): @@ -895,6 +913,7 @@ class UriTestCase(APIValidationTestCase): 'format': 'uri', }, }, + 'additionalProperties': False, } def test_validate_uri(self): @@ -946,6 +965,7 @@ class Ipv4TestCase(APIValidationTestCase): 'format': 'ipv4', }, }, + 'additionalProperties': False, } def test_validate_ipv4(self): @@ -983,6 +1003,7 @@ class Ipv6TestCase(APIValidationTestCase): 'format': 'ipv6', }, }, + 'additionalProperties': False, } def test_validate_ipv6(self): @@ -1018,6 +1039,7 @@ class Base64TestCase(APIValidationTestCase): 'format': 'base64', }, }, + 'additionalProperties': False, } def test_validate_base64(self): @@ -1045,6 +1067,7 @@ class RegexFormatTestCase(APIValidationTestCase): 'format': 'regex', }, }, + 'additionalProperties': False, } def test_validate_regex(self):