Skip to content

Commit 6e97587

Browse files
bclozelbclozel
committed
Guard against invalid id/event values in Server Sent Events
Prior to this commit, our implementation of Server Sent Events (SSE), `SseEmitter` (MVC) and `ServerSentEvent` (WebFlux), would not guard against invalid characters if the application mistakenly inserts such characters in the `id` or `event` types. Both implementations would also behave differently when it comes to escaping comment multi-line events. This commit ensures that both implementations handle multi-line comment events and reject invalid characters in id/event types. This commit also optimizes `String` concatenation and memory usage when writing data. Fixes gh-36440
1 parent 37e8aa7 commit 6e97587

6 files changed

Lines changed: 183 additions & 25 deletions

File tree

‎spring-web/src/main/java/org/springframework/http/codec/ServerSentEvent.java‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020

2121
import org.jspecify.annotations.Nullable;
2222

23+
import org.springframework.util.Assert;
2324
import org.springframework.util.ObjectUtils;
2425
import org.springframework.util.StringUtils;
2526

@@ -239,16 +240,23 @@ public BuilderImpl(T data) {
239240

240241
@Override
241242
public Builder<T> id(String id) {
243+
checkEvent(id);
242244
this.id = id;
243245
return this;
244246
}
245247

246248
@Override
247249
public Builder<T> event(String event) {
250+
checkEvent(event);
248251
this.event = event;
249252
return this;
250253
}
251254

255+
private static void checkEvent(String content) {
256+
Assert.isTrue(content.indexOf('\n') == -1 && content.indexOf('\r') == -1,
257+
"illegal character '\\n' or '\\r' in event content");
258+
}
259+
252260
@Override
253261
public Builder<T> retry(Duration retry) {
254262
this.retry = retry;

‎spring-web/src/main/java/org/springframework/http/codec/ServerSentEventHttpMessageWriter.java‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -40,14 +40,14 @@
4040
import org.springframework.http.server.reactive.ServerHttpRequest;
4141
import org.springframework.http.server.reactive.ServerHttpResponse;
4242
import org.springframework.util.Assert;
43-
import org.springframework.util.StringUtils;
4443

4544
/**
4645
* {@code HttpMessageWriter} for {@code "text/event-stream"} responses.
4746
*
4847
* @author Sebastien Deleuze
4948
* @author Arjen Poutsma
5049
* @author Rossen Stoyanchev
50+
* @author Brian Clozel
5151
* @since 5.0
5252
*/
5353
public class ServerSentEventHttpMessageWriter implements HttpMessageWriter<Object> {
@@ -129,8 +129,9 @@ private Flux<Publisher<DataBuffer>> encode(Publisher<?> input, ResolvableType el
129129
result = Flux.just(encodeText(sseText + "\n", mediaType, factory));
130130
}
131131
else if (data instanceof String text) {
132-
text = StringUtils.replace(text, "\n", "\ndata:");
133-
result = Flux.just(encodeText(sseText + text + "\n\n", mediaType, factory));
132+
StringBuilder sb = new StringBuilder(sseText);
133+
writeStringData(text, sb);
134+
result = Flux.just(encodeText(sb.toString(), mediaType, factory));
134135
}
135136
else {
136137
result = encodeEvent(sseText, data, dataType, mediaType, factory, hints);
@@ -140,6 +141,31 @@ else if (data instanceof String text) {
140141
});
141142
}
142143

144+
private void writeStringData(String input, StringBuilder sb) {
145+
if (input.indexOf('\n') == -1 && input.indexOf('\r') == -1) {
146+
sb.append(input);
147+
}
148+
else {
149+
int length = input.length();
150+
for (int i = 0; i < length; i++) {
151+
char c = input.charAt(i);
152+
if (c == '\r') {
153+
if (i + 1 < length && input.charAt(i + 1) == '\n') {
154+
i++;
155+
}
156+
sb.append("\ndata:");
157+
}
158+
else if (c == '\n') {
159+
sb.append("\ndata:");
160+
}
161+
else {
162+
sb.append(c);
163+
}
164+
}
165+
}
166+
sb.append("\n\n");
167+
}
168+
143169
@SuppressWarnings("unchecked")
144170
private <T> Flux<DataBuffer> encodeEvent(CharSequence sseText, T data, ResolvableType dataType,
145171
MediaType mediaType, DataBufferFactory factory, Map<String, Object> hints) {

‎spring-web/src/test/java/org/springframework/http/codec/ServerSentEventHttpMessageWriterTests.java‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -110,12 +110,13 @@ void writeMultiLineString(DataBufferFactory bufferFactory) {
110110
super.bufferFactory = bufferFactory;
111111

112112
MockServerHttpResponse outputMessage = new MockServerHttpResponse(super.bufferFactory);
113-
Flux<String> source = Flux.just("foo\nbar", "foo\nbaz");
113+
Flux<String> source = Flux.just("first\nsecond", "first\rsecond", "first\r\nsecond");
114114
testWrite(source, outputMessage, String.class);
115115

116116
StepVerifier.create(outputMessage.getBody())
117-
.consumeNextWith(stringConsumer("data:foo\ndata:bar\n\n"))
118-
.consumeNextWith(stringConsumer("data:foo\ndata:baz\n\n"))
117+
.consumeNextWith(stringConsumer("data:first\ndata:second\n\n"))
118+
.consumeNextWith(stringConsumer("data:first\ndata:second\n\n"))
119+
.consumeNextWith(stringConsumer("data:first\ndata:second\n\n"))
119120
.expectComplete()
120121
.verify();
121122
}

‎spring-web/src/test/java/org/springframework/http/codec/ServerSentEventTests.java‎

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,55 @@
1+
/*
2+
* Copyright 2002-present the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.http.codec;
18+
19+
import java.util.stream.Stream;
20+
21+
import org.junit.jupiter.params.ParameterizedTest;
22+
import org.junit.jupiter.params.provider.Arguments;
23+
import org.junit.jupiter.params.provider.MethodSource;
24+
25+
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
26+
27+
/**
28+
* Tests for {@link ServerSentEvent}.
29+
* @author Brian Clozel
30+
*/
31+
class ServerSentEventTests {
32+
33+
@ParameterizedTest(name = "{1}")
34+
@MethodSource("newLineCharacters")
35+
void rejectsInvalidId(String newLine, String description) {
36+
assertThatIllegalArgumentException().isThrownBy(() ->
37+
ServerSentEvent.<String>builder().id("first" + newLine + "second").build());
38+
}
39+
40+
@ParameterizedTest(name = "{1}")
41+
@MethodSource("newLineCharacters")
42+
void rejectsInvalidEvent(String newLine, String description) {
43+
assertThatIllegalArgumentException().isThrownBy(() ->
44+
ServerSentEvent.<String>builder().event("first" + newLine + "second").build());
45+
}
46+
47+
private static Stream<Arguments> newLineCharacters() {
48+
return Stream.of(
49+
Arguments.of("\n", "LF"),
50+
Arguments.of("\r", "CR"),
51+
Arguments.of("\r\n", "CRLF")
52+
);
53+
}
54+
55+
}

‎spring-webmvc/src/main/java/org/springframework/web/servlet/mvc/method/annotation/SseEmitter.java‎

Lines changed: 45 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
import org.springframework.http.HttpHeaders;
2828
import org.springframework.http.MediaType;
2929
import org.springframework.http.server.ServerHttpResponse;
30+
import org.springframework.util.Assert;
3031
import org.springframework.util.ObjectUtils;
3132
import org.springframework.util.StringUtils;
3233
import org.springframework.web.servlet.ModelAndView;
@@ -196,18 +197,20 @@ private static class SseEventBuilderImpl implements SseEventBuilder {
196197

197198
private final Set<DataWithMediaType> dataToSend = new LinkedHashSet<>(4);
198199

199-
private @Nullable StringBuilder sb;
200+
private final StringBuilder sb = new StringBuilder();
200201

201202
private boolean hasName;
202203

203204
@Override
204205
public SseEventBuilder id(String id) {
206+
checkEvent(id);
205207
append("id:").append(id).append('\n');
206208
return this;
207209
}
208210

209211
@Override
210212
public SseEventBuilder name(String name) {
213+
checkEvent(name);
211214
this.hasName = true;
212215
append("event:").append(name).append('\n');
213216
return this;
@@ -221,7 +224,7 @@ public SseEventBuilder reconnectTime(long reconnectTimeMillis) {
221224

222225
@Override
223226
public SseEventBuilder comment(String comment) {
224-
append(':').append(comment).append('\n');
227+
append(':').append(StringUtils.replace(comment, "\n", "\n:")).append('\n');
225228
return this;
226229
}
227230

@@ -236,27 +239,53 @@ public SseEventBuilder data(Object object, @Nullable MediaType mediaType) {
236239
name(mav.getViewName());
237240
}
238241
append("data:");
239-
saveAppendedText();
242+
saveAppendedText(TEXT_PLAIN);
240243
if (object instanceof String text) {
241-
object = StringUtils.replace(text, "\n", "\ndata:");
244+
writeStringData(text, mediaType);
245+
}
246+
else {
247+
this.dataToSend.add(new DataWithMediaType(object, mediaType));
242248
}
243-
this.dataToSend.add(new DataWithMediaType(object, mediaType));
244249
append('\n');
245250
return this;
246251
}
247252

248-
SseEventBuilderImpl append(String text) {
249-
if (this.sb == null) {
250-
this.sb = new StringBuilder();
253+
private static void checkEvent(String content) {
254+
Assert.isTrue(content.indexOf('\n') == -1 && content.indexOf('\r') == -1,
255+
"illegal character '\\n' or '\\r' in event content");
256+
}
257+
258+
private void writeStringData(String input, @Nullable MediaType mediaType) {
259+
if (input.indexOf('\n') == -1 && input.indexOf('\r') == -1) {
260+
this.dataToSend.add(new DataWithMediaType(input, mediaType));
261+
}
262+
else {
263+
int length = input.length();
264+
for (int i = 0; i < length; i++) {
265+
char c = input.charAt(i);
266+
if (c == '\r') {
267+
if (i + 1 < length && input.charAt(i + 1) == '\n') {
268+
i++;
269+
}
270+
this.sb.append("\ndata:");
271+
}
272+
else if (c == '\n') {
273+
this.sb.append("\ndata:");
274+
}
275+
else {
276+
this.sb.append(c);
277+
}
278+
}
279+
saveAppendedText(mediaType);
251280
}
281+
}
282+
283+
SseEventBuilderImpl append(String text) {
252284
this.sb.append(text);
253285
return this;
254286
}
255287

256288
SseEventBuilderImpl append(char ch) {
257-
if (this.sb == null) {
258-
this.sb = new StringBuilder();
259-
}
260289
this.sb.append(ch);
261290
return this;
262291
}
@@ -267,14 +296,14 @@ public Set<DataWithMediaType> build() {
267296
return Collections.emptySet();
268297
}
269298
append('\n');
270-
saveAppendedText();
299+
saveAppendedText(TEXT_PLAIN);
271300
return this.dataToSend;
272301
}
273302

274-
private void saveAppendedText() {
275-
if (this.sb != null) {
276-
this.dataToSend.add(new DataWithMediaType(this.sb.toString(), TEXT_PLAIN));
277-
this.sb = null;
303+
private void saveAppendedText(@Nullable MediaType mediaType) {
304+
if (StringUtils.hasLength(this.sb)) {
305+
this.dataToSend.add(new DataWithMediaType(this.sb.toString(), mediaType));
306+
this.sb.setLength(0);
278307
}
279308
}
280309
}

‎spring-webmvc/src/test/java/org/springframework/web/servlet/mvc/method/annotation/SseEmitterTests.java‎

Lines changed: 42 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,14 +22,19 @@
2222
import java.util.List;
2323
import java.util.Set;
2424
import java.util.function.Consumer;
25+
import java.util.stream.Stream;
2526

2627
import org.jspecify.annotations.Nullable;
2728
import org.junit.jupiter.api.BeforeEach;
2829
import org.junit.jupiter.api.Test;
30+
import org.junit.jupiter.params.ParameterizedTest;
31+
import org.junit.jupiter.params.provider.Arguments;
32+
import org.junit.jupiter.params.provider.MethodSource;
2933

3034
import org.springframework.http.MediaType;
3135

3236
import static org.assertj.core.api.Assertions.assertThat;
37+
import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
3338
import static org.springframework.web.servlet.mvc.method.annotation.SseEmitter.event;
3439

3540

@@ -105,16 +110,28 @@ void sendEventWithTwoDataLines() throws Exception {
105110
this.handler.assertWriteCount(1);
106111
}
107112

108-
@Test
109-
void sendEventWithMultiline() throws Exception {
110-
this.emitter.send(event().data("foo\nbar\nbaz"));
113+
@ParameterizedTest(name = "{1}")
114+
@MethodSource("newLineCharacters")
115+
void sendEventWithMultiline(String newLineChars, String description) throws Exception {
116+
this.emitter.send(event().data("foo" + newLineChars + "bar" + newLineChars + "baz"));
111117
this.handler.assertSentObjectCount(3);
112118
this.handler.assertObject(0, "data:", TEXT_PLAIN_UTF8);
113119
this.handler.assertObject(1, "foo\ndata:bar\ndata:baz");
114120
this.handler.assertObject(2, "\n\n", TEXT_PLAIN_UTF8);
115121
this.handler.assertWriteCount(1);
116122
}
117123

124+
@ParameterizedTest(name = "{1}")
125+
@MethodSource("newLineCharacters")
126+
void sendEventWithMultilineWithMediaType(String newLineChars, String description) throws Exception {
127+
this.emitter.send(event().data("foo" + newLineChars + "bar" + newLineChars + "baz", MediaType.TEXT_PLAIN));
128+
this.handler.assertSentObjectCount(3);
129+
this.handler.assertObject(0, "data:", TEXT_PLAIN_UTF8);
130+
this.handler.assertObject(1, "foo\ndata:bar\ndata:baz", MediaType.TEXT_PLAIN);
131+
this.handler.assertObject(2, "\n\n", TEXT_PLAIN_UTF8);
132+
this.handler.assertWriteCount(1);
133+
}
134+
118135
@Test
119136
void sendEventFull() throws Exception {
120137
this.emitter.send(event().comment("blah").name("test").reconnectTime(5000L).id("1").data("foo"));
@@ -137,6 +154,28 @@ void sendEventFullWithTwoDataLinesInTheMiddle() throws Exception {
137154
this.handler.assertWriteCount(1);
138155
}
139156

157+
@ParameterizedTest(name = "{1}")
158+
@MethodSource("newLineCharacters")
159+
void rejectInvalidId(String newLineChars, String description) {
160+
assertThatIllegalArgumentException().isThrownBy(() -> this.emitter
161+
.send(event().id("first" + newLineChars + "second")));
162+
}
163+
164+
@ParameterizedTest(name = "{1}")
165+
@MethodSource("newLineCharacters")
166+
void rejectInvalidName(String newLineChars, String description) {
167+
assertThatIllegalArgumentException().isThrownBy(() -> this.emitter
168+
.send(event().name("first" + newLineChars + "second")));
169+
}
170+
171+
private static Stream<Arguments> newLineCharacters() {
172+
return Stream.of(
173+
Arguments.of("\n", "LF"),
174+
Arguments.of("\r", "CR"),
175+
Arguments.of("\r\n", "CRLF")
176+
);
177+
}
178+
140179

141180
private static class TestHandler implements ResponseBodyEmitter.Handler {
142181

0 commit comments

Comments
 (0)