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 support for additional transport headers #521

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

FrEaKmAn
Copy link

@FrEaKmAn FrEaKmAn commented Nov 8, 2020

With this pull request we can add additional options to underlying transports. This is useful if we want for example add additional headers to either websocket or pooling requests.

const sockjs = new SockJs(url, null, {
    transportOptions: {
        websocket: {
            headers: {
              Cookie: 'my cookie value'
            }
        }
    }
});

@FrEaKmAn
Copy link
Author

FrEaKmAn commented Nov 9, 2020

@brycekahle Strange, it fails on the CI but all tests pass locally. Is there additional setting to run tests locally?

Could you please check the pull request and let me know if I need to do anything else. Thank you.

@brycekahle
Copy link
Contributor

Can you give a use case of where you need this? Not all transports support headers (WebSockets for example, don't support custom headers).

Adding support for Cookies is also not something we are looking to do. Please read https://github.com/sockjs/sockjs-node#authorisation

@avistramer
Copy link

I have an alternative implementation that is a bit more robust and adds custom header support for all transports that support headers (i.e. not websockets) + a way to read the headers from the /info call response.

My use case is that we use the SockJS client in a Node.js process for streaming. It's an agent application that needs streaming functionality and can run behind restrictive corporate proxies that block WebSockets when connecting out. Hence SockJS is a nice solution with the client being in Node.js.

The server side runs behind an AWS load balancer which only supports the AWSALB cookie for session stickiness. With sockjs-client in the browser this works great as is, but in Node we need to simulate the process of getting cookies from the set-cookie header in the response to the /info request and add them to all future requests in all transports except Websockets.

I was going to open a PR for this but noticed this PR already exists and figured I would explain our use case first here to see if this made sense as a valid use case to the SockJS team. Thanks!

@FrEaKmAn
Copy link
Author

Yes, I apologize for late reply. We also use SockJS client on nodejs level while communicating with API. API requires additional headers for websocket.

@brycekahle
Copy link
Contributor

@FrEaKmAn Your initial example would not work with this code change. My worry about a code change like this, is that it would be a Node.js only feature, since browser-based WebSockets do not support setting custom headers. That may only serve to confuse users.

@avistramer
Copy link

I ended up opening a separate PR #534 with my version of this that covers all the fallback transports and an info event.

I think this really comes down to a philosophical question of whether you want to truly support a Node.js client in a real load-balanced production setup. Given the way so many leading LBs like AWS are doing session stickiness today with cookies only, it's hard to see any way around some kind of change along these lines.

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.

3 participants