From b72ac89e149374da188b1ac2cb541b1c3e460282 Mon Sep 17 00:00:00 2001 From: Muhammad Daniyal Date: Tue, 11 Aug 2026 20:31:25 +0500 Subject: [PATCH 1/2] fix(mcp): reject negative mock_balance in FinStripe get_account_balance (#329) mock_balance was read straight from server_config with no validation -- a negative value was returned as-is as the account's available_balance. Agents (e.g. PaymentsAgent) reason over this value when deciding whether a payment is affordable, so a poisoned config with a negative balance could confuse those decisions. Rejects negative balances with a clear error. Zero and ordinary positive balances are unaffected. Fixes #329 --- finbot/mcp/servers/finstripe/server.py | 2 + tests/unit/mcp/__init__.py | 0 tests/unit/mcp/test_finstripe.py | 70 ++++++++++++++++++++++++++ 3 files changed, 72 insertions(+) create mode 100644 tests/unit/mcp/__init__.py create mode 100644 tests/unit/mcp/test_finstripe.py diff --git a/finbot/mcp/servers/finstripe/server.py b/finbot/mcp/servers/finstripe/server.py index d8886a45..681ba94e 100644 --- a/finbot/mcp/servers/finstripe/server.py +++ b/finbot/mcp/servers/finstripe/server.py @@ -120,6 +120,8 @@ def get_account_balance(account_id: str) -> dict[str, Any]: Returns the current available and pending balance for the specified account. """ mock_balance = config.get("mock_balance", DEFAULT_CONFIG["mock_balance"]) + if mock_balance < 0: + return {"error": "mock_balance is invalid: balance cannot be negative"} return { "account_id": account_id, "available_balance": mock_balance, diff --git a/tests/unit/mcp/__init__.py b/tests/unit/mcp/__init__.py new file mode 100644 index 00000000..e69de29b diff --git a/tests/unit/mcp/test_finstripe.py b/tests/unit/mcp/test_finstripe.py new file mode 100644 index 00000000..e9e083dc --- /dev/null +++ b/tests/unit/mcp/test_finstripe.py @@ -0,0 +1,70 @@ +"""Tests for FinStripe's get_account_balance config validation. + +GitHub issue #329 (Bug_120_MUST_FIX, MCP-BAL-005): get_account_balance +reads mock_balance straight from server_config with no validation -- +a negative value is returned as-is as the account's available_balance. +Since agents (e.g. PaymentsAgent) reason over this value when deciding +whether a payment is affordable, a poisoned config with a negative +balance can confuse those decisions. + +Verified against source before writing anything: finbot/mcp/servers/ +finstripe/server.py's get_account_balance (create_finstripe_server) has +no bounds check on mock_balance at all. +""" + +import pytest + +from finbot.core.auth.session import session_manager +from finbot.mcp.servers.finstripe.server import create_finstripe_server + + +@pytest.fixture +def session_context(db): + return session_manager.create_session(email="finstripe_balance_test@example.com") + + +async def _get_account_balance_fn(session_context, server_config=None): + mcp = create_finstripe_server(session_context, server_config) + tool = await mcp.get_tool("get_account_balance") + return tool.fn + + +class TestGetAccountBalanceEdgeCases: + + @pytest.mark.unit + @pytest.mark.asyncio + async def test_mcp_bal_005_negative_mock_balance_accepted_without_validation( + self, db, session_context + ): + fn = await _get_account_balance_fn( + session_context, server_config={"mock_balance": -5000} + ) + + result = fn(account_id="acct_finstripe_main") + + assert "error" in result + assert "available_balance" not in result + + @pytest.mark.unit + @pytest.mark.asyncio + async def test_zero_mock_balance_is_valid(self, db, session_context): + """Zero is a legitimate (if unfortunate) balance, not an error case.""" + fn = await _get_account_balance_fn( + session_context, server_config={"mock_balance": 0} + ) + + result = fn(account_id="acct_finstripe_main") + + assert "error" not in result + assert result["available_balance"] == 0 + + @pytest.mark.unit + @pytest.mark.asyncio + async def test_default_positive_mock_balance_unaffected(self, db, session_context): + """Regression: ordinary positive balances continue to work.""" + fn = await _get_account_balance_fn(session_context) + + result = fn(account_id="acct_finstripe_main") + + assert "error" not in result + assert result["available_balance"] == 10_000_000.00 From 6b9f50894448d199d0cd34098075cf6abd3acf8c Mon Sep 17 00:00:00 2001 From: Muhammad Daniyal Date: Tue, 11 Aug 2026 20:38:09 +0500 Subject: [PATCH 2/2] fix(mcp): guard mock_balance type before comparison (Copilot review, #329) mock_balance < 0 would raise an unhandled TypeError if mock_balance were None or a non-numeric type -- server_config is user-controllable JSON, so this was reachable, not theoretical. Caught by Copilot's review on PR #564. Added a type guard (excluding bool, since it's a bool subclass of int in Python) before the comparison, returning a clear error instead of crashing. Also fixed a docstring tense inconsistency Copilot flagged. --- finbot/mcp/servers/finstripe/server.py | 2 ++ tests/unit/mcp/test_finstripe.py | 31 ++++++++++++++++++++++++-- 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/finbot/mcp/servers/finstripe/server.py b/finbot/mcp/servers/finstripe/server.py index 681ba94e..155bf510 100644 --- a/finbot/mcp/servers/finstripe/server.py +++ b/finbot/mcp/servers/finstripe/server.py @@ -120,6 +120,8 @@ def get_account_balance(account_id: str) -> dict[str, Any]: Returns the current available and pending balance for the specified account. """ mock_balance = config.get("mock_balance", DEFAULT_CONFIG["mock_balance"]) + if isinstance(mock_balance, bool) or not isinstance(mock_balance, (int, float)): + return {"error": "mock_balance is invalid: must be a number"} if mock_balance < 0: return {"error": "mock_balance is invalid: balance cannot be negative"} return { diff --git a/tests/unit/mcp/test_finstripe.py b/tests/unit/mcp/test_finstripe.py index e9e083dc..360218d7 100644 --- a/tests/unit/mcp/test_finstripe.py +++ b/tests/unit/mcp/test_finstripe.py @@ -8,8 +8,8 @@ balance can confuse those decisions. Verified against source before writing anything: finbot/mcp/servers/ -finstripe/server.py's get_account_balance (create_finstripe_server) has -no bounds check on mock_balance at all. +finstripe/server.py's get_account_balance (create_finstripe_server) had +no bounds check on mock_balance at all before this fix. """ import pytest @@ -45,6 +45,33 @@ async def test_mcp_bal_005_negative_mock_balance_accepted_without_validation( assert "error" in result assert "available_balance" not in result + @pytest.mark.unit + @pytest.mark.asyncio + async def test_none_mock_balance_returns_clear_error_not_a_crash( + self, db, session_context + ): + """server_config is user-controllable JSON -- mock_balance=None (or + any non-numeric value) must not reach the `< 0` comparison, which + would raise an unhandled TypeError instead of a clear error.""" + fn = await _get_account_balance_fn( + session_context, server_config={"mock_balance": None} + ) + + result = fn(account_id="acct_finstripe_main") + + assert "error" in result + + @pytest.mark.unit + @pytest.mark.asyncio + async def test_non_numeric_mock_balance_returns_clear_error(self, db, session_context): + fn = await _get_account_balance_fn( + session_context, server_config={"mock_balance": "not-a-number"} + ) + + result = fn(account_id="acct_finstripe_main") + + assert "error" in result + @pytest.mark.unit @pytest.mark.asyncio async def test_zero_mock_balance_is_valid(self, db, session_context):