ServerSentEvent.toString interpolates eventType and id directly, and splits data on \n only. Since the WhatWG specification treats a CRLF pair, a lone LF and a lone CR as line terminators, a value containing any of these ends the current line on the wire, and the remainder is parsed by the client as a new field.
Current behaviour
ServerSentEvent(eventType = Some("a\ndata: x")).toString
// event: a
// data: x <- injected data field
ServerSentEvent(id = Some("a\ndata: x")).toString
// id: a
// data: x <- injected data field
ServerSentEvent(data = Some("x\revent: foo")).toString
// data: x\revent: foo <- a client splitting on CR sees an injected event fieldeventType and id are the more exposed of the three: they are interpolated with no splitting or validation at all, so a plain \n is enough — no CR needed.
val _data = data.map(_.split("\n")).map(_.map(line => Some(s"data: $line"))).getOrElse(Array.empty[Option[String]])
val _event = eventType.map(event => s"event: $event")
val _id = id.map(id => s"id: $id")This matters because toString is how applications render events built from their own (potentially user-supplied) values, so a caller can be made to emit fields it never intended — including data, event, id or retry.
Suggested fix
The right remedy differs per field:
data may legitimately span multiple lines, so it should split on all three terminators: split("\r\n|\r|\n"), each part emitted as its own data: line.eventType and id cannot span lines, so splitting is not meaningful. They should have line terminators rejected or stripped.
Related
The same class of bug in the new comments field was found in review and fixed in #478 (comments now splits on \r\n|\r|\n). The three fields above are pre-existing on master and are untouched by that PR, hence this separate issue.
ServerSentEvent.toStringinterpolateseventTypeandiddirectly, and splitsdataon\nonly. Since the WhatWG specification treats a CRLF pair, a lone LF and a lone CR as line terminators, a value containing any of these ends the current line on the wire, and the remainder is parsed by the client as a new field.Current behaviour
eventTypeandidare the more exposed of the three: they are interpolated with no splitting or validation at all, so a plain\nis enough — no CR needed.This matters because
toStringis how applications render events built from their own (potentially user-supplied) values, so a caller can be made to emit fields it never intended — includingdata,event,idorretry.Suggested fix
The right remedy differs per field:
datamay legitimately span multiple lines, so it should split on all three terminators:split("\r\n|\r|\n"), each part emitted as its owndata:line.eventTypeandidcannot span lines, so splitting is not meaningful. They should have line terminators rejected or stripped.Related
The same class of bug in the new
commentsfield was found in review and fixed in #478 (commentsnow splits on\r\n|\r|\n). The three fields above are pre-existing onmasterand are untouched by that PR, hence this separate issue.