Patchwork D1850: hgweb: when no agreement on compression can be found, fail for v2

login
register
mail settings
Submitter phabricator
Date Jan. 12, 2018, 8:04 a.m.
Message ID <differential-rev-PHID-DREV-e3ofc276d73zwieo5to4-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/26693/
State New
Headers show

Comments

phabricator - Jan. 12, 2018, 8:04 a.m.
joerg.sonnenberger created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  When the client supports v2 responses, the fallback to the legacy
  response is undesirable. If the zlib is acceptable for the client,
  it should have said so in first place.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/hgweb/common.py
  mercurial/hgweb/protocol.py
  tests/test-http-protocol.t

CHANGE DETAILS




To: joerg.sonnenberger, #hg-reviewers
Cc: mercurial-devel
phabricator - Jan. 13, 2018, 1:20 a.m.
indygreg requested changes to this revision.
indygreg added a comment.
This revision now requires changes to proceed.


  We're not using the `Accept*` HTTP headers, so the use of HTTP `406` is not appropriate. See https://tools.ietf.org/html/rfc7231#section-6.5.6. I believe if you scour the mailing list or even the commit messages, you'll find text explaining why we explicitly chose to not use the `Accept` headers for protocol negotiation.
  
  I do think I'm OK with aborting the request if the client request is "malformed." Although it's a BC break and needs to be called out as such. Since we already shipped forgiving code, others may insist we keep the existing behavior. They have a point.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - Jan. 13, 2018, 11:08 a.m.
joerg.sonnenberger added a comment.


  Which status code shall we use then, just plain 400?

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel
phabricator - Jan. 14, 2018, 9:35 p.m.
indygreg added a comment.


  In https://phab.mercurial-scm.org/D1850#31404, @joerg.sonnenberger wrote:
  
  > Which status code shall we use then, just plain 400?
  
  
  Good question.
  
  We use `400` for parts of hgweb. But not the wire protocol parts. And the use of `400` should arguably be avoided because it is supposed to mean the HTTP request message itself was malformed. A lot of people (including our uses in hgweb) extend `400` to mean things like the query string parameters are invalid. RFC 7231 is still vague in its wording and does seem to allow liberal use of this status code. That's probably because of common use of `400` in the wild.
  
  In the wire protocol today, it looks like we use `200` and a `Content-Type: application/hg-error` to indicate error. I think this is what we should use here (assuming existing client codes handles the error sanely). It should, since `httppeer.py` always checks for this content-type as part of processing every HTTP response.
  
  For the next submission, please add a test showing that an `hg` operation hitting this error in the context of `hg pull` or `hg clone` does something reasonable.

REPOSITORY
  rHG Mercurial

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

To: joerg.sonnenberger, #hg-reviewers, indygreg
Cc: indygreg, mercurial-devel

Patch

diff --git a/tests/test-http-protocol.t b/tests/test-http-protocol.t
--- a/tests/test-http-protocol.t
+++ b/tests/test-http-protocol.t
@@ -78,14 +78,15 @@ 
   server: * (glob)
   transfer-encoding: chunked
 
-Requesting a compression format that server doesn't support results will fall back to 0.1
+Server should fail when it can't agree with client on a compression format
 
   $ get-with-headers.py --hgproto '0.2 comp=aa' --headeronly $LOCALIP:$HGPORT '?cmd=getbundle&heads=e93700bd72895c5addab234c56d4024b487a362f&common=0000000000000000000000000000000000000000' -
-  200 Script output follows
+  406 No common compression format
+  content-length: 31
   content-type: application/mercurial-0.1
   date: * (glob)
   server: * (glob)
-  transfer-encoding: chunked
+  [1]
 
 #if zstd
 zstd is used if available
diff --git a/mercurial/hgweb/protocol.py b/mercurial/hgweb/protocol.py
--- a/mercurial/hgweb/protocol.py
+++ b/mercurial/hgweb/protocol.py
@@ -11,6 +11,8 @@ 
 import struct
 
 from .common import (
+    ErrorResponse,
+    HTTP_NOT_ACCEPTABLE,
     HTTP_OK,
 )
 
@@ -140,8 +142,8 @@ 
 
                     return HGTYPE2, engine, opts
 
-            # No mutually supported compression format. Fall back to the
-            # legacy protocol.
+            raise ErrorResponse(HTTP_NOT_ACCEPTABLE,
+                                'No common compression format')
 
         # Don't allow untrusted settings because disabling compression or
         # setting a very high compression level could lead to flooding
diff --git a/mercurial/hgweb/common.py b/mercurial/hgweb/common.py
--- a/mercurial/hgweb/common.py
+++ b/mercurial/hgweb/common.py
@@ -28,6 +28,7 @@ 
 HTTP_FORBIDDEN = 403
 HTTP_NOT_FOUND = 404
 HTTP_METHOD_NOT_ALLOWED = 405
+HTTP_NOT_ACCEPTABLE = 406
 HTTP_SERVER_ERROR = 500