Skip to content

add option to pass Origin header in ws_connect#607

Merged
asvetlov merged 3 commits intoaio-libs:masterfrom
rutsky:ws_origin_header
Oct 29, 2015
Merged

add option to pass Origin header in ws_connect#607
asvetlov merged 3 commits intoaio-libs:masterfrom
rutsky:ws_origin_header

Conversation

@rutsky
Copy link
Copy Markdown
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
Copy Markdown
Member

asvetlov commented Oct 29, 2015

Should we support origin parameter for regular request as well?

@rutsky
Copy link
Copy Markdown
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
Copy Markdown
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
@asvetlov
Merge pull request #607 from rutsky/ws_origin_header
944e0f6 
add option to pass Origin header in ws_connect
@asvetlov asvetlov merged commit 944e0f6 into aio-libs:master Oct 29, 2015
@asvetlov
Copy link
Copy Markdown
Member

asvetlov commented Oct 29, 2015

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
Copy Markdown
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 rutsky mentioned this pull request Oct 29, 2015
Closed
@rutsky
Copy link
Copy Markdown
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
Copy Markdown

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.

Reviewers

No reviews

Assignees

No one assigned

Labels

outdated

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@rutsky @asvetlov