fix(server): treat text/* mime types case-insensitively for FileResource - #2918
fix(server): treat text/* mime types case-insensitively for FileResource#2918ly-wang19 wants to merge 2 commits into
Conversation
Sorry, something went wrong.
Media types are case-insensitive (RFC 9110, section 8.3.1), and StreamableHTTPServerTransport._check_accept_headers already lowercases the Accept media types before comparing. _check_content_type did not, so a spec-valid request with a mixed/upper-case Content-Type (e.g. "Application/JSON") was rejected with 415 Unsupported Media Type. Lowercase the parsed Content-Type media type before comparing to CONTENT_TYPE_JSON, consistent with _check_accept_headers. Adds a unit test for case-insensitive matching (the _check_content_type path was previously no-cover).
FileResource.set_binary_from_mime_type decides whether to read a file as text
or bytes via `mime_type.startswith("text/")`, but media types are
case-insensitive (RFC 9110, section 8.3.1). A FileResource declared with an
upper/mixed-case text type (e.g. "Text/Markdown") was misclassified as binary
and read with read_bytes instead of read_text.
Normalize with .lower() before the prefix check, consistent with the other
media-type checks in the SDK (transport_security, client streamable_http).
Adds a regression test.
|
Thanks for catching the case-sensitivity issue, @ly-wang19. While verifying this I think I found a deeper problem in the same validator that affects this PR: set_binary_from_mime_type never runs when is_binary uses its default. In Pydantic v2, a field_validator does not run on a field's default value unless the field sets validate_default=True. Since is_binary defaults to False, the validator only fires when the caller passes is_binary explicitly — but the docstring's intent ("Set is_binary based on mime_type if not explicitly set") is exactly the path that never executes. A consequence for this PR: the new test test_uppercase_text_mime_type_is_treated_as_text passes regardless of the .lower() change, because with is_binary defaulted the validator is skipped entirely and is_binary stays False either way. So the test can't distinguish "fix works" from "validator didn't run", and the .lower() normalization is effectively dead code on the common path (mime_type set, is_binary left default). Minimal repro: from mcp.server.mcpserver.resources.types import FileResource
r = FileResource(uri="file:///x.png", name="x", path="/abs/x.png", mime_type="image/png")
print(r.is_binary) # False — expected True; read() will then read_text() a binary fileThe root-cause fix is one line — add validate_default=True to the is_binary field — after which both the auto-derivation and your .lower() fix actually take effect, and the test becomes meaningful (a binary mime with is_binary defaulted should yield True). Happy to either push that one-liner + a default-path test into this PR, or open a small complementary PR that lands first — whichever you and the maintainers prefer. Either way your case-insensitivity fix is needed; this just makes it reachable. |
Sorry, something went wrong.
|
Correction to my earlier comment: I was wrong about the root cause. After re-verification, the Resource base class at src/mcp/server/mcpserver/resources/base.py:20 already has model_config = ConfigDict(validate_default=True), which has been inherited since 2024-12-09 (commit 557e90d). This means the set_binary_from_mime_type validator does run on default values — the validate_default=True fix I suggested is unnecessary. The real bug is solely the case-sensitivity issue that this PR addresses: mime_type.startswith("text/") does not match TEXT/plain. I've confirmed with debug output:
So the .lower() fix in this PR is valid and the test is not a false positive. Apologies for the confusion. |
Sorry, something went wrong.
| content_type_parts = [part.strip() for part in content_type.split(";")[0].split(",")] | ||
| # Media types are case-insensitive (RFC 9110, section 8.3.1), so normalize | ||
| # to lower case before comparing — consistent with _check_accept_headers. | ||
| content_type_parts = [part.strip().lower() for part in content_type.split(";")[0].split(",")] |
There was a problem hiding this comment.
nice, I was reading through this today and it aligns with RFC 9110 to have the normalization here. I opened an issue for tracking
that you can link. Thank you
Sorry, something went wrong.
Reviewers
1 more reviewerAt least 1 approving review is required to merge this pull request.
Summary
FileResource.set_binary_from_mime_type decides whether to read a file as text or bytes from its mime_type via mime_type.startswith("text/"). Media types are case-insensitive (RFC 9110, §8.3.1), so a FileResource declared with an upper/mixed-case text type — e.g. mime_type="Text/Markdown" — is misclassified as binary and read with read_bytes instead of read_text, returning a bytes blob to clients instead of text.
Fix
Lower-case the mime type before the prefix check, consistent with the SDK's other media-type checks (transport_security, client streamable_http).
Test
Added test_uppercase_text_mime_type_is_treated_as_text: a FileResource(mime_type="Text/Markdown") keeps is_binary False.
Verified red→green: the new test fails on main (treated as binary) and passes with the fix. uv run pytest tests/server/mcpserver/resources/test_file_resources.py → 8 passed; ruff check / ruff format --check clean.