Patchwork D2837: wireproto: require POST for all HTTPv2 requests

login
register
mail settings
Submitter phabricator
Date March 13, 2018, 2:16 a.m.
Message ID <differential-rev-PHID-DREV-ft5ups2wvpvw2rd36c2c-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/29428/
State Superseded
Headers show

Comments

phabricator - March 13, 2018, 2:16 a.m.
indygreg created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  Wire protocol version 1 transfers argument data via request
  headers by default. This has historically caused problems because
  servers institute limits on the length of individual HTTP headers
  as well as the total size of all request headers. Mercurial servers
  can advertise the maximum length of an individual header. But
  there's no guarantee any intermediate HTTP agents will accept
  headers up to that length.
  
  In the existing wire protocol, server operators typically also
  key off the HTTP request method to implement authentication.
  For example, GET requests translate to read-only requests and
  can be allowed. But read-write commands must use POST and require
  authentication. This has typically worked because the only wire
  protocol commands that use POST modify the repo (e.g. the
  "unbundle" command).
  
  There is an experimental feature to enable clients to transmit
  argument data via POST request bodies. This is technically a
  better and more robust solution. But we can't enable it by default
  because of servers assuming POST means write access.
  
  In version 2 of the wire protocol, the permissions of a request
  are encoded in the URL. And with it being a new protocol in a new
  URL space, we're not constrained by backwards compatibility
  requirements.
  
  This commit adopts the technically superior mechanism of using
  HTTP request bodies to send argument data by requiring POST for
  all commands. Strictly speaking, it may be possible to send
  request bodies on GET requests. But my experience is that not all
  HTTP stacks support this. POST pretty much always works. Using POST
  for read-only operations does sacrifice some RESTful design
  purity. But this API cares about practicality, not about being
  in Roy T. Fielding's REST ivory tower.
  
  There's a chance we may relax this restriction in the future. But
  for now, I want to see how far we can get with a POST only API.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2837

AFFECTED FILES
  mercurial/help/internals/wireprotocol.txt
  mercurial/wireprotoserver.py
  tests/test-http-api-httpv2.t

CHANGE DETAILS




To: indygreg, #hg-reviewers
Cc: mercurial-devel

Patch

diff --git a/tests/test-http-api-httpv2.t b/tests/test-http-api-httpv2.t
--- a/tests/test-http-api-httpv2.t
+++ b/tests/test-http-api-httpv2.t
@@ -32,41 +32,53 @@ 
 
 Request to read-only command works out of the box
 
-  $ get-with-headers.py $LOCALIP:$HGPORT api/$HTTPV2/ro/known -
+  $ get-with-headers.py --method POST $LOCALIP:$HGPORT api/$HTTPV2/ro/known -
   200 OK
   content-length: 9
   content-type: text/plain
   
   ro/known/ (no-eol)
 
 Request to unknown command yields 404
 
-  $ get-with-headers.py $LOCALIP:$HGPORT api/$HTTPV2/ro/badcommand -
+  $ get-with-headers.py --method POST $LOCALIP:$HGPORT api/$HTTPV2/ro/badcommand -
   404 Not Found
   content-length: 42
   content-type: text/plain
   
   unknown wire protocol command: badcommand
   [1]
 
+Only POST is allowed
+
+  $ get-with-headers.py --method GET $LOCALIP:$HGPORT api/$HTTPV2/ro/known -
+  405 Method Not Allowed
+  allow: POST
+  content-length: 30
+  
+  commands require POST requests (no-eol)
+  [1]
+
 Request to read-write command fails because server is read-only by default
 
 GET request not allowed
 
   $ get-with-headers.py $LOCALIP:$HGPORT api/$HTTPV2/rw/known -
-  405 push requires POST request
-  content-length: 17
+  405 Method Not Allowed
+  allow: POST
+  content-length: 30
   
-  permission denied (no-eol)
+  commands require POST requests (no-eol)
   [1]
 
 Even for unknown commands
 
   $ get-with-headers.py $LOCALIP:$HGPORT api/$HTTPV2/rw/badcommand -
-  405 push requires POST request
-  content-length: 17
+  405 Method Not Allowed
+  allow: POST
+  content-length: 30
   
-  permission denied (no-eol)
+  commands require POST requests (no-eol)
   [1]
 
 
diff --git a/mercurial/wireprotoserver.py b/mercurial/wireprotoserver.py
--- a/mercurial/wireprotoserver.py
+++ b/mercurial/wireprotoserver.py
@@ -299,6 +299,12 @@ 
         res.setbodybytes(_('unknown permission: %s') % permission)
         return
 
+    if req.method != 'POST':
+        res.status = b'405 Method Not Allowed'
+        res.headers[b'Allow'] = b'POST'
+        res.setbodybytes(_('commands require POST requests'))
+        return
+
     # At some point we'll want to use our own API instead of recycling the
     # behavior of version 1 of the wire protocol...
     # TODO return reasonable responses - not responses that overload the
diff --git a/mercurial/help/internals/wireprotocol.txt b/mercurial/help/internals/wireprotocol.txt
--- a/mercurial/help/internals/wireprotocol.txt
+++ b/mercurial/help/internals/wireprotocol.txt
@@ -152,11 +152,14 @@ 
 Version 2 of the HTTP protocol is exposed under the ``/api/*`` URL space.
 It's final API name is not yet formalized.
 
-Commands are triggered by sending HTTP requests against URLs of the
+Commands are triggered by sending HTTP POST requests against URLs of the
 form ``<permission>/<command>``, where ``<permission>`` is ``ro`` or
 ``rw``, meaning read-only and read-write, respectively and ``<command>``
 is a named wire protocol command.
 
+Non-POST request methods MUST be rejected by the server with an HTTP
+405 response.
+
 Commands that modify repository state in meaningful ways MUST NOT be
 exposed under the ``ro`` URL prefix. All available commands MUST be
 available under the ``rw`` URL prefix.