Connexion: v2.4+ silently removes additionalProperties from nested objects

[mattspring]

Description

This change introduced new behavior that silently removes nested (i.e. not top-level) properties from request bodies when they aren't explicitly declared in the properties block, even if additionalProperties is true (implicitly or explicitly): https://github.com/zalando/connexion/commit/54e50f22787c814caf52f45521991da02ff5f84f#diff-476ae7e600ebfcb9bae2b8199fdd669bR325

Expected behaviour

All object properties are included in the sanitized object unless its schema explicitly sets additionalProperties: false

Actual behaviour

Nested object properties are removed from the request body if a properties block is present in the object's schema unless they are explicitly declared in the object's properties block.

Steps to reproduce

yaml:

components:
  schemas:
    FooBar:
      type: object
      additionalProperties: true
      properties:
        foo:
          type: string

paths:
  /foo:
    post:
      x-openapi-router-controller: app.controllers.foo
      operationId: foo
      requestBody:
        content:
          application/json:
            schema:
              type: object
              additionalProperties: true
              properties:
                bar: $ref: '#/components/schemas/FooBar'

call:

curl -XPOST -d'{
  "bar": {"foo": "foo", "qux": "qux"},
  "baz": "whatever"
}' /foo

controller:

def foo(body: dict):
    assert 'bar' in body         # no problem
    assert 'baz' in body         # additionalProperty also fine at top level
    assert 'foo' in body['bar']  # chill chill chill, everything as expected
    assert 'qux' in body['bar']  # NOPE! WHERE MY DATA AT THO?!

Additional info:

Output of the commands:

• python --version

Python 3.7.5+

• pip show connexion | grep "^Version\:"

Version: 2.4.0

[mattspring]

If community agrees this is a bug I can submit a PR, lmk.

[dtkav]

Is this still an issue on master? I changed that commit meaningfully here: https://github.com/zalando/connexion/commit/1abab0653f5ad2fd0fa10945d0e1a949d0dec2f4

[mattspring]

@dtkav I haven't tested on master (tested on 2.4 and 2.5.1), but I don't see why the change you linked would affect the issue.

As far as I can tell the problem is because in _get_val_from_param, we loop through query_schema['properties'].keys() (instead of e.g. set(value.keys()) + set(query_schema['properties'].keys())) to construct the return_dict, which is still the case on master: https://github.com/zalando/connexion/commit/1abab0653f5ad2fd0fa10945d0e1a949d0dec2f4#diff-476ae7e600ebfcb9bae2b8199fdd669bR350

[dtkav]

Oh yeah, you're right. That's definitely a bug :disappointed:

[mattspring]

Cool, simple fix is to loop over the union of schema properties and properties in the actual object — may want to guard it with a check for if additionalProperties is explicitly false, in which case stripping the unexpected props is a reasonable thing (then again, it shouldn't get to that point because the validator would/should throw a 400 error in that case)

If that solution sounds like something that would get accepted I can hack it up and submit.

[dtkav]

hmm, actually that code was intended to be added for query params only (with the introduction of deepObject) and shouldn't be relevant to requestBody objects (which should be passes as-is if they've passed validation in OAS3).

I'd have to test it to be sure, but I think a better approach might be to just scope this new code appropriately to query params.

Also, keep in mind that additionalParams should be taken care of at validation time, so if we got to this code, it's already valid.

[mattspring]

Cool that makes sense — my familiarity with the code is limited to the spelunking I did to find this, so it's not obvious to me how to appropriately ensure that block is only called for query params.

[dtkav]

hey @mattspring - please have a look at #1138 (especially the test for the bug you reported).

[mattspring]

Yeah that fixes my issue. So that code was _only_ ever being called at the wrong time?

[mattspring]

Also wow, such responsive, you're a credit to the OSS community 👏