Aias00 commented on code in PR #7219:
URL: https://github.com/apache/shenyu/pull/7219#discussion_r4110671479
##########
shenyu-plugin/shenyu-plugin-global/src/main/java/org/apache/shenyu/plugin/global/DefaultShenyuContextBuilder.java:
##########
@@ -70,7 +70,8 @@ private Pair<String, MetaData> buildData(final
ServerWebExchange exchange) {
return Pair.of(rpcType, new MetaData());
}
String upgrade = headers.getFirst(UPGRADE);
- if (StringUtils.isNotEmpty(upgrade) &&
RpcTypeEnum.WEB_SOCKET.getName().equals(upgrade)) {
+ // RFC 6455: the Upgrade header value is case-insensitive
+ if (StringUtils.isNotEmpty(upgrade) &&
RpcTypeEnum.WEB_SOCKET.getName().equalsIgnoreCase(upgrade)) {
Review Comment:
Non-blocking, but please coordinate with #7195 (it edits these same two
files).
`build()` resolves the decorator with no null check:
```java
return
decoratorMap.get(buildData.getLeft()).decorator(buildDefaultContext(exchange.getRequest()),
buildData.getRight()); // line 62
```
`decoratorMap` only holds the `ShenyuContextDecorator` beans actually on the
classpath (GlobalPluginConfiguration.java:71-74), so the `websocket` key exists
only when `shenyu-spring-boot-starter-plugin-websocket` is present.
Consequence of this line specifically: it widens the set of requests that
resolve to `rpcType = websocket`. In a deployment without the websocket
starter, `Upgrade: WebSocket` previously fell through to the `MetaDataCache`
lookup and was proxied as HTTP; now it NPEs here. That hazard already exists on
master for the lowercase spelling, so this is a widening rather than a new
defect - but it is precisely what #7195 fixes, and that PR modifies this file
and this test class too. Please land the null check (or merge #7195 first) so
the widened input set cannot turn into a 500.
##########
shenyu-plugin/shenyu-plugin-global/src/test/java/org/apache/shenyu/plugin/global/DefaultShenyuContextBuilderTest.java:
##########
@@ -57,4 +60,29 @@ public void testBuild() {
assertNotNull(shenyuContext);
assertEquals(RpcTypeEnum.HTTP.getName(), shenyuContext.getRpcType());
}
+
+ @Test
+ public void testBuildWithWebSocketUpgradeHeaderValueCaseInsensitive() {
+ // RFC 6455 requires the Upgrade header value to be compared
case-insensitively
+ for (String upgradeValue : Arrays.asList("websocket", "WebSocket",
"WEBSOCKET", "Websocket")) {
+ MockServerWebExchange exchange =
MockServerWebExchange.from(MockServerHttpRequest.get("http://localhost:8080/websocket";)
+ .remoteAddress(new InetSocketAddress(8092))
+ .header("Upgrade", upgradeValue)
+ .header("Connection", "Upgrade")
+ .build());
+ ShenyuContext shenyuContext =
defaultShenyuContextBuilder.build(exchange);
+ assertEquals(RpcTypeEnum.WEB_SOCKET.getName(),
shenyuContext.getRpcType(),
+ "Upgrade header value '" + upgradeValue + "' must be
detected as websocket rpc type");
+ }
+ }
+
+ @Test
+ public void testBuildWithNonWebSocketUpgradeHeaderValue() {
Review Comment:
Non-blocking: this one passes on master too, so it is a guard on the
negative path rather than a second regression test - fine to keep as-is, I only
note it so the test count is not read as two regression tests.
One related case worth knowing about (not asking you to handle it): because
the check above reads `headers.getFirst(UPGRADE)`, a comma-separated value such
as `Upgrade: websocket, h2c` still resolves to HTTP. That is currently
consistent rather than broken - Spring's `HandshakeWebSocketService` validates
with `"WebSocket".equalsIgnoreCase(headers.getUpgrade())`
(HandshakeWebSocketService.java:204) and `HttpHeaders#getUpgrade()` is also
`getFirst(UPGRADE)` (HttpHeaders.java:1393-1395), so both layers reject the
multi-token form identically. It would only become our problem if either side
started parsing the list.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
