Skip to content

Avoid duplicate URI construction in ReactorServerHttpRequest #30047

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Avoid duplicate URI construction in ReactorServerHttpRequest #30047

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 7 additions & 31 deletions ...rc/main/java/org/springframework/http/server/reactive/ReactorNetty2ServerHttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import io.netty5.handler.codec.http.HttpHeaderNames;
import io.netty5.handler.codec.http.headers.HttpCookiePair;
import io.netty5.handler.ssl.SslHandler;
import io.netty5.util.NetUtil;
import org.apache.commons.logging.Log;
import reactor.core.publisher.Flux;
import reactor.netty5.ChannelOperationsId;
Expand Down Expand Up @@ -75,48 +76,23 @@ public ReactorNetty2ServerHttpRequest(HttpServerRequest request, Netty5DataBuffe

private static URI initUri(HttpServerRequest request) throws URISyntaxException {
Assert.notNull(request, "HttpServerRequest must not be null");
return new URI(resolveBaseUrl(request) + resolveRequestUri(request));
return new URI(request.scheme() + "://" + resolveHost(request) + resolveRequestUri(request));
}

private static URI resolveBaseUrl(HttpServerRequest request) throws URISyntaxException {
String scheme = getScheme(request);

private static CharSequence resolveHost(HttpServerRequest request) {
InetSocketAddress hostAddress = request.hostAddress();
if (hostAddress != null) {
return new URI(scheme, null, hostAddress.getHostString(), hostAddress.getPort(), null, null, null);
return NetUtil.toSocketAddressString(hostAddress);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before this change, the URI constructor that accepts individual URI components would validate the host and port. After this change, the value returned from resolveHost is not validated, the URI constructor with a String (called on line 79) doesn't either.

When I tested whether this change would help with spring-projects/spring-boot#34395 where X-Forwarded-HOST is "localhost:3000" and X-Forwarded-Port is "3000", I saw that resolveHost returns "localhost:3000:3000", and the resulting URI was "http://localhost:3000:3000/hello". When I test without this change, there is an exception from the URI constructor that was previously used here because "localhost:3000" is not a valid host.

}

CharSequence charSequence = request.requestHeaders().get(HttpHeaderNames.HOST);
if (charSequence != null) {
String header = charSequence.toString();
final int portIndex;
if (header.startsWith("[")) {
portIndex = header.indexOf(':', header.indexOf(']'));
}
else {
portIndex = header.indexOf(':');
}
if (portIndex != -1) {
try {
return new URI(scheme, null, header.substring(0, portIndex),
Integer.parseInt(header, portIndex + 1, header.length(), 10), null, null, null);
}
catch (NumberFormatException ex) {
throw new URISyntaxException(header, "Unable to parse port", portIndex);
}
}
else {
return new URI(scheme, header, null, null);
}
CharSequence header = request.requestHeaders().get(HttpHeaderNames.HOST);
if (header != null) {
return header;
Copy link
Contributor

@rstoyanchev rstoyanchev Feb 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "Host" header can contain host and port, see for example https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Host. And its parsing needs to take into account that the host maybe an IPv6 address. This is what the removed code [89-110] does, and that needs to remain.

}

throw new IllegalStateException("Neither local hostAddress nor HOST header available");
}

private static String getScheme(HttpServerRequest request) {
return request.scheme();
}

private static String resolveRequestUri(HttpServerRequest request) {
String uri = request.uri();
for (int i = 0; i < uri.length(); i++) {
Expand Down
33 changes: 5 additions & 28 deletions ...-web/src/main/java/org/springframework/http/server/reactive/ReactorServerHttpRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import io.netty.handler.codec.http.HttpHeaderNames;
import io.netty.handler.codec.http.cookie.Cookie;
import io.netty.handler.ssl.SslHandler;
import io.netty.util.NetUtil;
import org.apache.commons.logging.Log;
import reactor.core.publisher.Flux;
import reactor.netty.ChannelOperationsId;
Expand Down Expand Up @@ -74,47 +75,23 @@ public ReactorServerHttpRequest(HttpServerRequest request, NettyDataBufferFactor

private static URI initUri(HttpServerRequest request) throws URISyntaxException {
Assert.notNull(request, "HttpServerRequest must not be null");
return new URI(resolveBaseUrl(request) + resolveRequestUri(request));
return new URI(request.scheme() + "://" + resolveHost(request) + resolveRequestUri(request));
}

private static URI resolveBaseUrl(HttpServerRequest request) throws URISyntaxException {
String scheme = getScheme(request);

private static String resolveHost(HttpServerRequest request) {
InetSocketAddress hostAddress = request.hostAddress();
if (hostAddress != null) {
return new URI(scheme, null, hostAddress.getHostString(), hostAddress.getPort(), null, null, null);
return NetUtil.toSocketAddressString(hostAddress);
}

String header = request.requestHeaders().get(HttpHeaderNames.HOST);
if (header != null) {
final int portIndex;
if (header.startsWith("[")) {
portIndex = header.indexOf(':', header.indexOf(']'));
}
else {
portIndex = header.indexOf(':');
}
if (portIndex != -1) {
try {
return new URI(scheme, null, header.substring(0, portIndex),
Integer.parseInt(header, portIndex + 1, header.length(), 10), null, null, null);
}
catch (NumberFormatException ex) {
throw new URISyntaxException(header, "Unable to parse port", portIndex);
}
}
else {
return new URI(scheme, header, null, null);
}
return header;
}

throw new IllegalStateException("Neither local hostAddress nor HOST header available");
}

private static String getScheme(HttpServerRequest request) {
return request.scheme();
}

private static String resolveRequestUri(HttpServerRequest request) {
String uri = request.uri();
for (int i = 0; i < uri.length(); i++) {
Expand Down