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

Updated address normalization to test for https first to fix #223 #233

Closed
wants to merge 3 commits into from
Closed

Updated address normalization to test for https first to fix #223 #233

wants to merge 3 commits into from

Conversation

TrueTechy
Copy link
Contributor

Resolves #223 where the http->https redirect was causing the server to not respond properly. This changes the default to try HTTPS first, then if that is not available goes down to http

@mcarlton00
Copy link
Member

I don't think the fallback to http is working. Particularly if you have a reverse proxy where some have ssl and some don't. At least with nginx, it ends up defaulting to using a different certificate since the request came in over 443.

2020-03-19 17:21:02.736 T:140175862986496  NOTICE: JELLYFIN.jellyfin.api -> INFO::jellyfin_kodi/jellyfin/api.py:385 Sending get request to system/info/public
2020-03-19 17:21:02.752 T:140175862986496   ERROR: EXCEPTION Thrown (PythonToCppException) : -->Python callback/script returned the following error<--
                                             - NOTE: IGNORING THIS CAN LEAD TO MEMORY LEAKS!
                                            Error Type: <class 'requests.exceptions.SSLError'>
                                            Error Contents: HTTPSConnectionPool(host='jf-test.example.com', port=443): Max retries exceeded with url: /system/info/public (Caused by SSLError(CertificateError("hostname 'jf-test.example.com' doesn't match 'other-domain.example.com'",),))
                                            Traceback (most recent call last):
                                              File "/home/matt/.kodi/addons/plugin.video.jellyfin/jellyfin_kodi/dialogs/servermanual.py", line 84, in onClick
                                                elif self._connect_to_server(server):
                                              File "/home/matt/.kodi/addons/plugin.video.jellyfin/jellyfin_kodi/dialogs/servermanual.py", line 121, in _connect_to_server
                                                result = self.connect_manager.connect_to_address(server)
                                              File "/home/matt/.kodi/addons/plugin.video.jellyfin/jellyfin_kodi/jellyfin/connection_manager.py", line 133, in connect_to_address
                                                address = self._normalize_address(address)
                                              File "/home/matt/.kodi/addons/plugin.video.jellyfin/jellyfin_kodi/jellyfin/connection_manager.py", line 293, in _normalize_address
                                                if self.API.get_public_info(url):
                                              File "/home/matt/.kodi/addons/plugin.video.jellyfin/jellyfin_kodi/jellyfin/api.py", line 432, in get_public_info
                                                response = self.send_request(server_address, "system/info/public")
                                              File "/home/matt/.kodi/addons/plugin.video.jellyfin/jellyfin_kodi/jellyfin/api.py", line 389, in send_request
                                                return request_method(url, **request_settings)
                                              File "/home/matt/.kodi/addons/script.module.requests/lib/requests/api.py", line 75, in get
                                                return request('get', url, params=params, **kwargs)
                                              File "/home/matt/.kodi/addons/script.module.requests/lib/requests/api.py", line 60, in request
                                                return session.request(method=method, url=url, **kwargs)
                                              File "/home/matt/.kodi/addons/script.module.requests/lib/requests/sessions.py", line 533, in request
                                                resp = self.send(prep, **send_kwargs)
                                              File "/home/matt/.kodi/addons/script.module.requests/lib/requests/sessions.py", line 646, in send
                                                r = adapter.send(request, **kwargs)
                                              File "/home/matt/.kodi/addons/script.module.requests/lib/requests/adapters.py", line 514, in send
                                                raise SSLError(e, request=request)
                                            SSLError: HTTPSConnectionPool(host='jf-test.example.com', port=443): Max retries exceeded with url: /system/info/public (Caused by SSLError(CertificateError("hostname 'jf-test.example.com' doesn't match 'other-domain.example.com'",),))
                                            -->End of Python script error report<--

@mcarlton00
Copy link
Member

That fixes the error I saw.

I wonder if we should put an extra log message or two in. Along the lines of "Attempting secure connection" and "Falling back to insecure http" otherwise users are likely to just see a connection error in the logs and send issues. It's kinda like the libraries error that gets thrown up on a fresh install. Thoughts?

@mcarlton00
Copy link
Member

Actually, I think I have a cleaner way.

def get_public_info(self, server_address):
response = self.send_request(server_address, "system/info/public")
return response.json() if response.status_code == 200 else {}

Instead of immediately returning the response here, we could do something involving the response.history to determine if a redirect happened.

def get_public_info(self, server_address): 
     response = self.send_request(server_address, "system/info/public") 
    if response.history:
        if response.history[0].response_code == 301
            # make new request with https
    return response.json() if response.status_code == 200 else {}

We'll probably have to adjust the response so that the url object in connection_manager can be updated correctly as well

@TrueTechy
Copy link
Contributor Author

The addon would still store the address as https with this method though. Using the current implementation it changes the stored url. Also that won't catch the ssl errors we've seen

@mcarlton00
Copy link
Member

Yes, that's part of the "adjust the response so the url object could be updated" that I mentioned. And it wouldn't need to catch the SSL error that I got earlier because it would never have tried https first. Following the redirect history allows us to try http, then move to https if the redirect happens.

I don't like the idea of forcing https first and throwing errors. We've been trying to get rid of the needless errors in the logs, plus it feels too much like using try/except for program flow.

@TrueTechy TrueTechy closed this Mar 23, 2020
@TrueTechy TrueTechy deleted the bug/https-redirection branch March 23, 2020 00:23
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.

HTTPs redirection issue when logging in
3 participants