| 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
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.
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.
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(), } }