fix(mcp): reject control characters in FinDrive filenames - #561
Open
Deez-Automations wants to merge 2 commits into
Open
fix(mcp): reject control characters in FinDrive filenames#561Deez-Automations wants to merge 2 commits into
Deez-Automations wants to merge 2 commits into
Conversation
…rity-Project#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 GenAI-Security-Project#360
There was a problem hiding this comment.
Pull request overview
This PR addresses FinDrive MCP upload_file filename log-injection risk by validating filenames before they reach persistence or logging, aligning with Issue #360’s acceptance criteria.
Changes:
- Added filename validation in
upload_fileto reject control characters (\n,\r,\t,\x00). - Added unit tests covering rejection of newline, carriage return, and null byte filenames, plus a regression test for valid filenames.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| finbot/mcp/servers/findrive/server.py | Adds a control-character guard in upload_file before DB write and logging. |
| tests/unit/mcp/test_findrive.py | Introduces unit/regression coverage for the filename validation behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+36
to
+38
| async def test_fd_fname_003_newline_in_filename_enables_log_injection( | ||
| self, db, session_context | ||
| ): |
Author
There was a problem hiding this comment.
Kept the name as-is on purpose — it matches issue #360's own acceptance-criteria test name verbatim so it's traceable back to the ticket, added a docstring note explaining that so it's not confusing on its own.
Comment on lines
+61
to
+63
| @pytest.mark.unit | ||
| @pytest.mark.asyncio | ||
| async def test_fd_fname_003c_null_byte_in_filename_rejected(self, db, session_context): |
Author
There was a problem hiding this comment.
Already added — test_fd_fname_003d_tab_in_filename_rejected covers it.
Clarified the naming convention on the test that verbatim-matches issue GenAI-Security-Project#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.
Author
|
Addressed the Copilot review feedback:
5/5 tests passing after both changes. |
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 #360.
upload_file(FinDrive MCP server) passedfilenamestraight through to storage and into a log line with no validation at all. 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 and potentially deceiving anyone monitoring logs for suspicious activity.Fix
Rejects
\n,\r,\t, and\x00in filenames before they reach storage or logging, matching the fix proposed in the issue. Ordinary filenames are unaffected.Test plan
tests/unit/mcp/test_findrive.py— reproduces the vulnerability first (confirmed failing against the unfixed code), then confirms the fixpytest tests/unit/mcp/test_findrive.py— 4/4 passing