Rev 6589: (vila) Stricter checks on configuration option names (Vincent Ladeuil) in file:///srv/pqm.bazaar-vcs.org/archives/thelove/bzr/%2Btrunk/

Patch Queue Manager pqm at pqm.ubuntu.com
Mon Oct 7 17:04:35 UTC 2013


At file:///srv/pqm.bazaar-vcs.org/archives/thelove/bzr/%2Btrunk/

------------------------------------------------------------
revno: 6589 [merge]
revision-id: pqm at pqm.ubuntu.com-20131007170434-mb0ahksmrzsnhi1i
parent: pqm at pqm.ubuntu.com-20131004093729-to7tbu9w6ws03wge
parent: v.ladeuil+lp at free.fr-20131007163559-28eg2hyu6m2pbuug
committer: Patch Queue Manager <pqm at pqm.ubuntu.com>
branch nick: +trunk
timestamp: Mon 2013-10-07 17:04:34 +0000
message:
  (vila) Stricter checks on configuration option names (Vincent Ladeuil)
modified:
  bzrlib/config.py               config.py-20051011043216-070c74f4e9e338e8
  bzrlib/errors.py               errors.py-20050309040759-20512168c4e14fbd
  bzrlib/tests/test_config.py    testconfig.py-20051011041908-742d0c15d8d8c8eb
  doc/en/release-notes/bzr-2.7.txt bzr2.7.txt-20130727124539-wnx897hy9l2h9f7x-1
=== modified file 'bzrlib/config.py'
--- a/bzrlib/config.py	2013-08-09 15:09:23 +0000
+++ b/bzrlib/config.py	2013-10-04 09:56:23 +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'
--- a/bzrlib/errors.py	2013-05-23 10:04:17 +0000
+++ b/bzrlib/errors.py	2013-10-04 09:56:23 +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'
--- a/bzrlib/tests/test_config.py	2012-08-01 14:05:11 +0000
+++ b/bzrlib/tests/test_config.py	2013-10-04 09:56:23 +0000
@@ -16,7 +16,6 @@
 
 """Tests for finding and reading the bzr config file[s]."""
 
-import base64
 from cStringIO import StringIO
 from textwrap import dedent
 import os
@@ -629,7 +628,7 @@
 class TestIniConfigBuilding(TestIniConfig):
 
     def test_contructs(self):
-        my_config = config.IniBasedConfig()
+        config.IniBasedConfig()
 
     def test_from_fp(self):
         my_config = config.IniBasedConfig.from_string(sample_config_text)
@@ -678,8 +677,8 @@
 
     def test_saved_with_content(self):
         content = 'foo = bar\n'
-        conf = config.IniBasedConfig.from_string(
-            content, file_name='./test.conf', save=True)
+        config.IniBasedConfig.from_string(content, file_name='./test.conf',
+                                          save=True)
         self.assertFileEqual(content, 'test.conf')
 
 
@@ -1047,7 +1046,7 @@
 class TestGetConfig(tests.TestCase):
 
     def test_constructs(self):
-        my_config = config.GlobalConfig()
+        config.GlobalConfig()
 
     def test_calls_read_filenames(self):
         # replace the class that is constructed, to check its parameters
@@ -1065,9 +1064,12 @@
 
 class TestBranchConfig(tests.TestCaseWithTransport):
 
-    def test_constructs(self):
+    def test_constructs_valid(self):
         branch = FakeBranch()
         my_config = config.BranchConfig(branch)
+        self.assertIsNot(None, my_config)
+
+    def test_constructs_error(self):
         self.assertRaises(TypeError, config.BranchConfig)
 
     def test_get_location_config(self):
@@ -1105,6 +1107,7 @@
         conf = config.LocationConfig.from_string(
             '[%s]\nnickname = foobar' % (local_url,),
             local_url, save=True)
+        self.assertIsNot(None, conf)
         self.assertEqual('foobar', branch.nick)
 
     def test_config_local_path(self):
@@ -1113,9 +1116,10 @@
         self.assertEqual('branch', branch.nick)
 
         local_path = osutils.getcwd().encode('utf8')
-        conf = config.LocationConfig.from_string(
+        config.LocationConfig.from_string(
             '[%s/branch]\nnickname = barry' % (local_path,),
             'branch',  save=True)
+        # Now the branch will find its nick via the location config
         self.assertEqual('barry', branch.nick)
 
     def test_config_creates_local(self):
@@ -1369,8 +1373,10 @@
 
 class TestLocationConfig(tests.TestCaseInTempDir, TestOptionsMixin):
 
-    def test_constructs(self):
-        my_config = config.LocationConfig('http://example.com')
+    def test_constructs_valid(self):
+        config.LocationConfig('http://example.com')
+
+    def test_constructs_error(self):
         self.assertRaises(TypeError, config.LocationConfig)
 
     def test_branch_calls_read_filenames(self):
@@ -1662,10 +1668,9 @@
         if location_config is None:
             location_config = sample_branches_text
 
-        my_global_config = config.GlobalConfig.from_string(global_config,
-                                                           save=True)
-        my_location_config = config.LocationConfig.from_string(
-            location_config, my_branch.base, save=True)
+        config.GlobalConfig.from_string(global_config, save=True)
+        config.LocationConfig.from_string(location_config, my_branch.base,
+                                          save=True)
         my_config = config.BranchConfig(my_branch)
         self.my_config = my_config
         self.my_location_config = my_config._get_location_config()
@@ -1736,11 +1741,10 @@
                           location_config=None, branch_data_config=None):
         my_branch = FakeBranch(location)
         if global_config is not None:
-            my_global_config = config.GlobalConfig.from_string(global_config,
-                                                               save=True)
+            config.GlobalConfig.from_string(global_config, save=True)
         if location_config is not None:
-            my_location_config = config.LocationConfig.from_string(
-                location_config, my_branch.base, save=True)
+            config.LocationConfig.from_string(location_config, my_branch.base,
+                                              save=True)
         my_config = config.BranchConfig(my_branch)
         if branch_data_config is not None:
             my_config.branch.control_files.files['branch.conf'] = \
@@ -2220,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):
@@ -2447,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):
@@ -2459,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."""
@@ -3320,7 +3381,7 @@
 
     def test_build_doesnt_load_store(self):
         store = self.get_store(self)
-        matcher = self.matcher(store, '/bar')
+        self.matcher(store, '/bar')
         self.assertFalse(store.is_loaded())
 
 
@@ -3461,16 +3522,16 @@
 
     def test_url_vs_local_paths(self):
         # The matcher location is an url and the section names are local paths
-        sections = self.assertSectionIDs(['/foo/bar', '/foo'],
-                                         'file:///foo/bar/baz', '''\
+        self.assertSectionIDs(['/foo/bar', '/foo'],
+                              'file:///foo/bar/baz', '''\
 [/foo]
 [/foo/bar]
 ''')
 
     def test_local_path_vs_url(self):
         # The matcher location is a local path and the section names are urls
-        sections = self.assertSectionIDs(['file:///foo/bar', 'file:///foo'],
-                                         '/foo/bar/baz', '''\
+        self.assertSectionIDs(['file:///foo/bar', 'file:///foo'],
+                              '/foo/bar/baz', '''\
 [file:///foo]
 [file:///foo/bar]
 ''')
@@ -3693,7 +3754,7 @@
 
     def test_build_stack(self):
         # Just a smoke test to help debug builders
-        stack = self.get_stack(self)
+        self.get_stack(self)
 
 
 class TestStackGet(TestStackWithTransport):
@@ -3920,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
@@ -4297,7 +4363,7 @@
         """
         sections = list(conf._get_sections(name))
         self.assertLength(len(expected), sections)
-        self.assertEqual(expected, [name for name, _, _ in sections])
+        self.assertEqual(expected, [n for n, _, _ in sections])
 
     def test_bazaar_default_section(self):
         self.assertSectionNames(['DEFAULT'], self.bazaar_config)

=== modified file 'doc/en/release-notes/bzr-2.7.txt'
--- a/doc/en/release-notes/bzr-2.7.txt	2013-10-01 13:13:46 +0000
+++ b/doc/en/release-notes/bzr-2.7.txt	2013-10-07 16:35:59 +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
 *************
 




More information about the bazaar-commits mailing list