8144100: Incorrect case-sensitive equality in com.sun.net.httpserver.BasicAuthenticator #19133
Conversation
|
👋 Welcome back nizarbenalla! A progress list of the required criteria for merging this PR into |
|
@nizarbenalla 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 224 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 (@jaikiran, @dfuch) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
|
@nizarbenalla 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. |
| static HttpServer server() throws Exception { | ||
| String realm = "someRealm"; | ||
| ServerAuthenticator authenticator = new ServerAuthenticator(realm); | ||
| HttpServer server = HttpServer.create(new InetSocketAddress(0), 0); |
There was a problem hiding this comment.
Hello Nizar, we should use loopback address to bind the server to. That's the common practice we follow in our networking tests. So:
new InetSocketAddress(InetAddress.getLoopbackAddress(), 0)) ...
| HttpServer server = HttpServer.create(new InetSocketAddress(0), 0); | ||
| server.createContext(someContext, exchange -> { | ||
| if (authenticator.authenticate(exchange) instanceof Authenticator.Failure) { | ||
| exchange.sendResponseHeaders(401, 0); |
There was a problem hiding this comment.
The second parameter here should be -1 implying no response body. 0 implies chunked response.
| exchange.close(); | ||
| return; | ||
| } | ||
| exchange.sendResponseHeaders(200, 1); |
There was a problem hiding this comment.
Second parameter here should be -1. 1 implies the response will contain a body of 1 byte in length.
| String statusLine = reader.readLine(); | ||
| System.err.println(statusLine); | ||
|
|
||
| if (statusLine.startsWith("HTTP/1.1 401")) { |
There was a problem hiding this comment.
It might be better to check that the status line is 200 and anything other than that should result in an exception from here.
| } | ||
|
|
||
| static class ServerAuthenticator extends BasicAuthenticator { | ||
| ServerAuthenticator(String realm) { |
There was a problem hiding this comment.
Since this test relies on the authenticator being invoked, just as an additional check, you might want to have a boolean field in this class (named something like invoked) which will be set to true only when the checkCredentials gets called. Then after checking the response code in the test, you can additionally assert that this field's value is true, just to be certain that this authenticator was indeed invoked.
| * @test | ||
| * @bug 8144100 | ||
| * @run main/othervm BasicAuthToken | ||
| * @summary checking token sent by client should be done in case-insensitive manner |
There was a problem hiding this comment.
As a guideline, jtreg recommended order of test definition tags is here https://openjdk.org/jtreg/tag-spec.html#ORDER. As per that the @summary should follow the @bug before the @run.
| } | ||
|
|
||
| static void client(int port) throws Exception { | ||
| try (Socket socket = new Socket("localhost", port)) { |
There was a problem hiding this comment.
Once you change the server to bind to loopback address, the Socket creation should then use loopback address too: new Socket(InetAddress.getLoopbackAddress(), port)
Webrevs
|
|
|
||
|
|
||
| static class ServerAuthenticator extends BasicAuthenticator { | ||
| private static boolean invoked = false; |
There was a problem hiding this comment.
this should be declared volatile since it will be mutated in one thread and read in the other.
There was a problem hiding this comment.
Fixed in 5003802.
I ran the tests again and it passes Tier 1-3.
| * @test | ||
| * @bug 8144100 | ||
| * @summary checking token sent by client should be done in case-insensitive manner | ||
| * @run main/othervm BasicAuthToken |
There was a problem hiding this comment.
Hello Nizar, I couldn't spot anything in the test that forces the use of "othervm". Is the othervm intentional? If not, I would recommend changing it to just "main".
There was a problem hiding this comment.
Sure, was just being extra careful.
Will fix it.
| System.err.println(statusLine); | ||
|
|
||
| if (!statusLine.startsWith("HTTP/1.1 200")) { | ||
| throw new RuntimeException("Basic Authentication failed: case-insensitive token" + |
There was a problem hiding this comment.
Nit: Might be better to just state:
throw new RuntimeException("unexpected status line: " + statusLine);
| throw new RuntimeException("Basic Authentication failed: case-insensitive token" + | ||
| " used to identify authentication scheme sent by client parsed incorrectly"); | ||
| } | ||
| assert ServerAuthenticator.wasChecked() : "Authenticator was not correctly invoked"; |
There was a problem hiding this comment.
As far as I know, the jtreg tests that we launch (through make) will always have asserts enabled. So I think using assert here is OK. However, to be consistent with other conditional checks in this test, I think it would be better to change this to a if block, something like:
if (!ServerAuthenticator.wasChecked()) {
throw new RuntimeException("Authenticator wasn't invoked");
}There was a problem hiding this comment.
Fixed it! And yes, all tests passed using assert
| * questions. | ||
| */ | ||
|
|
||
| /** |
There was a problem hiding this comment.
| /** | |
| /* |
It was recently suggested that test comments are not API documentation comments, and that we should avoid /** in that case. Maybe we will do a global pass on the test base at some point (or not) but in the mean time let's avoid propagating this pattern in new tests.
There was a problem hiding this comment.
Maybe we can use a script to fix them all in one a large change, similar to #18268
jaikiran
left a comment
jaikiran
left a comment
There was a problem hiding this comment.
The changes look good to me and I don't have any more review comments. Thank you for the updates as well as fixing this issue.
|
Thank you. |
|
Only tests failing tier 1-3 are Math related tests, https://bugs.openjdk.org/browse/JDK-8332066. |
|
/integrate |
|
@nizarbenalla |
|
/sponsor |
|
Going to push as commit b87a7e9.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@jaikiran @nizarbenalla Pushed as commit b87a7e9. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
3 participants
Passes Tier 1-3
Please review this change that aims to fix a bug when parsing the client's request.
RFC 9110 states
But in
BasicAuthenticator#authenticateit was done in a case sensitive mannerTIA
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19133/head:pull/19133$ git checkout pull/19133Update a local copy of the PR:
$ git checkout pull/19133$ git pull https://git.openjdk.org/jdk.git pull/19133/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19133View PR using the GUI difftool:
$ git pr show -t 19133Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19133.diff
Webrev
Link to Webrev Comment