Skip to content

Fix(finstripe): resolve missing invoice amount validation in create_transfer#463

Open
Jean-Regis-M wants to merge 1 commit intoGenAI-Security-Project:mainfrom
Jean-Regis-M:patch-53
Open

Fix(finstripe): resolve missing invoice amount validation in create_transfer#463
Jean-Regis-M wants to merge 1 commit intoGenAI-Security-Project:mainfrom
Jean-Regis-M:patch-53

Conversation

@Jean-Regis-M
Copy link
Copy Markdown
Contributor

Summary

Fixes #335

create_transfer committed any caller-supplied amount to the database without ever
comparing it against the linked invoice's recorded amount, enabling silent over- and
under-payment.


Problem

Inside create_finstripe_server, the create_transfer tool accepted invoice_id
as a parameter but passed it straight to repo.create_transaction() with no prior
lookup of the Invoice record:

# Before  invoice.amount is never fetched or compared
txn = repo.create_transaction(
    invoice_id=invoice_id,
    amount=amount,   # ← completely unchecked
    ...
)

Any caller could pass amount=9999.0 against an invoice of 1000.0 and receive a
"status": "completed" transfer. Both directions are exploitable:

  • Over-payment wastes company funds
  • Under-payment leaves vendor debts unresolved
  • Prompt injection surface exploitable by a malicious MCP tool description
    injected via MCPServerConfig.tool_overrides_json

Root Cause

invoice_id was used solely as a foreign-key reference for the transaction record.
The Invoice row was never queried, so no business-rule enforcement on amount
was possible. This is a validation gap at the service boundary the tool accepted
untrusted input and wrote it to the database without asserting a known invariant.


Solution

Added two sequential guards inside the existing with db_session() as db: block,
before repo.create_transaction() is called:

  1. Invoice existence check query Invoice filtered by both id and
    session_context.namespace (prevents cross-tenant leakage); return an error dict
    if not found.
  2. Amount equality check compare round(amount, 2) against
    round(invoice.amount, 2) to absorb floating-point representation noise at
    cent-level precision; return an error dict on mismatch.
# After
invoice = db.query(Invoice).filter(
    Invoice.id == invoice_id,
    Invoice.namespace == session_context.namespace,
).first()

if not invoice:
    return {"error": f"Invoice {invoice_id} not found"}

if round(amount, 2) != round(invoice.amount, 2):
    return {"error": f"Amount {amount} does not match invoice amount {invoice.amount}"}

# repo.create_transaction() only reached when both guards pass

The happy path matching amounts is completely unaffected. The return shape
of {"error": str} is consistent with the existing pattern used in get_transfer.


Impact

  • No breaking changes to the tool's public interface or return schema
  • Namespace-scoped invoice query no cross-tenant data exposure
  • All mutation is gated behind two deterministic, side-effect-free checks
  • Zero changes outside the create_transfer function body

Testing

# Must pass  validates the new guard rejects mismatched amounts
pytest tests/unit/mcp/test_finstripe.py::TestFloatEdgeCases::test_mcp_float_007_amount_matches_invoice_amount_not_enforced -v

# Must continue to pass validation matching amounts still complete successfully
pytest tests/unit/mcp/test_finstripe.py::test_mcp_create_transfer_001_successful_transfer -v

Tasks

  • Identified root cause: Invoice record never fetched before create_transaction() call
  • Added from finbot.core.data.models import Invoice import
  • Added namespace-scoped invoice existence guard with error return
  • Added cent-precision amount equality guard with error return
  • Confirmed happy path (matching amounts) is unaffected
  • Confirmed error return shape matches existing convention in get_transfer
  • Verified test_mcp_float_007 passes with fix applied
  • Verified test_mcp_create_transfer_001 continues to pass

…ing transaction

Root cause:
create_transfer passed invoice_id directly to repo.create_transaction() without
ever fetching the Invoice record, allowing arbitrary amounts to bypass validation.

Solution:
Query Invoice within the db_session block before create_transaction; return an
error dict if invoice is not found or amount (rounded to 2dp) does not match.

Impact:
No breaking changes. Matching amounts pass unchanged. Early return prevents
any DB mutation on mismatch. Namespace-scoped query prevents cross-tenant leakage.

Signed-off-by: JEAN REGIS <240509606@firat.edu.tr>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug_126_MUST_FIX: Test Case MCP-FLOAT-007: Transfer amount not validated against invoice amount — over/under-payment goes unchecked

1 participant