Skip to content

fix: stop truncating SSE field values containing U+2028/U+2029/U+0085 - #1142

Open
lzcGeek wants to merge 1 commit into
modelcontextprotocol:mainfrom
lzcGeek:fix/sse-line-separator-truncation
Open

lzcGeek wants to merge 1 commit into
modelcontextprotocol:mainfrom
lzcGeek:fix/sse-line-separator-truncation

Conversation

@lzcGeek

@lzcGeek lzcGeek commented Sep 23, 2026

Copy link
Copy Markdown

What

SseLineSubscriber extracted data:, id: and event: values with MULTILINE regexes. The Java regex engine treats U+2028 (LINE SEPARATOR), U+2029 (PARAGRAPH SEPARATOR) and U+0085 (NEXT LINE) as line terminators, so ^data:(.+)$ matched only a prefix of any line containing one of those characters — and everything after it was silently discarded. The client then failed to deserialise the truncated JSON and threw McpTransportException: Error parsing JSON-RPC message.

This PR removes the three regexes and extracts field values per the SSE specification: the characters after the colon with a single leading space removed. The line splitter feeding the subscriber (fromLineSubscriber) only splits on \n, \r and \r\n, so those characters now arrive inside a line and are preserved intact.

Why

Fixes #1136. Any tool result, resource content or prompt text containing one of these characters — they are legal unescaped inside a JSON string, and they turn up in real content such as text pasted from word processors, web pages and PDFs — was unreadable by the client.

How it was checked

  • New ResponseSubscribersTest asserts that a data: payload containing each of the three characters is preserved byte-for-byte, that a vertical tab still works, that only a single leading space is stripped per data line, and that multi-line data: accumulation plus id:/event: values containing those characters are captured intact.
  • mvn -pl mcp-core verify passes, including the spring-javaformat validation.

Notes

  • The per-line trim() was dropped together with the regex, matching the reporter's suggested fix: per the SSE spec only one leading space is stripped, so interior whitespace is no longer mangled. The existing whole-event trim() behaviour at dispatch is unchanged.
  • Prepared with AI coding assistance and reviewed by @lzcGeek.

Fixes #1136

Copy link
Copy Markdown

The prefix-based field extraction fixes the Unicode-line-separator truncation, but I think one SSE whitespace bug remains in the same path.

At event dispatch, SseLineSubscriber still does:

String eventData = this.eventBuilder.toString();
SseEvent sseEvent =
    new SseEvent(currentEventId.get(), currentEventType.get(), eventData.trim());

Per SSE parsing, each data: field removes at most one optional U+0020 after the colon, data lines are joined with \n, and then only the final synthetic newline is removed. A whole-event .trim() also removes significant whitespace from the first/last data line.

The current new test avoids this by putting the second significant space on an interior line:

data: first
data:  second

so " second" survives. A single-line regression exposes the remaining loss:

List<SseEvent> events = parse(List.of("data:  first  ", ""));
assertThat(events.get(0).data()).isEqualTo(" first  ");

I expect the current PR head still returns "first".

Since this PR is already replacing regex/trim-based extraction with the SSE field rule, it seems worth removing the whole-event .trim() as well and dropping only the trailing separator appended by the subscriber (e.g. remove one final \n). Otherwise U+2028/U+2029/U+0085 are fixed, but valid leading/trailing spaces are still silently changed.

@lzcGeek
lzcGeek force-pushed the fix/sse-line-separator-truncation branch from 98c9150 to 100e402 Compare September 26, 2026 16:27
@lzcGeek

lzcGeek commented Sep 26, 2026

Copy link
Copy Markdown
Author

You're absolutely right — thank you for catching this. The trim() was carried over from the old implementation, and my existing tests accidentally avoided it by placing the second space on an interior line, exactly as you described.

Fixed along the lines you suggested: dispatching now only removes the single trailing separator that the data: handler appended (concatenatedDataLines()), so significant leading/trailing whitespace of the first and last data lines is preserved. I also added two regression tests covering your single-line example ("data: padded " → " padded ") and the multi-line first/last whitespace case, and updated the comment on the existing single-leading-space test that implicitly documented the old trim behaviour.

Everything passes locally (7/7 in ResponseSubscribersTest), pushed as a single amended commit.

Copy link
Copy Markdown

Verified the updated head. The whole-event .trim() is gone, dispatch now removes only the synthetic final newline, and the two added regressions cover both the single-line and first/last multi-line whitespace cases I was concerned about.

That addresses my review point. Thanks for turning it around quickly.

This branch has not been deployed

No deployments
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.

SSE client silently truncates a data: line at U+2028/U+2029/U+0085, then fails with "Error parsing JSON-RPC message"

2 participants