fix(mcp): reject negative mock_balance in FinStripe get_account_balance - #564
Conversation
…ce (GenAI-Security-Project#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 GenAI-Security-Project#329
There was a problem hiding this comment.
Pull request overview
This PR fixes FinStripe MCP’s get_account_balance so a negative mock_balance from server_config is rejected instead of being returned as available_balance, addressing issue #329 where agents could make incorrect affordability decisions.
Changes:
- Add a negative-balance guard in
finbot/mcp/servers/finstripe/server.py:get_account_balance. - Add unit tests covering negative, zero, and default positive
mock_balancebehaviors.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| finbot/mcp/servers/finstripe/server.py | Rejects negative mock_balance values returned by get_account_balance. |
| tests/unit/mcp/test_finstripe.py | Adds regression/edge-case tests for mock_balance validation behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| mock_balance = config.get("mock_balance", DEFAULT_CONFIG["mock_balance"]) | ||
| if mock_balance < 0: | ||
| return {"error": "mock_balance is invalid: balance cannot be negative"} |
There was a problem hiding this comment.
Already handled — there's an isinstance(mock_balance, bool) or not isinstance(..., (int, float)) guard right before the < 0 check, returns a clean error instead of raising.
| 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. |
There was a problem hiding this comment.
Already past tense in the current docstring — 'had no bounds check... before this fix'. Good catch on the pattern though, would've been a real issue.
…enAI-Security-Project#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 GenAI-Security-Project#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.
|
Addressed the Copilot review feedback:
5/5 tests passing after both changes. |
Summary
Fixes #329.
get_account_balancereadmock_balancestraight fromserver_configwith no validation at all — a negative value was returned as-is as the account'savailable_balance. Agents (e.g. the Payments Agent) reason over this value when deciding whether a payment is affordable, so a poisoned config with a negative balance could confuse those decisions.Fix
Rejects negative balances with a clear error. Zero and ordinary positive balances are unaffected.
Test plan
tests/unit/mcp/test_finstripe.py— reproduces the exact issue repro steps (server_config={'mock_balance': -5000}) against the unfixed code first, then confirms the fixpytest tests/unit/mcp/test_finstripe.py— 3/3 passing