Skip to content

Commit 0d237d8

Browse files
rstoyanchevrstoyanchev
committed
Polish netty client support
Mostly updates based on: https://github.com/spring-projects/spring-framework/wiki/Spring-Framework-Code-Style
1 parent e120757 commit 0d237d8

8 files changed

+222
-83
lines changed

‎SPR-12354.txt

+137
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
diff --git a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java
2+
index e36c08c..acb9459 100644
3+
--- a/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java
4+
+++ b/spring-webmvc/src/main/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandler.java
5+
@@ -36,6 +36,7 @@ import org.springframework.http.MediaType;
6+
import org.springframework.util.Assert;
7+
import org.springframework.util.ClassUtils;
8+
import org.springframework.util.CollectionUtils;
9+
+import org.springframework.util.ResourceUtils;
10+
import org.springframework.util.StreamUtils;
11+
import org.springframework.util.StringUtils;
12+
import org.springframework.web.HttpRequestHandler;
13+
@@ -237,14 +238,29 @@ public class ResourceHttpRequestHandler extends WebContentGenerator implements H
14+
}
15+
16+
/**
17+
- * Validates the given path: returns {@code true} if the given path is not a valid resource path.
18+
- * <p>The default implementation rejects paths containing "WEB-INF" or "META-INF" as well as paths
19+
- * with relative paths ("../") that result in access of a parent directory.
20+
+ * Identifies invalid resource paths. By default rejects:
21+
+ * <ul>
22+
+ * <li>Paths that contain "WEB-INF" or "META-INF".
23+
+ * <li>Paths that contain "../" after {@link org.springframework.util.StringUtils#cleanPath}.
24+
+ * <li>Paths identified as a URL via {@link org.springframework.util.ResourceUtils#isUrl}.
25+
+ * </ul>
26+
* @param path the path to validate
27+
- * @return {@code true} if the path has been recognized as invalid, {@code false} otherwise
28+
+ * @return {@code true} if the path is invalid, {@code false} otherwise
29+
*/
30+
protected boolean isInvalidPath(String path) {
31+
- return (path.contains("WEB-INF") || path.contains("META-INF") || StringUtils.cleanPath(path).startsWith(".."));
32+
+ if (path.contains("WEB-INF") || path.contains("META-INF")) {
33+
+ return true;
34+
+ }
35+
+ if (path.contains("../")) {
36+
+ path = StringUtils.cleanPath(path);
37+
+ if (path.contains("../")) {
38+
+ return true;
39+
+ }
40+
+ }
41+
+ if (path.contains(":") && ResourceUtils.isUrl(path)) {
42+
+ return true;
43+
+ }
44+
+ return false;
45+
}
46+
47+
/**
48+
diff --git a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java
49+
index 07268a3..69ca52f 100644
50+
--- a/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java
51+
+++ b/spring-webmvc/src/test/java/org/springframework/web/servlet/resource/ResourceHttpRequestHandlerTests.java
52+
@@ -16,25 +16,26 @@
53+
54+
package org.springframework.web.servlet.resource;
55+
56+
+import static org.junit.Assert.assertEquals;
57+
+import static org.junit.Assert.assertTrue;
58+
+
59+
import java.io.IOException;
60+
import java.util.ArrayList;
61+
-import java.util.Arrays;
62+
import java.util.List;
63+
+
64+
import javax.servlet.http.HttpServletResponse;
65+
66+
import org.junit.Before;
67+
import org.junit.Test;
68+
-
69+
import org.springframework.core.io.ClassPathResource;
70+
import org.springframework.core.io.Resource;
71+
+import org.springframework.core.io.UrlResource;
72+
import org.springframework.mock.web.test.MockHttpServletRequest;
73+
import org.springframework.mock.web.test.MockHttpServletResponse;
74+
import org.springframework.mock.web.test.MockServletContext;
75+
import org.springframework.web.HttpRequestMethodNotSupportedException;
76+
import org.springframework.web.servlet.HandlerMapping;
77+
78+
-import static org.junit.Assert.*;
79+
-
80+
/**
81+
* Unit tests for ResourceHttpRequestHandler.
82+
*
83+
@@ -126,7 +127,7 @@ public class ResourceHttpRequestHandlerTests {
84+
}
85+
86+
@Test
87+
- public void getResourceViaDirectoryTraversal() throws Exception {
88+
+ public void directoryTraversalWithRelativePath() throws Exception {
89+
this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "../testsecret/secret.txt");
90+
this.handler.handleRequest(this.request, this.response);
91+
assertEquals(404, this.response.getStatus());
92+
@@ -136,13 +137,40 @@ public class ResourceHttpRequestHandlerTests {
93+
this.handler.handleRequest(this.request, this.response);
94+
assertEquals(404, this.response.getStatus());
95+
96+
- this.handler.setLocations(Arrays.<Resource>asList(new ClassPathResource("testsecret/", getClass())));
97+
- this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "secret.txt");
98+
+ // SPR-12354 for ":/../"
99+
+
100+
+ this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, ":/../../testsecret/secret.txt");
101+
+ this.response = new MockHttpServletResponse();
102+
+ this.handler.handleRequest(this.request, this.response);
103+
+ assertEquals(404, this.response.getStatus());
104+
+
105+
+ // Also "prove" secret.txt does exist
106+
+ Resource location = this.handler.getLocations().get(0);
107+
+ assertTrue(location.createRelative("../testsecret/secret.txt").exists());
108+
+ }
109+
+
110+
+ // SPR-12354 for "file:/"
111+
+
112+
+ @Test
113+
+ public void directoryTraversalWithUrl() throws Exception {
114+
+
115+
+ // Add any file based location
116+
+ this.handler.getLocations().add(new UrlResource(getClass().getResource(".")));
117+
+
118+
+ Resource resource = new UrlResource(getClass().getResource("testsecret/secret.txt"));
119+
+ this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, resource.getURL().toString());
120+
+ this.handler.handleRequest(this.request, this.response);
121+
+ assertEquals(404, this.response.getStatus());
122+
+
123+
+ resource = new UrlResource(getClass().getResource("testsecret/secret.txt"));
124+
+ this.request.setAttribute(HandlerMapping.PATH_WITHIN_HANDLER_MAPPING_ATTRIBUTE, "../" + resource.getURL().toString());
125+
this.response = new MockHttpServletResponse();
126+
this.handler.handleRequest(this.request, this.response);
127+
- assertEquals(200, this.response.getStatus());
128+
- assertEquals("text/plain", this.response.getContentType());
129+
- assertEquals("big secret", this.response.getContentAsString());
130+
+ assertEquals(404, this.response.getStatus());
131+
+
132+
+ // Also "prove" secret.txt does exist
133+
+ Resource location = this.handler.getLocations().get(0);
134+
+ assertTrue(location.createRelative("../testsecret/secret.txt").exists());
135+
}
136+
137+
@Test

‎spring-web/src/main/java/org/springframework/http/client/Netty4ClientHttpRequest.java

+34-37
Original file line numberDiff line numberDiff line change
@@ -60,13 +60,15 @@ class Netty4ClientHttpRequest extends AbstractAsyncClientHttpRequest implements
6060

6161
private final ByteBufOutputStream body;
6262

63+
6364
Netty4ClientHttpRequest(Bootstrap bootstrap, URI uri, HttpMethod method, int maxRequestSize) {
6465
this.bootstrap = bootstrap;
6566
this.uri = uri;
6667
this.method = method;
6768
this.body = new ByteBufOutputStream(Unpooled.buffer(maxRequestSize));
6869
}
6970

71+
7072
@Override
7173
public HttpMethod getMethod() {
7274
return this.method;
@@ -83,8 +85,7 @@ protected OutputStream getBodyInternal(HttpHeaders headers) throws IOException {
8385
}
8486

8587
@Override
86-
protected ListenableFuture<ClientHttpResponse> executeInternal(final HttpHeaders headers)
87-
throws IOException {
88+
protected ListenableFuture<ClientHttpResponse> executeInternal(final HttpHeaders headers) throws IOException {
8889
final SettableListenableFuture<ClientHttpResponse> responseFuture =
8990
new SettableListenableFuture<ClientHttpResponse>();
9091

@@ -93,42 +94,19 @@ protected ListenableFuture<ClientHttpResponse> executeInternal(final HttpHeaders
9394
public void operationComplete(ChannelFuture future) throws Exception {
9495
if (future.isSuccess()) {
9596
Channel channel = future.channel();
96-
channel.pipeline()
97-
.addLast(new SimpleChannelInboundHandler<FullHttpResponse>() {
98-
99-
@Override
100-
protected void channelRead0(
101-
ChannelHandlerContext ctx,
102-
FullHttpResponse msg) throws Exception {
103-
responseFuture
104-
.set(new Netty4ClientHttpResponse(ctx,
105-
msg));
106-
}
107-
108-
@Override
109-
public void exceptionCaught(
110-
ChannelHandlerContext ctx,
111-
Throwable cause) throws Exception {
112-
responseFuture.setException(cause);
113-
}
114-
});
115-
116-
FullHttpRequest nettyRequest =
117-
createFullHttpRequest(headers);
118-
97+
channel.pipeline().addLast(new RequestExecuteHandler(responseFuture));
98+
FullHttpRequest nettyRequest = createFullHttpRequest(headers);
11999
channel.writeAndFlush(nettyRequest);
120100
}
121101
else {
122102
responseFuture.setException(future.cause());
123103
}
124-
125104
}
126105
};
127106

128-
bootstrap.connect(uri.getHost(), getPort(uri)).addListener(connectionListener);
107+
this.bootstrap.connect(this.uri.getHost(), getPort(this.uri)).addListener(connectionListener);
129108

130109
return responseFuture;
131-
132110
}
133111

134112
@Override
@@ -142,7 +120,8 @@ public ClientHttpResponse execute() throws IOException {
142120
catch (ExecutionException ex) {
143121
if (ex.getCause() instanceof IOException) {
144122
throw (IOException) ex.getCause();
145-
} else {
123+
}
124+
else {
146125
throw new IOException(ex.getMessage(), ex);
147126
}
148127
}
@@ -163,17 +142,13 @@ else if ("https".equalsIgnoreCase(uri.getScheme())) {
163142

164143
private FullHttpRequest createFullHttpRequest(HttpHeaders headers) {
165144
io.netty.handler.codec.http.HttpMethod nettyMethod =
166-
io.netty.handler.codec.http.HttpMethod.valueOf(method.name());
145+
io.netty.handler.codec.http.HttpMethod.valueOf(this.method.name());
167146

168147
FullHttpRequest nettyRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1,
169-
nettyMethod, this.uri.getRawPath(),
170-
this.body.buffer());
148+
nettyMethod, this.uri.getRawPath(), this.body.buffer());
171149

172-
nettyRequest.headers()
173-
.set(io.netty.handler.codec.http.HttpHeaders.Names.HOST, uri.getHost());
174-
nettyRequest.headers()
175-
.set(io.netty.handler.codec.http.HttpHeaders.Names.CONNECTION,
176-
io.netty.handler.codec.http.HttpHeaders.Values.CLOSE);
150+
nettyRequest.headers().set(HttpHeaders.HOST, uri.getHost());
151+
nettyRequest.headers().set(HttpHeaders.CONNECTION, io.netty.handler.codec.http.HttpHeaders.Values.CLOSE);
177152

178153
for (Map.Entry<String, List<String>> entry : headers.entrySet()) {
179154
nettyRequest.headers().add(entry.getKey(), entry.getValue());
@@ -183,4 +158,26 @@ private FullHttpRequest createFullHttpRequest(HttpHeaders headers) {
183158
}
184159

185160

161+
/**
162+
* A SimpleChannelInboundHandler to update the given SettableListenableFuture.
163+
*/
164+
private static class RequestExecuteHandler extends SimpleChannelInboundHandler<FullHttpResponse> {
165+
166+
private final SettableListenableFuture<ClientHttpResponse> responseFuture;
167+
168+
public RequestExecuteHandler(SettableListenableFuture<ClientHttpResponse> responseFuture) {
169+
this.responseFuture = responseFuture;
170+
}
171+
172+
@Override
173+
protected void channelRead0(ChannelHandlerContext context, FullHttpResponse response) throws Exception {
174+
this.responseFuture.set(new Netty4ClientHttpResponse(context, response));
175+
}
176+
177+
@Override
178+
public void exceptionCaught(ChannelHandlerContext context, Throwable cause) throws Exception {
179+
this.responseFuture.setException(cause);
180+
}
181+
}
182+
186183
}

‎spring-web/src/main/java/org/springframework/http/client/Netty4ClientHttpRequestFactory.java

+26-25
Original file line numberDiff line numberDiff line change
@@ -45,16 +45,16 @@
4545
* @author Arjen Poutsma
4646
* @since 4.2
4747
*/
48-
public class Netty4ClientHttpRequestFactory
49-
implements ClientHttpRequestFactory, AsyncClientHttpRequestFactory,
50-
InitializingBean, DisposableBean {
48+
public class Netty4ClientHttpRequestFactory implements ClientHttpRequestFactory,
49+
AsyncClientHttpRequestFactory, InitializingBean, DisposableBean {
5150

5251
/**
5352
* The default maximum request size.
5453
* @see #setMaxRequestSize(int)
5554
*/
5655
public static final int DEFAULT_MAX_REQUEST_SIZE = 1024 * 1024 * 10;
5756

57+
5858
private final EventLoopGroup eventLoopGroup;
5959

6060
private final boolean defaultEventLoopGroup;
@@ -65,18 +65,19 @@ public class Netty4ClientHttpRequestFactory
6565

6666
private Bootstrap bootstrap;
6767

68+
6869
/**
69-
* Creates a new {@code Netty4ClientHttpRequestFactory} with a default
70+
* Create a new {@code Netty4ClientHttpRequestFactory} with a default
7071
* {@link NioEventLoopGroup}.
7172
*/
7273
public Netty4ClientHttpRequestFactory() {
7374
int ioWorkerCount = Runtime.getRuntime().availableProcessors() * 2;
74-
eventLoopGroup = new NioEventLoopGroup(ioWorkerCount);
75-
defaultEventLoopGroup = true;
75+
this.eventLoopGroup = new NioEventLoopGroup(ioWorkerCount);
76+
this.defaultEventLoopGroup = true;
7677
}
7778

7879
/**
79-
* Creates a new {@code Netty4ClientHttpRequestFactory} with the given
80+
* Create a new {@code Netty4ClientHttpRequestFactory} with the given
8081
* {@link EventLoopGroup}.
8182
*
8283
* <p><b>NOTE:</b> the given group will <strong>not</strong> be
@@ -89,17 +90,20 @@ public Netty4ClientHttpRequestFactory(EventLoopGroup eventLoopGroup) {
8990
this.defaultEventLoopGroup = false;
9091
}
9192

93+
9294
/**
93-
* Sets the default maximum request size. The default is
94-
* {@link #DEFAULT_MAX_REQUEST_SIZE}.
95+
* Set the default maximum request size.
96+
* <p>By default this is set to {@link #DEFAULT_MAX_REQUEST_SIZE}.
9597
* @see HttpObjectAggregator#HttpObjectAggregator(int)
9698
*/
9799
public void setMaxRequestSize(int maxRequestSize) {
98100
this.maxRequestSize = maxRequestSize;
99101
}
100102

101103
/**
102-
* Sets the SSL context.
104+
* Set the SSL context. When configured it is used to create and insert an
105+
* {@link io.netty.handler.ssl.SslHandler} in the channel pipeline.
106+
* <p>By default this is not set.
103107
*/
104108
public void setSslContext(SslContext sslContext) {
105109
this.sslContext = sslContext;
@@ -108,14 +112,14 @@ public void setSslContext(SslContext sslContext) {
108112
private Bootstrap getBootstrap() {
109113
if (this.bootstrap == null) {
110114
Bootstrap bootstrap = new Bootstrap();
111-
bootstrap.group(eventLoopGroup).channel(NioSocketChannel.class)
115+
bootstrap.group(this.eventLoopGroup).channel(NioSocketChannel.class)
112116
.handler(new ChannelInitializer<SocketChannel>() {
113117
@Override
114-
protected void initChannel(SocketChannel ch) throws Exception {
115-
ChannelPipeline pipeline = ch.pipeline();
118+
protected void initChannel(SocketChannel channel) throws Exception {
119+
ChannelPipeline pipeline = channel.pipeline();
116120

117121
if (sslContext != null) {
118-
pipeline.addLast(sslContext.newHandler(ch.alloc()));
122+
pipeline.addLast(sslContext.newHandler(channel.alloc()));
119123
}
120124
pipeline.addLast(new HttpClientCodec());
121125
pipeline.addLast(new HttpObjectAggregator(maxRequestSize));
@@ -131,29 +135,26 @@ public void afterPropertiesSet() throws Exception {
131135
getBootstrap();
132136
}
133137

134-
private Netty4ClientHttpRequest createRequestInternal(URI uri, HttpMethod httpMethod) {
135-
return new Netty4ClientHttpRequest(getBootstrap(), uri, httpMethod, maxRequestSize);
136-
}
137-
138138
@Override
139-
public ClientHttpRequest createRequest(URI uri, HttpMethod httpMethod)
140-
throws IOException {
139+
public ClientHttpRequest createRequest(URI uri, HttpMethod httpMethod) throws IOException {
141140
return createRequestInternal(uri, httpMethod);
142141
}
143142

144143
@Override
145-
public AsyncClientHttpRequest createAsyncRequest(URI uri, HttpMethod httpMethod)
146-
throws IOException {
144+
public AsyncClientHttpRequest createAsyncRequest(URI uri, HttpMethod httpMethod) throws IOException {
147145
return createRequestInternal(uri, httpMethod);
148146
}
149147

148+
private Netty4ClientHttpRequest createRequestInternal(URI uri, HttpMethod httpMethod) {
149+
return new Netty4ClientHttpRequest(getBootstrap(), uri, httpMethod, this.maxRequestSize);
150+
}
151+
150152
@Override
151153
public void destroy() throws InterruptedException {
152-
if (defaultEventLoopGroup) {
154+
if (this.defaultEventLoopGroup) {
153155
// clean up the EventLoopGroup if we created it in the constructor
154-
eventLoopGroup.shutdownGracefully().sync();
156+
this.eventLoopGroup.shutdownGracefully().sync();
155157
}
156158
}
157159

158-
159160
}

0 commit comments

Comments
 (0)