feat: Fix `maybeWebSocketUpgrade` to return true when Connection/Upgrade header has multiple… #5958

blue-hope · 2024-10-24T12:00:48Z

Motivation:

I requested websocket request for my armeria server with those headers:

Connection: keep-alive, websocket
Upgrade: websocket

It returns:

The upgrade header must contain:
  Upgrade: websocket
  Connection: Upgrade

I trouble-shooted and found:
Fix that WebSocketService rejects headers that have multiple values #5230
So I upgraded my armeria server's dependency from 1.23.1 to 1.26.0
But it still occurs even though I upgraded deps properly.
So I debugged, and found that there are one more conditional statement in toArmeria in ArmeriaHttpUtil.java

private static boolean maybeWebSocketUpgrade(AsciiString header, CharSequence value) {
    if (HttpHeaderNames.CONNECTION.contentEqualsIgnoreCase(header) &&
        HttpHeaderValues.UPGRADE.contentEqualsIgnoreCase(value)) {
        return true;
    }
    return HttpHeaderNames.UPGRADE.contentEqualsIgnoreCase(header) &&
           HttpHeaderValues.WEBSOCKET.contentEqualsIgnoreCase(value);
}

=> I wrote this PR because It is confusing that the multivalue header accepted by WebSocketUtil is not accepted by the websocket conditional statement of ArmeriaHttpUtil. If the two utils perform different roles in different scopes, and the strictness of websocket checks should be different, this PR can be closed. (But the question still remains as to whether multi-value headers should be supported.)

Modifications:

Like above PR's WebSocketUtil.java, change the conditional statement from equals to contains

Result:

Closes My websocket request returns 400 with header Connection: keep-alive, websocket #5957.
Can upgrade websocket request with multiple connection value

…value

CLAassistant · 2024-10-24T12:01:01Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

blue-hope · 2024-10-24T12:03:11Z

As for the test, I tried writing it, but it seems to be written only for abstract headers like {"a": "b"}, so I was not sure where to start, so I stopped. However, if you give me a guide, I think I can write it.

minwoox

Hi! Thanks for the PR. 😉
Would you mind adding a unit test, please?

blue-hope · 2024-10-24T12:36:42Z

@minwoox I wrote one, please check it 😄

github-actions · 2024-10-24T12:40:43Z

🔍 Build Scan® (commit: `b544f41`)

Job name	Status	Build Scan®

minwoox · 2024-10-24T13:21:10Z

I wrote one, please check it

It seems like the test case doesn't call maybeWebSocketUpgrade internally which means we can't verify if the fix is correct or not. Could you check it, please?

blue-hope · 2024-10-24T13:30:21Z

Oops, I'll check it

blue-hope · 2024-10-24T14:13:45Z

@minwoox Sorry for the confusion. I wrote toArmeria test again and checked that it calls maybeWebSocketUpgrade by debugger.

blue-hope · 2024-10-24T14:43:04Z

Github checks occur some errors but I don't know how I can deal with them. (I think it may be irrelevant to my PR, I ran ./gradlew :core:lint in local to lint my modified code and it finished successfully).

minwoox · 2024-10-28T01:10:48Z

Github checks occur some errors but I don't know how I can deal with them.

Let me see what happened. In the meantime, would you add e2e tests to see if the fix works on HTTP/1.1 and HTTP/2?
You can probably add an additional header to these test cases:
https://github.com/line/armeria/blob/main/core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java

blue-hope · 2024-10-29T07:25:01Z

@minwoox Can you please check my last commit? I added e2e test.
By the way, I found that similar test exists on WebSocketServiceCorsTest, which includes CONNECTION / UPGRADE header.
Do we need to add additional header on there, too?

minwoox · 2024-10-29T07:45:35Z

Do we need to add additional header on there, too?

I don't think so. 😉

minwoox · 2024-10-29T07:45:05Z

core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java

-                              .add(HttpHeaderNames.PROTOCOL, HttpHeaderValues.WEBSOCKET.toString());
+                              .add(HttpHeaderNames.PROTOCOL, HttpHeaderValues.WEBSOCKET.toString())
+                              // It works even if the header contains multiple values
+                              .add(HttpHeaderNames.CONNECTION, HttpHeaderValues.KEEP_ALIVE.toString())
+                              .add(HttpHeaderNames.UPGRADE, "additional_value");


Let's revert this change. I realized that HTTP/2 uses the different method so we probably don't need to check for HTTP/2.

minwoox

Thanks, @blue-hope! 👍 👍 👍

jrhee17 · 2024-11-05T01:36:55Z

core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java

@@ -102,7 +102,9 @@ void http1Handshake(SessionProtocol sessionProtocol, boolean useThreadRescheduli
                                          .build();
        final RequestHeadersBuilder headersBuilder =
                RequestHeaders.builder(HttpMethod.GET, "/chat")
-                              .add(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE.toString());
+                              // It works even if the header contains multiple values


Question) When I checked, it seems like this test passes with the main branch without the changes in this PR.

I'm curious, were you able to verify the same bug with the HEAD of the main branch? It seems a little odd since ArmeriaHttpUtil#maybeWebSocketUpgrade is only called for trailers in the server-side which doesn't seem to make sense to me.

Thanks, @jrhee17. In toArmeria released in version 1.28.0, I found that the purgeHttp1OnlyHeaders process was dropping useless additional headers, and that is what I intended to. I'll do it again on my server and figure out what was the issue.

ikhoon · 2024-11-05T05:36:41Z

core/src/main/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtil.java

            return true;
        }
        return HttpHeaderNames.UPGRADE.contentEqualsIgnoreCase(header) &&
-               HttpHeaderValues.WEBSOCKET.contentEqualsIgnoreCase(value);
+               AsciiString.containsIgnoreCase(value, HttpHeaderValues.WEBSOCKET);


I don't think either websocketxxx or xxxwebsocket is a valid header value. How about splitting the value with a comma , before checking the equality?

Can we also add a test case that makes sure it doesn't pick up headers like connection: not-upgrade-lol and upgrade: madam-websocket?

jrhee17 · 2024-11-07T03:56:13Z

gentle ping @blue-hope

feat: Fix to return true when Connection/Upgrade header has multiple …

eaa91e1

…value

blue-hope requested review from ikhoon, jrhee17, minwoox and trustin as code owners October 24, 2024 12:00

minwoox reviewed Oct 24, 2024

View reviewed changes

test: Add test for websocket upgrade

3b21593

test: Add test for upgrade multivalue header

0e3b66b

test: Add test toArmeriaForWebSocketUpgrade

82fe92a

minwoox added the defect label Oct 28, 2024

minwoox added this to the 1.31.0 milestone Oct 28, 2024

test: Test on websocket handshake e2e

02b2074

minwoox reviewed Oct 29, 2024

View reviewed changes

blue-hope added 2 commits October 29, 2024 17:22

test: Revert test for HTTP/2

46e8ad7

test: Revert test for HTTP/2

b544f41

minwoox approved these changes Oct 29, 2024

View reviewed changes

jrhee17 reviewed Nov 5, 2024

View reviewed changes

ikhoon reviewed Nov 5, 2024

View reviewed changes

jrhee17 modified the milestones: 1.31.0, 1.32.0 Nov 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Fix `maybeWebSocketUpgrade` to return true when Connection/Upgrade header has multiple… #5958

feat: Fix `maybeWebSocketUpgrade` to return true when Connection/Upgrade header has multiple… #5958

blue-hope commented Oct 24, 2024 •

edited

Loading

CLAassistant commented Oct 24, 2024

blue-hope commented Oct 24, 2024

minwoox left a comment

blue-hope commented Oct 24, 2024

github-actions bot commented Oct 24, 2024 •

edited

Loading

minwoox commented Oct 24, 2024

blue-hope commented Oct 24, 2024

blue-hope commented Oct 24, 2024 •

edited

Loading

blue-hope commented Oct 24, 2024 •

edited

Loading

minwoox commented Oct 28, 2024

blue-hope commented Oct 29, 2024

minwoox commented Oct 29, 2024

minwoox Oct 29, 2024

minwoox left a comment

jrhee17 Nov 5, 2024

blue-hope Nov 7, 2024 •

edited

Loading

ikhoon Nov 5, 2024

trustin Nov 11, 2024

jrhee17 commented Nov 7, 2024

feat: Fix maybeWebSocketUpgrade to return true when Connection/Upgrade header has multiple… #5958

Are you sure you want to change the base?

feat: Fix maybeWebSocketUpgrade to return true when Connection/Upgrade header has multiple… #5958

Conversation

blue-hope commented Oct 24, 2024 • edited Loading

Motivation:

Modifications:

CLAassistant commented Oct 24, 2024

blue-hope commented Oct 24, 2024

minwoox left a comment

Choose a reason for hiding this comment

blue-hope commented Oct 24, 2024

github-actions bot commented Oct 24, 2024 • edited Loading

🔍 Build Scan® (commit: b544f41)

minwoox commented Oct 24, 2024

blue-hope commented Oct 24, 2024

blue-hope commented Oct 24, 2024 • edited Loading

blue-hope commented Oct 24, 2024 • edited Loading

minwoox commented Oct 28, 2024

blue-hope commented Oct 29, 2024

minwoox commented Oct 29, 2024

minwoox Oct 29, 2024

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

jrhee17 Nov 5, 2024

Choose a reason for hiding this comment

blue-hope Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

ikhoon Nov 5, 2024

Choose a reason for hiding this comment

trustin Nov 11, 2024

Choose a reason for hiding this comment

jrhee17 commented Nov 7, 2024

feat: Fix `maybeWebSocketUpgrade` to return true when Connection/Upgrade header has multiple… #5958

feat: Fix `maybeWebSocketUpgrade` to return true when Connection/Upgrade header has multiple… #5958

blue-hope commented Oct 24, 2024 •

edited

Loading

github-actions bot commented Oct 24, 2024 •

edited

Loading

🔍 Build Scan® (commit: `b544f41`)

blue-hope commented Oct 24, 2024 •

edited

Loading

blue-hope commented Oct 24, 2024 •

edited

Loading

blue-hope Nov 7, 2024 •

edited

Loading