Patchwork [2,of,2] commitextras: make sure keys are non empty ASCII strings

login
register
mail settings
Submitter Pulkit Goyal
Date July 18, 2017, 12:18 p.m.
Message ID <6cd19a6fd50016034d2f.1500380291@workspace>
Download mbox | patch
Permalink /patch/22477/
State Accepted
Headers show

Comments

Pulkit Goyal - July 18, 2017, 12:18 p.m.
# HG changeset patch
# User Pulkit Goyal <7895pulkit@gmail.com>
# Date 1500378367 -19800
#      Tue Jul 18 17:16:07 2017 +0530
# Node ID 6cd19a6fd50016034d2faf7462bafdba4b90659e
# Parent  64137cfad7e30a459bc7fa5b6fc0e8757a6add8e
# EXP-Topic fbext
commitextras: make sure keys are non empty ASCII strings
Yuya Nishihara - July 18, 2017, 1:32 p.m.
On Tue, 18 Jul 2017 17:48:11 +0530, Pulkit Goyal wrote:
> # HG changeset patch
> # User Pulkit Goyal <7895pulkit@gmail.com>
> # Date 1500378367 -19800
> #      Tue Jul 18 17:16:07 2017 +0530
> # Node ID 6cd19a6fd50016034d2faf7462bafdba4b90659e
> # Parent  64137cfad7e30a459bc7fa5b6fc0e8757a6add8e
> # EXP-Topic fbext
> commitextras: make sure keys are non empty ASCII strings
> 
> diff --git a/hgext/commitextras.py b/hgext/commitextras.py
> --- a/hgext/commitextras.py
> +++ b/hgext/commitextras.py
> @@ -52,6 +52,11 @@
>                                  "KEY=VALUE format")
>                          raise error.Abort(msg % raw)
>                      k, v = raw.split('=', 1)
> +                    isascii = all(ord(c) < 128 for c in k)

Better to reject non-word characters. Perhaps ascii_letters + digits + '_-'
should be enough.

changelog uses ':' as a separator.

https://www.mercurial-scm.org/repo/hg/file/4.2.2/mercurial/changelog.py#l66
Pulkit Goyal - July 19, 2017, 2:30 p.m.
On Tue, Jul 18, 2017 at 7:02 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> On Tue, 18 Jul 2017 17:48:11 +0530, Pulkit Goyal wrote:
>> # HG changeset patch
>> # User Pulkit Goyal <7895pulkit@gmail.com>
>> # Date 1500378367 -19800
>> #      Tue Jul 18 17:16:07 2017 +0530
>> # Node ID 6cd19a6fd50016034d2faf7462bafdba4b90659e
>> # Parent  64137cfad7e30a459bc7fa5b6fc0e8757a6add8e
>> # EXP-Topic fbext
>> commitextras: make sure keys are non empty ASCII strings
>>
>> diff --git a/hgext/commitextras.py b/hgext/commitextras.py
>> --- a/hgext/commitextras.py
>> +++ b/hgext/commitextras.py
>> @@ -52,6 +52,11 @@
>>                                  "KEY=VALUE format")
>>                          raise error.Abort(msg % raw)
>>                      k, v = raw.split('=', 1)
>> +                    isascii = all(ord(c) < 128 for c in k)
>
> Better to reject non-word characters. Perhaps ascii_letters + digits + '_-'
> should be enough.
>
> changelog uses ':' as a separator.
>
> https://www.mercurial-scm.org/repo/hg/file/4.2.2/mercurial/changelog.py#l66

Can I still follow up on this?
Yuya Nishihara - July 20, 2017, 2:36 p.m.
On Wed, 19 Jul 2017 20:00:36 +0530, Pulkit Goyal wrote:
> On Tue, Jul 18, 2017 at 7:02 PM, Yuya Nishihara <yuya@tcha.org> wrote:
> > On Tue, 18 Jul 2017 17:48:11 +0530, Pulkit Goyal wrote:
> >> # HG changeset patch
> >> # User Pulkit Goyal <7895pulkit@gmail.com>
> >> # Date 1500378367 -19800
> >> #      Tue Jul 18 17:16:07 2017 +0530
> >> # Node ID 6cd19a6fd50016034d2faf7462bafdba4b90659e
> >> # Parent  64137cfad7e30a459bc7fa5b6fc0e8757a6add8e
> >> # EXP-Topic fbext
> >> commitextras: make sure keys are non empty ASCII strings
> >>
> >> diff --git a/hgext/commitextras.py b/hgext/commitextras.py
> >> --- a/hgext/commitextras.py
> >> +++ b/hgext/commitextras.py
> >> @@ -52,6 +52,11 @@
> >>                                  "KEY=VALUE format")
> >>                          raise error.Abort(msg % raw)
> >>                      k, v = raw.split('=', 1)
> >> +                    isascii = all(ord(c) < 128 for c in k)
> >
> > Better to reject non-word characters. Perhaps ascii_letters + digits + '_-'
> > should be enough.
> >
> > changelog uses ':' as a separator.
> >
> > https://www.mercurial-scm.org/repo/hg/file/4.2.2/mercurial/changelog.py#l66
> 
> Can I still follow up on this?

Maybe okay as long as it is considered a simple bug fix.

Patch

diff --git a/hgext/commitextras.py b/hgext/commitextras.py
--- a/hgext/commitextras.py
+++ b/hgext/commitextras.py
@@ -52,6 +52,11 @@ 
                                 "KEY=VALUE format")
                         raise error.Abort(msg % raw)
                     k, v = raw.split('=', 1)
+                    isascii = all(ord(c) < 128 for c in k)
+                    if not (k and isascii):
+                        msg = _("unable to parse '%s', keys must be non-empty"
+                                " ASCII words")
+                        raise error.Abort(msg % raw)
                     if k in usedinternally:
                         msg = _("key '%s' is used internally, can't be set "
                                 "manually")
diff --git a/tests/test-commit.t b/tests/test-commit.t
--- a/tests/test-commit.t
+++ b/tests/test-commit.t
@@ -141,6 +141,9 @@ 
   $ hg commit -m "adding internal used extras" --extra amend_source=hash
   abort: key 'amend_source' is used internally, can't be set manually
   [255]
+  $ hg commit -m "checking key format" --extra =nokeybutvalue
+  abort: unable to parse '=nokeybutvalue', keys must be non-empty ASCII words
+  [255]
 
 Make sure we do not obscure unknown requires file entries (issue2649)