Skip to content

Commit f3cff57

Browse files
fool1280fool1280
authored
Revert "Return bad request for invalid URI" (#2134)
Reverts #2127
1 parent 05e8df6 commit f3cff57

3 files changed

Lines changed: 57 additions & 155 deletions

File tree

‎zuul-core/src/main/java/com/netflix/zuul/context/CommonContextKeys.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,6 @@ public class CommonContextKeys {
4444
SessionContext.newKey("origin_status_category");
4545
public static final SessionContext.Key<String> ORIGIN_STATUS_CATEGORY_REASON =
4646
SessionContext.newKey("origin_status_category_reason");
47-
public static final SessionContext.Key<String> BAD_URI_REASON = SessionContext.newKey("bad_uri_reason");
4847
public static final SessionContext.Key<Integer> ORIGIN_STATUS = SessionContext.newKey("origin_status");
4948
public static final SessionContext.Key<RequestAttempts> REQUEST_ATTEMPTS =
5049
SessionContext.newKey("request_attempts");

‎zuul-core/src/main/java/com/netflix/zuul/netty/server/ClientRequestReceiver.java‎

Lines changed: 52 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,10 @@
7373
import java.util.Iterator;
7474
import java.util.List;
7575
import java.util.Locale;
76-
import java.util.Map;
7776
import java.util.Map.Entry;
7877
import java.util.Objects;
78+
import java.util.regex.Matcher;
79+
import java.util.regex.Pattern;
7980
import javax.net.ssl.SSLException;
8081
import lombok.NonNull;
8182
import org.slf4j.Logger;
@@ -95,6 +96,10 @@ public class ClientRequestReceiver extends ChannelDuplexHandler {
9596
private static final String SCHEME_HTTP = "http";
9697
private static final String SCHEME_HTTPS = "https";
9798

99+
// via @stephenhay https://mathiasbynens.be/demo/url-regex, groups added
100+
// group 1: scheme, group 2: domain, group 3: path+query
101+
private static final Pattern URL_REGEX = Pattern.compile("^(https?)://([^\\s/$.?#].[^\\s/]*)([^\\s]*)$");
102+
98103
private final SessionContextDecorator decorator;
99104

100105
private HttpRequestMessage zuulRequest;
@@ -153,27 +158,6 @@ private void channelReadInternal(ChannelHandlerContext ctx, Object msg) {
153158
RejectionUtils.rejectByClosingConnection(
154159
ctx, ZuulStatusCategory.FAILURE_CLIENT_BAD_REQUEST, "decodefailure", clientRequest, null);
155160
return;
156-
} else if (zuulRequest.getContext().containsKey(CommonContextKeys.BAD_URI_REASON)) {
157-
String clientIp = Objects.requireNonNullElse(getClientIp(ctx.channel()), "unknown");
158-
String badUriReason = zuulRequest.getContext().get(CommonContextKeys.BAD_URI_REASON);
159-
LOG.warn(
160-
"Invalid URI in request. reason = {}, clientRequest = {}, clientIp = {}, info = {}",
161-
badUriReason,
162-
clientRequest,
163-
clientIp,
164-
ChannelUtils.channelInfoForLogging(ctx.channel()));
165-
StatusCategoryUtils.setStatusCategory(
166-
zuulRequest.getContext(), ZuulStatusCategory.FAILURE_CLIENT_BAD_REQUEST, badUriReason);
167-
RejectionUtils.sendRejectionResponse(
168-
ctx,
169-
ZuulStatusCategory.FAILURE_CLIENT_BAD_REQUEST,
170-
"invaliduri",
171-
clientRequest,
172-
null,
173-
HttpResponseStatus.BAD_REQUEST,
174-
badUriReason,
175-
Map.of());
176-
return;
177161
} else if (zuulRequest.hasBody() && zuulRequest.getBodyLength() > zuulRequest.getMaxBodySize()) {
178162
String errorMsg = "Request too large. "
179163
+ "clientRequest = " + clientRequest.toString()
@@ -379,13 +363,7 @@ private HttpRequestMessage buildZuulHttpRequest(HttpRequest nativeRequest, Chann
379363
}
380364

381365
// Strip off the query from the path.
382-
String path;
383-
try {
384-
path = parsePath(preProcessPath(nativeRequest.uri()));
385-
} catch (URISyntaxException ex) {
386-
path = nativeRequest.uri();
387-
context.put(CommonContextKeys.BAD_URI_REASON, ex.getReason());
388-
}
366+
String path = parsePath(nativeRequest.uri());
389367

390368
// Setup the req/resp message objects.
391369
HttpRequestMessage request = new HttpRequestMessageImpl(
@@ -431,38 +409,56 @@ protected String getClientIp(Channel channel) {
431409
return channel.attr(SourceAddressChannelHandler.ATTR_SOURCE_ADDRESS).get();
432410
}
433411

434-
protected String preProcessPath(String uri) {
435-
return uri;
436-
}
412+
private String parsePath(String uri) {
413+
String path;
414+
try {
415+
URI uriObject = new URI(uri);
416+
uriObject = uriObject.normalize();
417+
path = uriObject.getRawPath();
418+
if (path == null) {
419+
// If we have an opaque URI, match existing behavior of using the URI as the path.
420+
return uri;
421+
}
422+
while (path.startsWith("/..")) {
423+
path = path.substring(3);
424+
}
425+
return path;
426+
} catch (URISyntaxException ex) {
427+
LOG.debug("URI syntax error", ex);
428+
}
429+
// manual path parsing
430+
// relative uri
431+
if (uri.startsWith("/")) {
432+
path = uri;
433+
} else {
434+
Matcher m = URL_REGEX.matcher(uri);
435+
436+
// absolute uri
437+
if (m.matches()) {
438+
String match = m.group(3);
439+
if (match == null) {
440+
// in case of no match, default to existing behavior
441+
path = uri;
442+
} else {
443+
path = match;
444+
}
445+
}
446+
// unknown value
447+
else {
448+
// in case of unknown value, default to existing behavior
449+
path = uri;
450+
}
451+
}
437452

438-
private String parsePath(String uri) throws URISyntaxException {
439-
int queryIndex = uri.indexOf('?');
453+
int queryIndex = path.indexOf('?');
440454
if (queryIndex > -1) {
441-
uri = uri.substring(0, queryIndex);
455+
path = path.substring(0, queryIndex);
442456
}
443457

444-
URI uriObject = new URI(uri);
445-
if (uriObject.isOpaque()) {
446-
throw new URISyntaxException(uri, "opaque URI");
447-
}
448-
String rawPath = uriObject.getRawPath();
449-
// Mirrors Envoy's REJECT_REQUEST
450-
// https://github.com/envoyproxy/envoy/blob/main/source/extensions/http/header_validators/envoy_default/path_normalizer.cc#L91
451-
if (containsEncodedSlash(rawPath)) {
452-
throw new URISyntaxException(uri, "encoded slash in path");
458+
while (path.startsWith("/..")) {
459+
path = path.substring(3);
453460
}
454-
// Fold %2E → '.' (unreserved per RFC 3986 §2.4) so normalize() can remove dot-segments.
455-
String prepared = rawPath.replace("%2e", ".").replace("%2E", ".");
456-
String normalized = new URI(prepared).normalize().getRawPath();
457-
// RFC 3986 §5.2.4 discards leading "/..", but Java preserves it for relative paths.
458-
while (normalized.startsWith("/..")) {
459-
normalized = normalized.substring(3);
460-
}
461-
return normalized;
462-
}
463-
464-
private static boolean containsEncodedSlash(String rawPath) {
465-
return rawPath.contains("%2F") || rawPath.contains("%2f");
461+
return path;
466462
}
467463

468464
private static Headers copyHeaders(HttpRequest req) {

‎zuul-core/src/test/java/com/netflix/zuul/netty/server/ClientRequestReceiverTest.java‎

Lines changed: 5 additions & 98 deletions
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,6 @@
4949
import org.checkerframework.checker.nullness.qual.NonNull;
5050
import org.junit.jupiter.api.Test;
5151
import org.junit.jupiter.api.extension.ExtendWith;
52-
import org.junit.jupiter.params.ParameterizedTest;
53-
import org.junit.jupiter.params.provider.ValueSource;
5452
import org.mockito.junit.jupiter.MockitoExtension;
5553

5654
/**
@@ -271,62 +269,6 @@ void maxHeaderSizeExceeded_setBadRequestStatus() {
271269
.isEqualTo("Invalid request provided: Decode failure");
272270
}
273271

274-
@Test
275-
void invalidUri_setBadRequestStatus() {
276-
ClientRequestReceiver receiver = new ClientRequestReceiver(null);
277-
EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestEncoder());
278-
PassportLoggingHandler loggingHandler = new PassportLoggingHandler(new DefaultRegistry());
279-
280-
channel.attr(SourceAddressChannelHandler.ATTR_SERVER_LOCAL_PORT).set(1234);
281-
channel.pipeline().addLast(new HttpServerCodec());
282-
channel.pipeline().addLast(receiver);
283-
channel.pipeline().addLast(loggingHandler);
284-
285-
HttpRequest httpRequest = new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/{invalid}");
286-
287-
channel.writeOutbound(httpRequest);
288-
ByteBuf byteBuf = channel.readOutbound();
289-
channel.writeInbound(byteBuf);
290-
channel.readInbound();
291-
channel.close();
292-
293-
HttpRequestMessage request = ClientRequestReceiver.getRequestFromChannel(channel);
294-
SessionContext context = request.getContext();
295-
assertThat(context.get(CommonContextKeys.BAD_URI_REASON)).isNotNull();
296-
assertThat(StatusCategoryUtils.getStatusCategory(context))
297-
.isEqualTo(ZuulStatusCategory.FAILURE_CLIENT_BAD_REQUEST);
298-
// Raw URI preserved for access logging, not replaced with a placeholder.
299-
assertThat(request.getPath()).isEqualTo("/{invalid}");
300-
}
301-
302-
@Test
303-
void invalidPercentEncoding_setBadRequestStatus() {
304-
// %GG is invalid (G is not a hex digit) — old fallback forwarded the un-normalized path to origin
305-
ClientRequestReceiver receiver = new ClientRequestReceiver(null);
306-
EmbeddedChannel channel = new EmbeddedChannel(new HttpRequestEncoder());
307-
PassportLoggingHandler loggingHandler = new PassportLoggingHandler(new DefaultRegistry());
308-
309-
channel.attr(SourceAddressChannelHandler.ATTR_SERVER_LOCAL_PORT).set(1234);
310-
channel.pipeline().addLast(new HttpServerCodec());
311-
channel.pipeline().addLast(receiver);
312-
channel.pipeline().addLast(loggingHandler);
313-
314-
HttpRequest httpRequest =
315-
new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/api/%GG/../../actuator/env");
316-
317-
channel.writeOutbound(httpRequest);
318-
ByteBuf byteBuf = channel.readOutbound();
319-
channel.writeInbound(byteBuf);
320-
channel.readInbound();
321-
channel.close();
322-
323-
HttpRequestMessage request = ClientRequestReceiver.getRequestFromChannel(channel);
324-
SessionContext context = request.getContext();
325-
assertThat(context.get(CommonContextKeys.BAD_URI_REASON)).isNotNull();
326-
assertThat(StatusCategoryUtils.getStatusCategory(context))
327-
.isEqualTo(ZuulStatusCategory.FAILURE_CLIENT_BAD_REQUEST);
328-
}
329-
330272
@Test
331273
void multipleHostHeaders_setBadRequestStatus() {
332274
ClientRequestReceiver receiver = new ClientRequestReceiver(null);
@@ -594,55 +536,20 @@ void pathTraversal_withQueryString() {
594536
channel.close();
595537
}
596538

597-
@ParameterizedTest
598-
@ValueSource(strings = {"/public/%2e%2e/admin/", "/public/%2E%2E/admin/"})
599-
void pathTraversal_encodedDotDot(String uri) {
539+
@Test
540+
void pathTraversal_withOpaqueURI() {
600541
EmbeddedChannel channel = new EmbeddedChannel(new ClientRequestReceiver(null));
601542
channel.attr(SourceAddressChannelHandler.ATTR_SERVER_LOCAL_PORT).set(1234);
602543
HttpRequestMessageImpl result;
603544
{
604-
channel.writeInbound(
605-
new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri, Unpooled.buffer()));
545+
channel.writeInbound(new DefaultFullHttpRequest(
546+
HttpVersion.HTTP_1_1, HttpMethod.GET, "foo.netflix.net:443", Unpooled.buffer()));
606547
result = channel.readInbound();
607548
result.disposeBufferedBody();
608549
}
609550

610-
assertThat(result.getPath()).isEqualTo("/admin/");
611-
channel.close();
612-
}
613-
614-
@ParameterizedTest
615-
@ValueSource(strings = {"/public/%2F../secret", "/public/%2f../secret"})
616-
void encodedSlash_rejected(String uri) {
617-
EmbeddedChannel channel = new EmbeddedChannel(new ClientRequestReceiver(null));
618-
channel.attr(SourceAddressChannelHandler.ATTR_SERVER_LOCAL_PORT).set(1234);
619-
channel.writeInbound(new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, uri, Unpooled.buffer()));
620-
channel.readInbound();
551+
assertThat(result.getPath()).isEqualTo("foo.netflix.net:443");
621552
channel.close();
622-
623-
SessionContext context =
624-
ClientRequestReceiver.getRequestFromChannel(channel).getContext();
625-
assertThat(context.get(CommonContextKeys.BAD_URI_REASON)).isNotNull();
626-
assertThat(StatusCategoryUtils.getStatusCategory(context))
627-
.isEqualTo(ZuulStatusCategory.FAILURE_CLIENT_BAD_REQUEST);
628-
assertThat(StatusCategoryUtils.getStatusCategoryReason(context)).isEqualTo("encoded slash in path");
629-
}
630-
631-
@Test
632-
void opaqueUri_rejected() {
633-
EmbeddedChannel channel = new EmbeddedChannel(new ClientRequestReceiver(null));
634-
channel.attr(SourceAddressChannelHandler.ATTR_SERVER_LOCAL_PORT).set(1234);
635-
channel.writeInbound(new DefaultFullHttpRequest(
636-
HttpVersion.HTTP_1_1, HttpMethod.GET, "foo.netflix.net:443", Unpooled.buffer()));
637-
channel.readInbound();
638-
channel.close();
639-
640-
SessionContext context =
641-
ClientRequestReceiver.getRequestFromChannel(channel).getContext();
642-
assertThat(context.get(CommonContextKeys.BAD_URI_REASON)).isNotNull();
643-
assertThat(StatusCategoryUtils.getStatusCategory(context))
644-
.isEqualTo(ZuulStatusCategory.FAILURE_CLIENT_BAD_REQUEST);
645-
assertThat(StatusCategoryUtils.getStatusCategoryReason(context)).isEqualTo("opaque URI");
646553
}
647554

648555
@Test

0 commit comments

Comments
 (0)