[jdk11u-dev] RFR: 8245245: WebSocket can lose the URL encoding of URI query parameters

[Paul Hohensee]

On Wed, 30 Nov 2022 01:52:47 GMT, Michal Karm Babacek <duke at openjdk.org> wrote:

> Proposes to backport [JDK-8245245](https://bugs.openjdk.org/browse/JDK-8245245).
> 
> The backport is clean as far as the actual `OpeningHandshake.java` goes. The test needed a little tweak so as to compile with `SimpleSSLContext` and also to handle the fact that the erroneous response does not bring a response body.
> 
> The test passes with the patch, fails without it.
> 
> 
> $ make clean run-test TEST="jtreg:test/jdk/java/net/httpclient/websocket/HandshakeUrlEncodingTest.java"
> ...
> ==============================
> Test summary
> ==============================
>    TEST                                              TOTAL  PASS  FAIL ERROR   
>    jtreg:test/jdk/java/net/httpclient/websocket/HandshakeUrlEncodingTest.java
>                                                          1     1     0     0   
> ==============================
> TEST SUCCESS
> 
> Stopping sjavac server
> Finished building targets 'clean run-test' in configuration 'linux-x86_64-normal-server-release'
> 
> In addition to that, I compiled and executed the original `WebSocketTest.java` reproducer found on  [JDK-8245245](https://bugs.openjdk.org/browse/JDK-8245245) JIRA.
> 
> 
> ## Unpatched Temurin-11.0.17+8  ❌ 
> 
> $ java WebSocketTest 
> Http Request
> http://localhost:8000/?raw=abc+def/ghi=xyz&encoded=abc%2Bdef%2Fghi%3Dxyz
> Server RequestURI: /?raw=abc+def/ghi=xyz&encoded=abc%2Bdef%2Fghi%3Dxyz
> WebSocket Request
> ws://localhost:8000/?&raw=abc+def/ghi=xyz&encoded=abc%2Bdef%2Fghi%3Dxyz
> Server RequestURI: /?&raw=abc+def/ghi=xyz&encoded=abc+def/ghi=xyz
> 
> 
> ## Patched jdk11u ✔ 
> 
> $ java WebSocketTest 
> Http Request
> http://localhost:8000/?raw=abc+def/ghi=xyz&encoded=abc%2Bdef%2Fghi%3Dxyz
> Server RequestURI: /?raw=abc+def/ghi=xyz&encoded=abc%2Bdef%2Fghi%3Dxyz
> WebSocket Request
> ws://localhost:8000/?&raw=abc+def/ghi=xyz&encoded=abc%2Bdef%2Fghi%3Dxyz
> Server RequestURI: /?&raw=abc+def/ghi=xyz&encoded=abc%2Bdef%2Fghi%3Dxyz
> 
> The patched version correctly leaves the latter part of the query param encoded.

Looks good except for using getResponse().uri() and a few formatting issues in HandshakeUrlEncodingTest.java.

The change to use getResponse().uri() instead of getResponse().body() fixes a bug in tip, so can't be part of a backport. The thing to do is use the buggy code from the original commit in this backport, fix the problem in tip, then backport that fix to 19u, 17u, and 11u.

On formatting issues, future backports are easier if you match the original formatting as closely as possible. Looks like you're using and IDE that auto-formats.

At line 59:

import static java.lang.System.out;
import static java.net.http.HttpClient.Builder.NO_PROXY;
import static org.testng.Assert.*;

should be

import static java.net.http.HttpClient.Builder.NO_PROXY;
import static org.testng.Assert.*;
import static java.lang.System.out;

to more closely follow the original. In fact, if all the test uses is assertEquals, assertNotNull, and fail, just use the original commit code.

Indent/format the same as the original commit: lines 85-88, 89-92, 106-108, 111, 115, 133-135, 137, 141-146, 153-158, 174-176, 188-190, and missing blank line after 196. Might have missed somthing, but you get the idea.

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

Changes requested by phh (Reviewer).

PR: https://git.openjdk.org/jdk11u-dev/pull/1558