Skip to content

fix headers, fix Sec-WebSocket-Protocol , add ORIGIN #5

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix headers, fix Sec-WebSocket-Protocol , add ORIGIN #5

wants to merge 1 commit into from

Conversation

gaxxx
Copy link

@gaxxx gaxxx commented Jul 14, 2014

work with golang & nodejs websocket server now.

@agentzh
Copy link
Member

agentzh commented Jul 14, 2014

@gaxxx Thank you for the contribution!

But I think you just add an origin header rather than "fixing" the Sec-WebSocket-Protocol header. They're different headers after all.

Also, regarding your change itself, I think we should add an "origin" option to the existing "opts" argument table rather than changing the current API (we need to maintain backward compatibility).

In addition, I think the "origin" thing should be optional, because according to RFC 6455, the Origin header is not mandatory. To quote: "This header field is sent by browser clients; for non-browser clients, this header field may be sent if it makes sense in the context of those clients."

Finally, will you add some test cases to the existing test suite for this new feature?

Thanks again!

@gaxxx
Copy link
Author

gaxxx commented Jul 14, 2014

Hi:
之前的Sec-WebSocket-Protocol, 格式有问题,http请求是这样的
GET /socket HTTP/1.1
Upgrade: websocket
Host: 127.0.0.1:9527
Sec-WebSocket-Key: Rlp4rZPMcy6oNpgk1JUG1g== Sec-WebSocket-Protocol: json

Sec-WebSocket-Version: 13
Connection: Upgrade

应该是用得人少,所以没有发现

On 07/14/2014 01:09 PM, Yichun Zhang wrote:

@gaxxx https://github.com/gaxxx Thank you for the contribution!

But I think you just add an origin header rather than "fixing" the
Sec-WebSocket-Protocol header. They're different headers after all.

Also, regarding your change itself, I think we should add an "origin"
option to the existing "opts" argument table rather than changing the
current API (we need to maintain backward compatibility). Also, I
think the "origin" thing should be optional, because according to RFC
6455, the Origin header is not mandatory. To quote: "This header field
is sent by browser clients; for non-browser clients, this header field
may be sent if it makes sense in the context of those clients."

Finally, will you add some test cases for this new feature?

Thanks again!


Reply to this email directly or view it on GitHub
#5 (comment).

@agentzh
Copy link
Member

agentzh commented Jul 14, 2014

@gaxxx Okay, now I see the problem with Sec-WebSocket-Protocol. There were no test cases for this "protocols" feature. I've committed the fix with some tests to git master already.

agentzh added a commit that referenced this pull request Jul 14, 2014
…otocols are specified. thanks woo for the report in #5.
agentzh added a commit that referenced this pull request Jul 14, 2014
…n to specify the value of the Origin request header. thanks woo for the original patch in #5.
@agentzh
Copy link
Member

agentzh commented Jul 14, 2014

@gaxxx I've committed two separate patches for these 2 things to git master. Will you check it out? Thank you for the report!

BTW, this place is considered English only. If you really want to use Chinese, please use the openresty (Chinese) mailing list instead: https://groups.google.com/group/openresty Thank you for your cooperation!

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.

2 participants