-
-
Notifications
You must be signed in to change notification settings - Fork 776
Fix/multiple content type headers lead to 415 consistently #2070
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
base: main
Are you sure you want to change the base?
Fix/multiple content type headers lead to 415 consistently #2070
Conversation
Took me too long to see the whole picture here. Tell me if I describe the current request processing correctly:
If I have that right, I think that means there are at least two functions that extract the content type? Finally returning to this PR, it changes |
Hi @chrisinmtown you 100% got it 💯 Apologies I had not made this clearer. My storytelling chip is clearly on the fritz. |
This may be a case of belt-and-suspenders, but please hear me out. Would you consider including this minor change to the
|
I agree that this PR makes Connexion behave similarly to how Flask/Werkzeug handles multiple |
@chrisinmtown I think that because Connexion uses multiple potential backends, one Starlette and one Flask, this is what leads to the situation. |
@chrisinmtown can I ask if we think that
might cause other issues? |
Of course it might :) Still, that line of code is inherently fragile, it assumes the split will always yield a list of exactly length 2. Adding the |
So it looks like I was confusing the charset detection, from the mime and subtype. 😊 Change made, and I've changed tests to use If one mime-type check fails, full visibility about impact of the other cases is there. I believe this is a developer experience win, but I'm open to feedback if this is less clear. |
LGTM. Now we wait for the maintainers to notice this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite see what you mean with "while subsequent implementations downstream returned
'application/json,application/json'. "
Can we instead fix those?
|
||
|
||
def test_multiple_json_content_type(json_validation_spec_dir, spec): | ||
"""ensure that defaults applied that modify the body""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring is the one from the test above, it should be about multiple content type headers?
class MyDefaultsJSONBodyValidator(DefaultsJSONRequestBodyValidator): | ||
pass | ||
|
||
validator_map = {"body": {"application/json": MyDefaultsJSONBodyValidator}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to update the custom body validator in this test
content_type = value | ||
break | ||
value = value.decode("latin-1") | ||
content_type = ",".join([content_type, value] if content_type else [value]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be only one content-type header. If there are multiple, I believe it is more appropriate to raise a BadRequest error to return a 400 status code
@pytest.mark.parametrize( | ||
"mime_type", | ||
[ | ||
"application/json,application/json", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a valid mimetype
I'm pleased to submit a pull request that I believe fixes #2040 on GitHub. The proposed changes aim to improve consistency of the behavior of content-type header inspection in Connexion.
Key modifications include:
The reason this fix is effective (tested in an interactive debugger with multiple clients and original tests) is that
the modified method was previously returning 'application/json', while subsequent implementations downstream returned
'application/json,application/json'. This resulted in three parts when splitting by '/' instead of two, which caused
issues.
I did investigate alternative approaches to fixing the '/'-split issue mentioned in the original report. However, this
led me to modify more code, which didn't resolve the 500 error and 415 response from Safari. I opted for a more precise
approach that focused on the specific method modification, as it yielded better results.
Please let me know if you have any questions or concerns about this pull request.