Do not sanitize body keys in OpenAPI 3 #1008
Conversation
The deserialized JSON form of the request body needs to be passed to the client applications * without further modification * so that they can work directly with objects that have been received over the network. The only names for which sanitization makes sense are the ones which are used as Python identifiers. Keys of the top-level JSON object within the request payload are never used by Connexion as Python identifiers. Also, no such sanitization of keys within request body is performed in OpenAPI v2. Closes issue spec-first#835.
|
Fully agreed with this PR. I was not expecting a framework of this calibre to quietly sanitise the payload. It's very counter-productive "feature". |
|
Thanks for the PR, @pbasista! |
|
This was definitely a oversight on my part when introducing openapi3. I was trying to get to bulk test coverage parity (I copied all of the swagger 2 tests), but I agree it's not needed due to the fact that the body is passed in as a single argument in openapi3. However, I think you may have unintentionally removed test coverage for sanitizing the body in swagger2. It would be great to add that back in, if you get a chance. Otherwise, this all makes sense to me! |
| @@ -1,22 +0,0 @@ | |||
| swagger: "2.0" | |||
There was a problem hiding this comment.
I don't think this should be removed.
There was a problem hiding this comment.
I believe that this file is only used by the query_sanitazion fixture which, if I am looking correctly, is also unused. Therefore, I have removed them.
In order to test that sanitization of request body is indeed disabled in both OpenAPI v2 and OpenAPI v3, the "simple" test application has been extended with a forward endpoint that is used in new test test_no_sanitization_in_request_body.
There was a problem hiding this comment.
I think there's a miscommunication - sorry if I was unclear.
- form-data sanitization still needs to happen in swagger2 (you haven't changed this)
- this fixture tests form-data sanitization for swagger2
- by removing this fixture, you've removed form-data sanitization testing for swagger2
- the test you added is related to a body with content-type: application/json
so, I think we still want coverage of form-data sanitization for swagger2.
hopefully this makes sense - let me know if I missed something.
There was a problem hiding this comment.
It's unfortunate that the coveralls plugin was removed from this repo - it would help to directly illustrate what I'm talking about
There was a problem hiding this comment.
form-data sanitization still needs to happen in swagger2 (you haven't changed this)
I agree.
this fixture tests form-data sanitization for swagger2
Yes, but that fixture is unused, if I am looking correctly.
I can try adding a new test for the sanitization of form data tomorrow, but that would be out of the original scope of this MPR.
There was a problem hiding this comment.
Ah - I see the disconnect - apparently we've already lost test coverage of the sanitization somewhere along the way, and you are just removing an unused fixture.
My mistake - I thought that you had removed a test as well.
I can see how from your perspective it doesn't make sense to add that test to this merge request.
There was a problem hiding this comment.
I have looked at the current code and there already seems to be a test test_param_sanitization which checks the sanitization of form data parameters in both OpenAPI v2 and OpenAPI v3.
It could be improved and extended, but I would not do it within this MPR because it is in my opinion out of scope.
There was a problem hiding this comment.
ok, that makes sense - thanks!
| type: object | ||
| parameters: | ||
| - name: name | ||
| in: formData |
There was a problem hiding this comment.
this fixture is testing formData, which requires sanitization for swagger2
|
@hjacobs if you have time to take a look - I think this bugfix is ready to go. |
|
@hjacobs can you please take a look at this bugfix? |
dtkav
changed the title
|
👍 |
|
Isn't it a breaking change ? Why is it released under a minor version bump ? |
|
This change broke my application when attempting to upgrade Connexion version. This has been released already but in the future, it would be prudent to make a major version bump or establish a deprecation policy to the old behaviour. |
Do not perform sanitization on request body keys in OpenAPI v3.
The deserialized JSON form of the request body needs to be passed to the client applications without further modification so that they can work directly with objects that have been received over the network. The only names for which sanitization makes sense are the ones which are used as Python identifiers.
Keys of the top-level JSON object within the request payload are never used by Connexion as Python identifiers. Also, no such sanitization of keys within request body is performed in OpenAPI v2.
Fixes issue #835.