From 81ab51670c634ea9484df51750e8f0064d6dccda Mon Sep 17 00:00:00 2001 From: dceoy <1938249+dceoy@users.noreply.github.com> Date: Fri, 3 Jul 2026 23:04:49 +0900 Subject: [PATCH 1/3] Mask MT5 passwords and fix stale docs (#93 #95) --- AGENTS.md | 13 ++++--- README.md | 7 ++-- pdmt5/dataframe.py | 35 +++++++++++++++---- tests/test_dataframe.py | 77 +++++++++++++++++++++++++++++++++++++++-- 4 files changed, 112 insertions(+), 20 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 0d424f8..e0bca4b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -4,9 +4,9 @@ This is a Python package for pandas-friendly MetaTrader 5 access. Core code lives in `pdmt5/`: `mt5.py` wraps MT5 calls, `dataframe.py` adds DataFrame/dict -conversions, `trading.py` contains trading helpers, and `constants.py`/`utils.py` -hold shared parsing and utilities. Tests live in `tests/`, documentation in -`docs/`, and project settings in `pyproject.toml`. Keep `uv.lock` in sync. +conversions, and `constants.py`/`utils.py` hold shared parsing and utilities. +Tests live in `tests/`, documentation in `docs/`, and project settings in +`pyproject.toml`. Keep `uv.lock` in sync. ## Build, Test, and Development Commands @@ -56,7 +56,6 @@ Windows/MT5 assumptions reviewers need to verify. ## Security & Configuration Tips -Do not commit MT5 account credentials, terminal paths containing secrets, or live -trading configuration. Use `Mt5Config` inputs or environment-specific setup in -local scripts. Preserve dry-run pathways and mock-based tests when modifying -trading behavior. +Do not commit MT5 account credentials or terminal paths containing secrets. Use +`Mt5Config` inputs or environment-specific setup in local scripts. Preserve +mock-based tests when modifying MT5 connection or data-access behavior. diff --git a/README.md b/README.md index 1f1b0f2..55b4f1b 100644 --- a/README.md +++ b/README.md @@ -22,7 +22,7 @@ High-level trading orchestration (market-order construction, margin-budget sizin and ORDER_TYPE values with official names, short aliases, or valid integers - **Context Manager Support**: Clean initialization and cleanup with `with` statements; `Mt5DataClient` applies `Mt5Config` automatically - **Time Series Ready**: OHLCV data with proper datetime indexing -- **Robust Error Handling**: Custom exceptions with detailed MT5 error information +- **Robust Error Handling**: Custom exceptions with detailed MT5 failure messages - **Direct Order Primitives**: `order_check` / `order_send` wrappers with DataFrame/dict conversions ## Requirements @@ -335,7 +335,7 @@ This project maintains high code quality standards: - **Type Checking**: Strict mode with pyright - **Linting**: Comprehensive ruff configuration with 40+ rule categories -- **Testing**: pytest with coverage tracking (minimum 90%) +- **Testing**: pytest with 100% branch coverage enforcement - **Documentation**: Google-style docstrings ## Error Handling @@ -352,8 +352,7 @@ try: ) except Mt5RuntimeError as e: print(f"MT5 Error: {e}") - print(f"Error code: {e.error_code}") - print(f"Description: {e.description}") + print("Inspect the exception message for the MT5 status details.") ``` ## Limitations diff --git a/pdmt5/dataframe.py b/pdmt5/dataframe.py index 9f1b6cd..5e6e650 100644 --- a/pdmt5/dataframe.py +++ b/pdmt5/dataframe.py @@ -8,7 +8,7 @@ from typing import Any, Self, cast import pandas as pd -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, SecretStr, field_validator from .mt5 import Mt5Client, Mt5RuntimeError from .utils import ( @@ -27,12 +27,26 @@ class Mt5Config(BaseModel): default=None, description="Path to MetaTrader5 terminal EXE file" ) login: int | None = Field(default=None, description="Trading account login") - password: str | None = Field(default=None, description="Trading account password") + password: str | SecretStr | None = Field( + default=None, description="Trading account password" + ) server: str | None = Field(default=None, description="Trading server name") timeout: int | None = Field( default=None, description="Connection timeout in milliseconds" ) + @field_validator("password", mode="before") + @classmethod + def _coerce_password(cls, value: str | SecretStr | None) -> SecretStr | None: + """Normalize password inputs to SecretStr for masked serialization. + + Returns: + Password value stored as SecretStr when provided. + """ + if value is None or isinstance(value, SecretStr): + return value + return SecretStr(value) + class Mt5DataClient(Mt5Client): """MetaTrader5 data client with pandas DataFrame and dictionary conversion. @@ -85,17 +99,22 @@ def _as_single_row_df(data: dict[str, Any]) -> pd.DataFrame: """Return a one-row DataFrame from a dictionary.""" return pd.DataFrame([data]) + @staticmethod + def _unwrap_password(password: str | SecretStr | None) -> str | None: + """Return a plain password string only at the MT5 call boundary.""" + if isinstance(password, SecretStr): + return password.get_secret_value() + return password + def initialize_and_login_mt5( self, path: str | None = None, login: int | None = None, - password: str | None = None, + password: str | SecretStr | None = None, server: str | None = None, timeout: int | None = None, ) -> None: - """Initialize MetaTrader5 connection with retry logic. - - This method overrides the base class to add retry logic and use config values. + """Initialize an MT5 connection with config-aware retry and login support. Args: path: Path to terminal EXE file (overrides config). @@ -109,7 +128,9 @@ def initialize_and_login_mt5( """ path = path or self.config.path login_value = login if login is not None else self.config.login - password_value = password if password is not None else self.config.password + password_value = self._unwrap_password( + password if password is not None else self.config.password + ) server_value = server if server is not None else self.config.server timeout_value = timeout if timeout is not None else self.config.timeout for i in range(1 + max(0, self.retry_count)): diff --git a/tests/test_dataframe.py b/tests/test_dataframe.py index 1f3720a..a36db7d 100644 --- a/tests/test_dataframe.py +++ b/tests/test_dataframe.py @@ -8,7 +8,7 @@ import numpy as np import pandas as pd import pytest -from pydantic import ValidationError +from pydantic import SecretStr, ValidationError from pytest_mock import MockerFixture from pdmt5.dataframe import Mt5Config, Mt5DataClient @@ -55,6 +55,7 @@ "market_book_release", "market_book_get", ) +_MASKED_SECRET = "*" * 10 @pytest.fixture(autouse=True) @@ -632,7 +633,8 @@ def test_custom_config(self) -> None: timeout=30000, ) assert config.login == 123456 - assert config.password == "secret" # noqa: S105 + assert isinstance(config.password, SecretStr) + assert config.password.get_secret_value() == "secret" assert config.server == "Demo-Server" assert config.timeout == 30000 @@ -642,6 +644,25 @@ def test_config_immutable(self) -> None: with pytest.raises(ValidationError): config.login = 123456 + def test_config_masks_password_in_string_representations(self) -> None: + """Test SecretStr masks the password in printable config output.""" + config = Mt5Config(password="secret") + dumped_password = config.model_dump()["password"] + + assert "secret" not in repr(config) + assert "secret" not in str(config) + assert "secret" not in repr(config.model_dump()) + assert isinstance(dumped_password, SecretStr) + assert str(dumped_password) == _MASKED_SECRET + assert config.model_dump(mode="json")["password"] == _MASKED_SECRET + + def test_config_accepts_secretstr_input(self) -> None: + """Test SecretStr inputs are preserved without exposing the secret.""" + config = Mt5Config(password=SecretStr("secret")) + + assert isinstance(config.password, SecretStr) + assert config.password.get_secret_value() == "secret" + class TestMt5DataClient: """Test Mt5DataClient class.""" @@ -666,8 +687,31 @@ def test_init_custom_config(self, mock_mt5_import: ModuleType | None) -> None: client = Mt5DataClient(mt5=mock_mt5_import, config=config) assert client.config == config assert client.config.login == 123456 + assert isinstance(client.config.password, SecretStr) + assert client.config.password.get_secret_value() == "test" assert client.config.timeout == 30000 + def test_client_masks_nested_password_in_dumps( + self, mock_mt5_import: ModuleType | None + ) -> None: + """Test nested client config dumps do not expose a literal password.""" + assert mock_mt5_import is not None + client = Mt5DataClient( + mt5=mock_mt5_import, + config=Mt5Config(password="secret"), + ) + dumped_password = client.model_dump()["config"]["password"] + + assert "secret" not in repr(client) + assert "secret" not in str(client) + assert "secret" not in repr(client.model_dump()) + assert isinstance(dumped_password, SecretStr) + assert str(dumped_password) == _MASKED_SECRET + assert ( + client.model_dump(mode="json", exclude={"mt5"})["config"]["password"] + == _MASKED_SECRET + ) + def test_mt5_module_default_factory(self, mocker: MockerFixture) -> None: """Test initialization with default mt5 module factory.""" # Mock the importlib.import_module to verify it's called @@ -860,6 +904,35 @@ def test_initialize_and_login_shuts_down_after_login_exception( mock_mt5_import.shutdown.assert_called_once() + def test_initialize_and_login_unwraps_secret_password_override( + self, mock_mt5_import: ModuleType | None + ) -> None: + """Test SecretStr overrides are unwrapped before MT5 calls.""" + assert mock_mt5_import is not None + mock_mt5_import.initialize.return_value = True + mock_mt5_import.login.return_value = True + client = Mt5DataClient(mt5=mock_mt5_import, retry_count=0) + + client.initialize_and_login_mt5( + login=123456, + password=SecretStr("secret"), + server="Demo", + timeout=60000, + ) + + mock_mt5_import.initialize.assert_called_once_with( + login=123456, + password="secret", + server="Demo", + timeout=60000, + ) + mock_mt5_import.login.assert_called_once_with( + 123456, + password="secret", + server="Demo", + timeout=60000, + ) + @pytest.mark.parametrize( ("client_method", "mt5_method"), [ From 7f97cc5c96da9dad99ca955206537c161e2460b9 Mon Sep 17 00:00:00 2001 From: dceoy <1938249+dceoy@users.noreply.github.com> Date: Fri, 3 Jul 2026 23:13:59 +0900 Subject: [PATCH 2/3] Fix SecretStr handling for direct MT5 login --- pdmt5/dataframe.py | 7 ------- pdmt5/mt5.py | 17 ++++++++++++----- tests/test_mt5.py | 43 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 12 deletions(-) diff --git a/pdmt5/dataframe.py b/pdmt5/dataframe.py index 5e6e650..60ab802 100644 --- a/pdmt5/dataframe.py +++ b/pdmt5/dataframe.py @@ -99,13 +99,6 @@ def _as_single_row_df(data: dict[str, Any]) -> pd.DataFrame: """Return a one-row DataFrame from a dictionary.""" return pd.DataFrame([data]) - @staticmethod - def _unwrap_password(password: str | SecretStr | None) -> str | None: - """Return a plain password string only at the MT5 call boundary.""" - if isinstance(password, SecretStr): - return password.get_secret_value() - return password - def initialize_and_login_mt5( self, path: str | None = None, diff --git a/pdmt5/mt5.py b/pdmt5/mt5.py index 4c43b5c..d4daf11 100644 --- a/pdmt5/mt5.py +++ b/pdmt5/mt5.py @@ -10,7 +10,7 @@ from types import ModuleType # noqa: TC003 from typing import TYPE_CHECKING, Any, Concatenate, ParamSpec, Self, TypeVar -from pydantic import BaseModel, ConfigDict, Field +from pydantic import BaseModel, ConfigDict, Field, SecretStr if TYPE_CHECKING: from collections.abc import Callable @@ -105,12 +105,19 @@ def __exit__( """Context manager exit.""" self.shutdown() + @staticmethod + def _unwrap_password(password: str | SecretStr | None) -> str | None: + """Return a plain password string only at the MT5 call boundary.""" + if isinstance(password, SecretStr): + return password.get_secret_value() + return password + @_log_mt5_last_status_code def initialize( self, path: str | None = None, login: int | None = None, - password: str | None = None, + password: str | SecretStr | None = None, server: str | None = None, timeout: int | None = None, ) -> bool: @@ -130,7 +137,7 @@ def initialize( k: v for k, v in { "login": login, - "password": password, + "password": self._unwrap_password(password), "server": server, "timeout": timeout, }.items() @@ -157,7 +164,7 @@ def initialize( def login( self, login: int, - password: str | None = None, + password: str | SecretStr | None = None, server: str | None = None, timeout: int | None = None, ) -> bool: @@ -179,7 +186,7 @@ def login( **{ k: v for k, v in { - "password": password, + "password": self._unwrap_password(password), "server": server, "timeout": timeout, }.items() diff --git a/tests/test_mt5.py b/tests/test_mt5.py index 4c6e9ef..f9c3ae1 100644 --- a/tests/test_mt5.py +++ b/tests/test_mt5.py @@ -6,6 +6,7 @@ from typing import TYPE_CHECKING, Any import pytest +from pydantic import SecretStr from pdmt5.mt5 import Mt5Client, Mt5RuntimeError @@ -117,6 +118,27 @@ def test_initialize_with_credentials_without_path( timeout=60000, ) + def test_initialize_unwraps_secret_password( + self, client: Mt5Client, mock_mt5: Mock + ) -> None: + """Test initialization unwraps SecretStr passwords before MT5 calls.""" + mock_mt5.initialize.return_value = True + + result = client.initialize( + login=12345, + password=SecretStr("secret"), + server="Demo", + timeout=60000, + ) + + assert result is True + mock_mt5.initialize.assert_called_once_with( + login=12345, + password="secret", + server="Demo", + timeout=60000, + ) + def test_initialize_failure(self, client: Mt5Client, mock_mt5: Mock) -> None: """Test initialization failure.""" mock_mt5.initialize.return_value = False @@ -175,6 +197,27 @@ def test_login_failure(self, initialized_client: Mt5Client, mock_mt5: Mock) -> N assert result is False + def test_login_unwraps_secret_password( + self, initialized_client: Mt5Client, mock_mt5: Mock + ) -> None: + """Test login unwraps SecretStr passwords before MT5 calls.""" + mock_mt5.login.return_value = True + + result = initialized_client.login( + login=12345, + password=SecretStr("secret"), + server="Demo", + timeout=60000, + ) + + assert result is True + mock_mt5.login.assert_called_once_with( + 12345, + password="secret", + server="Demo", + timeout=60000, + ) + def test_version( self, initialized_client: Mt5Client, From 6d7bf9fb48540ad61878213c8c0ac6750dbc72f4 Mon Sep 17 00:00:00 2001 From: dceoy <1938249+dceoy@users.noreply.github.com> Date: Fri, 3 Jul 2026 23:24:35 +0900 Subject: [PATCH 3/3] Parametrize password-related tests in test_mt5 and test_dataframe - Refactor initialize() tests: combine test_initialize_with_parameters, test_initialize_with_credentials_without_path, and test_initialize_unwraps_secret_password into parametrized test_initialize_with_password_types covering both path variations and password types (str and SecretStr). All cases verify MT5 receives unwrapped plain password strings. - Refactor login() tests: combine test_login_success, test_login_unwraps_secret_password, and test_login_without_timeout into parametrized test_login_with_password_types covering timeout variations and password types (str and SecretStr). All cases verify MT5 receives unwrapped plain password strings. Keep test_login_failure as separate test for failure path. - Parametrize Mt5Config password coercion: combine test_custom_config and test_config_accepts_secretstr_input into parametrized test_config_password_coercion covering both str and SecretStr inputs. Keep test_config_masks_password_in_string_representations and test_config_immutable as separate tests for clarity. - Maintain 100% branch coverage and explicit assertions that MT5 boundary calls receive unwrapped plain password strings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- tests/test_dataframe.py | 21 ++--- tests/test_mt5.py | 192 ++++++++++++++++++++-------------------- 2 files changed, 105 insertions(+), 108 deletions(-) diff --git a/tests/test_dataframe.py b/tests/test_dataframe.py index a36db7d..3f130d5 100644 --- a/tests/test_dataframe.py +++ b/tests/test_dataframe.py @@ -624,11 +624,19 @@ def test_default_config(self) -> None: assert config.server is None assert config.timeout is None - def test_custom_config(self) -> None: - """Test custom configuration.""" + @pytest.mark.parametrize( + "password_input", + [ + "secret", + SecretStr("secret"), + ], + ids=["str-password", "secretstr-password"], + ) + def test_config_password_coercion(self, password_input: str | SecretStr) -> None: + """Test Mt5Config coerces password inputs to SecretStr.""" config = Mt5Config( login=123456, - password="secret", + password=password_input, server="Demo-Server", timeout=30000, ) @@ -656,13 +664,6 @@ def test_config_masks_password_in_string_representations(self) -> None: assert str(dumped_password) == _MASKED_SECRET assert config.model_dump(mode="json")["password"] == _MASKED_SECRET - def test_config_accepts_secretstr_input(self) -> None: - """Test SecretStr inputs are preserved without exposing the secret.""" - config = Mt5Config(password=SecretStr("secret")) - - assert isinstance(config.password, SecretStr) - assert config.password.get_secret_value() == "secret" - class TestMt5DataClient: """Test Mt5DataClient class.""" diff --git a/tests/test_mt5.py b/tests/test_mt5.py index f9c3ae1..5b2a92d 100644 --- a/tests/test_mt5.py +++ b/tests/test_mt5.py @@ -74,70 +74,63 @@ def test_initialize_success(self, client: Mt5Client, mock_mt5: Mock) -> None: assert client._is_initialized is True # type: ignore[reportPrivateUsage] mock_mt5.initialize.assert_called_once_with() - def test_initialize_with_parameters( - self, client: Mt5Client, mock_mt5: Mock - ) -> None: - """Test initialization with parameters.""" - mock_mt5.initialize.return_value = True - - result = client.initialize( - path="/path/to/mt5.exe", - login=12345, - password="secret", - server="Demo", - timeout=60000, - ) - - assert result is True - mock_mt5.initialize.assert_called_once_with( - "/path/to/mt5.exe", - login=12345, - password="secret", - server="Demo", - timeout=60000, - ) - - def test_initialize_with_credentials_without_path( - self, client: Mt5Client, mock_mt5: Mock - ) -> None: - """Test initialization passes credentials even without a path.""" - mock_mt5.initialize.return_value = True - - result = client.initialize( - login=12345, - password="secret", - server="Demo", - timeout=60000, - ) - - assert result is True - mock_mt5.initialize.assert_called_once_with( - login=12345, - password="secret", - server="Demo", - timeout=60000, - ) - - def test_initialize_unwraps_secret_password( - self, client: Mt5Client, mock_mt5: Mock + @pytest.mark.parametrize( + ("path", "password_input"), + [ + ("/path/to/mt5.exe", "secret"), + ("/path/to/mt5.exe", SecretStr("secret")), + (None, "secret"), + (None, SecretStr("secret")), + ], + ids=[ + "with-path-str-password", + "with-path-secretstr-password", + "without-path-str-password", + "without-path-secretstr-password", + ], + ) + def test_initialize_with_password_types( + self, + client: Mt5Client, + mock_mt5: Mock, + path: str | None, + password_input: str | SecretStr, ) -> None: - """Test initialization unwraps SecretStr passwords before MT5 calls.""" + """Test initialization unwraps passwords of different types before MT5 calls.""" mock_mt5.initialize.return_value = True - result = client.initialize( - login=12345, - password=SecretStr("secret"), - server="Demo", - timeout=60000, - ) + if path: + result = client.initialize( + path=path, + login=12345, + password=password_input, + server="Demo", + timeout=60000, + ) + else: + result = client.initialize( + login=12345, + password=password_input, + server="Demo", + timeout=60000, + ) assert result is True - mock_mt5.initialize.assert_called_once_with( - login=12345, - password="secret", - server="Demo", - timeout=60000, - ) + if path: + mock_mt5.initialize.assert_called_once_with( + path, + login=12345, + password="secret", + server="Demo", + timeout=60000, + ) + else: + mock_mt5.initialize.assert_called_once_with( + login=12345, + password="secret", + server="Demo", + timeout=60000, + ) def test_initialize_failure(self, client: Mt5Client, mock_mt5: Mock) -> None: """Test initialization failure.""" @@ -176,18 +169,52 @@ def test_initialize_if_needed_calls_initialize( mock_mt5.initialize.assert_called_once() assert client._is_initialized is True # type: ignore[reportPrivateUsage] - def test_login_success(self, initialized_client: Mt5Client, mock_mt5: Mock) -> None: - """Test successful login.""" + @pytest.mark.parametrize( + ("password_input", "timeout"), + [ + ("secret", 60000), + (SecretStr("secret"), 60000), + ("secret", None), + (SecretStr("secret"), None), + ], + ids=[ + "str-password-with-timeout", + "secretstr-password-with-timeout", + "str-password-without-timeout", + "secretstr-password-without-timeout", + ], + ) + def test_login_with_password_types( + self, + initialized_client: Mt5Client, + mock_mt5: Mock, + password_input: str | SecretStr, + timeout: int | None, + ) -> None: + """Test login unwraps passwords of different types before MT5 calls.""" mock_mt5.login.return_value = True - result = initialized_client.login( - login=12345, password="secret", server="Demo", timeout=60000 - ) + if timeout: + result = initialized_client.login( + login=12345, + password=password_input, + server="Demo", + timeout=timeout, + ) + else: + result = initialized_client.login( + login=12345, password=password_input, server="Demo" + ) assert result is True - mock_mt5.login.assert_called_once_with( - 12345, password="secret", server="Demo", timeout=60000 - ) + if timeout: + mock_mt5.login.assert_called_once_with( + 12345, password="secret", server="Demo", timeout=timeout + ) + else: + mock_mt5.login.assert_called_once_with( + 12345, password="secret", server="Demo" + ) def test_login_failure(self, initialized_client: Mt5Client, mock_mt5: Mock) -> None: """Test login failure.""" @@ -197,27 +224,6 @@ def test_login_failure(self, initialized_client: Mt5Client, mock_mt5: Mock) -> N assert result is False - def test_login_unwraps_secret_password( - self, initialized_client: Mt5Client, mock_mt5: Mock - ) -> None: - """Test login unwraps SecretStr passwords before MT5 calls.""" - mock_mt5.login.return_value = True - - result = initialized_client.login( - login=12345, - password=SecretStr("secret"), - server="Demo", - timeout=60000, - ) - - assert result is True - mock_mt5.login.assert_called_once_with( - 12345, - password="secret", - server="Demo", - timeout=60000, - ) - def test_version( self, initialized_client: Mt5Client, @@ -853,16 +859,6 @@ def test_get_methods_with_parameters( assert result is not None getattr(mock_mt5, method_name).assert_called_with(**kwargs) - def test_login_without_timeout( - self, initialized_client: Mt5Client, mock_mt5: Mock - ) -> None: - """Test login method without timeout parameter.""" - mock_mt5.login.return_value = True - - result = initialized_client.login(12345, "secret", "Demo") - assert result is True - mock_mt5.login.assert_called_with(12345, password="secret", server="Demo") - def test_empty_market_book_get( self, initialized_client: Mt5Client, mock_mt5: Mock ) -> None: