SafeConfigParser can set unsafe values

[Hamish Moffatt]

SafeConfigParser is supposed to be safer than ConfigParser, but calling
set with a string value containing '%' generates exceptions when you
get() it back.

Python 2.5.1 (r251:54863, Apr 25 2007, 21:31:46)
[GCC 4.1.2 20061115 (prerelease) (Debian 4.1.1-21)] on linux2
Type "help", "copyright", "credits" or "license" for more information.Traceback (most recent call last):
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "/usr/lib/python2.5/ConfigParser.py", line 525, in get
return self._interpolate(section, option, value, d)
File "/usr/lib/python2.5/ConfigParser.py", line 593, in _interpolate
self._interpolate_some(option, L, rawval, section, vars, 1)
File "/usr/lib/python2.5/ConfigParser.py", line 634, in _interpolate_some
"'%%' must be followed by '%%' or '(', found: %r" % (rest,))
ConfigParser.InterpolationSyntaxError: '%' must be followed by '%' or
'(', found: '%there'


ConfigParser does not do this:
'hi%there'


Should SafeConfigParser.set() be escaping automatically?

Hamish

 

[Matimus]

Should SafeConfigParser.set() be escaping automatically?

It seems like that would be a nice feature. However, may I offer up
that if you are setting an option and then later on getting that value
back in the same program, you probably should have used some other
storage mechanism in the first place. That is, you shouldn't store
values needed during the runtime of your program in a ConfigParser
instance.

As far as I can tell, these are the valid use cases for ConfigParser:

1. Use ConfigParser to read values from an config file
- This implies .read() followed by .get()s
2. Use ConfigParser to create and write a config file
- This implies .set()s followed by .write()
3. Use ConfigParser to read, modify and write a config file.
- This implies .read() followed by .get()s followed by .set()s
followed by .write()

None of the above use cases involve calling .get() after a .set().
Perhaps I am missing a use case though.

While I think you have technically pointed out a potential bug, I'm
not sure why it matters. Such a bug only comes about for (IMHO) flawed
use cases.

Matt

 

[Gabriel Genellina]

It seems like that would be a nice feature. However, may I offer up
that if you are setting an option and then later on getting that value
back in the same program, you probably should have used some other
storage mechanism in the first place. That is, you shouldn't store
values needed during the runtime of your program in a ConfigParser
instance.

As far as I can tell, these are the valid use cases for ConfigParser:

1. Use ConfigParser to read values from an config file
- This implies .read() followed by .get()s
2. Use ConfigParser to create and write a config file
- This implies .set()s followed by .write()
3. Use ConfigParser to read, modify and write a config file.
- This implies .read() followed by .get()s followed by .set()s
followed by .write()

None of the above use cases involve calling .get() after a .set().
Perhaps I am missing a use case though.

While I think you have technically pointed out a potential bug, I'm
not sure why it matters. Such a bug only comes about for (IMHO) flawed
use cases.

This not only happens when get() after a set(), but with all the use cases
above. An intervening write()/read() does not change things.
But I'm not sure it is a bug really. If all % were escaped automatically,
there is no way to write a templatized value. Maybe SafeConfigParser.set
should grow an escape argument, controlling whether one wants the value
escaped or not. For compatibility reasons should default to False, for
usability reasons should default to True.

 

[Hamish Moffatt]

It seems like that would be a nice feature. However, may I offer up
that if you are setting an option and then later on getting that value
back in the same program, you probably should have used some other
storage mechanism in the first place. That is, you shouldn't store
values needed during the runtime of your program in a ConfigParser
instance.

I agree, but that was a trivial example to demonstrate the problem.
Writing the file out to disk writes it exactly as set(), causing a get()
to fail just the same later.

While I think you have technically pointed out a potential bug, I'm
not sure why it matters. Such a bug only comes about for (IMHO) flawed
use cases.

Sorry, that's incorrect.


Hamish

 

[Matimus]

I agree, but that was a trivial example to demonstrate the problem.

Writing the file out to disk writes it exactly as set(), causing a get()
to fail just the same later.

No... The above statement is not true.

The following code:

from ConfigParser import *
import sys

cp = SafeConfigParser()
cp.add_section("sect")
cp.set("sect","opt","hello%world")

cp.write(sys.stdout)

Produces this output:
[sect]
opt = hello%world

The write method never calls get. However, when you read the file that
was output by the above code using .get(...) will raise an error. You
can avoid that error by setting the optional 'raw' parameter to True.

 

[Matimus]

This not only happens when get() after a set(), but with all the use cases

above. An intervening write()/read() does not change things.
But I'm not sure it is a bug really. If all % were escaped automatically,
there is no way to write a templatized value. Maybe SafeConfigParser.set
should grow an escape argument, controlling whether one wants the value
escaped or not. For compatibility reasons should default to False, for
usability reasons should default to True.

The exception is only raised when get is called, the "raw" paremeter
for get(...) is set to False (default) and the string value for that
parameter contains a single "%". None of the cases I stated above call
get() after calling set(). So, the exception will never be raised
because of something the user set. It will be raised if the input file
happens to have a single "%" character in one of the parameter values,
but that content could have been user generated, and it is not
reasonable to assume that fixing the set() method would have prevented
it.

Adding an escape parameter to set will not be used properly. Its
purpose would be to escape lone "%" characters, but if users are
wanting to use the substitution they would always keep escaping off.
It wouldn't allow them catch situations like this:
cp.set("sect","param","this is my value %(key)s and here is a lone %
and here is another %(sub)s", escape=False)

The solution I would propose is to raise an exception on set() if the
value contains a single "%" not followed by a key name enclosed in
parens followed by "s". That puts the burden of escaping on the user,
before passing it to set.

 

[Matimus]

This not only happens when get() after a set(), but with all the use cases

above. An intervening write()/read() does not change things.
But I'm not sure it is a bug really. If all % were escaped automatically,
there is no way to write a templatized value. Maybe SafeConfigParser.set
should grow an escape argument, controlling whether one wants the value
escaped or not. For compatibility reasons should default to False, for
usability reasons should default to True.

The exception is only raised when get is called, the "raw" paremeter
for get(...) is set to False (default) and the string value for that
parameter contains a single "%". None of the cases I stated above call
get() after calling set(). So, the exception will never be raised
because of something the user set. It will be raised if the input file
happens to have a single "%" character in one of the parameter values,
but that content could have been user generated, and it is not
reasonable to assume that fixing the set() method would have prevented
it.

Adding an escape parameter to set will not be used properly. Its
purpose would be to escape lone "%" characters, but if users are
wanting to use the substitution they would always keep escaping off.
It wouldn't allow them catch situations like this:
cp.set("sect","param","this is my value %(key)s and here is a lone %
and here is another %(sub)s", escape=False)

The solution I would propose is to raise an exception on set() if the
value contains a single "%" not followed by a key name enclosed in
parens followed by "s". That puts the burden of escaping on the user,
before passing it to set.

 

[Hamish Moffatt]

No... The above statement is not true.

Yes, it is. Whatever you set gets written out directly. Your example
proves this:

cp.set("sect","opt","hello%world")
cp.write(sys.stdout)
[/code]

Produces this output:
[sect]
opt = hello%world

Then when you get() this value later, it fails.

The write method never calls get. However, when you read the file that
was output by the above code using .get(...) will raise an error. You
can avoid that error by setting the optional 'raw' parameter to True.

But then you have disabled substitution, which is not the same thing! I
don't necessarily want to disable substitution, I just want transparent
handling of lone %s.

Since SafeConfigParser.get() is fussy about the format of interpolation
instructions, SafeConfigParser.set() can safely know when you're not
trying to use them and escape lone percents.

To summarise: set() should not set something which get() will ALWAYS
fail on!


Hamish