websocket-sharp icon indicating copy to clipboard operation
websocket-sharp copied to clipboard

Added CustomHeaders to WebSocketClient

Open dallonf opened this issue 11 years ago • 64 comments

First off, thanks so much for this library! I was previously using WebSocket4Net which apparently doesn't handle large messages correctly.

This change adds a CustomHeaders property to WebSocketClient which will send additional HTTP headers on the handshake request. This isn't exactly per the WebSocket spec, but it was needed for a particular project and it could be useful for others as well.

dallonf avatar Oct 18 '13 15:10 dallonf

Hello, there.

Well, Your 'pull request' is similar to the issue #15. I think it's possible to do too many and it's not safe. So, I ask you, Which HTTP header did you add using that property?

sta avatar Oct 20 '13 15:10 sta

I added an "Authorization" header, which is proprietary to the app I'm working on.

Someone didn't tell our server engineer that the WebSockets spec doesn't support custom headers :P

dallonf avatar Oct 21 '13 14:10 dallonf

websocket-sharp supports the HTTP Basic/Digest auth as a client.

ws.SetCredentials ("name", "password", true); // 'true' if Basic auth

Is your "Authorization" header used for neither Basic or Digest auth?

sta avatar Oct 22 '13 01:10 sta

Nope, we have a proprietary format for the Authorization header. It consists of a keyword unique to the app, followed by a Base64 encoding of the user's credentials, followed by a Base64 encoding of the MD5 hash of a special string constructed from certain other HTTP headers... it's really a strange situation.

dallonf avatar Oct 24 '13 22:10 dallonf

I've understood your situation at your app. (I think it's more usual to use Cookie... Well, There is unique situation at your app?)

But, I don't plan to merge your 'pull request'. I think i should support for each necessary header. (e.g. WebSocket.Origin)

sta avatar Oct 25 '13 06:10 sta

"I think i should support for each necessary header." — this is wrong in so many ways. You can't support every possible header. It's against HTTP nature.

maqdev avatar May 28 '14 11:05 maqdev

@maqdev Huh? What do you mean?

Necessary means necessary for the WebSocket protocol defined in RFC 6455.

I've never said to support Every HTTP header.

sta avatar May 28 '14 11:05 sta

RFC 6455 defines that

... relationship to HTTP is that its handshake is interpreted by
HTTP servers as an Upgrade request

Also it says that

Additional header fields may also be
present, such as cookies [RFC6265].  The format and parsing of
headers is as defined in [RFC2616]

RFC 6455 doesn't restricts HTTP header possible values. And you can't strictly define what headers are necessary in a library designed for a public use.

Sure you can do it as an author of this great library. But I'm also sure that it doesn't make this library better.

maqdev avatar May 28 '14 13:05 maqdev

sta, what do you suggest for those who need custom headers? to not use your library?

naqoyqatsi avatar May 28 '14 14:05 naqoyqatsi

First of all, i don't plan to support user defined custom header.

what do you suggest for those who need custom headers? to not use your library?

I suggest to use SetCookie instead of custom header.

But I'm also sure that it doesn't make this library better.

I don't think so. The better, i mean, is to be more stable, and is not to be exaggerated specifications.

So, i don't think it's better to be able to add custom header or any header undefined in RFC 6455.

RFC 6455 doesn't restricts HTTP header possible values.

And also, RFC 6455 doesn't describe to use every HTTP header.

I (will) basically follow headers described in Section 1.3 of RFC 6455.

But i will not follow undescribed HTTP headers in RFC 6455.

sta avatar May 29 '14 02:05 sta

"I suggest to use SetCookie instead of custom header."

so... you claim that every possible WS server that requires custom headers from its clients is bad-designed - they all should use cookies... is it your thesis? )

naqoyqatsi avatar May 29 '14 06:05 naqoyqatsi

Hmm,,, It's an exaggerated story.

every possible WS server that requires custom headers from its clients is bad-designed

That's just user defined custom matter, and it's not related to the WebSocket protocol spec.

I think that to use cookie is better than to use custom header, because of closer to standard and easier.

But this is my idea, i don't claim that it's best.

sta avatar May 29 '14 07:05 sta

It's an interpretation problem. ) WS spec claims that "custom headers are ALLOWED", and it seems you interpret the word "ALLOWED" as "NOT NECESSARY" (not necessary to support custom headers), but imho it should be interpreted as "WS library implementation SHOULD ALLOW custom headers".

I think you'll agree that custom headers (in point of server-side developers' view) don't break any HTTP/WS rules and design guidelines - and no one can say that cookies are always better than custom headers.

And now suppose we have 3rd-party WS server (we cannot modify it) and it requires some custom headers from clients. In this normal(!) case your library becomes useless - is it OK for you?

naqoyqatsi avatar May 29 '14 09:05 naqoyqatsi

WS spec claims that "custom headers are ALLOWED"

Huh? Which section of RFC 6455 is it described in?

sta avatar May 29 '14 11:05 sta

In my scenario (the scenario that prompted me to make this pull request), I don't have any control over the server. The server expects a custom header and rejects my connection if it doesn't receive it. Cookies are not a workaround for this.

I agree that it's generally bad form to require custom headers on the server-side, especially because the WebSocket API on browsers don't support this. But please don't punish client-side developers when server developers don't follow best practices.

dallonf avatar May 29 '14 11:05 dallonf

I'm in the exact same case as @dallonf. The headers are often application dependent, and it's not a bad practice to use custom headers in HTTP... It should be possible to specify any custom header in the handshake request. @sta, I don't understand your position; by refusing this feature, you're making your library useless to many people...

thomaslevesque avatar Jul 25 '14 14:07 thomaslevesque

@thomaslevesque My position is very clear. I follow RFC 6455.

And RFC 6455 describes like the following in 4.1. Client Requirements:

The request MAY include any other header fields, for example, cookies RFC6265 and/or authentication-related header fields such as the |Authorization| header field RFC2616, which are processed according to documents that define them.

Does your custom header have public document that defines that?

sta avatar Jul 29 '14 06:07 sta

I think we disagree on how the specification should be understood...

The request MAY include any other header fields

I think that part is quite clear and unambiguous; what comes after is just examples of headers that can be included, not an exhaustive list ("for example", "such as"). The way I read it, it clearly means that you can include any header you like.

In any case, even if your interpretation of the specification were correct, for all practical purposes, support for custom headers is necessary; in many cases, the server will refuse the connection if they're not present, which makes this library unusable.

As for me, I know that I can't use it in its current state; perhaps I'll fork it at some point, but for now I'm using WebSocket4Net instead.

thomaslevesque avatar Jul 29 '14 08:07 thomaslevesque

Emphasis mine:

The request may include ANY other header fields

dallonf avatar Jul 29 '14 12:07 dallonf

I think:

The request may include any other header fields, ..., WHICH ARE PROCESSED ACCORDING TO DOCUMENTS THAT DEFINE THEM.

And @dallonf wrote:

..., especially because the WebSocket API on browsers don't support this.

That's right.

So, to add custom headers is weak for me, under the WebSocket spec.

sta avatar Jul 29 '14 13:07 sta

The request may include any other header fields, ..., WHICH ARE PROCESSED ACCORDING TO DOCUMENTS THAT DEFINE THEM.

Nothing says these documents have to be public RFCs. They can be internal design documents that describe the application's API.

And even if browsers don't support custom headers in the handshake request, the specification does say they're allowed, so it's not a valid argument against supporting them.

thomaslevesque avatar Jul 29 '14 14:07 thomaslevesque

Nothing says these documents have to be public RFCs. They can be internal design documents that describe the application's API.

How does many people see such INTERNAL design documents? Plz show me.

And even if browsers don't support custom headers in the handshake request, the specification does say they're allowed, so it's not a valid argument against supporting them.

The WebSocket spec allows the headers that have accessible documents (such as RFCs) that define them, not custom headers.

Which section of RFC 6455 is CUSTOM HEADER described in?

sta avatar Jul 29 '14 15:07 sta

RFC 6455 Section 1.2 (emphasis mine):

An unordered set of header fields comes after the leading line in both cases. The meaning of these header fields is specified in Section 4 of this document. Additional header fields may also be present, such as cookies [RFC6265]. The format and parsing of headers is as defined in [RFC2616].

And here's a link to RFC2616 for reference; it's just the HTTP spec for headers (HTTP allows for pretty much any header).

I'm curious why you are so opposed to this change.

It's not a difficult improvement, it took me maybe 15 minutes to submit this pull request the first time (it would probably have to be done again, since the library has changed significantly since then and this PR now doesn't merge cleanly).

It's not against the WebSocket spec to include arbitrary headers - at best, it's open for interpretation, which means it's a library's responsibility to enable as many use cases as possible.

And the lack of this feature has prevented more than one developer (that is, everybody in this thread except for you) from using this library without forking it.

I'm offering to make this library more useful for more people. All you have to do is click the Merge button. Is it that difficult?

dallonf avatar Jul 29 '14 15:07 dallonf

How does many people see such INTERNAL design documents? Plz show me.

What I mean is that not all web APIs are public; a company might have a specific API that uses custom headers, and these headers would typically be described in the technical specifications of the API. That's what I meant by "internal": internal to the company.

But actually, it could also be public documents that just happen not to be a RFC: for instance, if a website offers a public API that uses websockets, it might define custom headers that would be described in the API documentation.

For instance, my company develops an app that uses a custom X-DeviceId header; this header is clearly defined in our documentation, and is necessary for the server to correctly identify the device.

The WebSocket spec allows the headers that have accessible documents (such as RFCs) that define them, not custom headers.

Which section of RFC 6455 is CUSTOM HEADER described in?

They don't specifically mention "custom headers", but RFC 6455 very clearly says that "any other header fields" are allowed, which obviously includes custom headers.

thomaslevesque avatar Jul 29 '14 16:07 thomaslevesque

@dallonf

I'm curious why you are so opposed to this change.

It's because i hate user defined CHAOS custom headers.

All you have to do is click the Merge button.

I don't plan to do that for now.

@thomaslevesque

That's what I meant by "internal": internal to the company.

That's just user defined custom matter. Do you mean that i should support the feature that has the spec which i cannot see?

but RFC 6455 very clearly says that "any other header fields" are allowed, which obviously includes custom headers.

That's your interpretation, not mine.

You guys pay attention to MAY or ANY, but i do to DOCUMENTS THAT DEFINE THEM. I think that it's difference between library user and maintainer.

And i think you guys only forced me to support the feature which has ambiguous definition and can be a wide range of use.

Is there any possible those custom headers are utilized in an illegal attack?

And also MAY is not MUST, so i choose more safe rather than useful.

So, i don't plan to enable to add custom headers for now.

sta avatar Jul 30 '14 03:07 sta

It's hopeless, I give up... I, and probably many others, will just use another WebSocket library because this one can't solve actual real-world problems.

thomaslevesque avatar Jul 30 '14 08:07 thomaslevesque

-1, somebody just fork this. It's a good library, it's just a shame that the maintainer is so hopelessly idealistic. Removing my star and leaving this issue open.

dallonf avatar Jul 30 '14 16:07 dallonf

Welp, well this is disappointing.

markwalsh-liverpool avatar Oct 05 '15 11:10 markwalsh-liverpool

@dallonf Have you or anyone else forked it?

markwalsh-liverpool avatar Oct 05 '15 11:10 markwalsh-liverpool

I agree with dallong. Custom headers SHOULD be allowed, as it will be up to the client application to maintain. I too require a header to be added to the request, but there is no way around it for now. In fact, I was just about to make a very similar pull request to dallong!

louisferreira avatar Oct 05 '15 11:10 louisferreira