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

Suppress the user-agent from newer versions of urllib3 #24

Merged
merged 3 commits into from
Dec 30, 2020

Conversation

Kalkran
Copy link

@Kalkran Kalkran commented Nov 27, 2020

Newer versions of urllib3, such as those being used by Home Assistant, automatically add a User-Agent header to the request. It seems, from trial and error, that sending over a User-Agent header breaks the API leading to responses as "INVALID PARAMS".

This pull request adds an additional parameter in the _request body to suppress the User-Agent header from being automatically filled.

@Kalkran
Copy link
Author

Kalkran commented Nov 27, 2020

I'm not sure what is causing the Python 3.5 version to fail, but the module-related error does not seem related to the changes but to the testing setup.

This request may well fix #21.

@prilly-dev
Copy link

This pr could be a good temporarly fix for this issue until or if upstream pyrequest implement urlib3 skip_header function: psf/requests#5671

@Kalkran
Copy link
Author

Kalkran commented Dec 1, 2020

@filcole Would you kindly have a look at the pull request? Your module is being used by (at least) Home Assistant and these integrations break for a lot of users because of the newer urllib/requests version.

home-assistant/core#43194
home-assistant/core#43312

'POST',
url=BASE_URL + endpoint,
data=params,
headers={"User-Agent": ""}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might want to leave a developers/comment/note around here explaining why we're forging the UserAgent to empty

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From my research, so long as it's not clearly urllib (or cURL - that took a while to debug) any useragent (or no useragent) seems to work. Setting it to "My PyCarwings Lib/2.whatever" also works, and might be considered more valid

Copy link

@prilly-dev prilly-dev Dec 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But setting it to default User-agent that urllib3 forces does not work.

This has been tested here: home-assistant/core#43312 (comment)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I was saying that rather than forging it to empty and needing a note to explain why, it could instead be set to a human-readable value that isn't rejected by the carwings servers.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that is also something, it was promoted here: home-assistant/core#43312 (comment)
Problem is that no one has tested/mapped what User-Agent headers are accepted.

@fermulator
Copy link

overall LGTM! ✔️

@@ -118,7 +118,12 @@ def _request(self, endpoint, params):
else:
params["custom_sessionid"] = ""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the change here adds headers as a dictionary, and this conforms to the documentation
https://docs.python.org/3/library/urllib.request.html#urllib.request.Request
✔️

@marinopalsson
Copy link

@filcole Could you take a look at this?

@prilly-dev
Copy link

It seems that pyrequest is sorting out the problem with urllib3 here: psf/requests#5671

If this proposed pr are introduced into pyrequest it will require a change with the current pr for pycarwings2

headers={"User-Agent": ""}

Need to be:

headers={"User-Agent": "None"}

@filcole
Copy link
Owner

filcole commented Dec 29, 2020

I think we should include a User-Agent in pycarwings2 since I think it's good practice. I don't know what the Nissan Leaf app sends, but I think MyLeaf is sending Dart/<version> (dart:io). I've experimented with user agent strings with the following results:

Not sending User-Agent works ✔
blank works ✔
python-urllib3/1.26.2 breaks ❌
python-urllib3/1.0 breaks ❌
pythonurllib3/1.0 breaks ❌
python3/1.0 breaks ❌
mython3/1.0 works ✔
pyfhon3/1.0 works ✔
urllibpython/1.0 breaks ❌
urllibpyfhon/1.0 works ✔
urllibPYTHON/1.0 works ✔
pycarwings2/1.0 works ✔
pycarwings2/2.10 works ✔

So it appears that when lowercase 'python' is part of the user agent string the Nissan servers return an error. I will use pycarwings2/2.10 as the user agent for now.

@filcole filcole merged commit 178e10b into filcole:master Dec 30, 2020
@filcole
Copy link
Owner

filcole commented Dec 30, 2020

Thank you to all who contributed. I've tweaked the pull request and incorporated the changes via PR #25

@Kalkran Kalkran deleted the fix_useragent branch December 30, 2020 08:17
@fermulator
Copy link

Nice! Great work all. Once we get a pycarwings2 release/version, please notify, then we can have homeassistant pull in the changes https://github.com/home-assistant/core/blob/dev/homeassistant/components/nissan_leaf/manifest.json#L5

@filcole
Copy link
Owner

filcole commented Dec 31, 2020 via email

@fermulator
Copy link

FYI: Did not fix #21 sadly.

mkuschke added a commit to mkuschke/openWB that referenced this pull request Apr 14, 2021
Seit Nov 2020 braucht es wohl einen user-agent Heade4 in der Request (siehe filcole/pycarwings2#24).
Ist der einzige Unterschied zur in OpenWB verwendeten Funktion. Sollte also auch für OpenWB das Problem lösen.
Bitte in ein der nächsten Releases einfließen lassen.
@bwduncan bwduncan mentioned this pull request Aug 10, 2021
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.

6 participants