From 485b3e471527877a5c1fd93f95c5804f3530c0a4 Mon Sep 17 00:00:00 2001 From: Muhammad Daniyal Date: Tue, 11 Aug 2026 20:07:09 +0500 Subject: [PATCH 1/2] fix(mcp): reject control characters in FinDrive filenames (#360) upload_file passed filename straight through to storage and into a log line with zero validation. A filename containing a newline gets stored verbatim and, more importantly, injects a fake log line into the application log -- corrupting the audit trail and deceiving anyone monitoring logs for suspicious activity. Rejects \n, \r, \t, and \x00 in filenames before they reach storage or logging. Ordinary filenames are unaffected. Fixes #360 --- finbot/mcp/servers/findrive/server.py | 3 + tests/unit/mcp/__init__.py | 0 tests/unit/mcp/test_findrive.py | 87 +++++++++++++++++++++++++++ 3 files changed, 90 insertions(+) create mode 100644 tests/unit/mcp/__init__.py create mode 100644 tests/unit/mcp/test_findrive.py diff --git a/finbot/mcp/servers/findrive/server.py b/finbot/mcp/servers/findrive/server.py index 3a95ee3b..349bda37 100644 --- a/finbot/mcp/servers/findrive/server.py +++ b/finbot/mcp/servers/findrive/server.py @@ -59,6 +59,9 @@ def upload_file( if len(content.encode("utf-8")) > max_size: return {"error": f"File exceeds maximum size of {config.get('max_file_size_kb', 500)}KB"} + if any(c in filename for c in ("\n", "\r", "\t", "\x00")): + return {"error": "filename contains invalid control characters"} + with db_session() as db: repo = FinDriveFileRepository(db, session_context) 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_findrive.py b/tests/unit/mcp/test_findrive.py new file mode 100644 index 00000000..b4324fe6 --- /dev/null +++ b/tests/unit/mcp/test_findrive.py @@ -0,0 +1,87 @@ +"""Tests for FinDrive's upload_file filename validation. + +GitHub issue #360 (Bug_151_MUST_FIX, FD-FNAME-003): upload_file passes +filename straight through to repo.create_file with no control-character +filtering, and separately logs it verbatim via logger.info -- a filename +containing a newline is stored verbatim in the DB and, more importantly, +injects a fake log line into the application log, corrupting the audit +trail (a real log-injection vector, not just data hygiene). + +Verified against source before writing anything: finbot/mcp/servers/ +findrive/server.py's upload_file (create_findrive_server) has no +validation on filename at all beyond the file-size check. +""" + +import pytest + +from finbot.core.auth.session import session_manager +from finbot.mcp.servers.findrive.server import create_findrive_server + + +@pytest.fixture +def session_context(db): + return session_manager.create_session(email="findrive_test@example.com") + + +async def _upload_file(session_context, **kwargs): + mcp = create_findrive_server(session_context) + tool = await mcp.get_tool("upload_file") + return tool.fn(**kwargs) + + +class TestFileNameValidation: + + @pytest.mark.unit + @pytest.mark.asyncio + async def test_fd_fname_003_newline_in_filename_enables_log_injection( + self, db, session_context + ): + result = await _upload_file( + session_context, + filename="legit.pdf\nFAKE LOG ENTRY: admin login from 1.2.3.4", + content="some content", + ) + + assert "error" in result + assert "file_id" not in result + + @pytest.mark.unit + @pytest.mark.asyncio + async def test_fd_fname_003b_carriage_return_in_filename_rejected( + self, db, session_context + ): + result = await _upload_file( + session_context, + filename="legit.pdf\rFAKE LOG ENTRY", + content="some content", + ) + + assert "error" in result + + @pytest.mark.unit + @pytest.mark.asyncio + async def test_fd_fname_003c_null_byte_in_filename_rejected(self, db, session_context): + result = await _upload_file( + session_context, + filename="legit.pdf\x00hidden", + content="some content", + ) + + assert "error" in result + + @pytest.mark.unit + @pytest.mark.asyncio + async def test_fd_upload_001_upload_returns_file_id_and_metadata( + self, db, session_context + ): + """Regression: ordinary, valid filenames must continue to work.""" + result = await _upload_file( + session_context, + filename="invoice_march_2026.pdf", + content="ordinary invoice content", + ) + + assert "error" not in result + assert result["status"] == "uploaded" + assert result["filename"] == "invoice_march_2026.pdf" + assert isinstance(result["file_id"], int) From c16569a113d2c2688d96ccde41742fb55cc7bbc5 Mon Sep 17 00:00:00 2001 From: Muhammad Daniyal Date: Tue, 11 Aug 2026 20:39:24 +0500 Subject: [PATCH 2/2] test(mcp): address Copilot review on PR #561 Clarified the naming convention on the test that verbatim-matches issue #360's own acceptance-criteria test name (name describes the vulnerability being guarded against, not the assertion direction -- kept as-is for traceability rather than renamed, per Copilot's flag). Added the missing tab-character coverage: the fix already rejects \t, but the original test suite only exercised newline/CR/null byte. --- tests/unit/mcp/test_findrive.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/unit/mcp/test_findrive.py b/tests/unit/mcp/test_findrive.py index b4324fe6..02ba10d0 100644 --- a/tests/unit/mcp/test_findrive.py +++ b/tests/unit/mcp/test_findrive.py @@ -36,6 +36,10 @@ class TestFileNameValidation: async def test_fd_fname_003_newline_in_filename_enables_log_injection( self, db, session_context ): + """Name matches GitHub issue #360's own specified acceptance-criteria + test name verbatim (kept for traceability back to the issue) -- + describes the VULNERABILITY this guards against, not the assertion + direction: the test asserts the injection is now rejected.""" result = await _upload_file( session_context, filename="legit.pdf\nFAKE LOG ENTRY: admin login from 1.2.3.4", @@ -69,6 +73,17 @@ async def test_fd_fname_003c_null_byte_in_filename_rejected(self, db, session_co assert "error" in result + @pytest.mark.unit + @pytest.mark.asyncio + async def test_fd_fname_003d_tab_in_filename_rejected(self, db, session_context): + result = await _upload_file( + session_context, + filename="legit.pdf\thidden", + content="some content", + ) + + assert "error" in result + @pytest.mark.unit @pytest.mark.asyncio async def test_fd_upload_001_upload_returns_file_id_and_metadata(