[FEATURE] Provide an override for `isValidChallengeKey` using a custom validator about websocket HOT 8 OPEN

For what it's worth, I'm willing to just PR these changes myself. I saw in another issues comments that there's some transition happening with the library right now. So as long as a change like this is something you are interested in supporting I can just PR it. I didn't initially make the PR as I was not sure if it was something you would even be interested in supporting

from websocket.

It's weird to select the override based on the Sec-WebSocket-Protocol header.

• The security key and subprotocol are unrelated concepts. What if somebody else comes along with the same problem, but they don't use subprotocols at all or in a different way?
• Consider that the Sec-WebSocket-Protocol header value can contain multiple subprotocols and an arbitrary amount of whitespace between the syntax elements. Is the lookup based on the exact text of the header, a whitespace normalized version of the header, the first subprotocol or something else?

Does the client check the Sec-WebSocket-Accept in the handshake response? If not, your application can change Sec-Websocket-Key to a valid value before calling Upgrade and everything should work.

from websocket.

The security key and subprotocol are unrelated concepts.

I don't agree that they are entirely unrelated. They are both used as part of the handshake process to ensure that the client and server can even establish a connection. In that way, I would argue they are "related enough" to be used in this way, even if not directly dependent on each other as per the spec. In this case the fact that the client uses a non-standard key is directly related to it's use of a custom subprotocol. This is not meant to be a proposal to fix a spec related issue, just a proposal to give new options to developers in a non-breaking way. Using the subprotocol feels like the simplest way to go about this

Spec has already been violated since the key is non-standard, so I don't think it's worth worrying about the finer details of the spec when proposing a fix for it

What if somebody else comes along with the same problem, but they don't use subprotocols at all or in a different way?

Then another fix for that persons specific use case would be in order, in my opinion? That sounds like it would warrant a new issue with new solution, as it feels unrelated to this one? In this case the non-standard key is directly related to the client using a custom subprotocol. If someone else has a situation where a client is using a non-standard key, but not subprotocols, that feels like a different issue entirely to me. This proposal is aimed specifically at allowing subprotocols to define their own checks for the key, nothing more and nothing less. It's not designed to propose a universal way to provide custom validators, just an extra option for cases like this in a way that doesn't change the API in a breaking way

Consider that the Sec-WebSocket-Protocol header value can contain multiple subprotocols and an arbitrary amount of whitespace between the syntax elements. Is the lookup based on the exact text of the header, a whitespace normalized version of the header, the first subprotocol or something else?

I didn't suggest that. The spec clearly defines how subprotocols should be handled https://datatracker.ietf.org/doc/html/rfc6455#section-11.3.4. A client may send multiple, but the server selects only one, with the order of the clients list being based on the subprotocols priority. Gorilla already has a mechanism for selecting this subprotocol

The proposal would just shift the call to u.selectSubprotocol up a few lines, as it's currently called immediately after the key validation itself, and use the selected subprotocol to pull it's validator, if any

Another rather simple solution would be to just allow the server to disable this validation entirely if the developer wishes. Though I'm not a massive fan of this. The official Go library doesn't seem to validate the header, and the IETF spec does not say the server must validate it either

The |Sec-WebSocket-Key| header field in the client's handshake includes a base64-encoded value that, if decoded, is 16 bytes in length. This (encoded) value is used in the creation of the server's handshake to indicate an acceptance of the connection. It is not necessary for the server to base64-decode the |Sec-WebSocket-Key| value

So providing an option to disable this check would technically be within spec (though still not great)

A third rather simple solution would be to just have a single custom validator on Upgrader, and if it's defined use it instead of the built in one. That would be a more "universal" fix, but I'm not a huge fan of the "all or nothing" nature of it. It's less flexible in my opinion, and would mean that if there is a case like mine where the subprotocol changes the key structure, and there are multiple subprotocols, then a single custom validator would not be enough

I understand you may be looking for a single, general, solution to this problem but I don't believe there is, and so either a combination of solutions or fixes on a case-by-case basis would be in order. Especially when dealing with non-standard clients. I think the closest we can get to a "general" solution is to allow for both subprotocols to define their own validators, and to provide a single custom validator, and when selecting a validator going in order of:

• Nothing if disabled
• Subprotocol
• The single custom validator
• Built in

This would allow situations like mine to work while still providing a more general solution to non-standard keys in cases where the subprotocols do no matter. Though I'd hesitant to call this a truly general solution, as it assumes these are the only 2 cases where a client would have a non-standard key (hence why I did not try to provide a general solution in the first place, only a solution to this specific problem)

from websocket.

They are both used as part of the handshake process to ensure that the client and server can even establish a connection

Sec-Websocket-Key and Sec-WebSocket-Accept are used to establish the connection. Sec-WebSocket-Protocol establishes the messages used on the connection. The features are unrelated.

Then another fix for that persons specific use case would be in order, in my opinion?

The maintainers should not add features for very specific uses cases as you are requesting in this issue. The cost of understanding the package goes up with each new feature added to the API. Even if a feature is not something that a developer should use, the developer needs to understand the feature enough to know that the feature is not relevant.

I understand you may be looking for a single, general, solution to this problem

I am not in favor of changing anything in the package, but if a change is made, the change should be more general. Here's a proposal:

     type Upgrader struct {
          ⋮
          ComputeAcceptKey func(r *http.Request) (string, error)
          ⋮
     }

where the default validates the key and computes the accept key. Another benefit of this more general solution is that it's easier to understand than the proposal for your use case.

This more general solution requires you to write a few more lines of code than required with your proposal, but the cost to you should be balanced against the cost of other developers understanding and using the API. Forking may be a better balance of the costs.

from websocket.

Sec-Websocket-Key and Sec-WebSocket-Accept are used to establish the connection. Sec-WebSocket-Protocol establishes the messages used on the connection. The features are unrelated

My understanding of the spec is that if no agreed on subprotocol is selected, then the connection would be terminated. Relevant MDN docs

The client may close the connection if it doesn't get the subprotocol it wants

So, yes, there does seem to be some form of correlation here. I understand that the subprotocol determines the messages, but it's not unreasonable to count this as part of the connection process. If the client and server do not agree on the messages, how are they even expected to communicate

I acknowledged in my original reply that they are not directly dependent on each other as per spec, but that they are "related enough" to be talked about in the same context here

But again, spec has already been violated here. So I don't see the benefit in worrying about the finer details of spec when proposing solutions to out of spec issues. That seems pointless to me?

The maintainers should not add features for very specific uses cases as you are requesting in this issue. The cost of understanding the package goes up with each new feature added to the API. Even if a feature is not something that a developer should use, the developer needs to understand the feature enough to know that the feature is not relevant

I'm struggling to understand why you seem to make it seem like this would drastically raise the complexity of the package and it's API? This is a very simple issue, with a very simple to understand proposal. I do generally agree that library maintainers should not provide fixes for very specific one-off issues, but having a WebSocket client which uses a non-standard key does not seem like it would be a one-off issue. There could be many such cases

Like in this case. For context, the client I am talking about is the Nintendo Switch. Nintendo's standard multiplayer library implements it's clients through WebSockets, and they use these non-standard keys. Every Nintendo Switch game/client which uses that library, of which there are many, would use these non-standard keys, and be incompatible with gorilla. So this is not an isolated issue with just one specific client

Another benefit of this more general solution is that it's easier to understand than the proposal for your use case.

I fail to see how it is, objectively, easier to understand? The proposal I am making is simple, and can be documented very simply. It seems like you're implying this is a complex, hard to understand, feature, when it can be described very easily. Such as

type Upgrader struct {
	// SubprotocolKeyValidators defines an optional mapping of subprotocols
	// to Sec-Websocket-Key validators. Should not be used unless a non-standard
	// client, which changes how the security key is formatted, is used. If
	// no validator is set, the default validator is used
	SubprotocolKeyValidators map[string]func(string) bool
}

I am not in favor of changing anything in the package, but if a change is made, the change should be more general. Here's a proposal

This proposal looks reasonable to me 👍 doing extra handling on our end to pull out the subprotocol is not an issue, that wasn't ever really the point. Just a way to override the default key validation, if that means offloading more of that to the developer that's fine by me

Though the name of the function in your proposal seems more related to the Sec-WebSocket-Accept header? The issue still remains of passing the key validation itself. The accept header is not an issue, and is calculated like normal. The server just needs to allow the key to pass to that point

Having a function like this, though, which takes in the request and is expected to validate the key is a fine, general, solution. It puts more work on the developer but that was never an issue

Something like this maybe?

type Upgrader struct {
	// ValidateSecurityKey defines an optional custom validator for validating
	// the Sec-Websocket-Key header. Should only be used in cases where a client
	// uses a non-standard key
	ValidateSecurityKey func(r *http.Request) bool
}

I'm more in favor of a change like this than my original proposal, to be honest. It's more flexible in how to handle these cases

Forking may be a better balance of the costs.

I don't agree that forking is a better balance of costs. This single header check is the only thing preventing the client from connecting. Forking and maintaining our own copy of this package places far more long term cost on me and my team, as we now have to ensure our fork remains up to date with upstream when the only change needed is a single check. That feels like a far worse balance when the issue presented does have multiple possible solutions and are all very easy to understand/implement. There is next to no cost on the maintainers side, and other developers only have to read a single extra optional field which clearly defines when it should be used. At that point, we might as well just roll our own server and not use gorilla at all

from websocket.

I am not implying that your proposal drastically raise the complexity. I am trying to say two things.

• Every API feature has a cost and cost that should be balanced with the benefits. In this case, we are balancing work on your team with the cost of every developer reading that comment and deciding if it applies to their application.
• A general feature can eliminate the need for multiple more specific features.

My proposal is easier to understand because my proposal does not require the developer to understand subprotocols and a relationship between subprotocols and keys (which is unexpected).

I have a few observations based on the PR to add validation in 2022.

• Key validation may not be widespread given that Autobahn does not test for key validation. The Switch developers would not have used an invalid key if they were working with a sever that validates the key.
• There's no indication in the PR that key validation was added to address a security issue.

After reading the PR comments, I think a better fix is:

type Upgrader struct {
	// DisableSecurityKeyValidation determines whether Upgrade validates the format 
        // of the Sec-Websocket-Key request header.
	DisableSecurityKeyValidation bool
}

When this flag is set to true, the server becomes more compatible with what I suspect other servers do. More research here may be warranted.

Because the words "disable" and "security" are used together, I'd think that many developers will quickly decide that they don't need to set the flag. Disabling features related to security is scary.

from websocket.

I am not implying that your proposal drastically raise the complexity

By bringing up the cost of the feature to other developers, you are directly implying that the feature is complex to understand. If the feature would not raise the complexity of the API, then the "cost" would be negligible at worst

Every API feature has a cost and cost that should be balanced with the benefits. In this case, we are balancing work on your team with the cost of every developer reading that comment and deciding if it applies to their application.

Is the cost of reading a 3 line comment, which very clearly outlines when it should be used, really something you're going to argue is more costly than maintaining an entire fork? That feels a bit absurd to me

The "cost" here is reading about 3 lines of extra docs for a single new feature. The benefits being a new feature which brings a lot of flexibility to those who need to use it. A fork would only serve to put all cost, including the (much higher) cost of maintaining an active fork, on me and my team while providing no benefits to other developers who may also find this feature useful. In terms of balances, that feels much worse

A general feature can eliminate the need for multiple more specific features.

I agree, which is why I added one to the comment you are replying to and have already updated my original post and title of the issue

My proposal is easier to understand because my proposal does not require the developer to understand subprotocols and a relationship between subprotocols and keys (which is unexpected).

Nor does mine? The proposed comments clearly indicate that it should be used in cases where a client which does not follow spec is being used. In cases where someone must handle a client which is out of spec, it's reasonable to assume they are familiar with spec. Even still, my proposed doc comments clearly outline the relationship between the headers, removing the need for prior knowledge?

I have a few observations based on the PR to add validation in 2022.

• Key validation may not be widespread given that Autobahn does not test for key validation. The Switch developers would not have used an invalid key if they were working with a sever that validates the key.
• There's no indication in the PR that key validation was added to address a security issue.

I'm not sure why any of this is relevant? I did not bring up security as being an issue. That's an unrelated topic in the context of this feature request. As for the key validation, no the official Nintendo-made first party servers do not seem to validate it. But that's not a guarantee, and more than just Nintendo use Nintendo's standard library

Regardless, the Nintendo Switch was just one example. It should not be used as the prime example. There may be cases where this validation of an out of spec key is expected, and it should be accounted for

When this flag is set to true, the server becomes more compatible with what I suspect other servers do. More research here may be warranted.

Because the words "disable" and "security" are used together, I'd think that many developers will quickly decide that they don't need to set the flag. Disabling features related to security is scary.

I mentioned this already as a possible solution in my original reply to you. Also, this header is not related to security in the traditional way that most people would assume. The only thing the header does is try to make sure the client is a real WebSocket client, and then it's just used as part of the hash for the accept header

This helps ensure that the server does not accept connections from non-WebSocket clients (e.g., HTTP clients) that are being abused to send data to unsuspecting WebSocket servers

So calling it such, and relying on just "scaring developers" seems like a poor option

In my updated proposal, which I already described to you in my previous comment, this same kind of "disabling" can be accomplished through just defining a custom validator that always immediately returns true. That is the more appropriate solution in my opinion, as it gives the most amount of freedom on how to handle these cases rather than just "do the default or turn it all off"

from websocket.

I'm going to leave this feature request open for now, as I feel like it's still a useful feature others may want/need in the future. But we have decided to move on from gorilla to gws as it already works with our clients out of the box. It does not validate the header at all, which I still think is a potential issue, but it at least means my team can continue development without maintaining a fork

from websocket.