Connection Header Behavior is breaking websocket upgrades in Firefox + socket.io

[ifandelse]

Hi @tjanczuk! We've started using iisnode at LeanKit and are pretty excited about how it's bridging our .NET and node work. I think we've discovered a problem, however. We're using socket.io for websocket connectivity and noticed that once our node module was being loaded up via iisnode, our Firefox clients started received 400 Bad Request when attempting to upgrade their connection from xhr polling to websockets. I'm not a C++ guy by any means, but I believe this line might be the issue:

https://github.com/tjanczuk/iisnode/blob/bd95f4210c398bff8f8a3c68d031d03cff052512/src/iisnode/cprotocolbridge.cpp#L726

Firefox insists on sending keep-alive in the connection header, so a Firefox client's upgrade attempt will have a connection header set to keep-alive, Upgrade. Would it be possible to change the stricmp to a contains-style comparison, or forward the Connection header value on if already set? (I assume the former option is better than the latter, since I'm guessing there's other behavior which requires keep-alive to be set for iisnode to work?)

[chiefego]

We have the same problem. Works great on all browsers except FF. Getting 400 on connect.

[tobiasWenger]

Same problem but with nodejs-websocket instead of socket.io

[ferk6a]

Any update on this issue? I'm facing the same problem with sockjs.

[ferk6a]

I decided to try and bring a temporary fix and came up with this:

diff --git a/src/iisnode/cprotocolbridge.cpp b/src/iisnode/cprotocolbridge.cpp
index 726758f..49d4806 100644
--- a/src/iisnode/cprotocolbridge.cpp
+++ b/src/iisnode/cprotocolbridge.cpp
@@ -722,6 +722,11 @@ void CProtocolBridge::SendHttpRequestHeaders(CNodeHttpStoredContext* context)
     request = context->GetHttpContext()->GetRequest();

     pszConnectionHeader = request->GetHeader(HttpHeaderConnection);
+
+   if (pszConnectionHeader != NULL && strchr(pszConnectionHeader, ',') != NULL) {
+       pszConnectionHeader = strchr(pszConnectionHeader, ',') + 2;
+   }
+
     if( pszConnectionHeader == NULL || 
         (pszConnectionHeader != NULL && stricmp(pszConnectionHeader, "upgrade") != 0))
     {

It's hacky; and I assume that some code is also needed for the tests. A better check would probably split the header in "," sections and compare each for the upgrade header. But I can't say it doesn't work:

[NoelAbrahams]

We've run into the same problem (we use websocket ws). I initially thought this was a problem with node, so reported it there (nodejs/node/issues/7435). However, the problem seems to be with IISNode because sending direct requests to node works, but sending requests via IIS fails.

@tomfin46 thanks for attempting a fix. @tjanczuk, @rramachand21 this is an urgent production issue for us.

[NoelAbrahams]

@tjanczuk, @rramachand21 do you intend to make a release anytime soon?

[devsoft]

For those want a hot fix, you can temporary add a new rule for Url Rewrite 2.0 to overcome the issue:

1. Add HTTP_CONNECTION in list of "Allowed Server Variables"
2. In web.config, add this rule:

<rule name="Change Connection Header">
          <match url="(.*)socket\.io/" />
          <conditions>
              <add input="{HTTP_CONNECTION}" pattern="keep-alive, Upgrade" />
          </conditions>
          <serverVariables>
              <set name="HTTP_CONNECTION" value="Upgrade" />
          </serverVariables>
          <action type="Rewrite" url="index.js"/>
        </rule>

[jjmmontemayor]

Hello, I tried the hot fix provided by @devsoft, however it doesn't work on my side. Do we have an update for this issue? Thank you

[teslalight]

Hi, I tried the hot fix by @devsoft and definitely worked! Thanks so much I was lost on this for some time... I needed some tweaking for my application and thought to leave some notes for others. I totally missed Step 1, the first time I looked so I am adding some more details on where to find it......

Step 1 is in IIS under the Website > URL Rewrite > View Server Variable > Add HTTP_CONNECTION

Step 2 I modified my existing Rule using URLRewrite instead of adding a new one. By removing the Condition I was able to see this working in both FF and Chrome.

            <rule name="xxxxxWebsocket">
                  <match url="ws/*" />      
                  <serverVariables>
                      <set name="HTTP_CONNECTION" value="Upgrade" />
                  </serverVariables>                    
                  <action type="Rewrite" url="websocket-server.js" />
            </rule> 

[aventrax]

The workaround is working but shouldn't this issue be closed after 4 years?

[imtodor]

This one took me a few days to find... Thanks a lot for the workaround! Works with:

• socket.io, WSS, Firefox 87+

[LoaderB0T]

I am with imtodor here... Took me quite a while to figure this one out.

Using Socket.IO ,WSS, Firefox 91 (and 92 dev) I could also reproduce this with postman's new SocketIO integration by manually overwriting the connection header.

I am very thankful for the provided workaround, I adjusted it a bit myself, to better match my needs:

<rewrite>
  <rules>
    <rule name="WebSocket remove keep-alive header" stopProcessing="true">
      <match url="(.*)socket\.io/" />
      <serverVariables>
        <set name="HTTP_CONNECTION" value="Upgrade" />
      </serverVariables>
      <action type="Rewrite" url="main.js" />
      <conditions logicalGrouping="MatchAny">
        <add input="{HTTP_CONNECTION}" pattern="keep-alive, Upgrade" />
        <add input="{HTTP_CONNECTION}" pattern="Upgrade, keep-alive" />
      </conditions>
    </rule>

    <rule name="NestJs" patternSyntax="Wildcard">
      <match url="*" />
      <action type="Rewrite" url="main.js" />
    </rule>
  </rules>
</rewrite>

However, it would still be great if iisnode would handle the Firefox headers gracefully.