Skip to content
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

Fix URL parsing issue (#391) #393

Merged
merged 3 commits into from
Apr 5, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
10 changes: 5 additions & 5 deletions components/api/src/main/java/com/hotels/styx/api/Url.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright (C) 2013-2018 Expedia Inc.
Copyright (C) 2013-2019 Expedia Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -66,7 +66,7 @@ private Url(Builder builder) {
* @return scheme
*/
public String scheme() {
return this.scheme;
return this.scheme != null ? this.scheme : "";
}

/**
Expand Down Expand Up @@ -121,7 +121,7 @@ public boolean isFullyQualified() {
* @return true if the URL is absolute.
*/
public boolean isAbsolute() {
return path.startsWith("/");
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this method used for? We have changed from it's an absolute path to i_t's an absolute URI_ . What's the real purpose?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can only guess. I believe the original intention was to determine if the request is in origin-form or absolute-form. My gut feeling is that we don't really need this method in the URL class.

As to why this change? It is because every path component always starts with / therefore it always returned true.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would at least mark it as deprecated then. I think from previous discussions our goal could be to completely remove Url from the API.

return Objects.nonNull(scheme) && !scheme.isEmpty();
}

/**
Expand All @@ -131,7 +131,7 @@ public boolean isAbsolute() {
* @see {@link #isAbsolute()}
*/
public boolean isRelative() {
return !isAbsolute();
return scheme == null || scheme.isEmpty();
}

/**
Expand Down Expand Up @@ -455,7 +455,7 @@ public static Builder url(String value) {
.fragment(uri.getFragment());
}

Builder rawQuery(String rawQuery) {
public Builder rawQuery(String rawQuery) {
this.queryBuilder = rawQuery == null ? null : new UrlQuery.Builder(rawQuery);
return this;
}
Expand Down
10 changes: 5 additions & 5 deletions components/api/src/test/java/com/hotels/styx/api/UrlTest.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright (C) 2013-2018 Expedia Inc.
Copyright (C) 2013-2019 Expedia Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -192,13 +192,13 @@ public void authorityReturnsHostAndPortStringWithoutPort() {
public void identifiesCorrectlyIfUrlIsFullyQualified() throws Exception {
Url fqUrl = url("http://example.com").build();
assertThat(fqUrl.isFullyQualified(), is(true));
assertThat(fqUrl.isAbsolute(), is(false));
assertThat(fqUrl.isRelative(), is(true));
assertThat(fqUrl.isAbsolute(), is(true));
assertThat(fqUrl.isRelative(), is(false));

Url nonFqUrl = url("/somepath").build();
assertThat(nonFqUrl.isFullyQualified(), is(false));
assertThat(nonFqUrl.isAbsolute(), is(true));
assertThat(nonFqUrl.isRelative(), is(false));
assertThat(nonFqUrl.isAbsolute(), is(false));
assertThat(nonFqUrl.isRelative(), is(true));
}

@Test
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
Copyright (C) 2013-2018 Expedia Inc.
Copyright (C) 2013-2019 Expedia Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
Expand All @@ -26,7 +26,6 @@
import io.netty.buffer.ByteBuf;
import io.netty.buffer.ByteBufHolder;
import io.netty.channel.ChannelHandlerContext;
import io.netty.channel.embedded.EmbeddedChannel;
import io.netty.handler.codec.MessageToMessageDecoder;
import io.netty.handler.codec.TooLongFrameException;
import io.netty.handler.codec.http.HttpContent;
Expand All @@ -39,7 +38,6 @@
import rx.Producer;
import rx.Subscriber;

import java.net.InetSocketAddress;
import java.net.MalformedURLException;
import java.net.URL;
import java.util.ArrayDeque;
Expand All @@ -50,7 +48,6 @@
import static com.google.common.collect.Iterables.size;
import static com.hotels.styx.api.HttpHeaderNames.EXPECT;
import static com.hotels.styx.api.HttpHeaderNames.HOST;
import static com.hotels.styx.api.Url.Builder.url;
import static com.hotels.styx.server.UniqueIdSuppliers.UUID_VERSION_ONE_SUPPLIER;
import static com.hotels.styx.server.netty.codec.UnwiseCharsEncoder.IGNORE;
import static java.util.Objects.requireNonNull;
Expand Down Expand Up @@ -165,8 +162,7 @@ private static ByteBuf content(HttpObject httpObject) {

@VisibleForTesting
LiveHttpRequest.Builder makeAStyxRequestFrom(HttpRequest request, Observable<ByteBuf> content) {
Url url = url(unwiseCharEncoder.encode(request.uri()))
.build();
Url url = UrlDecoder.decodeUrl(unwiseCharEncoder, request);
LiveHttpRequest.Builder requestBuilder = new LiveHttpRequest.Builder()
.method(toStyxMethod(request.method()))
.url(url)
Expand All @@ -188,14 +184,6 @@ private com.hotels.styx.api.HttpMethod toStyxMethod(HttpMethod method) {
return com.hotels.styx.api.HttpMethod.httpMethod(method.name());
}

private static InetSocketAddress remoteAddress(ChannelHandlerContext ctx) {
if (ctx.channel() instanceof EmbeddedChannel) {
return new InetSocketAddress(0);
}

return (InetSocketAddress) ctx.channel().remoteAddress();
}

private static class FlowControllingHttpContentProducer implements Producer {
private final ChannelHandlerContext ctx;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
Copyright (C) 2013-2019 Expedia Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package com.hotels.styx.server.netty.codec;

import com.hotels.styx.api.Url;
import io.netty.handler.codec.http.HttpRequest;

import java.net.URI;

import static com.hotels.styx.api.HttpHeaderNames.HOST;
import static com.hotels.styx.api.Url.Builder.url;

final class UrlDecoder {
private UrlDecoder() {
}

static Url decodeUrl(UnwiseCharsEncoder unwiseCharEncoder, HttpRequest request) {
String host = request.headers().get(HOST);

if (request.uri().startsWith("/") && host != null) {
URI uri = URI.create("http://" + host + unwiseCharEncoder.encode(request.uri()));
return new Url.Builder()
.path(uri.getRawPath())
.rawQuery(uri.getRawQuery())
.fragment(uri.getFragment())
.build();
} else {
return url(unwiseCharEncoder.encode(request.uri())).build();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ public void throwsBadRequestExceptionWhenRequestMessageContainsMoreThanOneHostHe
}

@Test
public void callsTheEscaperForUnwiseChars() throws Exception {
public void callsTheEscaperForUnwiseChars() {
UnwiseCharsEncoder encoder = mock(UnwiseCharsEncoder.class);
NettyToStyxRequestDecoder decoder = new NettyToStyxRequestDecoder.Builder()
.uniqueIdSupplier(uniqueIdSupplier)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
/*
Copyright (C) 2013-2019 Expedia Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package com.hotels.styx.server.netty.codec;

import com.hotels.styx.api.Url;
import io.netty.handler.codec.http.DefaultFullHttpRequest;
import org.testng.annotations.Test;

import java.util.Optional;

import static io.netty.handler.codec.http.HttpHeaders.Names.HOST;
import static io.netty.handler.codec.http.HttpMethod.GET;
import static io.netty.handler.codec.http.HttpVersion.HTTP_1_1;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;

public class UrlDecoderTest {
@Test
public void decodesOriginForm() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have made some changes to the test files. Please have another look.

DefaultFullHttpRequest request = new DefaultFullHttpRequest(HTTP_1_1, GET, "/foo");
request.headers().add(HOST, "example.com");

Url url = UrlDecoder.decodeUrl(x -> x, request);

assertThat(url.authority(), is(Optional.empty()));
assertThat(url.path(), is("/foo"));
assertThat(url.encodedUri(), is("/foo"));
assertThat(url.isAbsolute(), is(false));
assertThat(url.isRelative(), is(true));
assertThat(url.host(), is(Optional.empty()));
assertThat(url.query(), is(Optional.empty()));
assertThat(url.scheme(), is(""));
}

// From GitHub issue #391.
@Test
public void exposesPathComponentsWithDoubleSlashSeparators() {
DefaultFullHttpRequest request = new DefaultFullHttpRequest(HTTP_1_1, GET, "//www.abc.com//abc123Z");
request.headers().add(HOST, "example.com");

Url url = UrlDecoder.decodeUrl(x -> x, request);

assertThat(url.authority(), is(Optional.empty()));
assertThat(url.path(), is("//www.abc.com//abc123Z"));
assertThat(url.encodedUri(), is("//www.abc.com//abc123Z"));
assertThat(url.isAbsolute(), is(false));
assertThat(url.isRelative(), is(true));
assertThat(url.host(), is(Optional.empty()));
assertThat(url.query(), is(Optional.empty()));
assertThat(url.scheme(), is(""));
}

// Demonstrates that Host header handling is case insensitive.
@Test
public void decodesOriginFormWithUppercaseHostHeader() {
DefaultFullHttpRequest request = new DefaultFullHttpRequest(HTTP_1_1, GET, "/foo");
request.headers().add("HOST", "example.com");

Url url = UrlDecoder.decodeUrl(x -> x, request);

assertThat(url.authority(), is(Optional.empty()));
assertThat(url.path(), is("/foo"));
assertThat(url.encodedUri(), is("/foo"));
assertThat(url.isAbsolute(), is(false));
assertThat(url.isRelative(), is(true));
assertThat(url.host(), is(Optional.empty()));
assertThat(url.query(), is(Optional.empty()));
assertThat(url.scheme(), is(""));
}


@Test
public void decodesAbsoluteForm() {
DefaultFullHttpRequest request = new DefaultFullHttpRequest(HTTP_1_1, GET, "http://example.com/foo");

Url url = UrlDecoder.decodeUrl(x -> x, request);

assertThat(url.authority().isPresent(), is(true));
assertThat(url.path(), is("/foo"));
assertThat(url.encodedUri(), is("http://example.com/foo"));
assertThat(url.isAbsolute(), is(true));
assertThat(url.isRelative(), is(false));
assertThat(url.host(), is(Optional.of("example.com")));
assertThat(url.query(), is(Optional.empty()));
assertThat(url.scheme(), is("http"));
}


}