Skip to content

treat a lone CR as an SSE line terminator - #890

Merged
arturobernalg merged 2 commits into
apache:masterfrom
dxbjavid:sse-lone-cr-line-terminator
Oct 7, 2026
Merged

arturobernalg merged 2 commits into
apache:masterfrom
dxbjavid:sse-lone-cr-line-terminator

Conversation

@dxbjavid

@dxbjavid dxbjavid commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

Both SSE entity consumers break the event stream on LF only and just trim a trailing CR, so a lone CR (one not followed by LF) is kept inside the line instead of ending it; that diverges from the SSE line grammar, which terminates a line on CR, LF or CRLF, and mis-frames a stream that uses bare-CR separators. It also has a security edge: a server can place a lone CR in an id: value, which then survives into the parsed id and is copied straight into the Last-Event-ID header on the next reconnect, so the origin ends up controlling a carriage return in one of our own outgoing request headers. This makes a lone CR end the line in both the char and byte consumers while keeping CRLF a single break, so the id no longer carries a CR and bare-CR streams are framed correctly, with a regression test added to each consumer.

@arturobernalg

Copy link
Copy Markdown
Member

Both SSE entity consumers break the event stream on LF only and just trim a trailing CR, so a lone CR (one not followed by LF) is kept inside the line instead of ending it; that diverges from the SSE line grammar, which terminates a line on CR, LF or CRLF, and mis-frames a stream that uses bare-CR separators. It also has a security edge: a server can place a lone CR in an id: value, which then survives into the parsed id and is copied straight into the Last-Event-ID header on the next reconnect, so the origin ends up controlling a carriage return in one of our own outgoing request headers. This makes a lone CR end the line in both the char and byte consumers while keeping CRLF a single break, so the id no longer carries a CR and bare-CR streams are framed correctly, with a regression test added to each consumer.

Hi @dxbjavid

I think there is still one edge case in ByteSseEntityConsumer.

If the stream starts with a lone CR, that first byte is consumed while BOM detection is being resolved and is appended directly to lineBuf, bypassing the new CR/LF handling.

This valid SSE input:

"\rdata: v\r\r"

should produce data = "v", but currently produces null.

I think the first non-BOM byte should go through the same byte-processing path as the normal parsing loop rather than being appended directly.

…ityConsumer

Signed-off-by: Javid Khan <dxbjavid@gmail.com>
@dxbjavid

dxbjavid commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

good catch, you're right. the problem was that the BOM-miss path in the byte consumer appended the resolved byte straight to the line buffer, so a leading lone CR never saw the CR/LF logic. i've pulled that per-byte handling into a small processByte helper and the BOM path now routes its bytes (including any backfilled 0xEF/0xBB from a partial match) through it, so \rdata: v\r\r now produces data = v. added a regression test for the leading-CR case too. the char consumer peeks the BOM without consuming it, so it already drops the byte into the normal loop and wasn't affected.

@arturobernalg
arturobernalg self-requested a review October 7, 2026 09:06

@arturobernalg arturobernalg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@arturobernalg
arturobernalg merged commit c8d231f into apache:master Oct 7, 2026
10 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