Skip to content

Commit 8bc9b3b

Browse files
ejona86ejona86
committed
netty: Support pseudo headers in all GrpcHttp2RequestHeaders methods
The previous code assumed that only gRPC would be using these methods. But twice now Netty has made a change (generally relating to security) that used a method for pseudo headers that previously wasn't supported. Let's stop the whack-a-mole and just implement them all. This restores compatibility with Netty 4.1.75.Final. Fixes #8981
1 parent 1234fd3 commit 8bc9b3b

File tree

2 files changed

+161
-25
lines changed

2 files changed

+161
-25
lines changed

‎netty/src/main/java/io/grpc/netty/GrpcHttp2HeadersUtils.java

Lines changed: 36 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -340,7 +340,12 @@ public Http2Headers add(CharSequence csName, CharSequence csValue) {
340340
AsciiString name = validateName(requireAsciiString(csName));
341341
AsciiString value = requireAsciiString(csValue);
342342
if (isPseudoHeader(name)) {
343-
addPseudoHeader(name, value);
343+
AsciiString previous = getPseudoHeader(name);
344+
if (previous != null) {
345+
PlatformDependent.throwException(
346+
connectionError(PROTOCOL_ERROR, "Duplicate %s header", name));
347+
}
348+
setPseudoHeader(name, value);
344349
return this;
345350
}
346351
if (equals(TE_HEADER, name)) {
@@ -353,44 +358,42 @@ public Http2Headers add(CharSequence csName, CharSequence csValue) {
353358
@Override
354359
public CharSequence get(CharSequence csName) {
355360
AsciiString name = requireAsciiString(csName);
356-
checkArgument(!isPseudoHeader(name), "Use direct accessor methods for pseudo headers.");
361+
if (isPseudoHeader(name)) {
362+
return getPseudoHeader(name);
363+
}
357364
if (equals(TE_HEADER, name)) {
358365
return te;
359366
}
360367
return get(name);
361368
}
362369

363-
private void addPseudoHeader(CharSequence csName, CharSequence csValue) {
364-
AsciiString name = requireAsciiString(csName);
365-
AsciiString value = requireAsciiString(csValue);
370+
private AsciiString getPseudoHeader(AsciiString name) {
371+
if (equals(PATH_HEADER, name)) {
372+
return path;
373+
} else if (equals(AUTHORITY_HEADER, name)) {
374+
return authority;
375+
} else if (equals(METHOD_HEADER, name)) {
376+
return method;
377+
} else if (equals(SCHEME_HEADER, name)) {
378+
return scheme;
379+
} else {
380+
return null;
381+
}
382+
}
366383

384+
private void setPseudoHeader(AsciiString name, AsciiString value) {
367385
if (equals(PATH_HEADER, name)) {
368-
if (path != null) {
369-
PlatformDependent.throwException(
370-
connectionError(PROTOCOL_ERROR, "Duplicate :path header"));
371-
}
372386
path = value;
373387
} else if (equals(AUTHORITY_HEADER, name)) {
374-
if (authority != null) {
375-
PlatformDependent.throwException(
376-
connectionError(PROTOCOL_ERROR, "Duplicate :authority header"));
377-
}
378388
authority = value;
379389
} else if (equals(METHOD_HEADER, name)) {
380-
if (method != null) {
381-
PlatformDependent.throwException(
382-
connectionError(PROTOCOL_ERROR, "Duplicate :method header"));
383-
}
384390
method = value;
385391
} else if (equals(SCHEME_HEADER, name)) {
386-
if (scheme != null) {
387-
PlatformDependent.throwException(
388-
connectionError(PROTOCOL_ERROR, "Duplicate :scheme header"));
389-
}
390392
scheme = value;
391393
} else {
392394
PlatformDependent.throwException(
393395
connectionError(PROTOCOL_ERROR, "Illegal pseudo-header '%s' in request.", name));
396+
throw new AssertionError(); // Make flow control obvious to javac
394397
}
395398
}
396399

@@ -418,8 +421,12 @@ public CharSequence scheme() {
418421
public List<CharSequence> getAll(CharSequence csName) {
419422
AsciiString name = requireAsciiString(csName);
420423
if (isPseudoHeader(name)) {
421-
// This code should never be reached.
422-
throw new IllegalArgumentException("Use direct accessor methods for pseudo headers.");
424+
AsciiString value = getPseudoHeader(name);
425+
if (value == null) {
426+
return Collections.emptyList();
427+
} else {
428+
return Collections.singletonList(value);
429+
}
423430
}
424431
if (equals(TE_HEADER, name)) {
425432
return Collections.singletonList((CharSequence) te);
@@ -431,8 +438,12 @@ public List<CharSequence> getAll(CharSequence csName) {
431438
public boolean remove(CharSequence csName) {
432439
AsciiString name = requireAsciiString(csName);
433440
if (isPseudoHeader(name)) {
434-
// This code should never be reached.
435-
throw new IllegalArgumentException("Use direct accessor methods for pseudo headers.");
441+
if (getPseudoHeader(name) == null) {
442+
return false;
443+
} else {
444+
setPseudoHeader(name, null);
445+
return true;
446+
}
436447
}
437448
if (equals(TE_HEADER, name)) {
438449
boolean wasPresent = te != null;

‎netty/src/test/java/io/grpc/netty/GrpcHttp2HeadersUtilsTest.java

Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import static io.netty.util.AsciiString.of;
2323
import static org.junit.Assert.assertEquals;
2424
import static org.junit.Assert.assertTrue;
25+
import static org.junit.Assert.fail;
2526

2627
import com.google.common.collect.Iterables;
2728
import com.google.common.io.BaseEncoding;
@@ -133,6 +134,130 @@ public void decode_emptyHeaders() throws Http2Exception {
133134
assertThat(decodedHeaders.toString()).contains("[]");
134135
}
135136

137+
// contains() is used by Netty 4.1.75+. https://github.com/grpc/grpc-java/issues/8981
138+
// Just implement everything pseudo headers for all methods; too many recent breakages.
139+
@Test
140+
public void grpcHttp2RequestHeaders_pseudoHeaders_notPresent() {
141+
Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2);
142+
assertThat(http2Headers.get(AsciiString.of(":path"))).isNull();
143+
assertThat(http2Headers.get(AsciiString.of(":authority"))).isNull();
144+
assertThat(http2Headers.get(AsciiString.of(":method"))).isNull();
145+
assertThat(http2Headers.get(AsciiString.of(":scheme"))).isNull();
146+
assertThat(http2Headers.get(AsciiString.of(":status"))).isNull();
147+
148+
assertThat(http2Headers.getAll(AsciiString.of(":path"))).isEmpty();
149+
assertThat(http2Headers.getAll(AsciiString.of(":authority"))).isEmpty();
150+
assertThat(http2Headers.getAll(AsciiString.of(":method"))).isEmpty();
151+
assertThat(http2Headers.getAll(AsciiString.of(":scheme"))).isEmpty();
152+
assertThat(http2Headers.getAll(AsciiString.of(":status"))).isEmpty();
153+
154+
assertThat(http2Headers.contains(AsciiString.of(":path"))).isFalse();
155+
assertThat(http2Headers.contains(AsciiString.of(":authority"))).isFalse();
156+
assertThat(http2Headers.contains(AsciiString.of(":method"))).isFalse();
157+
assertThat(http2Headers.contains(AsciiString.of(":scheme"))).isFalse();
158+
assertThat(http2Headers.contains(AsciiString.of(":status"))).isFalse();
159+
160+
assertThat(http2Headers.remove(AsciiString.of(":path"))).isFalse();
161+
assertThat(http2Headers.remove(AsciiString.of(":authority"))).isFalse();
162+
assertThat(http2Headers.remove(AsciiString.of(":method"))).isFalse();
163+
assertThat(http2Headers.remove(AsciiString.of(":scheme"))).isFalse();
164+
assertThat(http2Headers.remove(AsciiString.of(":status"))).isFalse();
165+
}
166+
167+
@Test
168+
public void grpcHttp2RequestHeaders_pseudoHeaders_present() {
169+
Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2);
170+
http2Headers.add(AsciiString.of(":path"), AsciiString.of("mypath"));
171+
http2Headers.add(AsciiString.of(":authority"), AsciiString.of("myauthority"));
172+
http2Headers.add(AsciiString.of(":method"), AsciiString.of("mymethod"));
173+
http2Headers.add(AsciiString.of(":scheme"), AsciiString.of("myscheme"));
174+
175+
assertThat(http2Headers.get(AsciiString.of(":path"))).isEqualTo(AsciiString.of("mypath"));
176+
assertThat(http2Headers.get(AsciiString.of(":authority")))
177+
.isEqualTo(AsciiString.of("myauthority"));
178+
assertThat(http2Headers.get(AsciiString.of(":method"))).isEqualTo(AsciiString.of("mymethod"));
179+
assertThat(http2Headers.get(AsciiString.of(":scheme"))).isEqualTo(AsciiString.of("myscheme"));
180+
181+
assertThat(http2Headers.getAll(AsciiString.of(":path")))
182+
.containsExactly(AsciiString.of("mypath"));
183+
assertThat(http2Headers.getAll(AsciiString.of(":authority")))
184+
.containsExactly(AsciiString.of("myauthority"));
185+
assertThat(http2Headers.getAll(AsciiString.of(":method")))
186+
.containsExactly(AsciiString.of("mymethod"));
187+
assertThat(http2Headers.getAll(AsciiString.of(":scheme")))
188+
.containsExactly(AsciiString.of("myscheme"));
189+
190+
assertThat(http2Headers.contains(AsciiString.of(":path"))).isTrue();
191+
assertThat(http2Headers.contains(AsciiString.of(":authority"))).isTrue();
192+
assertThat(http2Headers.contains(AsciiString.of(":method"))).isTrue();
193+
assertThat(http2Headers.contains(AsciiString.of(":scheme"))).isTrue();
194+
195+
assertThat(http2Headers.remove(AsciiString.of(":path"))).isTrue();
196+
assertThat(http2Headers.remove(AsciiString.of(":authority"))).isTrue();
197+
assertThat(http2Headers.remove(AsciiString.of(":method"))).isTrue();
198+
assertThat(http2Headers.remove(AsciiString.of(":scheme"))).isTrue();
199+
200+
assertThat(http2Headers.contains(AsciiString.of(":path"))).isFalse();
201+
assertThat(http2Headers.contains(AsciiString.of(":authority"))).isFalse();
202+
assertThat(http2Headers.contains(AsciiString.of(":method"))).isFalse();
203+
assertThat(http2Headers.contains(AsciiString.of(":scheme"))).isFalse();
204+
}
205+
206+
@Test
207+
public void grpcHttp2RequestHeaders_pseudoHeaders_set() {
208+
Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2);
209+
http2Headers.set(AsciiString.of(":path"), AsciiString.of("mypath"));
210+
http2Headers.set(AsciiString.of(":authority"), AsciiString.of("myauthority"));
211+
http2Headers.set(AsciiString.of(":method"), AsciiString.of("mymethod"));
212+
http2Headers.set(AsciiString.of(":scheme"), AsciiString.of("myscheme"));
213+
214+
assertThat(http2Headers.getAll(AsciiString.of(":path")))
215+
.containsExactly(AsciiString.of("mypath"));
216+
assertThat(http2Headers.getAll(AsciiString.of(":authority")))
217+
.containsExactly(AsciiString.of("myauthority"));
218+
assertThat(http2Headers.getAll(AsciiString.of(":method")))
219+
.containsExactly(AsciiString.of("mymethod"));
220+
assertThat(http2Headers.getAll(AsciiString.of(":scheme")))
221+
.containsExactly(AsciiString.of("myscheme"));
222+
223+
http2Headers.set(AsciiString.of(":path"), AsciiString.of("mypath2"));
224+
http2Headers.set(AsciiString.of(":authority"), AsciiString.of("myauthority2"));
225+
http2Headers.set(AsciiString.of(":method"), AsciiString.of("mymethod2"));
226+
http2Headers.set(AsciiString.of(":scheme"), AsciiString.of("myscheme2"));
227+
228+
assertThat(http2Headers.getAll(AsciiString.of(":path")))
229+
.containsExactly(AsciiString.of("mypath2"));
230+
assertThat(http2Headers.getAll(AsciiString.of(":authority")))
231+
.containsExactly(AsciiString.of("myauthority2"));
232+
assertThat(http2Headers.getAll(AsciiString.of(":method")))
233+
.containsExactly(AsciiString.of("mymethod2"));
234+
assertThat(http2Headers.getAll(AsciiString.of(":scheme")))
235+
.containsExactly(AsciiString.of("myscheme2"));
236+
}
237+
238+
@Test
239+
public void grpcHttp2RequestHeaders_pseudoHeaders_addWhenPresent_throws() {
240+
Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2);
241+
http2Headers.add(AsciiString.of(":path"), AsciiString.of("mypath"));
242+
try {
243+
http2Headers.add(AsciiString.of(":path"), AsciiString.of("mypath2"));
244+
fail("Expected exception");
245+
} catch (Exception ex) {
246+
// expected
247+
}
248+
}
249+
250+
@Test
251+
public void grpcHttp2RequestHeaders_pseudoHeaders_addInvalid_throws() {
252+
Http2Headers http2Headers = new GrpcHttp2RequestHeaders(2);
253+
try {
254+
http2Headers.add(AsciiString.of(":status"), AsciiString.of("mystatus"));
255+
fail("Expected exception");
256+
} catch (Exception ex) {
257+
// expected
258+
}
259+
}
260+
136261
@Test
137262
public void dupBinHeadersWithComma() {
138263
Key<byte[]> key = Key.of("bytes-bin", BINARY_BYTE_MARSHALLER);

0 commit comments

Comments
 (0)