fix(mcp): reject negative limit in FinStripe list_transfers - #565
Open
Deez-Automations wants to merge 2 commits into
Open
fix(mcp): reject negative limit in FinStripe list_transfers#565Deez-Automations wants to merge 2 commits into
Deez-Automations wants to merge 2 commits into
Conversation
…curity-Project#330) limit flowed straight through to the repository's SQLAlchemy .limit() call with no validation. A negative value produces undefined database behavior instead of a clear error -- confirmed it's silently treated as "no limit" on SQLite, returning the full result set rather than failing or being ignored as one might expect. Rejects negative limits with a clear error. Zero (a legitimate "give me nothing" request) and ordinary positive limits are unaffected. Fixes GenAI-Security-Project#330
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #330 by adding input validation to the FinStripe MCP server’s list_transfers tool so negative limit values are rejected instead of being passed through to SQLAlchemy’s .limit() (which can behave unexpectedly, e.g., SQLite treating negative limits as “no limit”).
Changes:
- Add a guard in
list_transfersto rejectlimit < 0with a clear error payload. - Add new unit tests covering negative limit rejection, zero-limit behavior, and a positive-limit regression case.
- Add
tests/unit/mcp/__init__.pyto ensure the new test module is import/package-safe.
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 | Adds negative-limit validation in list_transfers before calling the repository. |
| tests/unit/mcp/test_finstripe.py | Adds regression/edge-case tests for list_transfers limit handling. |
| tests/unit/mcp/init.py | Ensures tests.unit.mcp is treated as a package for test discovery/import behavior. |
Suppressed comments (1)
tests/unit/mcp/test_finstripe.py:11
- This part of the docstring states that list_transfers "has no validation on
limit", but the PR adds validation. Reword to clarify it was verified against the pre-fix source so the docstring stays correct.
Verified against source before writing anything: finbot/mcp/servers/
finstripe/server.py's list_transfers (create_finstripe_server) has no
validation on `limit`; it flows straight into PaymentTransactionRepository
.list_for_vendor's SQLAlchemy .limit(limit) call.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+3
to
+6
| GitHub issue #330 (Bug_121_MUST_FIX, MCP-LIST-006): list_transfers passes | ||
| `limit` straight through to the repository's query with no bounds check | ||
| at all -- a negative limit produces undefined database behavior instead | ||
| of a clear, diagnosable error. |
Author
There was a problem hiding this comment.
Already past tense in the current docstring, thought I'd caught both spots but let me know if you still see present tense anywhere.
Comment on lines
+139
to
144
| if limit < 0: | ||
| return {"error": "limit must be zero or a positive integer"} | ||
|
|
||
| with db_session() as db: | ||
| repo = PaymentTransactionRepository(db, session_context) | ||
| transactions = repo.list_for_vendor(vendor_id, limit=limit) |
Author
There was a problem hiding this comment.
Already moved to the repository level — list_for_vendor itself validates limit and offset now, so every call site (including get_payment_transactions) inherits the guard instead of just the MCP tool wrapper.
…pilot review, GenAI-Security-Project#330) The original fix only guarded the MCP tool's list_transfers wrapper. PaymentTransactionRepository.list_for_vendor is a shared repository with other real callers -- finbot/apps/vendor/routes/api.py's GET /payments/transactions route takes limit/offset directly as user-controlled query parameters with zero validation of its own, and was still reachable with a negative limit even after the MCP-only fix. Caught by Copilot's review on PR GenAI-Security-Project#565. Moved the authoritative guard into list_for_vendor itself, covering both limit AND offset (offset had the identical undefined-behavior gap, not mentioned in the original issue but caught while fixing the same root cause). The vendor route now catches the resulting ValueError and returns a proper 400, matching this file's own existing error-handling convention used elsewhere (e.g. invoice creation). The two other list_for_vendor callers in that file use a hardcoded limit=1000 with no user input, so they're unaffected.
Author
|
Addressed the Copilot review feedback:
9/9 tests passing after both changes (6 in the original file, 3 new ones directly exercising the repository-level guard). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #330.
list_transfers'slimitparameter flowed straight through to the repository's SQLAlchemy.limit()call with no validation. Confirmed the actual behavior: on SQLite, a negative limit is silently treated as "no limit at all," returning the full result set — undefined/surprising behavior rather than a clear error.Fix
Rejects negative limits with a clear error. Zero (a legitimate "give me nothing" request) and ordinary positive limits are unaffected.
Test plan
tests/unit/mcp/test_finstripe.py— reproduces the exact issue repro steps against the unfixed code first (confirmed the negative limit was silently returning all transfers instead of erroring), then confirms the fixpytest tests/unit/mcp/test_finstripe.py— 3/3 passing