Patchwork [03,of,11] py3: raw stringify various things in the LFS server module

login
register
mail settings
Submitter Matt Harbison
Date Jan. 28, 2019, 5:20 a.m.
Message ID <9a35d8754bdfaac20d19.1548652849@Envy>
Download mbox | patch
Permalink /patch/38129/
State Accepted
Headers show

Comments

Matt Harbison - Jan. 28, 2019, 5:20 a.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1548568239 18000
#      Sun Jan 27 00:50:39 2019 -0500
# Node ID 9a35d8754bdfaac20d1931dbd6a82233d11f4722
# Parent  7768ad6f53d8e08e90918e50201fc2fab4f9eff6
py3: raw stringify various things in the LFS server module

Some of this is based on code written by Augie.  I'm slightly unsure if these
are the correct pycompat bytes <-> str conversion methods.
Yuya Nishihara - Jan. 28, 2019, 11:58 a.m.
On Mon, 28 Jan 2019 00:20:49 -0500, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1548568239 18000
> #      Sun Jan 27 00:50:39 2019 -0500
> # Node ID 9a35d8754bdfaac20d1931dbd6a82233d11f4722
> # Parent  7768ad6f53d8e08e90918e50201fc2fab4f9eff6
> py3: raw stringify various things in the LFS server module

> -    operation = lfsreq.get('operation')
> -    if operation not in ('upload', 'download'):
> +    operation = lfsreq.get(r'operation')
> +    operation = pycompat.bytestr(operation)

If we don't care non-ASCII "operation" value and don't want Unicode exception,
using pycompat.sysbytes() is safer.
Matt Harbison - Feb. 2, 2019, 6:23 p.m.
On Mon, 28 Jan 2019 06:58:40 -0500, Yuya Nishihara <yuya@tcha.org> wrote:

> On Mon, 28 Jan 2019 00:20:49 -0500, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1548568239 18000
>> #      Sun Jan 27 00:50:39 2019 -0500
>> # Node ID 9a35d8754bdfaac20d1931dbd6a82233d11f4722
>> # Parent  7768ad6f53d8e08e90918e50201fc2fab4f9eff6
>> py3: raw stringify various things in the LFS server module
>
>> -    operation = lfsreq.get('operation')
>> -    if operation not in ('upload', 'download'):
>> +    operation = lfsreq.get(r'operation')
>> +    operation = pycompat.bytestr(operation)
>
> If we don't care non-ASCII "operation" value and don't want Unicode  
> exception,
> using pycompat.sysbytes() is safer.

I don't think we care, because there are only 3 defined values, and they  
are all ASCII.  From a philosophical POV, wouldn't we *want* it to explode  
if someone put a garbage value in, instead of masking it by either  
ignoring or replacing bad bytes?  The only difference I guess is between a  
4xx and 500 error code.
Yuya Nishihara - Feb. 3, 2019, 1:44 a.m.
On Sat, 02 Feb 2019 13:23:18 -0500, Matt Harbison wrote:
> On Mon, 28 Jan 2019 06:58:40 -0500, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Mon, 28 Jan 2019 00:20:49 -0500, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1548568239 18000
> >> #      Sun Jan 27 00:50:39 2019 -0500
> >> # Node ID 9a35d8754bdfaac20d1931dbd6a82233d11f4722
> >> # Parent  7768ad6f53d8e08e90918e50201fc2fab4f9eff6
> >> py3: raw stringify various things in the LFS server module
> >
> >> -    operation = lfsreq.get('operation')
> >> -    if operation not in ('upload', 'download'):
> >> +    operation = lfsreq.get(r'operation')
> >> +    operation = pycompat.bytestr(operation)
> >
> > If we don't care non-ASCII "operation" value and don't want Unicode  
> > exception,
> > using pycompat.sysbytes() is safer.
> 
> I don't think we care, because there are only 3 defined values, and they  
> are all ASCII.  From a philosophical POV, wouldn't we *want* it to explode  
> if someone put a garbage value in, instead of masking it by either  
> ignoring or replacing bad bytes?  The only difference I guess is between a  
> 4xx and 500 error code.

I think 4xx (Bad Requests) is the proper way to report the error since
'uploàd' is merely an unsupported operation.

Patch

diff --git a/hgext/lfs/wireprotolfsserver.py b/hgext/lfs/wireprotolfsserver.py
--- a/hgext/lfs/wireprotolfsserver.py
+++ b/hgext/lfs/wireprotolfsserver.py
@@ -133,25 +133,27 @@  def _processbatchrequest(repo, req, res)
     lfsreq = json.loads(req.bodyfh.read())
 
     # If no transfer handlers are explicitly requested, 'basic' is assumed.
-    if 'basic' not in lfsreq.get('transfers', ['basic']):
+    if r'basic' not in lfsreq.get(r'transfers', [r'basic']):
         _sethttperror(res, HTTP_BAD_REQUEST,
                       b'Only the basic LFS transfer handler is supported')
         return True
 
-    operation = lfsreq.get('operation')
-    if operation not in ('upload', 'download'):
+    operation = lfsreq.get(r'operation')
+    operation = pycompat.bytestr(operation)
+
+    if operation not in (b'upload', b'download'):
         _sethttperror(res, HTTP_BAD_REQUEST,
                       b'Unsupported LFS transfer operation: %s' % operation)
         return True
 
     localstore = repo.svfs.lfslocalblobstore
 
-    objects = [p for p in _batchresponseobjects(req, lfsreq.get('objects', []),
+    objects = [p for p in _batchresponseobjects(req, lfsreq.get(r'objects', []),
                                                 operation, localstore)]
 
     rsp = {
-        'transfer': 'basic',
-        'objects': objects,
+        r'transfer': r'basic',
+        r'objects': objects,
     }
 
     res.status = hgwebcommon.statusmessage(HTTP_OK)
@@ -190,11 +192,12 @@  def _batchresponseobjects(req, objects, 
 
     for obj in objects:
         # Convert unicode to ASCII to create a filesystem path
-        oid = obj.get('oid').encode('ascii')
+        soid = obj.get(r'oid')
+        oid = soid.encode(r'ascii')
         rsp = {
-            'oid': oid,
-            'size': obj.get('size'),  # XXX: should this check the local size?
-            #'authenticated': True,
+            r'oid': soid,
+            r'size': obj.get(r'size'),  # XXX: should this check the local size?
+            #r'authenticated': True,
         }
 
         exists = True
@@ -217,9 +220,9 @@  def _batchresponseobjects(req, objects, 
             if inst.errno != errno.ENOENT:
                 _logexception(req)
 
-                rsp['error'] = {
-                    'code': 500,
-                    'message': inst.strerror or 'Internal Server Server'
+                rsp[r'error'] = {
+                    r'code': 500,
+                    r'message': inst.strerror or r'Internal Server Server'
                 }
                 yield rsp
                 continue
@@ -230,17 +233,17 @@  def _batchresponseobjects(req, objects, 
         # IFF they already exist locally.
         if action == b'download':
             if not exists:
-                rsp['error'] = {
-                    'code': 404,
-                    'message': "The object does not exist"
+                rsp[r'error'] = {
+                    r'code': 404,
+                    r'message': r"The object does not exist"
                 }
                 yield rsp
                 continue
 
             elif not verifies:
-                rsp['error'] = {
-                    'code': 422,   # XXX: is this the right code?
-                    'message': "The object is corrupt"
+                rsp[r'error'] = {
+                    r'code': 422,   # XXX: is this the right code?
+                    r'message': r"The object is corrupt"
                 }
                 yield rsp
                 continue
@@ -256,22 +259,22 @@  def _batchresponseobjects(req, objects, 
             # a gratuitous deviation from lfs-test-server in the test
             # output.
             hdr = {
-                'Accept': 'application/vnd.git-lfs'
+                r'Accept': r'application/vnd.git-lfs'
             }
 
             auth = req.headers.get(b'Authorization', b'')
             if auth.startswith(b'Basic '):
-                hdr['Authorization'] = auth
+                hdr[r'Authorization'] = pycompat.strurl(auth)
 
             return hdr
 
-        rsp['actions'] = {
-            '%s' % action: {
-                'href': '%s%s/.hg/lfs/objects/%s'
-                    % (req.baseurl, req.apppath, oid),
+        rsp[r'actions'] = {
+            r'%s' % pycompat.strurl(action): {
+                r'href': pycompat.strurl(b'%s%s/.hg/lfs/objects/%s'
+                    % (req.baseurl, req.apppath, oid)),
                 # datetime.isoformat() doesn't include the 'Z' suffix
-                "expires_at": expiresat.strftime('%Y-%m-%dT%H:%M:%SZ'),
-                'header': _buildheader(),
+                r"expires_at": expiresat.strftime(r'%Y-%m-%dT%H:%M:%SZ'),
+                r'header': _buildheader(),
             }
         }