Skip to content

Detect base64 from the data URL header, not the payload - #309

Merged
varun06 merged 2 commits into
modelcontextprotocol:mainfrom
akhilcb:oss/data-url-base64-detection
Oct 24, 2026
Merged

varun06 merged 2 commits into
modelcontextprotocol:mainfrom
akhilcb:oss/data-url-base64-detection

Conversation

@akhilcb

@akhilcb akhilcb commented Oct 24, 2026

Copy link
Copy Markdown
Contributor

Motivation and Context

Data.parseDataURL decided whether a data URL is base64 with
string.contains(";base64,"), which searches the payload too, not just the header. A
plain-text data URL whose content contains ;base64, was treated as base64, failed to
decode, and returned nil:

data:text/plain,hello;base64,world  →  nil   (expected: "hello;base64,world")

isDataURL returns true for the same string, so the two functions disagreed.

The data URL regex now captures the optional ;base64 marker, and that capture decides
whether to decode base64. The payload is never searched.

How Has This Been Tested?

Adds tests for:

  • the input above, also checking that isDataURL and parseDataURL agree;
  • a regression test confirming that standard ;base64, data URLs still decode as base64;
  • ;base64 following a charset parameter (data:text/plain;charset=utf-8;base64,SGk=).

swift test passes.

Breaking Changes

None. Value decoding follows parseDataURL, so a JSON string like the one above now
decodes as .data, as other data-URL strings already do.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

@akhilcb
akhilcb requested a review from andreyvelich October 24, 2026 00:16
@akhilcb akhilcb assigned varun06 and unassigned varun06 Oct 24, 2026
@akhilcb
akhilcb requested a review from varun06 October 24, 2026 00:16
varun06
varun06 previously approved these changes Oct 24, 2026

@varun06 varun06 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @akhilcb, the fix looks right to me. Capturing the ;base64 marker, which the regex already
limits to the header, is exactly what's needed, and the new test fails on main without the fix.

One request: could you drop the last commit (test: disable reconnection in the heartbeat failure test)?

  • It's unrelated to data URLs.
  • With reconnection disabled, nothing in the transport touches the connection after the simulated failure, so the test only checks the state the mock set itself.
  • It also conflicts with #283: once that cancels the connection when reconnection is disabled, the state becomes .cancelled and this test fails. I reproduced that locally.

If the test is flaky as written, a separate PR that asserts the transport's behavior would be great.

Minor: once #295 lands, Value no longer turns data URLs into .data, so the "Breaking Changes" note can be updated then.

@akhilcb
akhilcb force-pushed the oss/data-url-base64-detection branch from 86fb7bf to 2d1d8c4 Compare October 24, 2026 18:45
@akhilcb
akhilcb requested a review from a team as a code owner October 24, 2026 18:45
Data.parseDataURL decided base64 with string.contains(";base64,"), which
also searched the payload, so "data:text/plain,hello;base64,world" was
treated as base64 and returned nil even though isDataURL accepted it.
The regex now captures the optional ;base64 marker and that capture
drives the decision.
@akhilcb
akhilcb force-pushed the oss/data-url-base64-detection branch from 2d1d8c4 to 34c8cfb Compare October 24, 2026 18:50
@akhilcb

akhilcb commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

@varun06 Thanks for the review.

I have dropped the heartbeat commit, so this PR now has only the two data URL commits. I also rebased it onto the latest main while I was at it.

You are right that the reconnection change doesn't belong here and good catch on #283. With reconnection disabled, the test only checks the state the mock set itself, so it isn't testing anything in the transport. I'll take another look and open a separate PR that asserts the transport's behavior instead. I will update the Breaking Changes note once #295 lands.

@akhilcb

akhilcb commented Oct 24, 2026

Copy link
Copy Markdown
Contributor Author

I have raised this PR based on our above discussion: #311. Thank you!

@varun06
varun06 merged commit 05a54fe into modelcontextprotocol:main Oct 24, 2026
15 of 18 checks passed
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