Merge lp:~vila/bzr/1235099-illegal-option-names into lp:bzr

[Vincent Ladeuil]

Stricter checks on configuration option names.

There are several use case where config values expansion fails in unhelpful
ways:

- {1,2} as reported in http://pad.lv/1235099

- When templates are used to produce bash scripts (though that case could be
  better addressed by allowing some escape mechanism, I'd rather fix that as
  a separate bug).

Unfortunately for readability, I had to move the regexp definition so here
are the old and new ones for clarity:

-_option_ref_re = lazy_regex.lazy_compile('({[^{}\n]+})')
+_option_ref_re = lazy_regex.lazy_compile('({[^\d\W](?:\.\w|\w)*})')

I.e. valid python identifier (including dotted ones).

As the bug demonstrate we were allowing some illegal option names in the option values and were giving bad error reporting.

This proposal doesn't change which option names are legal (only invalid references could be used before and led to failures) but give better error messages.

I've made a few cleanup changes in lp:~vila/bzr/1235099-prereq which don't seem worth presenting here.

[John A Meinel (jameinel)]

Seems like a good change to me.

review: Approve

[getriebesand (getriebesand)]

Hi, after that change bzr-svn raise an exception:

Option "guessed-layout" is not allowed.
bzr: ERROR: exceptions.TypeError: 'NoneType' object is not callable

Your regex doesnt support - in option names.

When I modify the regex to:
_option_ref_re = lazy_regex.lazy_compile('({[^\d\W](?:\.\w|\w|-\w)*})')

it seems to fix that. But I am no regex or python expert. Can you have a look please?

[dreamcat4 (dreamcat4)]

Me too.
With bzr-svn v1.2.3, from here: https://www.samba.org/~jelmer/bzr/

id@emachines-e520:~/.bazaar/plugins/svn$ bzr svn
Option "guessed-layout" is not allowed.
bzr: ERROR: unknown command "svn"

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2013-08-09 15:09:23 +0000
+++ bzrlib/config.py	2013-10-04 10:33:57 +0000
@@ -2552,6 +2552,23 @@
         return "".join(ret)
 
 
+_option_ref_re = lazy_regex.lazy_compile('({[^\d\W](?:\.\w|\w)*})')
+"""Describes an expandable option reference.
+
+We want to match the most embedded reference first.
+
+I.e. for '{{foo}}' we will get '{foo}',
+for '{bar{baz}}' we will get '{baz}'
+"""
+
+def iter_option_refs(string):
+    # Split isolate refs so every other chunk is a ref
+    is_ref = False
+    for chunk  in _option_ref_re.split(string):
+        yield is_ref, chunk
+        is_ref = not is_ref
+
+
 class OptionRegistry(registry.Registry):
     """Register config options by their name.
 
@@ -2559,11 +2576,20 @@
     some information from the option object itself.
     """
 
+    def _check_option_name(self, option_name):
+        """Ensures an option name is valid.
+
+        :param option_name: The name to validate.
+        """
+        if _option_ref_re.match('{%s}' % option_name) is None:
+            raise errors.IllegalOptionName(option_name)
+
     def register(self, option):
         """Register a new option to its name.
 
         :param option: The option to register. Its name is used as the key.
         """
+        self._check_option_name(option.name)
         super(OptionRegistry, self).register(option.name, option,
                                              help=option.help)
 
@@ -2578,6 +2604,7 @@
         :param member_name: the member of the module to return.  If empty or 
                 None, get() will return the module itself.
         """
+        self._check_option_name(key)
         super(OptionRegistry, self).register_lazy(key,
                                                   module_name, member_name)
 
@@ -3622,22 +3649,6 @@
             yield self.store, section
 
 
-_option_ref_re = lazy_regex.lazy_compile('({[^{}\n]+})')
-"""Describes an expandable option reference.
-
-We want to match the most embedded reference first.
-
-I.e. for '{{foo}}' we will get '{foo}',
-for '{bar{baz}}' we will get '{baz}'
-"""
-
-def iter_option_refs(string):
-    # Split isolate refs so every other chunk is a ref
-    is_ref = False
-    for chunk  in _option_ref_re.split(string):
-        yield is_ref, chunk
-        is_ref = not is_ref
-
 # FIXME: _shared_stores should be an attribute of a library state once a
 # library_state object is always available.
 _shared_stores = {}
 
=== modified file 'bzrlib/errors.py'
--- bzrlib/errors.py	2013-05-23 10:04:17 +0000
+++ bzrlib/errors.py	2013-10-04 10:33:57 +0000
@@ -3250,13 +3250,21 @@
 
 class ExpandingUnknownOption(BzrError):
 
-    _fmt = 'Option %(name)s is not defined while expanding "%(string)s".'
+    _fmt = 'Option "%(name)s" is not defined while expanding "%(string)s".'
 
     def __init__(self, name, string):
         self.name = name
         self.string = string
 
 
+class IllegalOptionName(BzrError):
+
+    _fmt = 'Option "%(name)s" is not allowed.'
+
+    def __init__(self, name):
+        self.name = name
+
+
 class NoCompatibleInter(BzrError):
 
     _fmt = ('No compatible object available for operations from %(source)r '
 
=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2013-10-04 10:33:57 +0000
+++ bzrlib/tests/test_config.py	2013-10-04 10:33:57 +0000
@@ -2224,6 +2224,44 @@
         self.assertSaveHook(remote_bzrdir._get_config())
 
 
+class TestOptionNames(tests.TestCase):
+
+    def is_valid(self, name):
+        return config._option_ref_re.match('{%s}' % name) is not None
+
+    def test_valid_names(self):
+        self.assertTrue(self.is_valid('foo'))
+        self.assertTrue(self.is_valid('foo.bar'))
+        self.assertTrue(self.is_valid('f1'))
+        self.assertTrue(self.is_valid('_'))
+        self.assertTrue(self.is_valid('__bar__'))
+        self.assertTrue(self.is_valid('a_'))
+        self.assertTrue(self.is_valid('a1'))
+
+    def test_invalid_names(self):
+        self.assertFalse(self.is_valid(' foo'))
+        self.assertFalse(self.is_valid('foo '))
+        self.assertFalse(self.is_valid('1'))
+        self.assertFalse(self.is_valid('1,2'))
+        self.assertFalse(self.is_valid('foo$'))
+        self.assertFalse(self.is_valid('!foo'))
+        self.assertFalse(self.is_valid('foo.'))
+        self.assertFalse(self.is_valid('foo..bar'))
+        self.assertFalse(self.is_valid('{}'))
+        self.assertFalse(self.is_valid('{a}'))
+        self.assertFalse(self.is_valid('a\n'))
+
+    def assertSingleGroup(self, reference):
+        # the regexp is used with split and as such should match the reference
+        # *only*, if more groups needs to be defined, (?:...) should be used.
+        m = config._option_ref_re.match('{a}')
+        self.assertLength(1, m.groups())
+
+    def test_valid_references(self):
+        self.assertSingleGroup('{a}')
+        self.assertSingleGroup('{{a}}')
+
+
 class TestOption(tests.TestCase):
 
     def test_default_value(self):
@@ -2451,6 +2489,12 @@
         self.registry.register(opt)
         self.assertEquals('A simple option', self.registry.get_help('foo'))
 
+    def test_dont_register_illegal_name(self):
+        self.assertRaises(errors.IllegalOptionName,
+                          self.registry.register, config.Option(' foo'))
+        self.assertRaises(errors.IllegalOptionName,
+                          self.registry.register, config.Option('bar,'))
+
     lazy_option = config.Option('lazy_foo', help='Lazy help')
 
     def test_register_lazy(self):
@@ -2463,6 +2507,19 @@
                                     'TestOptionRegistry.lazy_option')
         self.assertEquals('Lazy help', self.registry.get_help('lazy_foo'))
 
+    def test_dont_lazy_register_illegal_name(self):
+        # This is where the root cause of http://pad.lv/1235099 is better
+        # understood: 'register_lazy' doc string mentions that key should match
+        # the option name which indirectly requires that the option name is a
+        # valid python identifier. We violate that rule here (using a key that
+        # doesn't match the option name) to test the option name checking.
+        self.assertRaises(errors.IllegalOptionName,
+                          self.registry.register_lazy, ' foo', self.__module__,
+                          'TestOptionRegistry.lazy_option')
+        self.assertRaises(errors.IllegalOptionName,
+                          self.registry.register_lazy, '1,2', self.__module__,
+                          'TestOptionRegistry.lazy_option')
+
 
 class TestRegisteredOptions(tests.TestCase):
     """All registered options should verify some constraints."""
@@ -3924,6 +3981,11 @@
         self.assertRaises(errors.ExpandingUnknownOption,
                           self.conf.expand_options, '{foo}')
 
+    def test_illegal_def_is_ignored(self):
+        self.assertExpansion('{1,2}', '{1,2}')
+        self.assertExpansion('{ }', '{ }')
+        self.assertExpansion('${Foo,f}', '${Foo,f}')
+
     def test_indirect_ref(self):
         self.conf.store._load_from_string('''
 foo=xxx
 
=== modified file 'doc/en/release-notes/bzr-2.7.txt'
--- doc/en/release-notes/bzr-2.7.txt	2013-10-01 13:13:46 +0000
+++ doc/en/release-notes/bzr-2.7.txt	2013-10-04 10:33:57 +0000
@@ -32,6 +32,10 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* Option names are now checked to be valid [dotted] python identifiers. Also
+  ignore invalid references (i.e. using invalid option names) while
+  expanding option values. (Vincent Ladeuil, #1235099)
+
 Documentation
 *************