Last Call Review of draft-ietf-jmap-websocket-04
review-ietf-jmap-websocket-04-tsvart-lc-briscoe-2019-12-19-00

This document has been reviewed as part of the transport area review team's
ongoing effort to review key IETF documents. These comments were written
primarily for the transport area directors, but are copied to the document's
authors and WG to allow them to address any issues raised and also to the IETF
discussion list for information.

When done at the time of IETF Last Call, the authors should consider this
review as part of the last-call comments they receive. Please always CC
tsv-art@ietf.org if you reply to or forward this review.


==Transport-specific review comments==

The only transport-specific issues that I can think of would relate to 
holding open a connection for a long time, which increases the risk of 
brute-force connection hijacking and has interactions with NATs and 
mobility. However, they are all WebSockets issues, and adding jmap over 
WebSockets doesn't change any of that.

==Non-transport-related review comments==

I found the document readable and comprehensible.

The following is my only concern...

===Shouldn't extensibility be discussed?===

The specification is full of statements saying what peer A MUST do, but 
lacks statements saying what peer B MUST do if peer A doesn't do what it 
is supposed to.  

Perhaps there needs to be a default case for what to do if one peer 
receives a message that violates the Websockets JMAP subprotocol (or at 
least one peer believes it violates the version of the protocol that it 
supports).

Examples:

4.  JMAP Subprotocol
   Binary data MUST NOT be uploaded or downloaded 
   through a WebSocket JMAP connection.
What if they are?

4.1 Handshake
   Other message types MUST
   NOT be transmitted over this connection.
What if they are?

4.2 WebSocket Messages
The lists of allowed messages following "The messages MUST be in the 
form of..." do not say what to do if they are not, and do not seem to 
allow for extensibility.

===Nits===

4.1 Handshake

CURRENT:
   If a client receives a handshake response that does not include
   "jmap" in the "Sec-WebSocket-Protocol" header, ...
PROPOSED:
   If a client receives a handshake response in which the value of the
   "Sec-WebSocket-Protocol" header is not "jmap", ...
REASON:
'include' implies it would have been valid for the server to have sent 
a list of subprotocols.