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

[Michal Karm Babacek]

On Thu, 1 Dec 2022 21:29:48 GMT, Paul Hohensee <phh 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.

Helo, @phohensee,

I changed the behavior of the test in the tip (HEAD), so as I can then backport it along the JDK-8245245 patch to 19u, 17u, and 11u. 

https://github.com/openjdk/jdk/pull/11486

@jerboaa If I am reading the [spec](https://www.rfc-editor.org/rfc/rfc6455.html#section-5.5.1) right, there is IMHO no need to change the behavior of the implementation.

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

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