Bazaar

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

  1. 1235099-illegal-option-names
  2. Merge into bzr.dev
Proposed by Vincent Ladeuil
Status: Merged
Approved by: Vincent Ladeuil
Approved revision: no longer in the source branch.
Merged at revision: 6589
Proposed branch: lp:~vila/bzr/1235099-illegal-option-names
Merge into: lp:bzr
Prerequisite: lp:~vila/bzr/1235099-prereq
Diff against target: 214 lines (+102/-17)
4 files modified
bzrlib/config.py (+27/-16)
bzrlib/errors.py (+9/-1)
bzrlib/tests/test_config.py (+62/-0)
doc/en/release-notes/bzr-2.7.txt (+4/-0)
To merge this branch: bzr merge lp:~vila/bzr/1235099-illegal-option-names
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+189262@code.launchpad.net

Commit message

Stricter checks on configuration option names.

Description of the change

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.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

Seems like a good change to me.

review: Approve
Revision history for this message
getriebesand (getriebesand) wrote :

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?

Revision history for this message
dreamcat4 (dreamcat4) wrote :

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"

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/config.py'
2--- bzrlib/config.py 2013-08-09 15:09:23 +0000
3+++ bzrlib/config.py 2013-10-04 10:33:57 +0000
4@@ -2552,6 +2552,23 @@
5 return "".join(ret)
6
7
8+_option_ref_re = lazy_regex.lazy_compile('({[^\d\W](?:\.\w|\w)*})')
9+"""Describes an expandable option reference.
10+
11+We want to match the most embedded reference first.
12+
13+I.e. for '{{foo}}' we will get '{foo}',
14+for '{bar{baz}}' we will get '{baz}'
15+"""
16+
17+def iter_option_refs(string):
18+ # Split isolate refs so every other chunk is a ref
19+ is_ref = False
20+ for chunk in _option_ref_re.split(string):
21+ yield is_ref, chunk
22+ is_ref = not is_ref
23+
24+
25 class OptionRegistry(registry.Registry):
26 """Register config options by their name.
27
28@@ -2559,11 +2576,20 @@
29 some information from the option object itself.
30 """
31
32+ def _check_option_name(self, option_name):
33+ """Ensures an option name is valid.
34+
35+ :param option_name: The name to validate.
36+ """
37+ if _option_ref_re.match('{%s}' % option_name) is None:
38+ raise errors.IllegalOptionName(option_name)
39+
40 def register(self, option):
41 """Register a new option to its name.
42
43 :param option: The option to register. Its name is used as the key.
44 """
45+ self._check_option_name(option.name)
46 super(OptionRegistry, self).register(option.name, option,
47 help=option.help)
48
49@@ -2578,6 +2604,7 @@
50 :param member_name: the member of the module to return. If empty or
51 None, get() will return the module itself.
52 """
53+ self._check_option_name(key)
54 super(OptionRegistry, self).register_lazy(key,
55 module_name, member_name)
56
57@@ -3622,22 +3649,6 @@
58 yield self.store, section
59
60
61-_option_ref_re = lazy_regex.lazy_compile('({[^{}\n]+})')
62-"""Describes an expandable option reference.
63-
64-We want to match the most embedded reference first.
65-
66-I.e. for '{{foo}}' we will get '{foo}',
67-for '{bar{baz}}' we will get '{baz}'
68-"""
69-
70-def iter_option_refs(string):
71- # Split isolate refs so every other chunk is a ref
72- is_ref = False
73- for chunk in _option_ref_re.split(string):
74- yield is_ref, chunk
75- is_ref = not is_ref
76-
77 # FIXME: _shared_stores should be an attribute of a library state once a
78 # library_state object is always available.
79 _shared_stores = {}
80
81=== modified file 'bzrlib/errors.py'
82--- bzrlib/errors.py 2013-05-23 10:04:17 +0000
83+++ bzrlib/errors.py 2013-10-04 10:33:57 +0000
84@@ -3250,13 +3250,21 @@
85
86 class ExpandingUnknownOption(BzrError):
87
88- _fmt = 'Option %(name)s is not defined while expanding "%(string)s".'
89+ _fmt = 'Option "%(name)s" is not defined while expanding "%(string)s".'
90
91 def __init__(self, name, string):
92 self.name = name
93 self.string = string
94
95
96+class IllegalOptionName(BzrError):
97+
98+ _fmt = 'Option "%(name)s" is not allowed.'
99+
100+ def __init__(self, name):
101+ self.name = name
102+
103+
104 class NoCompatibleInter(BzrError):
105
106 _fmt = ('No compatible object available for operations from %(source)r '
107
108=== modified file 'bzrlib/tests/test_config.py'
109--- bzrlib/tests/test_config.py 2013-10-04 10:33:57 +0000
110+++ bzrlib/tests/test_config.py 2013-10-04 10:33:57 +0000
111@@ -2224,6 +2224,44 @@
112 self.assertSaveHook(remote_bzrdir._get_config())
113
114
115+class TestOptionNames(tests.TestCase):
116+
117+ def is_valid(self, name):
118+ return config._option_ref_re.match('{%s}' % name) is not None
119+
120+ def test_valid_names(self):
121+ self.assertTrue(self.is_valid('foo'))
122+ self.assertTrue(self.is_valid('foo.bar'))
123+ self.assertTrue(self.is_valid('f1'))
124+ self.assertTrue(self.is_valid('_'))
125+ self.assertTrue(self.is_valid('__bar__'))
126+ self.assertTrue(self.is_valid('a_'))
127+ self.assertTrue(self.is_valid('a1'))
128+
129+ def test_invalid_names(self):
130+ self.assertFalse(self.is_valid(' foo'))
131+ self.assertFalse(self.is_valid('foo '))
132+ self.assertFalse(self.is_valid('1'))
133+ self.assertFalse(self.is_valid('1,2'))
134+ self.assertFalse(self.is_valid('foo$'))
135+ self.assertFalse(self.is_valid('!foo'))
136+ self.assertFalse(self.is_valid('foo.'))
137+ self.assertFalse(self.is_valid('foo..bar'))
138+ self.assertFalse(self.is_valid('{}'))
139+ self.assertFalse(self.is_valid('{a}'))
140+ self.assertFalse(self.is_valid('a\n'))
141+
142+ def assertSingleGroup(self, reference):
143+ # the regexp is used with split and as such should match the reference
144+ # *only*, if more groups needs to be defined, (?:...) should be used.
145+ m = config._option_ref_re.match('{a}')
146+ self.assertLength(1, m.groups())
147+
148+ def test_valid_references(self):
149+ self.assertSingleGroup('{a}')
150+ self.assertSingleGroup('{{a}}')
151+
152+
153 class TestOption(tests.TestCase):
154
155 def test_default_value(self):
156@@ -2451,6 +2489,12 @@
157 self.registry.register(opt)
158 self.assertEquals('A simple option', self.registry.get_help('foo'))
159
160+ def test_dont_register_illegal_name(self):
161+ self.assertRaises(errors.IllegalOptionName,
162+ self.registry.register, config.Option(' foo'))
163+ self.assertRaises(errors.IllegalOptionName,
164+ self.registry.register, config.Option('bar,'))
165+
166 lazy_option = config.Option('lazy_foo', help='Lazy help')
167
168 def test_register_lazy(self):
169@@ -2463,6 +2507,19 @@
170 'TestOptionRegistry.lazy_option')
171 self.assertEquals('Lazy help', self.registry.get_help('lazy_foo'))
172
173+ def test_dont_lazy_register_illegal_name(self):
174+ # This is where the root cause of http://pad.lv/1235099 is better
175+ # understood: 'register_lazy' doc string mentions that key should match
176+ # the option name which indirectly requires that the option name is a
177+ # valid python identifier. We violate that rule here (using a key that
178+ # doesn't match the option name) to test the option name checking.
179+ self.assertRaises(errors.IllegalOptionName,
180+ self.registry.register_lazy, ' foo', self.__module__,
181+ 'TestOptionRegistry.lazy_option')
182+ self.assertRaises(errors.IllegalOptionName,
183+ self.registry.register_lazy, '1,2', self.__module__,
184+ 'TestOptionRegistry.lazy_option')
185+
186
187 class TestRegisteredOptions(tests.TestCase):
188 """All registered options should verify some constraints."""
189@@ -3924,6 +3981,11 @@
190 self.assertRaises(errors.ExpandingUnknownOption,
191 self.conf.expand_options, '{foo}')
192
193+ def test_illegal_def_is_ignored(self):
194+ self.assertExpansion('{1,2}', '{1,2}')
195+ self.assertExpansion('{ }', '{ }')
196+ self.assertExpansion('${Foo,f}', '${Foo,f}')
197+
198 def test_indirect_ref(self):
199 self.conf.store._load_from_string('''
200 foo=xxx
201
202=== modified file 'doc/en/release-notes/bzr-2.7.txt'
203--- doc/en/release-notes/bzr-2.7.txt 2013-10-01 13:13:46 +0000
204+++ doc/en/release-notes/bzr-2.7.txt 2013-10-04 10:33:57 +0000
205@@ -32,6 +32,10 @@
206 .. Fixes for situations where bzr would previously crash or give incorrect
207 or undesirable results.
208
209+* Option names are now checked to be valid [dotted] python identifiers. Also
210+ ignore invalid references (i.e. using invalid option names) while
211+ expanding option values. (Vincent Ladeuil, #1235099)
212+
213 Documentation
214 *************
215