Skip to content

fix(server): treat text/* mime types case-insensitively for FileResource#2918

Open
ly-wang19 wants to merge 2 commits into
modelcontextprotocol:mainfrom
ly-wang19:fix/file-resource-mime-type-case-insensitive
Open

fix(server): treat text/* mime types case-insensitively for FileResource#2918
ly-wang19 wants to merge 2 commits into
modelcontextprotocol:mainfrom
ly-wang19:fix/file-resource-mime-type-case-insensitive

Conversation

@ly-wang19

Copy link
Copy Markdown

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.py8 passed; ruff check / ruff format --check clean.

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.
@Robin1987China

Copy link
Copy Markdown

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 file

The 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.

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.

2 participants