api: Add runtime check for general additionalProperties

Change-Id: I959afd6e6fa89f0656c10599e50ecb179c87d354
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>

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']},

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,
 }

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'],

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)

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):