RFR: 8292876: Do not include the deprecated userinfo component of the URI in HTTP/2 headers [v5]

Jaikiran Pai jpai at openjdk.org
Tue Oct 11 12:18:25 UTC 2022


On Tue, 11 Oct 2022 11:31:37 GMT, Darragh Clarke <duke at openjdk.org> wrote:

>> Changed the way the `:authority` pseudo header is set to only include host and, if available, port.
>> I added a test to cover this change that consists of a HttpClient that makes a request which contains userInfo, the test passes if the request is carried out with the userInfo not being added to the `:authority` header.
>> 
>> 
>> ### Tests
>> I ran Tier 1 - Tier 3 tests, as well as paying special attention to the http client tests to make sure they consistently passed
>
> Darragh Clarke has updated the pull request incrementally with one additional commit since the last revision:
> 
>   fixed test

test/jdk/java/net/httpclient/http2/UserInfoTest.java line 62:

> 60:             is.readAllBytes();
> 61:             is.close();
> 62:             if (e.getRequestURI().getAuthority().contains("user@")) {

I think we should be checking the value of the `:authority` header here. I haven't yet checked the test server code to see how we construct the URI returned by `Http2TestExchange.getRequestURI`, but even if we used the `:authority` header to create that URI, I still think we should be checking the request headers to verify that the right request header value is received.

test/jdk/java/net/httpclient/http2/UserInfoTest.java line 96:

> 94:                 .loopback()
> 95:                 .port(port)
> 96:                 .build();

Should we explicitly set the version to `HTTP_2` here to make it clear that we are interested in HTTP2 `:authority` header?

test/jdk/java/net/httpclient/http2/UserInfoTest.java line 106:

> 104:             HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString());
> 105: 
> 106:             assertEquals(200, response.statusCode(), "Test Failed : " + response.uri().getAuthority());

I think the `response.uri().getAuthority()` is not necessary in the message and perhaps could be confusing. But it's OK with me if you want to use it in the message.

-------------

PR: https://git.openjdk.org/jdk/pull/10592


More information about the net-dev mailing list