What the developer wrote
Authored by Gijs van Dam
lnrpc: fix Sec-Websocket-Protocol parsing
forwardHeaders assumed that a Sec-Websocket-Protocol value matching one
of the allowed prefixes also contains the "+" delimiter, and indexed the
split result at [1] unconditionally. A client that sends only the
protocol name, for example "Grpc-Metadata-Macaroon" with no delimiter,
therefore caused an index out of range panic.
The panic is currently contained because forwardHeaders runs on the
net/http handler goroutine, whose deferred recover catches it, so only
the client's own connection dies. That containment is incidental to the
current call graph rather than by design though: neither the read loop
nor the backend forwarder goroutine has a recover of its own.
The prefix match was too loose in the other direction as well. The field
is a comma separated list of sub protocols, but it was matched and split
as a single string, so "Grpc-Metadata-Macaroon,other+value" passed the
prefix check and forwarded "value" from the second entry as the macaroon
for the first.
Split the field on "," first and cut each entry at its own delimiter,
forwarding only entries whose name adjoins that delimiter and is
allowed. Add a table test covering the forwarding rules including these
cases.
What changed, and why it matters
This commit fixes a bug in how LND's WebSocket proxy handled a special browser header called Sec-Websocket-Protocol. Previously, a client could send just the protocol name without a required separator, causing the server to crash with a panic. The crash was only contained by accident, not by design. The fix also prevents a trick where a client could make one protocol entry steal a value from another entry in the same list. The patch adds tests to confirm the correct behavior.
Apply the patch and ensure any custom WebSocket clients send protocol values in the exact "Name+value" form. Review whether other goroutines in the WebSocket proxy path need explicit panic recovery. No additional vendor advisory is supplied in the materials.
Security signals we found
Denial of service via unhandled index out-of-range panic in request header parsing
Authentication bypass / credential confusion risk from loose prefix matching on comma-separated protocol list
Fix hardens parsing by requiring exact allowed protocol names and a delimiter before forwarding values
Incident containment described as incidental rather than by design, indicating latent reliability risk
Test coverage added for parsing edge cases
Evidence from the diff
In lnrpc/websocket_proxy.go, forwardHeaders previously read Sec-Websocket-Protocol as a single string, matched allowed prefixes with strings.HasPrefix, then split on WebSocketProtocolDelimiter and indexed values[1] unconditionally. A value like “Grpc-Metadata-Macaroon” with no delimiter caused an index out-of-range panic. Because forwardHeaders runs on the net/http handler goroutine, the existing deferred recover caught it, killing only that connection. The prefix match was also too loose: a comma-separated list such as “Grpc-Metadata-Macaroon,other+value” passed the prefix check and forwarded “value” as the macaroon. The patch splits the field on commas, trims whitespace, uses strings.Cut to require both an allowed name and a delimiter, and only then forwards the value. A table-driven test is added covering empty headers, direct forwarding, dropped disallowed headers, valid protocol forwarding, bare protocol names, unknown protocols, and comma-list edge cases.
Changed components
lnrpc/websocket_proxy.goforwardHeaders functionWebSocket-to-HTTP header forwarding pathInspect captured patch +121 / −10
### lnrpc/websocket_proxy.go
@@ -374,17 +374,23 @@ func forwardHeaders(source, target http.Header) {
// requests. We need to allow them to submit the macaroon as a WS
// protocol, which is the only allowed header. Set any "protocols" we
// declare valid as header fields on the forwarded request.
- protocol := source.Get(HeaderWebSocketProtocol)
- for key := range defaultProtocolsToAllow {
- if strings.HasPrefix(protocol, key) {
- // The format is "<protocol name>+<value>". We know the
- // protocol string starts with the name so we only need
- // to set the value.
- values := strings.Split(
- protocol, WebSocketProtocolDelimiter,
- )
- target.Set(key, values[1])
+ //
+ // The field is a comma separated list of protocols, so we need to look
+ // at each entry on its own. Only an entry of the form
+ // "<protocol name>+<value>" carries something to forward. A client is
+ // free to send a bare protocol name without the delimiter and value,
+ // in which case there is nothing to set on the target.
+ protocols := strings.Split(source.Get(HeaderWebSocketProtocol), ",")
+ for _, protocol := range protocols {
+ name, value, hasValue := strings.Cut(
+ strings.TrimSpace(protocol),
+ WebSocketProtocolDelimiter,
+ )
+ if !hasValue || !defaultProtocolsToAllow[name] {
+ continue
}
+
+ target.Set(name, value)
}
}
### lnrpc/websocket_proxy_test.go
@@ -0,0 +1,105 @@
+package lnrpc
+
+import (
+ "net/http"
+ "testing"
+
+ "github.com/stretchr/testify/require"
+)
+
+// TestForwardHeaders makes sure the headers of an incoming WebSocket request
+// are forwarded to the upstream HTTP request correctly. That includes the
+// special Sec-Websocket-Protocol field, which browsers use to transport header
+// fields they aren't allowed to set on a WebSocket request directly.
+func TestForwardHeaders(t *testing.T) {
+ t.Parallel()
+
+ const macaroon = "0201036c6e6402eb01030a10"
+
+ testCases := []struct {
+ name string
+ source http.Header
+ expected http.Header
+ }{{
+ name: "no headers",
+ source: http.Header{},
+ expected: http.Header{},
+ }, {
+ name: "allowed header is forwarded",
+ source: http.Header{
+ "Grpc-Metadata-Macaroon": []string{macaroon},
+ },
+ expected: http.Header{
+ "Grpc-Metadata-Macaroon": []string{macaroon},
+ },
+ }, {
+ name: "disallowed header is dropped",
+ source: http.Header{
+ "Authorization": []string{"Bearer foo"},
+ },
+ expected: http.Header{},
+ }, {
+ name: "macaroon in protocol field is forwarded",
+ source: http.Header{
+ HeaderWebSocketProtocol: []string{
+ "Grpc-Metadata-Macaroon+" + macaroon,
+ },
+ },
+ expected: http.Header{
+ "Grpc-Metadata-Macaroon": []string{macaroon},
+ },
+ }, {
+ // A client is free to send the protocol name without the
+ // delimiter and value. There is nothing to forward in that
+ // case, and we must not attempt to read a value that isn't
+ // there.
+ name: "protocol field without delimiter is ignored",
+ source: http.Header{
+ HeaderWebSocketProtocol: []string{
+ "Grpc-Metadata-Macaroon",
+ },
+ },
+ expected: http.Header{},
+ }, {
+ name: "unknown protocol field is ignored",
+ source: http.Header{
+ HeaderWebSocketProtocol: []string{"some-protocol"},
+ },
+ expected: http.Header{},
+ }, {
+ // The protocol field is a comma separated list, so a bare
+ // allowed protocol name must not be able to borrow the value
+ // of a different sub protocol in the same list.
+ name: "bare allowed protocol followed by valued protocol",
+ source: http.Header{
+ HeaderWebSocketProtocol: []string{
+ "Grpc-Metadata-Macaroon,other+value",
+ },
+ },
+ expected: http.Header{},
+ }, {
+ // An allowed protocol is forwarded no matter where in the
+ // list it appears.
+ name: "allowed protocol in list is forwarded",
+ source: http.Header{
+ HeaderWebSocketProtocol: []string{
+ "other+value, Grpc-Metadata-Macaroon+" +
+ macaroon,
+ },
+ },
+ expected: http.Header{
+ "Grpc-Metadata-Macaroon": []string{macaroon},
+ },
+ }}
+
+ for _, tc := range testCases {
+ t.Run(tc.name, func(t *testing.T) {
+ t.Parallel()
+
+ target := http.Header{}
+ forwardHeaders(tc.source, target)
+
+ require.Equal(t, tc.expected, target)
+ })
+ }
+}Why this scored 67/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.