Skip to content
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

add option to pass Origin header in ws_connect #607

Merged
merged 3 commits into from
Oct 29, 2015

Conversation

rutsky
Copy link
Member

@rutsky rutsky commented Oct 29, 2015

This PR allows passing of Origin header to ws_connect().

I believe WebSockets specification requires passing of Origin header (https://tools.ietf.org/html/rfc6455#section-4.2.2).

Some servers requires proper Origin headers (according to specification), so without this patch it's not possible to connect to such servers using ws_connect().

@asvetlov
Copy link
Member

Should we support origin parameter for regular request as well?

@rutsky
Copy link
Member Author

rutsky commented Oct 29, 2015

Should we support origin parameter for regular request as well?

I think no, in regular request() Origin header can be added manually using headers keyword argument.

I don't know any use cases of Origin header, but in CORS and Web Sockets.
CORS is not needed when building requests manually (client can present himself as same-origin client).

Origin has special meaning in WebSocket protocol (and currently ws_connect doesn't have any way to change headers) so I think it deserves separate keyword argument.

Maybe we should also add headers parameter to ws_connect()?

@rutsky
Copy link
Member Author

rutsky commented Oct 29, 2015

According to RFC WS server may require authorization, so adding other headers (Authorization:) would be useful.

asvetlov added a commit that referenced this pull request Oct 29, 2015
add option to pass Origin header in ws_connect
@asvetlov asvetlov merged commit 944e0f6 into aio-libs:master Oct 29, 2015
@asvetlov
Copy link
Member

I've merged the PR.
headers parameter worth to be added also in separate PR.
Not sure about authorization: it looks like edge case and may be solved by headers.

@rutsky
Copy link
Member Author

rutsky commented Oct 29, 2015

Not sure about authorization: it looks like edge case and may be solved by headers.

I used Authorization as an example of additional headers that someone may require in ws_connect(), I don't think it should be separate argument --- it definitely should go in headers.

@rutsky
Copy link
Member Author

rutsky commented Oct 29, 2015

I have opened bug report for adding headers: #608. I think it deserve "easy" tag.

@lock
Copy link

lock bot commented Oct 29, 2019

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.

@lock lock bot added the outdated label Oct 29, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 29, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants