fix(websocket): address review findings on the ws/wss proxy path - #13977
Merged
bzp2010 merged 2 commits intoSep 22, 2026
Merged
Conversation
Fixes found while reviewing the port of apache#13939 to another codebase, each verified against this tree first and covered by a test that fails without the change: - honor the scheme handle_upstream() dispatched on instead of the route's matched upstream, and have traffic-split set it for inline ws/wss upstreams, which used to fall through to the plain HTTP proxy path - create a fresh proxy for every connection attempt: a client that got a non-101 answer is fatal and closed, so a retry after e.g. a 503 could never carry a session - send the Host proxy_pass would send (a host set by proxy-rewrite, and the retried node's host with pass_host = node) and use it, port stripped, as the SNI / certificate name - answer the client with the subprotocol the upstream selected, or none, instead of echoing the client's whole offer - keep the request URI, whose query string may carry credentials, out of the connect failure log, and stop the example plugin dumping its config - set request_type to websocket, which no header filter does on this path - reject tls.ca_certs on ws/wss upstreams, which the cosocket client cannot apply, instead of silently ignoring it - complete the client handshake separately from the upstream one, so a bad client handshake is a 400 and no longer counted against the upstream Tests: wss, subprotocol, traffic-split, proxy-rewrite host, retry after a non-101 answer, ws_handshake / ws_close via the error log, and a passive health check that the active check can no longer mask.
…ample-plugin example-plugin's messages stay as they are; a serverless-post-function in the log phase reports request_type instead.
shreemaan-abhishek
approved these changes
Sep 21, 2026
nic-6443
approved these changes
Sep 21, 2026
AlinsRan
approved these changes
Sep 22, 2026
shreemaan-abhishek
pushed a commit
that referenced
this pull request
Sep 22, 2026
shreemaan-abhishek
added a commit
that referenced
this pull request
Sep 22, 2026
shreemaan-abhishek
added a commit
that referenced
this pull request
Sep 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Review of #13939 turned up a set of problems on the
ws/wssproxy path. Each one was checked against this tree first, and the tests added here fail without the fix.Fixes
websocket_content_phasebuilt its endpoint and made its TLS decision from the route'smatched_upstream.scheme, whilehandle_upstreamdispatches onctx.upstream_scheme. An inlinews/wssupstream picked bytraffic-splitnever set the latter, so it fell through to the plain HTTP proxy path. The content phase now usesctx.upstream_scheme, andtraffic-splitsets it forws/wsslike it already does forhttps.resty.websocket.clientcloses its socket and marks itself fatal on a non-101 response and never resets that, so a retry after e.g. a 503 could not carry a session. Every connection attempt now gets a fresh proxy.proxy_passwould send (a host set byproxy-rewrite, and the retried node's host withpass_host: node), and uses it with the port stripped as the SNI and certificate name. Previously a port in the upstream host made certificate verification fail.Sec-WebSocket-Protocoloffer back. The client is now answered with the subprotocol the upstream selected, or none.request_type. It is set towebsocketafter the handshake, since this path has no header filter phase to do it.tls.ca_certs. It was silently ignored forws/wss, because the cosocket client can only trust the globallua_ssl_trusted_certificate. It is now rejected by the Admin API, and the docs say so.Not fixed here (needs
api7-lua-resty-websocket)ws://[::1]:portnodes are rejected: the host pattern inclient:connectdoes not accept brackets.client:connectdoes not resetfatal, which is what the fresh proxy per attempt works around.tls.ca_certsneeds a per-connection trust store on the cosocket, which the client cannot offer.Tests
Added cases for
wss(verification off, verification with a port in the upstream host, a mismatching certificate), subprotocol negotiation,traffic-split, aproxy-rewritehost, retry after a non-101 answer, andca_certsrejection. The passive health check test now uses a node that answers the handshake with a 503, which the active TCP check cannot flag on its own.ws_handshake/ws_closeandrequest_typeare asserted through the error log.Checklist