Skip to content

Commit

Permalink
Convert scheme to lower case when parsing URI (#2938)
Browse files Browse the repository at this point in the history
Motivation:

`HttpRequestMetaData#scheme()` javadocs state the result will be lowercase, but this is not always the case because `Uri3986` parses the scheme as-is. As a result, `DefaultMultiAddressUrlHttpClientBuilder` may have more than 1 client instance for the same address if users use URI with different scheme letters case. 
See #1423 for details.

Modifications:

- Convert scheme to lower case when parsing `Uri3986` reusing well-known schemes ("http" and "https")

Result:

Fixes #1423.
  • Loading branch information
0x1306e6d authored Jun 2, 2024
1 parent b49e66b commit bc6c3c4
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 4 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package io.servicetalk.http.api;

import java.nio.charset.Charset;
import java.util.Locale;
import java.util.Objects;
import javax.annotation.Nullable;

Expand All @@ -38,6 +39,8 @@
final class Uri3986 implements Uri {
@SuppressWarnings({"StringOperationCanBeSimplified", "PMD.StringInstantiation"})
private static final String NULL_COMPONENT = new String(""); // instance equality required!
private static final String HTTP_SCHEME = "http";
private static final String HTTPS_SCHEME = "https";
private final String uri;
@Nullable
private final String scheme;
Expand Down Expand Up @@ -169,7 +172,7 @@ final class Uri3986 implements Uri {
if (i == 0) {
throw new IllegalArgumentException("Invalid URI format: no scheme before colon (':')");
}
parsedScheme = uri.substring(0, i);
parsedScheme = getLowerCaseScheme(uri, i);
begin = ++i;
// We don't enforce the following, browsers still generate these types of requests.
// https://tools.ietf.org/html/rfc3986#section-3.3
Expand All @@ -196,6 +199,15 @@ final class Uri3986 implements Uri {
this.uri = uri;
}

private static String getLowerCaseScheme(final String uri, final int endIndex) {
if (endIndex == 4 && uri.regionMatches(true, 0, HTTP_SCHEME, 0, 4)) {
return HTTP_SCHEME;
} else if (endIndex == 5 && uri.regionMatches(true, 0, HTTPS_SCHEME, 0, 5)) {
return HTTPS_SCHEME;
}
return uri.substring(0, endIndex).toLowerCase(Locale.ENGLISH);
}

@Override
public String uri() {
return uri;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,10 @@
import static java.util.regex.Pattern.compile;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalToIgnoringCase;
import static org.hamcrest.Matchers.lessThan;
import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.Matchers.sameInstance;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertThrows;

Expand Down Expand Up @@ -162,8 +165,21 @@ void emptyUserInfo() {
}

@Test
void schemeParsedCaseSensative() {
verifyUri3986("HTTP://test.com", "HTTP", null, "test.com", -1, "", "", null, null, null);
void schemeParsedCaseInsensitive() {
verifyUri3986("HTTP://test.com", "http", null, "test.com", -1, "", "", null, null, null);
verifyUri3986("HTTPS://test.com", "https", null, "test.com", -1, "", "", null, null, null);
verifyUri3986("ANY://test.com", "any", null, "test.com", -1, "", "", null, null, null);
}

@Test
void reuseWellKnownScheme() {
String http1 = new Uri3986("http://test.com").scheme();
String http2 = new Uri3986("HTTP://test.com").scheme();
assertThat("different http", http1, sameInstance(http2));

String https1 = new Uri3986("https://test.com").scheme();
String https2 = new Uri3986("HTTPS://test.com").scheme();
assertThat("different https", https1, sameInstance(https2));
}

@Test
Expand Down Expand Up @@ -637,7 +653,9 @@ private static void verifyUri3986(String expectedUri, @Nullable String expectedS
// Validate against the RFC's regex
Matcher matcher = VALID_PATTERN.matcher(expectedUri);
assertThat(true, is(matcher.matches()));
assertThat("invalid scheme()", uri.scheme(), is(matcher.group(2)));
String matcherScheme = matcher.group(2);
assertThat("invalid scheme()", uri.scheme(),
matcherScheme == null ? is(nullValue()) : equalToIgnoringCase(matcherScheme));
assertThat("invalid authority()", uri.authority(), is(matcher.group(4)));
assertThat("invalid path()", uri.path(), is(matcher.group(5)));
assertThat("invalid query()", uri.query(), is(matcher.group(7)));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
import org.junit.jupiter.api.extension.RegisterExtension;

import java.net.InetSocketAddress;
import java.util.concurrent.atomic.AtomicInteger;
import java.util.concurrent.atomic.AtomicReference;

import static io.servicetalk.buffer.api.Matchers.contentEqualTo;
Expand Down Expand Up @@ -222,6 +223,28 @@ void internalClientsUseDifferentExecutionContextWhenConfigured() throws Exceptio
}
}

@Test
void internalClientSchemeShouldBeCaseInsensitive() throws Exception {
AtomicInteger counter = new AtomicInteger();

try (ServerContext serverContext = HttpServers.forAddress(localAddress(0))
.listenStreamingAndAwait((ctx, request, responseFactory) -> succeeded(responseFactory.ok()));
BlockingHttpClient blockingHttpClient = HttpClients.forMultiAddressUrl(getClass().getSimpleName())
.initializer((scheme, address, builder) -> counter.incrementAndGet())
.buildBlocking()) {

HttpResponse response = blockingHttpClient.request(
blockingHttpClient.get("http://" + serverHostAndPort(serverContext)));
assertThat(response.status(), is(OK));

response = blockingHttpClient.request(
blockingHttpClient.get("HTTP://" + serverHostAndPort(serverContext)));
assertThat(response.status(), is(OK));

assertThat(counter.get(), is(1));
}
}

private static void assertExecutionContext(HttpExecutionContext expected, HttpExecutionContext actual) {
assertThat(actual, is(notNullValue()));
assertThat(actual.ioExecutor(), is(sameInstance(expected.ioExecutor())));
Expand Down

0 comments on commit bc6c3c4

Please sign in to comment.