Skip to content

fix(websocket): address review findings on the ws/wss proxy path - #13977

Merged
bzp2010 merged 2 commits into
apache:masterfrom
bzp2010:bzp/fix-websocket-review-findings
Sep 22, 2026
Merged

bzp2010 merged 2 commits into
apache:masterfrom
bzp2010:bzp/fix-websocket-review-findings

Conversation

@bzp2010

@bzp2010 bzp2010 commented Sep 21, 2026 •

Copy link
Copy Markdown
Contributor

Description

Review of #13939 turned up a set of problems on the ws/wss proxy path. Each one was checked against this tree first, and the tests added here fail without the fix.

Fixes

  • Scheme dispatch. websocket_content_phase built its endpoint and made its TLS decision from the route's matched_upstream.scheme, while handle_upstream dispatches on ctx.upstream_scheme. An inline ws/wss upstream picked by traffic-split never set the latter, so it fell through to the plain HTTP proxy path. The content phase now uses ctx.upstream_scheme, and traffic-split sets it for ws/wss like it already does for https.
  • Retry after a non-101 answer. resty.websocket.client closes 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.
  • Host and SNI. The handshake now sends the Host proxy_pass would send (a host set by proxy-rewrite, and the retried node's host with pass_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.
  • Subprotocol. The server side of the proxy echoed the client's whole Sec-WebSocket-Protocol offer back. The client is now answered with the subprotocol the upstream selected, or none.
  • Log hygiene. The connect failure log no longer includes the request URI, whose query string may carry credentials.
  • request_type. It is set to websocket after the handshake, since this path has no header filter phase to do it.
  • tls.ca_certs. It was silently ignored for ws/wss, because the cosocket client can only trust the global lua_ssl_trusted_certificate. It is now rejected by the Admin API, and the docs say so.
  • Client handshake. It is completed separately from the upstream one, so a bad client handshake is a 400 and is no longer counted as an upstream failure.

Not fixed here (needs api7-lua-resty-websocket)

  • ws://[::1]:port nodes are rejected: the host pattern in client:connect does not accept brackets.
  • client:connect does not reset fatal, which is what the fresh proxy per attempt works around.
  • The server side echoes the client's subprotocol offer, which is what the response header rewrite works around.
  • tls.ca_certs needs 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, a proxy-rewrite host, retry after a non-101 answer, and ca_certs rejection. 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_close and request_type are asserted through the error log.

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible

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.
@bzp2010 bzp2010 self-assigned this Sep 22, 2026
@bzp2010
bzp2010 merged commit 271e799 into apache:master Sep 22, 2026
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants