wireproto: service multiple command requests per HTTP request

Now that our new frame-based protocol server can understand how
to ingest multiple, possibly interleaved, command requests, let's
hook it up to the HTTP server.

The code on the HTTP side of things is still a bit hacky. We need
a bit of work around error handling, content types, etc. But it's
a start.

Among the added tests, we demonstrate that a client can send frames
for multiple commands iterleaved with each other and that a later
issued command can respond before the first one has finished
sending. This makes our multi-request model technically superior
to the previous "batch" command.

Differential Revision: https://phab.mercurial-scm.org/D2871

mercurial/help/internals/wireprotocol.txt

@@ -203,6 +203,28 b' an HTTP 406.'
 Servers receiving requests with an invalid ``Content-Type`` header SHOULD
 respond with an HTTP 415.
 
+The command to run is specified in the POST payload as defined by the
+*Unified Frame-Based Protocol*. This is redundant with data already
+encoded in the URL. This is by design, so server operators can have
+better understanding about server activity from looking merely at
+HTTP access logs.
+
+In most circumstances, the command specified in the URL MUST match
+the command specified in the frame-based payload or the server will
+respond with an error. The exception to this is the special
+``multirequest`` URL. (See below.) In addition, HTTP requests
+are limited to one command invocation. The exception is the special
+``multirequest`` URL.
+
+The ``multirequest`` command endpoints (``ro/multirequest`` and
+``rw/multirequest``) are special in that they allow the execution of
+*any* command and allow the execution of multiple commands. If the
+HTTP request issues multiple commands across multiple frames, all
+issued commands will be processed by the server. Per the defined
+behavior of the *Unified Frame-Based Protocol*, commands may be
+issued interleaved and responses may come back in a different order
+than they were issued. Clients MUST be able to deal with this.
+
 SSH Protocol
 ============
 

mercurial/wireprotoserver.py

@@ -327,7 +327,12 b' def _handlehttpv2request(rctx, req, res,'
         _processhttpv2reflectrequest(rctx.repo.ui, rctx.repo, req, res)
         return
 
-    if command not in wireproto.commands:
+    # Extra commands that we handle that aren't really wire protocol
+    # commands. Think extra hard before making this hackery available to
+    # extension.
+    extracommands = {'multirequest'}
+
+    if command not in wireproto.commands and command not in extracommands:
         res.status = b'404 Not Found'
         res.headers[b'Content-Type'] = b'text/plain'
         res.setbodybytes(_('unknown wire protocol command: %s\n') % command)
@@ -338,7 +343,8 b' def _handlehttpv2request(rctx, req, res,'
 
     proto = httpv2protocolhandler(req, ui)
 
-    if not wireproto.commands.commandavailable(command, proto):
+    if (not wireproto.commands.commandavailable(command, proto)
+        and command not in extracommands):
         res.status = b'404 Not Found'
         res.headers[b'Content-Type'] = b'text/plain'
         res.setbodybytes(_('invalid wire protocol command: %s') % command)
@@ -434,18 +440,14 b' def _processhttpv2request(ui, repo, req,'
             # Need more data before we can do anything.
             continue
         elif action == 'runcommand':
-            # We currently only support running a single command per
-            # HTTP request.
-            if seencommand:
-                # TODO define proper error mechanism.
-                res.status = b'200 OK'
-                res.headers[b'Content-Type'] = b'text/plain'
-                res.setbodybytes(_('support for multiple commands per request '
-                                   'not yet implemented'))
+            sentoutput = _httpv2runcommand(ui, repo, req, res, authedperm,
+                                           reqcommand, reactor, meta,
+                                           issubsequent=seencommand)
+
+            if sentoutput:
                 return
 
-            _httpv2runcommand(ui, repo, req, res, authedperm, reqcommand,
-                              reactor, meta)
+            seencommand = True
 
         elif action == 'error':
             # TODO define proper error mechanism.
@@ -471,7 +473,7 b' def _processhttpv2request(ui, repo, req,'
                                      % action)
 
 def _httpv2runcommand(ui, repo, req, res, authedperm, reqcommand, reactor,
-                      command):
+                      command, issubsequent):
     """Dispatch a wire protocol command made from HTTPv2 requests.
 
     The authenticated permission (``authedperm``) along with the original
@@ -484,34 +486,57 b' def _httpv2runcommand(ui, repo, req, res'
     # run doesn't have permissions requirements greater than what was granted
     # by ``authedperm``.
     #
-    # For now, this is no big deal, as we only allow a single command per
-    # request and that command must match the command in the URL. But when
-    # things change, we need to watch out...
-    if reqcommand != command['command']:
-        # TODO define proper error mechanism
-        res.status = b'200 OK'
-        res.headers[b'Content-Type'] = b'text/plain'
-        res.setbodybytes(_('command in frame must match command in URL'))
-        return
-
-    # TODO once we get rid of the command==URL restriction, we'll need to
-    # revalidate command validity and auth here. checkperm,
-    # wireproto.commands.commandavailable(), etc.
+    # Our rule for this is we only allow one command per HTTP request and
+    # that command must match the command in the URL. However, we make
+    # an exception for the ``multirequest`` URL. This URL is allowed to
+    # execute multiple commands. We double check permissions of each command
+    # as it is invoked to ensure there is no privilege escalation.
+    # TODO consider allowing multiple commands to regular command URLs
+    # iff each command is the same.
 
     proto = httpv2protocolhandler(req, ui, args=command['args'])
-    assert wireproto.commands.commandavailable(command['command'], proto)
-    wirecommand = wireproto.commands[command['command']]
 
-    assert authedperm in (b'ro', b'rw')
-    assert wirecommand.permission in ('push', 'pull')
+    if reqcommand == b'multirequest':
+        if not wireproto.commands.commandavailable(command['command'], proto):
+            # TODO proper error mechanism
+            res.status = b'200 OK'
+            res.headers[b'Content-Type'] = b'text/plain'
+            res.setbodybytes(_('wire protocol command not available: %s') %
+                             command['command'])
+            return True
+
+        assert authedperm in (b'ro', b'rw')
+        wirecommand = wireproto.commands[command['command']]
+        assert wirecommand.permission in ('push', 'pull')
 
-    # We already checked this as part of the URL==command check, but
-    # permissions are important, so do it again.
-    if authedperm == b'ro':
-        assert wirecommand.permission == 'pull'
-    elif authedperm == b'rw':
-        # We are allowed to access read-only commands under the rw URL.
-        assert wirecommand.permission in ('push', 'pull')
+        if authedperm == b'ro' and wirecommand.permission != 'pull':
+            # TODO proper error mechanism
+            res.status = b'403 Forbidden'
+            res.headers[b'Content-Type'] = b'text/plain'
+            res.setbodybytes(_('insufficient permissions to execute '
+                               'command: %s') % command['command'])
+            return True
+
+        # TODO should we also call checkperm() here? Maybe not if we're going
+        # to overhaul that API. The granted scope from the URL check should
+        # be good enough.
+
+    else:
+        # Don't allow multiple commands outside of ``multirequest`` URL.
+        if issubsequent:
+            # TODO proper error mechanism
+            res.status = b'200 OK'
+            res.headers[b'Content-Type'] = b'text/plain'
+            res.setbodybytes(_('multiple commands cannot be issued to this '
+                               'URL'))
+            return True
+
+        if reqcommand != command['command']:
+            # TODO define proper error mechanism
+            res.status = b'200 OK'
+            res.headers[b'Content-Type'] = b'text/plain'
+            res.setbodybytes(_('command in frame must match command in URL'))
+            return True
 
     rsp = wireproto.dispatch(repo, proto, command['command'])
 
@@ -527,6 +552,7 b' def _httpv2runcommand(ui, repo, req, res'
 
     if action == 'sendframes':
         res.setbodygen(meta['framegen'])
+        return True
     elif action == 'noop':
         pass
     else:

tests/test-http-api-httpv2.t

@@ -412,4 +412,153 b' Command frames can be reflected via debu'
   s>     received: <no frame>\n
   s>     {"action": "noop"}
 
+Multiple requests to regular command URL are not allowed
+
+  $ send << EOF
+  > httprequest POST api/$HTTPV2/ro/customreadonly
+  >     accept: $MEDIATYPE
+  >     content-type: $MEDIATYPE
+  >     user-agent: test
+  >     frame 1 command-name eos customreadonly
+  >     frame 3 command-name eos customreadonly
+  > EOF
+  using raw connection to peer
+  s>     POST /api/exp-http-v2-0001/ro/customreadonly HTTP/1.1\r\n
+  s>     Accept-Encoding: identity\r\n
+  s>     accept: application/mercurial-exp-framing-0002\r\n
+  s>     content-type: application/mercurial-exp-framing-0002\r\n
+  s>     user-agent: test\r\n
+  s>     content-length: 40\r\n
+  s>     host: $LOCALIP:$HGPORT\r\n (glob)
+  s>     \r\n
+  s>     \x0e\x00\x00\x01\x00\x11customreadonly\x0e\x00\x00\x03\x00\x11customreadonly
+  s> makefile('rb', None)
+  s>     HTTP/1.1 200 OK\r\n
+  s>     Server: testing stub value\r\n
+  s>     Date: $HTTP_DATE$\r\n
+  s>     Content-Type: text/plain\r\n
+  s>     Content-Length: 46\r\n
+  s>     \r\n
+  s>     multiple commands cannot be issued to this URL
+
+Multiple requests to "multirequest" URL are allowed
+
+  $ send << EOF
+  > httprequest POST api/$HTTPV2/ro/multirequest
+  >     accept: $MEDIATYPE
+  >     content-type: $MEDIATYPE
+  >     user-agent: test
+  >     frame 1 command-name eos customreadonly
+  >     frame 3 command-name eos customreadonly
+  > EOF
+  using raw connection to peer
+  s>     POST /api/exp-http-v2-0001/ro/multirequest HTTP/1.1\r\n
+  s>     Accept-Encoding: identity\r\n
+  s>     accept: application/mercurial-exp-framing-0002\r\n
+  s>     content-type: application/mercurial-exp-framing-0002\r\n
+  s>     user-agent: test\r\n
+  s>     *\r\n (glob)
+  s>     host: $LOCALIP:$HGPORT\r\n (glob)
+  s>     \r\n
+  s>     \x0e\x00\x00\x01\x00\x11customreadonly\x0e\x00\x00\x03\x00\x11customreadonly
+  s> makefile('rb', None)
+  s>     HTTP/1.1 200 OK\r\n
+  s>     Server: testing stub value\r\n
+  s>     Date: $HTTP_DATE$\r\n
+  s>     Content-Type: application/mercurial-exp-framing-0002\r\n
+  s>     Transfer-Encoding: chunked\r\n
+  s>     \r\n
+  s>     *\r\n (glob)
+  s>     \x1d\x00\x00\x01\x00Bcustomreadonly bytes response
+  s>     \r\n
+  s>     23\r\n
+  s>     \x1d\x00\x00\x03\x00Bcustomreadonly bytes response
+  s>     \r\n
+  s>     0\r\n
+  s>     \r\n
+
+Interleaved requests to "multirequest" are processed
+
+  $ send << EOF
+  > httprequest POST api/$HTTPV2/ro/multirequest
+  >     accept: $MEDIATYPE
+  >     content-type: $MEDIATYPE
+  >     user-agent: test
+  >     frame 1 command-name have-args listkeys
+  >     frame 3 command-name have-args listkeys
+  >     frame 3 command-argument eoa \x09\x00\x09\x00namespacebookmarks
+  >     frame 1 command-argument eoa \x09\x00\x0a\x00namespacenamespaces
+  > EOF
+  using raw connection to peer
+  s>     POST /api/exp-http-v2-0001/ro/multirequest HTTP/1.1\r\n
+  s>     Accept-Encoding: identity\r\n
+  s>     accept: application/mercurial-exp-framing-0002\r\n
+  s>     content-type: application/mercurial-exp-framing-0002\r\n
+  s>     user-agent: test\r\n
+  s>     content-length: 85\r\n
+  s>     host: $LOCALIP:$HGPORT\r\n (glob)
+  s>     \r\n
+  s>     \x08\x00\x00\x01\x00\x12listkeys\x08\x00\x00\x03\x00\x12listkeys\x16\x00\x00\x03\x00"	\x00	\x00namespacebookmarks\x17\x00\x00\x01\x00"	\x00\n
+  s>     \x00namespacenamespaces
+  s> makefile('rb', None)
+  s>     HTTP/1.1 200 OK\r\n
+  s>     Server: testing stub value\r\n
+  s>     Date: $HTTP_DATE$\r\n
+  s>     Content-Type: application/mercurial-exp-framing-0002\r\n
+  s>     Transfer-Encoding: chunked\r\n
+  s>     \r\n
+  s>     6\r\n
+  s>     \x00\x00\x00\x03\x00B
+  s>     \r\n
+  s>     24\r\n
+  s>     \x1e\x00\x00\x01\x00Bbookmarks	\n
+  s>     namespaces	\n
+  s>     phases	
+  s>     \r\n
+  s>     0\r\n
+  s>     \r\n
+
+Restart server to disable read-write access
+
+  $ killdaemons.py
+  $ cat > server/.hg/hgrc << EOF
+  > [experimental]
+  > web.apiserver = true
+  > web.api.debugreflect = true
+  > web.api.http-v2 = true
+  > [web]
+  > push_ssl = false
+  > EOF
+
+  $ hg -R server serve -p $HGPORT -d --pid-file hg.pid -E error.log
+  $ cat hg.pid > $DAEMON_PIDS
+
+Attempting to run a read-write command via multirequest on read-only URL is not allowed
+
+  $ send << EOF
+  > httprequest POST api/$HTTPV2/ro/multirequest
+  >     accept: $MEDIATYPE
+  >     content-type: $MEDIATYPE
+  >     user-agent: test
+  >     frame 1 command-name eos unbundle
+  > EOF
+  using raw connection to peer
+  s>     POST /api/exp-http-v2-0001/ro/multirequest HTTP/1.1\r\n
+  s>     Accept-Encoding: identity\r\n
+  s>     accept: application/mercurial-exp-framing-0002\r\n
+  s>     content-type: application/mercurial-exp-framing-0002\r\n
+  s>     user-agent: test\r\n
+  s>     content-length: 14\r\n
+  s>     host: $LOCALIP:$HGPORT\r\n (glob)
+  s>     \r\n
+  s>     \x08\x00\x00\x01\x00\x11unbundle
+  s> makefile('rb', None)
+  s>     HTTP/1.1 403 Forbidden\r\n
+  s>     Server: testing stub value\r\n
+  s>     Date: $HTTP_DATE$\r\n
+  s>     Content-Type: text/plain\r\n
+  s>     Content-Length: 53\r\n
+  s>     \r\n
+  s>     insufficient permissions to execute command: unbundle
+
   $ cat error.log