Added passing cookies from long polling session to web scoket

[OneSman7]

No description provided.

[nuclearace]

Overall looks fine. Do you think this should only be done when some config is set? I'm not sure if there's any chance some polling header would interfere with the WS side of things.

[nuclearace]

You can condense this line and the one above, right?

[OneSman7]

yes, made this too

[nuclearace]

nit: nix the space

[OneSman7]

done

[OneSman7]

Overall looks fine. Do you think this should only be done when some config is set?

I looked where addHeaders was called, it was only on making long poll request and creating web socket request. As for the first case, all cookies/headers work is done by URLSession, so I thought I will only make my changes where this work is not done because of Starscream.

[vonox7]

This commit introduces a major flaw: ALL cookies for ALL domains used in the app are sent to the socket-io-Server. So if the app uses a WebView, even those cookies set in the WebView will be sent to the socket.io server. @nuclearace @OneSman7
The problem is, that SocketEngine.resetEngine() uses the default URLSession configuration. The cookies are stored to disk AND are shared to with other libraries/usecases in the app. Shouldn't we use an own URLSessionConfiguration just for socket.io to allow the usecase from this pull request?
I see a very big security issue in this change.

[OneSman7]

My intention was to pass cookies to support load balancers, not to change the way long polling sessions are created.
According to docs (https://developer.apple.com/documentation/foundation/httpcookiestorage) we may have cookies obtained by other means by the app or from UIWebView (which is deprecated). WKWebView do not use shared storage. Thus I see this problem as minor for most apps.
It may be possible to create custom session configuration from default session configuration, but it will also refer to the shared cookie storage. The problem is with httpCookieStorage property: "For default and background sessions, the default value is the shared cookie storage object.". I cannot see a way to create custom HTTPCookieStorage objects :(
I'd say that the best way to mitigate your issue will be to filter cookies by domain to match request domain in addHeaders() method.
@nuclearace @vonox7 I can create PR with these changes later today or tomorrow. I am in process of updating XCode to the latest version and migrating my project with Socket.IO to the latest Swift version. After I complete theses tasks I will make changes, test it on our server to see if load balancers still work and then create PR.

[OneSman7]

@vonox7 also look here #1080. I guess it will also fix the issue

[2h4u]

@OneSman7 Multiple points:

1. I don't share your opinion on the minor issue thing because a lot of other common librarys are using the shared HTTPCookieStorage e.g. Alamofire (https://github.com/Alamofire/Alamofire).
   The shared HTTPCookieStorage can also be used by the Foundation framework if you doing some networking with it: (https://developer.apple.com/documentation/foundation/nsmutableurlrequest/1415485-httpshouldhandlecookies)
   So I don't think it's minor at all.

2. fix cookies import when create the websocket #1080 is just a partial fix because usually you don't want to use the cookies from the shared HTTPCookieStorage. You want to use the cookies which you passed the SocketManager via it's config (SocketManager().config = [ .cookies([HTTPCookie]) ]).
   It's very confusing if you are passing the SocketManager just the needed cookie but then its using a lot more cookies.

[OneSman7]

@2h4u

1. I wanted to say that there is no need for a critical fix. The issue may be fixed in the next update. I wanted to accentuate that it is difficult to create a session with not shared cookie storage since it will almost certainly require to implement custom cookie storage system.
2. The whole idea of this PR was to pass to web socket connection more cookies than were set in config. Cookies from config were already being passed. But load balancers installed on our servers used cookies to indicate to which machine our client should send packets after initial connection. Some upgrades to web sockets failed since request to upgrade was sent to the default machine which may not match machine it was connected to before. It happened because the request lacked cookie which would tell load balancer how to route request. These are the cookies that originated after polling connection was established, I could not assign them before connecting. fix cookies import when create the websocket #1080 will fix issue since it will only pass to socket.io server cookies that were created by this server. I see no problem in that.

[2h4u]

@OneSman7 okay I see, you are right. But #1080 isn't a full fix because it can happen that you interact with a server in multiple ways, for example: via websocket and REST-API. In this scenario it wouldn't be a surprise if you don't want to share the cookies between the REST-API calls and the websocket. But with the current implementation there is just no way to achieve this.

So in my opinion we should support 2 cases:

1. Keeping the cookies of websocket and REST-API separated.
2. Sharing the cookies between websocket and REST-API but only as opt-in option to prevent unindented behavior.

This could be achieved if we create a new field for SocketManager.config, for a HTTPCookieStorage, so the developer can set whatever he wants either a custom one or the "default"-shared one. If it's not set, the SocketEngine should create a custom one.
Something like this:

[Modified snippet from SocketEngine.swift]:
    // Puts the engine back in its default state
    private func resetEngine() {
        let queue = OperationQueue()
        queue.underlyingQueue = engineQueue

        closed = false
        connected = false
        fastUpgrade = false
        polling = true
        probing = false
        invalidated = false
        session = Foundation.URLSession(configuration: .ephemeral, delegate: sessionDelegate, delegateQueue: queue)
        session!.configuration.httpCookieStorage = cookieStorage ?? HTTPCookieStorage.init()
        sid = ""
        waitingForPoll = false
        waitingForPost = false
    }

[OneSman7]

@2h4u

Ok. I see you seek not to prevent passing cookies from polling to web socket connection, but to prevent polling from using shared cookie storage altogether. I see no problem in that since load balancers support needs only to pass cookies from the current polling session storage, it does not matter if it is shared or not.
You could create a PR with these changes and make them merged in the upcoming release.
One comment though. It may be simpler to add just a single flag to the config useSharedCookieStorage which defaults to false. Config already has custom cookies property to set custom cookies, so no need to pass whole custom storage.
And also please test that cookies that are added after polling connection is established pass on successfully to web socket connection request.
@nuclearace what do you think?

[2h4u]

@OneSman7 ok i testet this now but unfortunately I can't set a new HTTPCookieStorage because session!.configuration is not mutable and I didn't find a way to set it another way.

Another approach would be that we use the .ephemeral configuration instead of the .default configuration for the initialization of the Foundation.URLSession() to solve this problem.
But I can't test if the cookies which are set during a long poll are still available for the websockets since my applications doesn't use such a mechanic. Could you try this with your application? @OneSman7