Patchwork [2,of,2] wireproto: unescape argument names in batch command (BC)

login
register
mail settings
Submitter Gregory Szorc
Date Aug. 6, 2016, 8:59 p.m.
Message ID <cee9356ebc8570a560d5.1470517166@ubuntu-vm-main>
Download mbox | patch
Permalink /patch/16165/
State Accepted
Headers show

Comments

Gregory Szorc - Aug. 6, 2016, 8:59 p.m.
# HG changeset patch
# User Gregory Szorc <gregory.szorc@gmail.com>
# Date 1470516921 25200
#      Sat Aug 06 13:55:21 2016 -0700
# Node ID cee9356ebc8570a560d5c940e36f0e910c9a3e91
# Parent  a700e61e5b1b74884e3537c9268c08bc11a8b1e8
wireproto: unescape argument names in batch command (BC)

Clients escape both argument names and values when using the
batch command. Yet the server was only unescaping argument values.

Fortunately we don't have any argument names that need escaped. But
that isn't an excuse to lack symmetry in the code.

Since the server wasn't properly unescaping argument names, this
means we can never introduce an argument to a batchable command that
needs escaped because an old server wouldn't properly decode its name.
So we've introduced an assertion to detect the accidental introduction
of this in the future. Of course, we could introduce a server
capability that says the server knows how to decode argument names and
allow special argument names to go through. But until there is a need
for it (which I doubt there will be), we shouldn't bother with adding
an unused capability.
Pierre-Yves David - Aug. 6, 2016, 10:27 p.m.
On 08/06/2016 10:59 PM, Gregory Szorc wrote:
> # HG changeset patch
> # User Gregory Szorc <gregory.szorc@gmail.com>
> # Date 1470516921 25200
> #      Sat Aug 06 13:55:21 2016 -0700
> # Node ID cee9356ebc8570a560d5c940e36f0e910c9a3e91
> # Parent  a700e61e5b1b74884e3537c9268c08bc11a8b1e8
> wireproto: unescape argument names in batch command (BC)
>
> Clients escape both argument names and values when using the
> batch command. Yet the server was only unescaping argument values.
>
> Fortunately we don't have any argument names that need escaped. But
> that isn't an excuse to lack symmetry in the code.
>
> Since the server wasn't properly unescaping argument names, this
> means we can never introduce an argument to a batchable command that
> needs escaped because an old server wouldn't properly decode its name.
> So we've introduced an assertion to detect the accidental introduction
> of this in the future. Of course, we could introduce a server
> capability that says the server knows how to decode argument names and
> allow special argument names to go through. But until there is a need
> for it (which I doubt there will be), we shouldn't bother with adding
> an unused capability.

arg…
Thanks for spotting and fixing this. Pushed.

>
> diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
> --- a/mercurial/wireproto.py
> +++ b/mercurial/wireproto.py
> @@ -186,16 +186,21 @@ def unescapearg(escaped):
>              .replace(':s', ';')
>              .replace(':o', ',')
>              .replace(':c', ':'))
>
>  def encodebatchcmds(req):
>      """Return a ``cmds`` argument value for the ``batch`` command."""
>      cmds = []
>      for op, argsdict in req:
> +        # Old servers didn't properly unescape argument names. So prevent
> +        # the sending of argument names that may not be decoded properly by
> +        # servers.
> +        assert all(escapearg(k) == k for k in argsdict)
> +
>          args = ','.join('%s=%s' % (escapearg(k), escapearg(v))
>                          for k, v in argsdict.iteritems())
>          cmds.append('%s %s' % (op, args))
>
>      return ';'.join(cmds)
>
>  # mapping of options accepted by getbundle and their types
>  #
> @@ -611,17 +616,17 @@ def batch(repo, proto, cmds, others):
>      repo = repo.filtered("served")
>      res = []
>      for pair in cmds.split(';'):
>          op, args = pair.split(' ', 1)
>          vals = {}
>          for a in args.split(','):
>              if a:
>                  n, v = a.split('=')
> -                vals[n] = unescapearg(v)
> +                vals[unescapearg(n)] = unescapearg(v)
>          func, spec = commands[op]
>          if spec:
>              keys = spec.split()
>              data = {}
>              for k in keys:
>                  if k == '*':
>                      star = {}
>                      for key in vals.keys():
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

Patch

diff --git a/mercurial/wireproto.py b/mercurial/wireproto.py
--- a/mercurial/wireproto.py
+++ b/mercurial/wireproto.py
@@ -186,16 +186,21 @@  def unescapearg(escaped):
             .replace(':s', ';')
             .replace(':o', ',')
             .replace(':c', ':'))
 
 def encodebatchcmds(req):
     """Return a ``cmds`` argument value for the ``batch`` command."""
     cmds = []
     for op, argsdict in req:
+        # Old servers didn't properly unescape argument names. So prevent
+        # the sending of argument names that may not be decoded properly by
+        # servers.
+        assert all(escapearg(k) == k for k in argsdict)
+
         args = ','.join('%s=%s' % (escapearg(k), escapearg(v))
                         for k, v in argsdict.iteritems())
         cmds.append('%s %s' % (op, args))
 
     return ';'.join(cmds)
 
 # mapping of options accepted by getbundle and their types
 #
@@ -611,17 +616,17 @@  def batch(repo, proto, cmds, others):
     repo = repo.filtered("served")
     res = []
     for pair in cmds.split(';'):
         op, args = pair.split(' ', 1)
         vals = {}
         for a in args.split(','):
             if a:
                 n, v = a.split('=')
-                vals[n] = unescapearg(v)
+                vals[unescapearg(n)] = unescapearg(v)
         func, spec = commands[op]
         if spec:
             keys = spec.split()
             data = {}
             for k in keys:
                 if k == '*':
                     star = {}
                     for key in vals.keys():