From eaa91e18b756f9d0584b19cc5bd755ec3bb5652f Mon Sep 17 00:00:00 2001 From: blue-hope Date: Thu, 24 Oct 2024 20:42:38 +0900 Subject: [PATCH 1/7] feat: Fix to return true when Connection/Upgrade header has multiple value --- .../com/linecorp/armeria/internal/common/ArmeriaHttpUtil.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtil.java b/core/src/main/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtil.java index e768ba4a9fe..2a61ecf239a 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtil.java +++ b/core/src/main/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtil.java @@ -885,11 +885,11 @@ private static boolean isContentAlwaysEmpty(HttpMethod method) { private static boolean maybeWebSocketUpgrade(AsciiString header, CharSequence value) { if (HttpHeaderNames.CONNECTION.contentEqualsIgnoreCase(header) && - HttpHeaderValues.UPGRADE.contentEqualsIgnoreCase(value)) { + AsciiString.containsIgnoreCase(value, HttpHeaderValues.UPGRADE)) { return true; } return HttpHeaderNames.UPGRADE.contentEqualsIgnoreCase(header) && - HttpHeaderValues.WEBSOCKET.contentEqualsIgnoreCase(value); + AsciiString.containsIgnoreCase(value, HttpHeaderValues.WEBSOCKET); } private static CaseInsensitiveMap toLowercaseMap(Iterator valuesIter, From 3b21593ce63d0ddb524c6e4f274eb334638f6ff1 Mon Sep 17 00:00:00 2001 From: blue-hope Date: Thu, 24 Oct 2024 21:36:18 +0900 Subject: [PATCH 2/7] test: Add test for websocket upgrade --- .../internal/common/ArmeriaHttpUtilTest.java | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/core/src/test/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java b/core/src/test/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java index d478c8256ae..b664e37b60c 100644 --- a/core/src/test/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java @@ -502,6 +502,26 @@ void toArmeriaRequestHeaders() { assertThat(headers.authority()).isEqualTo("foo:36462"); } + @Test + void toArmeriaRequestHeadersForWebSocketUpgrade() { + final Http2Headers in = new ArmeriaHttp2Headers().set("a", "b"); + + final ChannelHandlerContext ctx = mockChannelHandlerContext(); + + in.set(HttpHeaderNames.METHOD, "GET") + .set(HttpHeaderNames.PATH, "/") + // It works even if the header contains multiple values + .set(HttpHeaderNames.CONNECTION, "keep-alive, upgrade") + .set(HttpHeaderNames.UPGRADE, "websocket"); + // Request headers without pseudo headers. + final RequestTarget reqTarget = RequestTarget.forServer(in.path().toString()); + final RequestHeaders headers = + ArmeriaHttpUtil.toArmeriaRequestHeaders(ctx, in, false, "https", + serverConfig(), reqTarget); + assertThat(headers.get(HttpHeaderNames.CONNECTION)).isEqualTo("keep-alive, upgrade"); + assertThat(headers.get(HttpHeaderNames.UPGRADE)).isEqualTo("websocket"); + } + @Test void isAbsoluteUri() { final String good = "none+http://a.com"; From 0e3b66bb29a098581742fc089d27319eb7f1331b Mon Sep 17 00:00:00 2001 From: blue-hope Date: Thu, 24 Oct 2024 21:38:09 +0900 Subject: [PATCH 3/7] test: Add test for upgrade multivalue header --- .../linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java b/core/src/test/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java index b664e37b60c..4372531526e 100644 --- a/core/src/test/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java @@ -512,14 +512,14 @@ void toArmeriaRequestHeadersForWebSocketUpgrade() { .set(HttpHeaderNames.PATH, "/") // It works even if the header contains multiple values .set(HttpHeaderNames.CONNECTION, "keep-alive, upgrade") - .set(HttpHeaderNames.UPGRADE, "websocket"); + .set(HttpHeaderNames.UPGRADE, "websocket, additional_value"); // Request headers without pseudo headers. final RequestTarget reqTarget = RequestTarget.forServer(in.path().toString()); final RequestHeaders headers = ArmeriaHttpUtil.toArmeriaRequestHeaders(ctx, in, false, "https", serverConfig(), reqTarget); assertThat(headers.get(HttpHeaderNames.CONNECTION)).isEqualTo("keep-alive, upgrade"); - assertThat(headers.get(HttpHeaderNames.UPGRADE)).isEqualTo("websocket"); + assertThat(headers.get(HttpHeaderNames.UPGRADE)).isEqualTo("websocket, additional_value"); } @Test From 82fe92a16afd0d1bbf5310eb63b92370da5b8561 Mon Sep 17 00:00:00 2001 From: blue-hope Date: Thu, 24 Oct 2024 23:07:58 +0900 Subject: [PATCH 4/7] test: Add test toArmeriaForWebSocketUpgrade --- .../internal/common/ArmeriaHttpUtilTest.java | 26 +++++++------------ 1 file changed, 10 insertions(+), 16 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java b/core/src/test/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java index 4372531526e..dd4f7d281a4 100644 --- a/core/src/test/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/common/ArmeriaHttpUtilTest.java @@ -503,23 +503,17 @@ void toArmeriaRequestHeaders() { } @Test - void toArmeriaRequestHeadersForWebSocketUpgrade() { - final Http2Headers in = new ArmeriaHttp2Headers().set("a", "b"); - - final ChannelHandlerContext ctx = mockChannelHandlerContext(); + void toArmeriaForWebSocketUpgrade() { + final io.netty.handler.codec.http.HttpHeaders in = new DefaultHttpHeaders() + .add(HttpHeaderNames.CONNECTION, HttpHeaderValues.KEEP_ALIVE + ", " + HttpHeaderValues.UPGRADE) + .add(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET + ", additional_value"); + final HttpHeadersBuilder out = HttpHeaders.builder(); + out.sizeHint(in.size()); + toArmeria(in, out); + final HttpHeaders outHeaders = out.build(); - in.set(HttpHeaderNames.METHOD, "GET") - .set(HttpHeaderNames.PATH, "/") - // It works even if the header contains multiple values - .set(HttpHeaderNames.CONNECTION, "keep-alive, upgrade") - .set(HttpHeaderNames.UPGRADE, "websocket, additional_value"); - // Request headers without pseudo headers. - final RequestTarget reqTarget = RequestTarget.forServer(in.path().toString()); - final RequestHeaders headers = - ArmeriaHttpUtil.toArmeriaRequestHeaders(ctx, in, false, "https", - serverConfig(), reqTarget); - assertThat(headers.get(HttpHeaderNames.CONNECTION)).isEqualTo("keep-alive, upgrade"); - assertThat(headers.get(HttpHeaderNames.UPGRADE)).isEqualTo("websocket, additional_value"); + assertThat(outHeaders.get(HttpHeaderNames.CONNECTION)).isEqualTo("keep-alive, upgrade"); + assertThat(outHeaders.get(HttpHeaderNames.UPGRADE)).isEqualTo("websocket, additional_value"); } @Test From 02b20747da72c60b98af0f68d2697f05fdfd24bf Mon Sep 17 00:00:00 2001 From: blue-hope Date: Tue, 29 Oct 2024 16:22:49 +0900 Subject: [PATCH 5/7] test: Test on websocket handshake e2e --- .../websocket/WebSocketServiceHandshakeTest.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java b/core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java index 77e920cf394..2315ef98391 100644 --- a/core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java +++ b/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 + .add(HttpHeaderNames.CONNECTION, HttpHeaderValues.UPGRADE + + ", " + HttpHeaderValues.KEEP_ALIVE); final Channel channel; try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { final AggregatedHttpResponse res = client.execute(headersBuilder.build()).aggregate().join(); @@ -114,7 +116,7 @@ void http1Handshake(SessionProtocol sessionProtocol, boolean useThreadRescheduli "Connection: Upgrade"); } - headersBuilder.add(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET.toString()); + headersBuilder.add(HttpHeaderNames.UPGRADE, HttpHeaderValues.WEBSOCKET + ", additional_value"); try (ClientRequestContextCaptor captor = Clients.newContextCaptor()) { final AggregatedHttpResponse res = client.execute(headersBuilder.build()).aggregate().join(); @@ -192,7 +194,10 @@ void http2Handshake(SessionProtocol sessionProtocol) { final RequestHeadersBuilder headersBuilder = RequestHeaders.builder(HttpMethod.CONNECT, "/chat") - .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"); SplitHttpResponse split = client.execute(headersBuilder.build()).split(); ResponseHeaders responseHeaders = split.headers().join(); split.body().abort(); From 46e8ad75102ca231c7ce285b1dc5ebb2c3329aa7 Mon Sep 17 00:00:00 2001 From: blue-hope Date: Tue, 29 Oct 2024 17:22:27 +0900 Subject: [PATCH 6/7] test: Revert test for HTTP/2 --- .../server/websocket/WebSocketServiceHandshakeTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java b/core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java index 2315ef98391..08eb2a4870f 100644 --- a/core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java @@ -195,9 +195,6 @@ void http2Handshake(SessionProtocol sessionProtocol) { final RequestHeadersBuilder headersBuilder = RequestHeaders.builder(HttpMethod.CONNECT, "/chat") .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"); SplitHttpResponse split = client.execute(headersBuilder.build()).split(); ResponseHeaders responseHeaders = split.headers().join(); split.body().abort(); From b544f41c19d303ce965c9ca89365d1c4f23417c5 Mon Sep 17 00:00:00 2001 From: blue-hope Date: Tue, 29 Oct 2024 17:22:37 +0900 Subject: [PATCH 7/7] test: Revert test for HTTP/2 --- .../armeria/server/websocket/WebSocketServiceHandshakeTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java b/core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java index 08eb2a4870f..60dbeb89fea 100644 --- a/core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java +++ b/core/src/test/java/com/linecorp/armeria/server/websocket/WebSocketServiceHandshakeTest.java @@ -194,7 +194,7 @@ void http2Handshake(SessionProtocol sessionProtocol) { final RequestHeadersBuilder headersBuilder = RequestHeaders.builder(HttpMethod.CONNECT, "/chat") - .add(HttpHeaderNames.PROTOCOL, HttpHeaderValues.WEBSOCKET.toString()) + .add(HttpHeaderNames.PROTOCOL, HttpHeaderValues.WEBSOCKET.toString()); SplitHttpResponse split = client.execute(headersBuilder.build()).split(); ResponseHeaders responseHeaders = split.headers().join(); split.body().abort();