-
Notifications
You must be signed in to change notification settings - Fork 73
fix: Fix SSE corrupted data #4530
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: v2.x.x
Are you sure you want to change the base?
Changes from all commits
0d09ba7
51cf8ba
446c386
c834af5
40560ec
b091bb1
26936a0
280aeb6
92e4cb7
641163e
20dbcde
443fc08
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,16 +15,23 @@ | |
| import io.netty.handler.ssl.SslContextBuilder; | ||
| import lombok.RequiredArgsConstructor; | ||
| import lombok.extern.slf4j.Slf4j; | ||
| import org.apache.commons.lang3.StringUtils; | ||
| import org.apache.commons.lang3.Strings; | ||
| import org.springframework.beans.factory.annotation.Value; | ||
| import org.springframework.cloud.client.ServiceInstance; | ||
| import org.springframework.cloud.client.discovery.DiscoveryClient; | ||
| import org.springframework.core.ParameterizedTypeReference; | ||
| import org.springframework.http.MediaType; | ||
| import org.springframework.http.client.reactive.ReactorClientHttpConnector; | ||
| import org.springframework.http.codec.ServerSentEvent; | ||
| import org.springframework.lang.Nullable; | ||
| import org.springframework.stereotype.Component; | ||
| import org.springframework.stereotype.Controller; | ||
| import org.springframework.util.Assert; | ||
| import org.springframework.web.bind.annotation.GetMapping; | ||
| import org.springframework.web.reactive.function.client.WebClient; | ||
| import org.springframework.web.servlet.ModelAndView; | ||
| import org.springframework.web.servlet.mvc.method.annotation.ResponseBodyEmitter; | ||
| import org.springframework.web.servlet.mvc.method.annotation.SseEmitter; | ||
| import org.zowe.apiml.message.core.Message; | ||
| import org.zowe.apiml.message.core.MessageService; | ||
|
|
@@ -45,13 +52,11 @@ | |
| import java.security.KeyStoreException; | ||
| import java.security.NoSuchAlgorithmException; | ||
| import java.security.cert.CertificateException; | ||
| import java.util.ArrayList; | ||
| import java.util.Arrays; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.*; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.function.Consumer; | ||
|
|
||
| import static org.springframework.http.MediaType.TEXT_PLAIN; | ||
| import static org.zowe.apiml.security.SecurityUtils.loadKeyStore; | ||
|
|
||
| @Slf4j | ||
|
|
@@ -60,6 +65,8 @@ | |
| @Component("ServerSentEventProxyHandler") | ||
| public class ServerSentEventProxyHandler implements RoutedServicesUser { | ||
|
|
||
| private static final String N_DATA = "\ndata:"; | ||
|
|
||
| @Value("${server.ssl.trustStore:#{null}}") | ||
| private String trustStore; | ||
|
|
||
|
|
@@ -103,7 +110,12 @@ private SslContext getSslContext() throws CertificateException, IOException, NoS | |
|
|
||
| @GetMapping({"/sse/**","/*/sse/**"}) | ||
| public SseEmitter getEmitter(HttpServletRequest request, HttpServletResponse response) throws IOException { | ||
| SseEmitter emitter = new SseEmitter(-1L); | ||
| SseEmitter emitter = new SseEmitter(-1L) { | ||
| @Override | ||
| public void send(Object object, MediaType mediaType) throws IOException { | ||
| super.send(new SseEventBuilderFixedImpl().data(object, mediaType)); | ||
| } | ||
| }; | ||
|
|
||
| String uri = request.getRequestURI(); | ||
| List<String> uriParts = getUriParts(uri); | ||
|
|
@@ -141,11 +153,52 @@ public SseEmitter getEmitter(HttpServletRequest request, HttpServletResponse res | |
| return emitter; | ||
| } | ||
|
|
||
| boolean hasEnter(String in) { | ||
| return StringUtils.containsAny(in, '\n', '\r'); | ||
| } | ||
|
|
||
| boolean hasEnter(ServerSentEvent<String> event) { | ||
| return | ||
| hasEnter(event.data()) || | ||
| hasEnter(event.event()) || | ||
| hasEnter(event.comment()) || | ||
| hasEnter(event.id()); | ||
| } | ||
|
|
||
| @SuppressWarnings("squid:S4449") | ||
| ServerSentEvent<String> sanitize(ServerSentEvent<String> event) { | ||
| if (!hasEnter(event)) { | ||
| return event; | ||
| } | ||
|
|
||
| Assert.isTrue(!hasEnter(event.event()), "Illegal character in event content"); | ||
| Assert.isTrue(!hasEnter(event.id()), "Illegal character in event content"); | ||
|
|
||
| String data = event.data(); | ||
| if (hasEnter(data)) { | ||
| data = data.replace("\r\n", "\n"); | ||
| data = data.replace("\n", N_DATA); | ||
| } | ||
|
|
||
| String comment = event.comment(); | ||
| if (hasEnter(comment)) { | ||
| comment = comment.replace("\n", "\n:"); | ||
| } | ||
|
|
||
| return ServerSentEvent.<String>builder() | ||
| .comment(comment) | ||
| .event(event.event()) | ||
| .id(event.id()) | ||
| .data(data) | ||
| .retry(event.retry()) | ||
| .build(); | ||
| } | ||
|
|
||
| // package protected for unit testing | ||
| Consumer<ServerSentEvent<String>> consumer(SseEmitter emitter) { | ||
| return content -> { | ||
| try { | ||
| emitter.send(content.data()); | ||
| emitter.send(sanitize(content).data()); | ||
| } catch (IOException error) { | ||
| emitter.completeWithError(error); | ||
| } | ||
|
|
@@ -209,4 +262,122 @@ private void writeError(HttpServletResponse response, SseErrorMessages errorMess | |
| public void addRoutedServices(String serviceId, RoutedServices routedServices) { | ||
| routedServicesMap.put(serviceId, routedServices); | ||
| } | ||
|
|
||
| static class SseEventBuilderFixedImpl implements SseEmitter.SseEventBuilder { | ||
|
|
||
| private final Set<ResponseBodyEmitter.DataWithMediaType> dataToSend = new LinkedHashSet<>(4); | ||
|
|
||
| private final StringBuilder sb = new StringBuilder(); | ||
|
|
||
|
|
||
| private boolean hasName; | ||
|
|
||
| @Override | ||
| public SseEventBuilderFixedImpl id(String id) { | ||
| checkEvent(id); | ||
| append("id:").append(id).append('\n'); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Literal \n is used a lot. Can it be a constant?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is following the original implementation to be easier to maintain. |
||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public SseEventBuilderFixedImpl name(String name) { | ||
| checkEvent(name); | ||
| this.hasName = true; | ||
| append("event:").append(name).append('\n'); | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public SseEventBuilderFixedImpl reconnectTime(long reconnectTimeMillis) { | ||
| append("retry:").append(String.valueOf(reconnectTimeMillis)).append('\n'); | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public SseEventBuilderFixedImpl comment(String comment) { | ||
| append(':').append(Strings.CS.replace(comment, "\n", "\n:")).append('\n'); | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public SseEventBuilderFixedImpl data(Object object) { | ||
| return data(object, null); | ||
| } | ||
|
|
||
| @Override | ||
| public SseEventBuilderFixedImpl data(Object object, @Nullable MediaType mediaType) { | ||
| if (object instanceof ModelAndView && !this.hasName && ((ModelAndView) object).getViewName() != null) { | ||
| name(((ModelAndView) object).getViewName()); | ||
| } | ||
| append("data:"); | ||
| saveAppendedText(TEXT_PLAIN); | ||
| if (object instanceof String) { | ||
| writeStringData((String) object, mediaType); | ||
| } | ||
| else { | ||
| this.dataToSend.add(new ResponseBodyEmitter.DataWithMediaType(object, mediaType)); | ||
| } | ||
|
|
||
| append('\n'); | ||
| return this; | ||
| } | ||
|
|
||
| private static void checkEvent(String content) { | ||
| Assert.isTrue(content.indexOf('\n') == -1 && content.indexOf('\r') == -1, | ||
| "illegal character '\\n' or '\\r' in event content"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is inconsistent with what is on lines 174 and 175. Can we please make them consistent, and use a constant?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is following the original implementation to be easier to maintain. |
||
| } | ||
|
|
||
| private void writeStringData(String input, @Nullable MediaType mediaType) { | ||
| if (input.indexOf('\n') == -1 && input.indexOf('\r') == -1) { | ||
| this.dataToSend.add(new ResponseBodyEmitter.DataWithMediaType(input, mediaType)); | ||
| } | ||
| else { | ||
| int length = input.length(); | ||
| for (int i = 0; i < length; i++) { | ||
| char c = input.charAt(i); | ||
| if (c == '\r') { | ||
| if (i + 1 < length && input.charAt(i + 1) == '\n') { | ||
| i++; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sonar doesn't like the incrementation of the counter within the loop.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is following the original implementation to be easier to maintain. |
||
| } | ||
| this.sb.append(N_DATA); | ||
| } | ||
| else if (c == '\n') { | ||
| this.sb.append(N_DATA); | ||
| } | ||
| else { | ||
| this.sb.append(c); | ||
| } | ||
| } | ||
| saveAppendedText(mediaType); | ||
| } | ||
| } | ||
|
|
||
| SseEventBuilderFixedImpl append(String text) { | ||
| this.sb.append(text); | ||
| return this; | ||
| } | ||
|
|
||
| SseEventBuilderFixedImpl append(char ch) { | ||
| this.sb.append(ch); | ||
| return this; | ||
| } | ||
|
|
||
| @Override | ||
| public Set<ResponseBodyEmitter.DataWithMediaType> build() { | ||
| if (!org.springframework.util.StringUtils.hasLength(this.sb) && this.dataToSend.isEmpty()) { | ||
| return Collections.emptySet(); | ||
| } | ||
| append('\n'); | ||
| saveAppendedText(TEXT_PLAIN); | ||
| return this.dataToSend; | ||
| } | ||
|
|
||
| private void saveAppendedText(@Nullable MediaType mediaType) { | ||
| if (org.springframework.util.StringUtils.hasLength(this.sb)) { | ||
| this.dataToSend.add(new ResponseBodyEmitter.DataWithMediaType(this.sb.toString(), mediaType)); | ||
| this.sb.setLength(0); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idea has removed imports again. Concern as with the previous PR.