1589068 - Formatting for SignalR JSON messages in WebSocket Inspector

Status: RESOLVED FIXED

(DevTools :: Netmonitor, enhancement, P2)

Description

[David Walsh :davidwalsh]

As requested here:

• https://twitter.com/davidfowl/status/1184364405766545408
• https://twitter.com/DamianEdwards/status/1184364583319982080

Comment 1

[:Harald Kirschner :digitarald]

More format docs:

• http://blogs.microsoft.co.il/applisec/2014/03/12/signalr-message-format/
• https://blog.3d-logic.com/2015/03/29/signalr-on-the-wire-an-informal-description-of-the-signalr-protocol/

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Also requested here:
https://hacks.mozilla.org/2019/10/firefoxs-new-websocket-inspector/#comment-25328

Some instructions for anyone interested in fixing this:

1. Here is the place where we parse the WS payload to figure out what's the underlying protocol
   https://searchfox.org/mozilla-central/rev/97976753a21c1731e18177de9e5ce78ea3b3da2d/devtools/client/netmonitor/src/components/websockets/FramePayload.js#93-122

2. This bug needs to introduce a new parser (we might use an existing parser if the license allows that) and use it within the parsePayload method.

Honza

Comment 3

[:Harald Kirschner :digitarald]

Via David Fowler:

They are 2 versions of SignalR and 2 protocols (old and new).
New: https://github.com/aspnet/AspNetCore/blob/master/src/SignalR/docs/specs/HubProtocol.md
Old: https://blog.3d-logic.com/2015/03/29/signalr-on-the-wire-an-informal-description-of-the-signalr-protocol/

Regarding pulling in a parser, the JSON protocol is baked into the main library:
https://www.npmjs.com/package/@microsoft/signalr and https://www.npmjs.com/package/@microsoft/signalr-protocol-msgpack
Relevant types are JsonHubProtocol and MessagePackHubProtocol.

Test pages:

Everything: http://signalr-samples.azurewebsites.net/hubs.html
https://signalrcore-chatsample.azurewebsites.net/
https://streamr.azurewebsites.net/

MVP fix suggestion, correctly parse the data using JSON despite the binary-postfix. Example: {"protocol":"json"}

Comment 4

[:Harald Kirschner :digitarald]

With the last line in the summary, this should probably be the focus of this bug, detecting the end-of-text character and parsing the JSON.

What do you think, David?

Comment 5

[David Walsh :davidwalsh]

Attachment: Bug 1589068 - Provide MVP Signalr support for the websocket inspector r=Honza

Comment 6

[David Walsh :davidwalsh]

Submitted a very simple patch for myself or someone else to work from.

Comment 7

[Bryan Kok [:transfusion]]

Attachment: Bug 1589068 - Add support for HandshakeResponse, Invocation, StreamItem, and Completion incoming messages, including multiple messages in one WS frame, by including SignalR client sources r=Honza

Add support for HandshakeResponse, Invocation, StreamItem, and Completion incoming messages, including multiple messages in one WS frame, by including SignalR client sources

Comment 8

[Bryan Kok [:transfusion]]

https://github.com/aspnet/AspNetCore/blob/master/src/SignalR/docs/specs/HubProtocol.md

I ran JsonHubProtocol.ts and HandshakeProtocol.js from the link above through tsc and prettified it, along with necessary imports in order to make use of its parseMessages method, which handles the splitting of all server -> client messages except on the 0x1E separator and does some basic checks to ensure that fields of a message of a certain type shouldn't be empty.

Here's one such multipart message in one WS frame.

HandshakeProtocol is the message immediately succeeding the initial {"protocol":"json","version":1}[0x1E] protocol negotiation message, i.e. {}[0x1E]{"type":1,"target":"Send","arguments":["S6iL70sj2wn12tZP2aJVwg joined"]}[0x1E]. According to their code, the handshake response may contain textual data.

// content before separator is handshake response
// optional content after is additional messages

https://github.com/aspnet/AspNetCore/blob/master/src/SignalR/clients/ts/signalr/src/HandshakeProtocol.ts#L26-L69

The client library only handles server -> client messages; it doesn't handle StreamInvocation or Invocation client -> server messages as in the https://streamr.azurewebsites.net/ demo, in which case I fall back to :davidwalsh's MVP JSON decoding. Furthermore, their parseMessages method doesn't add any contextually useful info, it simply calls JSON.parse after splitting, so perhaps if the license permits we can be more selective about which code we include from the library.

P.S. I tried to base my revision D51498 off of :davidwalsh's initial commit f322f85aee82. The first time I did arc diff --create, it only uploaded my commit and the code review bot wasn't able to apply the patch, the 2nd time, I ran arc diff .^^ again, now the original commit shows up in the Commits tab but the code review bot tries to apply my wrongly created patch first; I want to delete Diff 1 185601 but I don't know how.

Comment 9

[brecon]

Hello, I'm on the SignalR team and would love to help out if needed.

A few things worth noting:

• The handshake message being sent and received will always be JSON and have the [0x1E] character as a separator.
• Messages after the handshake message might be written in a different protocol such as MessagePack
  • This means if a message in MessagePack ends up in the same websocket frame as the handshake, you wont have a [0x1E] character at the end
• "it doesn't handle StreamInvocation" - that's true, but the messages should just be passable to JSON.parse as is
• Would it be possible to map the "type": X values to a friendly name? Like 1 is Invocation etc.

Comment 10

[:Harald Kirschner :digitarald]

Would it be possible to map the "type": X values to a friendly name? Like 1 is Invocation etc.

SGTM. I think the second patch is doing that.

Comment 11

[brecon]

Ah, I didn't see the second change, I'm not familiar with browsing this site!

I tried add a couple comments on the change but can't unfortunately.

Comment 12

[:Harald Kirschner :digitarald]

No worries. You probably need additional rights for commenting in phabricator. Feel free to comment here for any recommendations you got.

Comment 13

[brecon]

Line 49 of JSONHubProtocol.js could probably be break; instead of continue; so new kinds of messages will show up without having to update Firefox.

The case statements in JSONHubProtocol.js could add parsedMessage.type = MessageType[parsedMessage.type]; for making the "type": X visual nicer.

Example:

case IHubProtocol.MessageType.Invocation:
          this.isInvocationMessage(parsedMessage);
          parsedMessage.type = IHubProtocol.MessageType[parsedMessage.type];
          break;

Comment 14

[Jan Honza Odvarko [:Honza] (always need-info? me)]

I just posted some review comments mainly related to licensing, but also mentioning the comment #13

We also need to update the about:licensing page before landing this, see bug 1595738

Honza

Comment 15

[Phabricator Automation]

Comment on attachment 9105316 [details]
Bug 1589068 - Provide MVP Signalr support for the websocket inspector r=Honza

Revision D51149 was moved to bug 1594434. Setting attachment 9105316 [details] to obsolete.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f8c0e12d1ae
Add support for HandshakeResponse, Invocation, StreamItem, and Completion incoming messages, including multiple messages in one WS frame, by including SignalR client sources r=Honza,mhoye

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/2f8c0e12d1ae

Comment 18

[:Harald Kirschner :digitarald]

Brecon, I am trying to test this – which page did you use? Honza, does this work for you on http://signalr-samples.azurewebsites.net/hubs.html ?

Comment 19

[:Harald Kirschner :digitarald]

Sorry, wrong user ni?. I meant to ping :transfusion with my message above.

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: image.png

(In reply to :Harald Kirschner :digitarald from comment #18)

Brecon, I am trying to test this – which page did you use? Honza, does this work for you on http://signalr-samples.azurewebsites.net/hubs.html ?

Correct, works for me, see the attached screenshot.

Honza

Comment 21

[:Harald Kirschner :digitarald]

Works in a new profile on DE and Nightly. Gotta debug my default profile a bit :). Clearing ni?