Rev 6589: Stricter checks on configuration option names in file:///home/vila/src/bzr/bugs/1235099-illegal-option-names/
Vincent Ladeuil
v.ladeuil+lp at free.fr
Fri Oct 4 09:56:24 UTC 2013
At file:///home/vila/src/bzr/bugs/1235099-illegal-option-names/
------------------------------------------------------------
revno: 6589
revision-id: v.ladeuil+lp at free.fr-20131004095623-xlan34vg0y51gdb5
parent: v.ladeuil+lp at free.fr-20131004071312-9c5pfxzzxjm7xrvn
fixes bug: https://launchpad.net/bugs/1235099
committer: Vincent Ladeuil <v.ladeuil+lp at free.fr>
branch nick: 1235099-illegal-option-names
timestamp: Fri 2013-10-04 11:56:23 +0200
message:
Stricter checks on configuration option names
-------------- next part --------------
=== 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 2013-10-04 07:13:12 +0000
+++ b/bzrlib/tests/test_config.py 2013-10-04 09:56:23 +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'
--- a/doc/en/release-notes/bzr-2.7.txt 2013-07-27 12:53:28 +0000
+++ b/doc/en/release-notes/bzr-2.7.txt 2013-10-04 09:56:23 +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