diff --git a/slack_sdk/proxy_env_variable_loader.py b/slack_sdk/proxy_env_variable_loader.py index 52f881470..5d5351a9c 100644 --- a/slack_sdk/proxy_env_variable_loader.py +++ b/slack_sdk/proxy_env_variable_loader.py @@ -2,7 +2,9 @@ import logging import os +import re from typing import Optional +from urllib.parse import urlsplit _default_logger = logging.getLogger(__name__) @@ -21,5 +23,18 @@ def load_http_proxy_from_env(logger: logging.Logger = _default_logger) -> Option logger.debug("The Slack SDK ignored the proxy env variable as an empty value is set.") return None - logger.debug(f"HTTP proxy URL has been loaded from an env variable: {proxy_url}") + logger.debug(f"HTTP proxy URL has been loaded from an env variable: {_redact_credentials(proxy_url)}") return proxy_url + + +def _redact_credentials(url: str) -> str: + """Replaces the user info (e.g., user:password@) in a URL so that it can be safely logged.""" + if "@" in url: + scheme = re.match(r"^[a-zA-Z][a-zA-Z0-9+.-]*://", url) + prefix = scheme.group(0) if scheme else "" + return f"{prefix}***@{url.rpartition('@')[2]}" + try: + urlsplit(url) + except ValueError: + return "(unparsable URL)" + return url diff --git a/slack_sdk/socket_mode/builtin/internals.py b/slack_sdk/socket_mode/builtin/internals.py index fae732871..dad78b438 100644 --- a/slack_sdk/socket_mode/builtin/internals.py +++ b/slack_sdk/socket_mode/builtin/internals.py @@ -14,6 +14,8 @@ from typing import Tuple, Optional, Union, List, Callable, Dict from urllib.parse import urlparse, unquote +from slack_sdk.proxy_env_variable_loader import _redact_credentials + from .frame_header import FrameHeader @@ -86,7 +88,9 @@ def _establish_new_socket_connection( log_message = f"Proxy connect response (session id: {session_id}):\n{text}" logger.debug(log_message) if status != 200: - raise Exception(f"Failed to connect to the proxy (proxy: {proxy}, connect status code: {status})") + raise Exception( + f"Failed to connect to the proxy (proxy: {_redact_credentials(proxy)}, connect status code: {status})" + ) sock = ssl_context.wrap_socket( sock, diff --git a/tests/test_proxy_env_variable_loader.py b/tests/test_proxy_env_variable_loader.py index 3e9681f22..3a19a5afb 100644 --- a/tests/test_proxy_env_variable_loader.py +++ b/tests/test_proxy_env_variable_loader.py @@ -1,7 +1,11 @@ +import logging import os import unittest +from unittest.mock import Mock, patch +from threading import Lock from slack_sdk.proxy_env_variable_loader import load_http_proxy_from_env +from slack_sdk.socket_mode.builtin.internals import _establish_new_socket_connection from tests.helpers import remove_os_env_temporarily, restore_os_env @@ -38,3 +42,74 @@ def test_proxy_url_is_none_case(self): os.environ.pop("http_proxy", None) url = load_http_proxy_from_env() self.assertEqual(url, None) + + def test_credentials_are_not_logged(self): + os.environ["HTTPS_PROXY"] = "http://bob:secret@example.com:8080" + logger = logging.getLogger("test_proxy_env_variable_loader") + with self.assertLogs(logger, level="DEBUG") as logs: + url = load_http_proxy_from_env(logger) + # the proxy URL itself is returned unchanged + self.assertEqual(url, "http://bob:secret@example.com:8080") + output = "\n".join(logs.output) + self.assertNotIn("bob", output) + self.assertNotIn("secret", output) + self.assertIn("http://***@example.com:8080", output) + + def test_url_without_credentials_is_logged_as_is(self): + os.environ["HTTPS_PROXY"] = "http://localhost:9999" + logger = logging.getLogger("test_proxy_env_variable_loader") + with self.assertLogs(logger, level="DEBUG") as logs: + load_http_proxy_from_env(logger) + self.assertIn("http://localhost:9999", "\n".join(logs.output)) + + def test_credentials_in_unusual_proxy_urls_are_not_logged(self): + for value in ( + "bob:secret@proxy.example.com:3128", + "http://bob:pa/ss@proxy.example.com:3128", + "http://bob:pa#ss@proxy.example.com:3128", + "http://bob:pa?ss@proxy.example.com:3128", + "http://token@proxy.example.com:3128", + "http://bob:pa@ss@proxy.example.com:3128", + ): + with self.subTest(value=value): + os.environ["HTTPS_PROXY"] = value + logger = logging.getLogger("test_proxy_env_variable_loader") + with self.assertLogs(logger, level="DEBUG") as logs: + self.assertEqual(load_http_proxy_from_env(logger), value) + expected = ( + "http://***@proxy.example.com:3128" if value.startswith("http://") else "***@proxy.example.com:3128" + ) + self.assertEqual( + logs.records[0].getMessage(), "HTTP proxy URL has been loaded from an env variable: " + expected + ) + + def test_unparsable_proxy_url_is_not_logged(self): + os.environ["HTTPS_PROXY"] = "http://[invalid" + logger = logging.getLogger("test_proxy_env_variable_loader") + with self.assertLogs(logger, level="DEBUG") as logs: + self.assertEqual(load_http_proxy_from_env(logger), "http://[invalid") + self.assertIn("(unparsable URL)", logs.output[0]) + self.assertNotIn("[invalid", logs.output[0]) + + def test_socket_mode_proxy_error_redacts_credentials(self): + with patch("slack_sdk.socket_mode.builtin.internals.socket.create_connection"), patch( + "slack_sdk.socket_mode.builtin.internals._parse_connect_response", + return_value=(407, "Proxy Authentication Required"), + ): + with self.assertRaises(Exception) as error: + _establish_new_socket_connection( + session_id="test", + server_hostname="example.com", + server_port=443, + logger=Mock(), + sock_send_lock=Lock(), + receive_timeout=1, + proxy="http://bob:secret@proxy.example.com:3128", + proxy_headers=None, + trace_enabled=False, + ssl_context=Mock(), + ) + self.assertNotIn("bob", str(error.exception)) + self.assertNotIn("secret", str(error.exception)) + self.assertIn("http://***@proxy.example.com:3128", str(error.exception)) + self.assertIn("407", str(error.exception))