Skip to content

Commit

Permalink
H2 TE header parsing bug fix (#2398)
Browse files Browse the repository at this point in the history 
Motivation:
HTTP/2 has special rules that TE header is only allowed to have
a value of `trailers`. The sanitizing of the TE headers may have
infinite looped in some scenarios.
  • Loading branch information
@Scottmitch
Scottmitch authored Oct 19, 2022
1 parent b6a0e6d commit 6b17ccd
Show file tree
Hide file tree
Showing 4 changed files with 155 additions and 13 deletions.
3 changes: 3 additions & 0 deletions servicetalk-buffer-api/build.gradle
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,5 +29,8 @@ dependencies {
testImplementation "org.hamcrest:hamcrest:$hamcrestVersion"
testImplementation "org.mockito:mockito-core:$mockitoCoreVersion"

testFixturesImplementation platform(project(":servicetalk-dependencies"))
testFixturesImplementation project(":servicetalk-annotations")
testFixturesImplementation "com.google.code.findbugs:jsr305"
testFixturesImplementation "org.hamcrest:hamcrest:$hamcrestVersion"
}
42 changes: 41 additions & 1 deletion servicetalk-buffer-api/src/testFixtures/java/io/servicetalk/buffer/api/Matchers.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,19 @@
*/
package io.servicetalk.buffer.api;

import org.hamcrest.BaseMatcher;
import org.hamcrest.Description;
import org.hamcrest.Matcher;
import org.hamcrest.TypeSafeMatcher;

import java.util.ArrayList;
import java.util.List;
import javax.annotation.Nullable;

import static io.servicetalk.buffer.api.CharSequences.contentEquals;
import static io.servicetalk.buffer.api.CharSequences.contentEqualsIgnoreCase;
import static java.util.Objects.requireNonNull;
import static org.hamcrest.collection.IsIterableContainingInOrder.contains;

/**
* Custom {@link Matcher}s specific to http-api.
Expand All @@ -40,7 +47,6 @@ private Matchers() {
public static Matcher<CharSequence> contentEqualTo(final CharSequence expected) {
requireNonNull(expected);
return new TypeSafeMatcher<CharSequence>() {

@Override
protected boolean matchesSafely(final CharSequence item) {
return contentEquals(expected, item);
Expand All @@ -52,4 +58,38 @@ public void describeTo(final Description description) {
}
};
}

/**
* Matcher which compares in order while ignoring case.
* @param items The expected items.
* @param <E> {@link CharSequence} type to match.
* @return Matcher which compares in order while ignoring case.
*/
@SafeVarargs
public static <E extends CharSequence> Matcher<Iterable<? extends E>> containsIgnoreCase(E... items) {
final List<Matcher<? super E>> matchers = new ArrayList<>(items.length);
for (E item : items) {
matchers.add(new CharsEqualsIgnoreCase(item));
}
return contains(matchers);
}

private static final class CharsEqualsIgnoreCase extends BaseMatcher<CharSequence> {
@Nullable
private final CharSequence expected;

private CharsEqualsIgnoreCase(final CharSequence expected) {
this.expected = expected;
}

@Override
public boolean matches(final Object actual) {
return actual instanceof CharSequence && contentEqualsIgnoreCase(expected, (CharSequence) actual);
}

@Override
public void describeTo(final Description description) {
description.appendValue(expected);
}
}
}
38 changes: 27 additions & 11 deletions servicetalk-http-netty/src/main/java/io/servicetalk/http/netty/H2ToStH1Utils.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import static io.netty.handler.codec.http.HttpHeaderNames.TE;
import static io.netty.handler.codec.http.HttpHeaderValues.TRAILERS;
import static io.servicetalk.buffer.api.CharSequences.contentEqualsIgnoreCase;
import static io.servicetalk.buffer.api.CharSequences.newAsciiString;
import static io.servicetalk.http.api.HttpHeaderNames.CONNECTION;
import static io.servicetalk.http.api.HttpHeaderNames.COOKIE;
Expand Down Expand Up @@ -185,19 +186,34 @@ static Http2Headers h1HeadersToH2Headers(HttpHeaders h1Headers) {
Iterator<? extends CharSequence> teItr = h1Headers.valuesIterator(TE);
boolean addTrailers = false;
while (teItr.hasNext()) {
String teValue = teItr.next().toString();
int i = teValue.indexOf(',');
if (i != -1) {
int start = 0;
do {
if (teValue.substring(start, i).compareToIgnoreCase(TRAILERS.toString()) == 0) {
final CharSequence teSequence = teItr.next();
if (addTrailers) {
teItr.remove();
} else {
int i = indexOf(teSequence, ',', 0);
if (i != -1) {
int start = 0;
do {
if (contentEqualsIgnoreCase(teSequence.subSequence(start, i), TRAILERS)) {
addTrailers = true;
break;
}
start = i + 1;
// Check if we need to skip OWS
// https://www.rfc-editor.org/rfc/rfc9110.html#section-10.1.4
if (start < teSequence.length() && teSequence.charAt(start) == ' ') {
++start;
}
} while (start < teSequence.length() && (i = indexOf(teSequence, ',', start)) != -1);

if (!addTrailers && start < teSequence.length() &&
contentEqualsIgnoreCase(teSequence.subSequence(start, teSequence.length()), TRAILERS)) {
addTrailers = true;
break;
}
} while (start < teValue.length() && (i = teValue.indexOf(',', start)) != -1);
teItr.remove();
} else if (teValue.compareToIgnoreCase(TRAILERS.toString()) != 0) {
teItr.remove();
teItr.remove();
} else if (!contentEqualsIgnoreCase(teSequence, TRAILERS)) {
teItr.remove();
}
}
}
if (addTrailers) { // add after iteration to avoid concurrent modification.
Expand Down
85 changes: 84 additions & 1 deletion ...http-netty/src/test/java/io/servicetalk/http/netty/H2PriorKnowledgeFeatureParityTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,9 @@
import io.servicetalk.http.api.HttpEventKey;
import io.servicetalk.http.api.HttpExecutionStrategy;
import io.servicetalk.http.api.HttpHeaders;
import io.servicetalk.http.api.HttpHeadersFactory;
import io.servicetalk.http.api.HttpMetaData;
import io.servicetalk.http.api.HttpProtocolConfig;
import io.servicetalk.http.api.HttpRequest;
import io.servicetalk.http.api.HttpRequestMethod;
import io.servicetalk.http.api.HttpResponse;
Expand Down Expand Up @@ -123,9 +125,11 @@

import static io.netty.handler.codec.http.HttpHeaderNames.CONTENT_TYPE;
import static io.netty.handler.codec.http.HttpHeaderNames.TRAILER;
import static io.netty.handler.codec.http.HttpHeaderValues.TRAILERS;
import static io.netty.handler.codec.http2.Http2CodecUtil.SMALLEST_MAX_CONCURRENT_STREAMS;
import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR;
import static io.servicetalk.buffer.api.EmptyBuffer.EMPTY_BUFFER;
import static io.servicetalk.buffer.api.Matchers.containsIgnoreCase;
import static io.servicetalk.buffer.api.Matchers.contentEqualTo;
import static io.servicetalk.concurrent.api.Completable.completed;
import static io.servicetalk.concurrent.api.Processors.newPublisherProcessor;
Expand All @@ -140,6 +144,7 @@
import static io.servicetalk.http.api.HttpHeaderNames.COOKIE;
import static io.servicetalk.http.api.HttpHeaderNames.EXPECT;
import static io.servicetalk.http.api.HttpHeaderNames.SET_COOKIE;
import static io.servicetalk.http.api.HttpHeaderNames.TE;
import static io.servicetalk.http.api.HttpHeaderNames.TRANSFER_ENCODING;
import static io.servicetalk.http.api.HttpHeaderNames.UPGRADE;
import static io.servicetalk.http.api.HttpHeaderValues.CHUNKED;
Expand Down Expand Up @@ -389,6 +394,70 @@ void cookiesRoundTrip(HttpTestExecutionStrategy strategy, boolean h2PriorKnowled
}
}

@ParameterizedTest(name = "{displayName} [{index}] client={0}, h2PriorKnowledge={1}")
@MethodSource("clientExecutors")
void teHeaderOnlyAllowsTrailers(HttpTestExecutionStrategy strategy, boolean h2PriorKnowledge) throws Exception {
setUp(strategy, h2PriorKnowledge);
// Newer versions of Netty validate at addition time so for using ServiceTalk headers, so we can add invalid
// headers and assert that ServiceTalk filters them.
final HttpHeadersFactory headersFactory = DefaultHttpHeadersFactory.INSTANCE;
InetSocketAddress serverAddress = bindHttpEchoServer(null, null, headersFactory);
try (BlockingHttpClient client = forSingleAddress(HostAndPort.of(serverAddress))
.protocols(applyHeadersFactory(h2PriorKnowledge, headersFactory))
.executionStrategy(clientExecutionStrategy).buildBlocking()) {
// Test individual headers
HttpRequest request = client.get("/");
request.addHeader(TE, "foo");
request.addHeader(TE, TRAILERS);
request.addHeader(TE, "bar");
HttpResponse response = client.request(request);
assertThat(response.headers().values(TE), h2PriorKnowledge ?
containsIgnoreCase(TRAILERS) : containsIgnoreCase("foo", TRAILERS, "bar"));

// Test single header value, comma separated trailers last
request = client.get("/");
request.addHeader(TE, "foo," + TRAILERS);
response = client.request(request);
assertThat(response.headers().values(TE), h2PriorKnowledge ?
containsIgnoreCase(TRAILERS) : containsIgnoreCase("foo," + TRAILERS));

// Test single header value, comma separated trailers last with OWS
request = client.get("/");
request.addHeader(TE, "foo, " + TRAILERS);
response = client.request(request);
assertThat(response.headers().values(TE), h2PriorKnowledge ?
containsIgnoreCase(TRAILERS) : containsIgnoreCase("foo, " + TRAILERS));

// Test single header value, comma separated trailers first
request = client.get("/");
request.addHeader(TE, TRAILERS + ",foo");
response = client.request(request);
assertThat(response.headers().values(TE), h2PriorKnowledge ?
containsIgnoreCase(TRAILERS) : containsIgnoreCase(TRAILERS + ",foo"));

// Test single header value, comma separated trailers first with OWS
request = client.get("/");
request.addHeader(TE, TRAILERS + ", foo");
response = client.request(request);
assertThat(response.headers().values(TE), h2PriorKnowledge ?
containsIgnoreCase(TRAILERS) : containsIgnoreCase(TRAILERS + ", foo"));

// Test single header value, comma separated trailers middle
request = client.get("/");
request.addHeader(TE, "foo," + TRAILERS + ",bar");
response = client.request(request);
assertThat(response.headers().values(TE), h2PriorKnowledge ?
containsIgnoreCase(TRAILERS) : containsIgnoreCase("foo," + TRAILERS + ",bar"));

// Test single header value, comma separated trailers middle with OWS
request = client.get("/");
request.addHeader(TE, "foo, " + TRAILERS + ", bar");
response = client.request(request);
assertThat(response.headers().values(TE), h2PriorKnowledge ?
containsIgnoreCase(TRAILERS) : containsIgnoreCase("foo, " + TRAILERS + ", bar"));
}
}

@ParameterizedTest(name = "{displayName} [{index}] client={0}, h2PriorKnowledge={1}")
@MethodSource("clientExecutors")
void serverAllowDropTrailers(HttpTestExecutionStrategy strategy, boolean h2PriorKnowledge) throws Exception {
Expand Down Expand Up @@ -1735,8 +1804,21 @@ private InetSocketAddress bindHttpEchoServer() throws Exception {
private InetSocketAddress bindHttpEchoServer(@Nullable StreamingHttpServiceFilterFactory filterFactory,
@Nullable CountDownLatch connectionOnClosingLatch)
throws Exception {
return bindHttpEchoServer(filterFactory, connectionOnClosingLatch, null);
}

private static HttpProtocolConfig applyHeadersFactory(boolean h2PriorKnowledge,
@Nullable HttpHeadersFactory headersFactory) {
return h2PriorKnowledge ?
headersFactory == null ? h2Default() : h2().headersFactory(headersFactory).build() :
headersFactory == null ? h1Default() : h1().headersFactory(headersFactory).build();
}

private InetSocketAddress bindHttpEchoServer(@Nullable StreamingHttpServiceFilterFactory filterFactory,
@Nullable CountDownLatch connectionOnClosingLatch,
@Nullable HttpHeadersFactory headersFactory) throws Exception {
HttpServerBuilder serverBuilder = HttpServers.forAddress(localAddress(0))
.protocols(h2PriorKnowledge ? h2Default() : h1Default());
.protocols(applyHeadersFactory(h2PriorKnowledge, headersFactory));
if (filterFactory != null) {
serverBuilder.appendServiceFilter(filterFactory);
}
Expand Down Expand Up @@ -1779,6 +1861,7 @@ public Completable accept(final ConnectionContext context) {
resp.setHeader(TRANSFER_ENCODING, transferEncoding);
}
resp.headers().set(COOKIE, request.headers().valuesIterator(COOKIE));
resp.headers().set(TE, request.headers().valuesIterator(TE));
return succeeded(resp);
}).toFuture().get();
return (InetSocketAddress) h1ServerContext.listenAddress();
Expand Down

0 comments on commit 6b17ccd

Please sign in to comment.