8292876: Do not include the deprecated userinfo component of the URI in HTTP/2 headers #10592
Conversation
|
👋 Welcome back DarraghClarke! A progress list of the required criteria for merging this PR into |
|
@DarraghClarke The following label will be automatically applied to this pull request:
When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command. |
Webrevs
|
src/java.net.http/share/classes/jdk/internal/net/http/Stream.java
Outdated
Show resolved
Hide resolved
|
Thanks for the feedback, I implemented all those changes in the latest commit |
| // TODO: userinfo deprecated. Needs to be removed | ||
| hdrs.setHeader(":authority", uri.getAuthority()); | ||
| // TODO: ensure header names beginning with : not in user headers | ||
| if (uri.getHost() != null && uri.getPort() != -1) { |
There was a problem hiding this comment.
I would suggest creating local variables for host and port in order to avoid calling getHost/getPort multiple times. It will also make possible to assert host != null since that's what we expect (it should not reach here if getHost() returns null)
| try { | ||
| HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString()); | ||
|
|
||
| if (response.statusCode() == 500) { |
There was a problem hiding this comment.
I believe the correct test here is if (response.statusCode() != 200) - if we receive anything other than 200 it is unexpected and the test should fail.
| server.start(); | ||
|
|
||
| HttpClient client = HttpClient | ||
| .newBuilder() |
There was a problem hiding this comment.
It's always better to make sure that no proxy will be selected when building an HttpClient - unless the test has a proxy that should be used.
| .sslContext(sslContext) | ||
| .build(); | ||
|
|
||
| URI uri = new URI("https://user@127.0.0.1:" + Integer.toString(port)); |
There was a problem hiding this comment.
Some machines may not have an IPv4 stack configured. It would be better to use the test lib URIBuilder here to take care of this possibility. Also there's no need for Integer.toString()
|
Also you could start using junit5 for the test instead of testng. |
| @@ -70,7 +71,6 @@ private static Http2TestServer createServer(SSLContext sslContext) throws Except | |||
| return http2TestServer; | |||
| } | |||
|
|
|||
| @Test | |||
| public void main() throws Exception { | |||
There was a problem hiding this comment.
Have you verified in the log that main() is actually called?
| return http2TestServer; | ||
| } | ||
|
|
||
| public void main() throws Exception { |
There was a problem hiding this comment.
This method needs to be annotated with org.junit.jupiter.api.Test. Without this annotation it won't be executed.
| .build(); | ||
|
|
||
| URI uri = URIBuilder.newBuilder() | ||
| .userInfo("user") |
There was a problem hiding this comment.
You need to specify "https" scheme here (.scheme("https")) - without it newBuilder(uri) will fail with java.lang.IllegalArgumentException: URI with undefined scheme.
| .userInfo("user") | ||
| .loopback() | ||
| .port(port) | ||
| .buildUnchecked(); |
There was a problem hiding this comment.
Minor: I think build() would be enough here since the test method throws an Exception.
| try { | ||
| HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString()); | ||
|
|
||
| if (response.statusCode() != 200) { |
There was a problem hiding this comment.
Maybe you could use here one of junit assertions from org.junit.jupiter.api.Assertions to check for status code
|
Thanks for pointing out those issues! pushing changes to address them now |
| if (port != -1) { | ||
| hdrs.setHeader(":authority", host + ":" + port); | ||
| } else { | ||
| hdrs.setHeader(":authority", host); |
There was a problem hiding this comment.
Hello Darragh, the RFC-7540 https://www.rfc-editor.org/rfc/rfc7540.html#section-8.1.2.3 states:
The ":authority" pseudo-header field includes the authority
portion of the target URI ([[RFC3986], Section 3.2](https://www.rfc-editor.org/rfc/rfc3986#section-3.2)). The authority
MUST NOT include the deprecated "userinfo" subcomponent for "http"
or "https" schemed URIs.
So it has specific text about the scheme being "http" or "https". Should we add a check here to check the scheme, before creating this authority header with just the host:port?
I am unfamiliar with websocket (which the HttpClient API supports) which I think will have a different scheme, but a quick check suggests that for websockets, we probably won't reach this part of the code. So it probably is just a theoretical case that the scheme would be anything other than http or https. Perhaps we should just assert instead?
There was a problem hiding this comment.
Furthermore, on the HTTP/1.1 side, in the Http1Request class, there's a hostString() method which creates the value for the Host header field. In that method we have an additional check where we see if the port is unspecified (i.e. -1) and if it is, then we default the port to 443 for secure schemes or 80 for others, when creating the Host header value. I don't know why we do that though (couldn't find anything in spec which says we should be doing that). Should we be doing something similar here while constructing the authority header, to be consistent?
There was a problem hiding this comment.
Hello Darragh, the RFC-7540 https://www.rfc-editor.org/rfc/rfc7540.html#section-8.1.2.3 states:
FWIW, RFC 7540 is now irrelevant; see https://www.rfc-editor.org/rfc/rfc9113.html instead
There was a problem hiding this comment.
Should we be doing something similar here while constructing the authority header, to be consistent?
Well https://www.rfc-editor.org/rfc/rfc9113.html#name-simple-request has an example where authority doesn't have the port - so I don't think we need to add it. FWIW the HTTP/1.1 code seems to be removing the port when it's the default one. Probably for normalization of the host string?
There was a problem hiding this comment.
So it has specific text about the scheme being "http" or "https". Should we add a check here to check the scheme, before creating this authority header with just the host:port?
I don't see how we could reach here if the scheme isn't "http" or "https". Do you have anything in mind Jaikiran? Oh websocket - I see. We don't support websocket over HTTP/2. We could possibly in the future, and if we did, we probably still wouldn't want to send the user-info in the upgrade request?
There was a problem hiding this comment.
Should we be doing something similar here while constructing the authority header, to be consistent?
Well https://www.rfc-editor.org/rfc/rfc9113.html#name-simple-request has an example where authority doesn't have the port - so I don't think we need to add it. FWIW the HTTP/1.1 code seems to be removing the port when it's the default one. Probably for normalization of the host string?
Hello Daniel, you are right - the HTTP/1.1 code is removing the port for default ports. I agree - it's fine to leave this new code in the current form here, without trying to match what's in HTTP/1.1 code.
There was a problem hiding this comment.
Oh websocket - I see. We don't support websocket over HTTP/2. We could possibly in the future, and if we did, we probably still wouldn't want to send the user-info in the upgrade request?
The current state of this new code is fine. I was only checking if there's a chance that any other scheme other than http or https could reach here. Based on what you confirmed, I think this is fine in the current form.
| StringBuilder sb = new StringBuilder(); | ||
| for (int ch; (ch = is.read()) != -1; ) { | ||
| sb.append((char) ch); | ||
| } |
There was a problem hiding this comment.
Is this a left over code perhaps? You could perhaps just replace this completely with:
try(InputStream is = e.getRequestBody()) {
is.readAllBytes();
}
| } | ||
|
|
||
| @Test | ||
| public void main() throws Exception { |
There was a problem hiding this comment.
Nit - perhaps just rename the method to something other than main? Because anyway, this can't be used as a main method without changing its signature to accept a String[]. Perhaps call it testAuthorityHeader? instead?
| Http2TestServer server = createServer(sslContext); | ||
| int port = server.getAddress().getPort(); | ||
|
|
||
| server.start(); |
There was a problem hiding this comment.
junit (and even testng) have before/after constructs (for example junit jupiter has @BeforeAll and @AfterAll https://junit.org/junit5/docs/current/user-guide/#writing-tests-annotations) which get invoked by the test framework. I think moving the server creation and start to @BeforeAll and the server stop to @AfterAll would be better and will keep the test code in this method simpler. Additionally, you won't need the try/finally block here. In its current form, the server won't get stopped if there are any exceptions thrown in this method before the control reaches client.send call.
| } | ||
| is.readAllBytes(); | ||
| is.close(); | ||
| if (e.getRequestURI().getAuthority().contains("user@")) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes - good catch! I missed that. The Http2TestServerConnection does that:
String us = scheme + "://" + authority + path;
URI uri = new URI(us);
but checking the :authority pseudo header would be cleaner.
| try { | ||
| HttpResponse<String> response = client.send(request, HttpResponse.BodyHandlers.ofString()); | ||
|
|
||
| assertEquals(200, response.statusCode(), "Test Failed : " + response.uri().getAuthority()); |
There was a problem hiding this comment.
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.
| .userInfo("user") | ||
| .loopback() | ||
| .port(port) | ||
| .build(); |
There was a problem hiding this comment.
Should we explicitly set the version to HTTP_2 here to make it clear that we are interested in HTTP2 :authority header?
There was a problem hiding this comment.
HTTP_2 is the default and our server is a TLS server supporting only HTTP/2 so it doesn't seem necessary.
For the record we're using TLS here to avoid the upgrade, because for the upgrade request the ":authority" pseudo header is reconstituted on the server side by the test server from the HTTP/1.1 host header - and that's not the code we want to test.
|
Thanks for the notes jaikiran, I think I addressed all of them in the latest commits |
|
Thank you Darragh for those changes. The latest state of this PR in |
|
@DarraghClarke This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 92 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@AlekseiEfimov, @dfuch, @jaikiran) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
/integrate |
|
@DarraghClarke |
|
/sponsor |
|
Going to push as commit b30d922.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@jaikiran @DarraghClarke Pushed as commit b30d922. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Changed the way the
:authoritypseudo 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
:authorityheader.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
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/10592/head:pull/10592$ git checkout pull/10592Update a local copy of the PR:
$ git checkout pull/10592$ git pull https://git.openjdk.org/jdk pull/10592/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 10592View PR using the GUI difftool:
$ git pr show -t 10592Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/10592.diff