Skip to content

Commit 8dc888e

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 131f94f commit 8dc888e

6 files changed

Lines changed: 183 additions & 26 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
@@ -19,6 +19,7 @@
1919
import java.time.Duration;
2020

2121
import org.springframework.lang.Nullable;
22+
import org.springframework.util.Assert;
2223
import org.springframework.util.ObjectUtils;
2324
import org.springframework.util.StringUtils;
2425

@@ -253,16 +254,23 @@ public BuilderImpl(T data) {
253254

254255
@Override
255256
public Builder<T> id(String id) {
257+
checkEvent(id);
256258
this.id = id;
257259
return this;
258260
}
259261

260262
@Override
261263
public Builder<T> event(String event) {
264+
checkEvent(event);
262265
this.event = event;
263266
return this;
264267
}
265268

269+
private static void checkEvent(String content) {
270+
Assert.isTrue(content.indexOf('\n') == -1 && content.indexOf('\r') == -1,
271+
"illegal character '\\n' or '\\r' in event content");
272+
}
273+
266274
@Override
267275
public Builder<T> retry(Duration retry) {
268276
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.ServerHttpResponse;
4141
import org.springframework.lang.Nullable;
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> {
@@ -131,8 +131,9 @@ private Flux<Publisher<DataBuffer>> encode(Publisher<?> input, ResolvableType el
131131
result = Flux.just(encodeText(sseText + "\n", mediaType, factory));
132132
}
133133
else if (data instanceof String text) {
134-
text = StringUtils.replace(text, "\n", "\ndata:");
135-
result = Flux.just(encodeText(sseText + text + "\n\n", mediaType, factory));
134+
StringBuilder sb = new StringBuilder(sseText);
135+
writeStringData(text, sb);
136+
result = Flux.just(encodeText(sb.toString(), mediaType, factory));
136137
}
137138
else {
138139
result = encodeEvent(sseText, data, dataType, mediaType, factory, hints);
@@ -142,6 +143,31 @@ else if (data instanceof String text) {
142143
});
143144
}
144145

146+
private void writeStringData(String input, StringBuilder sb) {
147+
if (input.indexOf('\n') == -1 && input.indexOf('\r') == -1) {
148+
sb.append(input);
149+
}
150+
else {
151+
int length = input.length();
152+
for (int i = 0; i < length; i++) {
153+
char c = input.charAt(i);
154+
if (c == '\r') {
155+
if (i + 1 < length && input.charAt(i + 1) == '\n') {
156+
i++;
157+
}
158+
sb.append("\ndata:");
159+
}
160+
else if (c == '\n') {
161+
sb.append("\ndata:");
162+
}
163+
else {
164+
sb.append(c);
165+
}
166+
}
167+
}
168+
sb.append("\n\n");
169+
}
170+
145171
@SuppressWarnings("unchecked")
146172
private <T> Flux<DataBuffer> encodeEvent(CharSequence sseText, T data, ResolvableType dataType,
147173
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 & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import org.springframework.http.MediaType;
2727
import org.springframework.http.server.ServerHttpResponse;
2828
import org.springframework.lang.Nullable;
29+
import org.springframework.util.Assert;
2930
import org.springframework.util.ObjectUtils;
3031
import org.springframework.util.StringUtils;
3132
import org.springframework.web.servlet.ModelAndView;
@@ -195,19 +196,20 @@ private static class SseEventBuilderImpl implements SseEventBuilder {
195196

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

198-
@Nullable
199-
private StringBuilder sb;
199+
private final StringBuilder sb = new StringBuilder();
200200

201201
private boolean hasName;
202202

203203
@Override
204204
public SseEventBuilder id(String id) {
205+
checkEvent(id);
205206
append("id:").append(id).append('\n');
206207
return this;
207208
}
208209

209210
@Override
210211
public SseEventBuilder name(String name) {
212+
checkEvent(name);
211213
this.hasName = true;
212214
append("event:").append(name).append('\n');
213215
return this;
@@ -221,7 +223,7 @@ public SseEventBuilder reconnectTime(long reconnectTimeMillis) {
221223

222224
@Override
223225
public SseEventBuilder comment(String comment) {
224-
append(':').append(comment).append('\n');
226+
append(':').append(StringUtils.replace(comment, "\n", "\n:")).append('\n');
225227
return this;
226228
}
227229

@@ -236,27 +238,53 @@ public SseEventBuilder data(Object object, @Nullable MediaType mediaType) {
236238
name(mav.getViewName());
237239
}
238240
append("data:");
239-
saveAppendedText();
241+
saveAppendedText(TEXT_PLAIN);
240242
if (object instanceof String text) {
241-
object = StringUtils.replace(text, "\n", "\ndata:");
243+
writeStringData(text, mediaType);
244+
}
245+
else {
246+
this.dataToSend.add(new DataWithMediaType(object, mediaType));
242247
}
243-
this.dataToSend.add(new DataWithMediaType(object, mediaType));
244248
append('\n');
245249
return this;
246250
}
247251

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

256287
SseEventBuilderImpl append(char ch) {
257-
if (this.sb == null) {
258-
this.sb = new StringBuilder();
259-
}
260288
this.sb.append(ch);
261289
return this;
262290
}
@@ -267,14 +295,14 @@ public Set<DataWithMediaType> build() {
267295
return Collections.emptySet();
268296
}
269297
append('\n');
270-
saveAppendedText();
298+
saveAppendedText(TEXT_PLAIN);
271299
return this.dataToSend;
272300
}
273301

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

‎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.junit.jupiter.api.BeforeEach;
2728
import org.junit.jupiter.api.Test;
29+
import org.junit.jupiter.params.ParameterizedTest;
30+
import org.junit.jupiter.params.provider.Arguments;
31+
import org.junit.jupiter.params.provider.MethodSource;
2832

2933
import org.springframework.http.MediaType;
3034
import org.springframework.lang.Nullable;
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)