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

[client] Handle invalid proxy settings #3198

Merged
merged 3 commits into from
Mar 5, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 36 additions & 2 deletions web/client/codechecker_client/helpers/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,17 @@
Base Helper class for Thrift api calls.
"""


import sys
from thrift.transport import THttpClient
from thrift.protocol import TJSONProtocol

from codechecker_client.credential_manager import SESSION_COOKIE_NAME
from codechecker_client.product import create_product_url

from codechecker_common.logger import get_logger

LOG = get_logger('system')


class BaseClientHelper(object):

Expand All @@ -28,13 +32,43 @@ def __init__(self, protocol, host, port, uri, session_token=None,
self.__port = port
url = create_product_url(protocol, host, port, uri)

self.transport = THttpClient.THttpClient(url)
self.transport = None

try:
self.transport = THttpClient.THttpClient(url)
except ValueError:
# Initalizing THttpClient may raise an exception if proxy settings
# are used but the port number is not a valid integer.
pass
finally:
# Thrift do not handle the use case when invalid proxy format is
# used (e.g.: no schema is specified). For this reason we need to
# verify the proxy format in our side.
self._validate_proxy_format()
Copy link
Contributor

Choose a reason for hiding this comment

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

If THttpClient throws TTransportException for some reason but not because something bad with proxy settings then this finally branch try to validate proxy settings.
Please call self._validate_proxy_format() in the ValueError branch not in the finally branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only the THttpClient object creation is inside a try/except block, which cannot raise such exception like TTransportException.
Two scenarios are possible here:

  • The user specified the following environment variable: HTTP_PROXY=http://localhost / HTTP_PROXY=http://localhost:not_a_number where the port is not specified or it is not an integer. In this case it will raise a ValueError exception.
  • The user specified the following environment variable: HTTP_PROXY=localhost:8001 where the schema is not given. In this case the THttpClient object creation will not raise any exception (it will raise exception when an API call happens). So this is the reason when this object creation is done, we need to check the proxy format before any API call happens.

In both cases we need to check the proxy format. So it is not enough to call this function inside the ValueError branch. We need to call it every time when there is no exception.


self.protocol = TJSONProtocol.TJSONProtocol(self.transport)
self.client = None

self.get_new_token = get_new_token
self._set_token(session_token)

def _validate_proxy_format(self):
"""
Validate the proxy settings.
If the proxy settings are invalid, it will print an error message and
stop the program.
"""
if self.transport and not self.transport.using_proxy():
return

if not self.transport or not self.transport.host or \
not isinstance(self.transport.port, int):
LOG.error("Invalid proxy format! Check your "
"HTTP_PROXY/HTTPS_PROXY environment variables if "
"these are in the right format: "
"'http[s]://host:port'.")
sys.exit(1)

def _set_token(self, session_token):
""" Set the given token in the transport layer. """
if not session_token:
Expand Down
26 changes: 24 additions & 2 deletions web/tests/functional/cmdline/test_cmdline.py
Original file line number Diff line number Diff line change
Expand Up @@ -47,9 +47,9 @@ def setUp(self):
test_class = self.__class__.__name__
print('Running ' + test_class + ' tests in ' + test_workspace)

codechecker_cfg = env.import_test_cfg(test_workspace)[
self.codechecker_cfg = env.import_test_cfg(test_workspace)[
'codechecker_cfg']
self.server_url = env.parts_to_url(codechecker_cfg)
self.server_url = env.parts_to_url(self.codechecker_cfg)

# Get the test project configuration from the prepared test workspace.
self._testproject_data = env.setup_test_proj_cfg(test_workspace)
Expand Down Expand Up @@ -123,6 +123,28 @@ def test_runs_filter(self):
self.assertEqual(0, ret)
self.assertEqual(1, len(json.loads(res)))

def test_proxy_settings(self):
""" Test proxy settings validation. """
server_url = f"{self.codechecker_cfg['viewer_host']}:" \
f"{str(self.codechecker_cfg['viewer_port'])}"

env = self.codechecker_cfg['check_env'].copy()
env['HTTP_PROXY'] = server_url

res_cmd = [self._codechecker_cmd, 'cmd', 'runs',
'--url', str(self.server_url)]
ret, _, err = run_cmd(res_cmd, env=env)
self.assertEqual(1, ret)
self.assertIn("Invalid proxy format", err)

env['HTTP_PROXY'] = f"http://{server_url}"
_, _, err = run_cmd(res_cmd, env=env)

# We can't check the return code here, because on host machine it will
# be zero, but on the GitHub action's job it will be 1 with "Failed to
# connect to the server" error message.
self.assertNotIn("Invalid proxy format", err)

def test_runs_row(self):
""" Test cmd row output type. """
env = self._test_config['codechecker_cfg']['check_env']
Expand Down