Skip to content
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

Fix error caused by invalid charset in response headers #6123

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

deosrc
Copy link

@deosrc deosrc commented May 6, 2022

Ran into an error where we were having an exception raised from the requests library: 'bool' object has no attribute 'strip'

Seems the server was returning an empty charset value in the Content-Type response header of application/json;charset. This was causing the line below to default the value to a boolean True:

requests/requests/utils.py

Lines 524 to 529 in 2a6f290

key, value = param, True
index_of_equals = param.find("=")
if index_of_equals != -1:
key = param[:index_of_equals].strip(items_to_strip)
value = param[index_of_equals + 1 :].strip(items_to_strip)
params_dict[key.lower()] = value

This meant that attempting format the charset on the line below, the value was a boolean rather than a string:

requests/requests/utils.py

Lines 545 to 548 in 2a6f290

content_type, params = _parse_content_type_header(content_type)
if "charset" in params:
return params["charset"].strip("'\"")

This change adds a type check so that the charset is ignored if a value is not provided. The unit test has been modified for coverage, along with expanding the other test scenarios slightly.

@deosrc deosrc force-pushed the bugfix/invalid-charset branch from f541ffa to 7d2b7b9 Compare May 6, 2022 16:22
@deosrc deosrc marked this pull request as ready for review May 6, 2022 16:22
@nateprewitt
Copy link
Member

Thanks for the PR, @deosrc! You've chosen an interesting time to raise this. We're currently working on options for typing in Requests and this was a case where there actually appears to be a bug in the code. Note that the response the server is giving you is not a valid response though and should be fixed on their end.

True was chosen for a default in #4442 but doesn't make sense according to the RFC. A parameter is defined to be:

     media-type = type "/" subtype *( OWS ";" OWS parameter )
     parameter = token "=" ( token / quoted-string )

Which means the minimum viable parameter is <token>=. If we read farther in the RFC we find that quoting in parameters is interchangeable without affecting the semantics so <token>= is equivalent to <token>="". Our default in this case should just be an empty string rather than True.

We don't rely on value being boolean anywhere and there's no way to leave the function with the boolean and emit a successful request. The fix we'd proposed on the typing branch was to change the default. This will result in an empty charset= being treated as UTF-8 when decoding rather than an unhelpful ValueError.

@deosrc
Copy link
Author

deosrc commented May 9, 2022

Note that the response the server is giving you is not a valid response though and should be fixed on their end.

Absolutley, this has been fixed.

Your response all makes sense to me and sounds like it's fixed in the typing branch. I'll leave this up to you if you want to merge it in as an fix while the typing is ready, or for those unable to use typing. Happy for it to just be closed if it's not worth it as it's a rather obscure bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants