At file:///srv/pqm.bazaar-vcs.org/archives/thelove/bzr/%2Btrunk/
------------------------------------------------------------
revno: 6589 [merge]
revision-id: p...@pqm.ubuntu.com-20131007170434-mb0ahksmrzsnhi1i
parent: p...@pqm.ubuntu.com-20131004093729-to7tbu9w6ws03wge
parent: v.ladeuil...@free.fr-20131007163559-28eg2hyu6m2pbuug
committer: Patch Queue Manager <p...@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
*************
--
bazaar-commits mailing list
bazaar-commits@lists.canonical.com
https://lists.ubuntu.com/mailman/listinfo/bazaar-commits
