mORMot Open Source

keinn

Member

Registered: 2014-10-20

Posts: 100

websocket bug report

unit mormot.net.ws.core
in TWebSocketProtocolList.ServerUpgrade proc

   // identify the Websockets protocol
  prot := Http.HeaderGetValue('SEC-WEBSOCKET-PROTOCOL');
  P := pointer(prot);
  if P <> nil then
  begin
    repeat
      GetNextItemTrimed(P, ',', subprot);
      Protocol := CloneByName(subprot, uri);
    until (P = nil) or
          (Protocol <> nil);
    if (Protocol <> nil) and
       (Protocol.Uri = '') and
       not Protocol.ProcessHandshakeUri(prot) then
    begin
      Protocol.Free;
      result := HTTP_NOTFOUND;
      exit;
    end;
  end

line 2376 Protocol := CloneByName(subprot, uri); should be "Protocol := CloneByName(subprot, prot);"

and the final 101 upgrade response , if we use CloneByUri(uri) then 
Response must NOT include 'Sec-WebSocket-Protocol' header if not present in request ,otherwise js client will fail(if it doesnt use the protocol header).

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,204

Website

Re: websocket bug report

So what do you propose?
Your message is a bit difficult to follow.

We used this code with a JS client with success, so I am a bit afraid of breaking things.
Please provide more information, to state for the exact use case of the problem you face.

keinn

Member

Registered: 2014-10-20

Posts: 100

Re: websocket bug report

in short
we should remove the 'Sec-WebSocket-Protocol:' section when return the 101 header if client dos not specify the SEC-WEBSOCKET-PROTOCOL in request header

Scenario 1：client specify the protocol in reqeust header

 
prot := Http.HeaderGetValue('SEC-WEBSOCKET-PROTOCOL');
  P := pointer(prot);
  if P <> nil then

these three line identify the protocol name from  request Header, 
then the following code should be:

Protocol := CloneByName(subprot, prot); 

NOT

Protocol := CloneByName(subprot, uri); 

i think it just a type error.
-------------------------------

Scenario 2：client do NOT specify the protocol in reqeust header,
then our code detect the protocol from url, 
at the end our code return  101 header by this

  FormatUtf8('HTTP/1.1 101 Switching Protocols'#13#10 +
             'Upgrade: websocket'#13#10 +
             'Connection: Upgrade'#13#10 +
             'Sec-WebSocket-Connection-ID: %'#13#10 +
             'Sec-WebSocket-Protocol: %'#13#10 +
             '%Sec-WebSocket-Accept: %'#13#10#13#10,
    [ConnectionID, Protocol.Name, extout,
     BinToBase64Short(@Digest, SizeOf(Digest))], Response)

this all work fine if client make the request by put SEC-WEBSOCKET-PROTOCOL in header,
but if js code like this

 ws = new WebSocket('ws://127.0.0.1/chat');

the brower will report error :
"Response must not include ‘Sec-WebSocket-Protocol’ header if not present in request;"
u can see it in chrome console.

there are many Scenarios we can not ask the client to specify the SEC-WEBSOCKET-PROTOCOL in header, like work with 3rd party.

Last edited by keinn (2023-02-21 16:18:54)

keinn

Member

Registered: 2014-10-20

Posts: 100

Re: websocket bug report

another problem, not sure is a bug :
unit mormot.net.async line 3323 ,when used as websocket server :

function THttpAsyncConnection.DoRequest: TPollAsyncSocketOnReadWrite;
var
  output: PRawByteStringBuffer;
  remoteID: THttpServerConnectionID;
  sent: integer;
  p: PByte;
  flags: THttpServerRequestFlags;
begin
  // check the status
  if nfHeadersParsed in fHttp.HeaderFlags then
    fServer.IncStat(grBodyReceived)
  else
    begin
      // content-length was 0, so hrsGetBody* and DoHeaders() were not called
      result := DoHeaders;
      if (result <> soContinue) or
         (fHttp.State = hrsUpgraded) then
        exit; // rejected or upgraded  <<<<<!!!
     <<<<--- if our server is behind a reverse  proxy . even we use the (fhttpServer).RemoteIPHeader :='X-Forwarded-For' 
       because we exit here ,following  fServer.ParseRemoteIPConnID(fHttp.Headers, fRemoteIP, remoteid); never get a chance to exec
    ---->>
    end;
  // optionaly uncompress content
  if fHttp.CompressContentEncoding >= 0 then
    fHttp.UncompressData;
  // prepare the HTTP/REST process reusing the THttpServerRequest instance
  result := soClose;
  remoteid := fHandle;
  fServer.ParseRemoteIPConnID(fHttp.Headers, fRemoteIP, remoteid);

     <<<<--- if our server is behind a reverse  proxy . even we use the (fhttpServer).RemoteIPHeader :='X-Forwarded-For'
       because we exit here ,following  fServer.ParseRemoteIPConnID(fHttp.Headers, fRemoteIP, remoteid); never get a chance to exec
    ---->>
when we need to get the Real RemoteIP from header ,
those code works fine when it handle regular http rest request ,but not work in websocket mode.

Last edited by keinn (2023-02-21 16:14:48)

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,204

Website

Re: websocket bug report

1) CloneByName(subprot, uri) seems correct to me.
subprot was extracted as CSV from prot, and the uri should be the URI to filter the sub-protocol per URI.

2) Should be fixed by
https://github.com/synopse/mORMot2/commit/2b3516e0

3) About the remote ip issue over a proxy, it is indeed a bug.
Please try https://github.com/synopse/mORMot2/commit/08a99075

keinn

Member

Registered: 2014-10-20

Posts: 100

Re: websocket bug report

last commit Parse Real ip header in proxy still buggy,
fHttp.Headers is empty when ParseRemoteIPConnID called,
fix should do in THttpAsyncConnection.DoHeaders function ,not in THttpAsyncConnection.DoRequest,
this works in both rest and websocket , direct connection or proxy connection

function THttpAsyncConnection.DoHeaders: TPollAsyncSocketOnReadWrite;
var
  status: integer;
  //!!!! fix sample
  procedure DoParseRealIP(var RemoteIP:RawUtf8);
  begin
    if fServer.fRemoteIPHeaderUpper <> '' then
      FindNameValue(fHttp.Headers, pointer(fServer.fRemoteIPHeaderUpper),
        RemoteIP, {keepnotfound=}true);
  end;
begin
  // finalize the headers
  result := soClose;
  if (nfHeadersParsed in fHttp.HeaderFlags) or
     not fHttp.ParseCommand then
    exit;
  fHttp.ParseHeaderFinalize;
  //!!!!! parse proxy x-forward-forx ..realip
  //!!!!! just after fHttp.Headers retrieved before fServer.OnBeforeBody get called by following DecodeHeaders
  //!!!!! we need to hnow everything in business layer callback like OnBeforeBody 、OnWebSocketUpgraded、OnWebSocketConnect、 
  //!!!!! OnWebSocketDisconnect etc... so we wont break things
  DoParseRealIP(fRemoteIP);
  // immediate reject of clearly invalid requests
  status := DecodeHeaders; // may handle hfConnectionUpgrade when overriden
  if status <> HTTP_SUCCESS then
  begin
    // on fatal error (e.g. OnBeforeBody) direct reject and close the connection
    DoReject(status);
    exit;
  end;

and the fServer.fRemoteIPHeader should set to 'X-Forwarded-For:' , because FindNameValue does not process ':'

my fix is just a sample which ignore RemoteConnID parse

Last edited by keinn (2023-02-22 07:11:07)

ab

Administrator

From: France

Registered: 2010-06-21

Posts: 14,204

Website

Re: websocket bug report

Please try https://github.com/synopse/mORMot2/commit/0eb1a70d