wierd behaviour for some custom property values from force build page

[rmdugal]

On the force build page I enter a custom property called 'test'. If I assign an integer or text value containing no spaces then the force build button works as it should. However, if I enter a value of 'this is a test', with or without quotes, then the force build button instead sends me to a list of builders page and the build is not started. Here are some values for the custom property that work:
1234
foo
-foo
/foo

Here are some values for the custom property that break the force build, sending me to list of builders page:
foo boo
'foo boo'
"foo"
\foo
-foo -boo
foo*
foo,
foo=boo

Obviously some of these property values wouldn't make sense. However I don't see why I cannot enter a custom property/value pair like:
TEST_OPTIONS = -A -b -CD

[dustin]

Here's the offending code:

def getAndCheckProperties(req):                                                                         
    """                                                                                                 
Fetch custom build properties from the HTTP request of a "Force build" or                               
"Resubmit build" HTML form.                                                                             
Check the names for valid strings, and return None if a problem is found.                               
Return a new Properties object containing each property found in req.                                   
"""                                                                                                     
    properties = Properties()                                                                           
    i = 1                                                                                               
    while True:                                                                                         
        pname = req.args.get("property%dname" % i, [""])[0]                                             
        pvalue = req.args.get("property%dvalue" % i, [""])[0]                                           
        if not pname or not pvalue:                                                                     
            break                                                                                       
        if not re.match(r'^[\w\.\-\/\~:]*$', pname) \                                                   
                or not re.match(r'^[\w\.\-\/\~:]*$', pvalue):                                           
            log.msg("bad property name='%s', value='%s'" % (pname, pvalue))                             
            return None                                                                                 
        properties.setProperty(pname, pvalue, "Force Build Form")                                       
        i = i + 1                                                                                       
                                                                                                        
    return properties                                                                                   

Obviously this is somewhat security-sensitive, but the use of some regexes like this is probably not the best solution. I'm sure there are harmful values that *could* fit through this filter, and you've listed one of many useful values that does not fit through it.

I'm open to suggestions as to how this should be handled. I'm tempted to just scrap the validation entirely, on the theory that no validation technique is going to be universally useful to the various projects that are built by Buildbot. Thoughts?

[dustin]

Jc2k suggests adding a property-validation callback to the WebStatus? constructor, where the default uses the regexes in getAndCheckProperties.

Is that something you might be able to cook up and test?

[dustin]

I don't see a good solution here, and I'm loathe to just remove the checks, lest that inadvertently expose installations to compromise. BUMP!

[dustin]

..and bump again.

[ayust]

Probably the simplest and most straightforward solution to this for now is to just add a configuration option for a different regex to use, so that people can override it as long as they're aware of what's going on. Caveat emptor.

[Dustin J. Mitchell]

Allow users to customize the sanity-checking regexes

This doesn't add security, but at least allows users to open up the regexes where it's safe to do so. Fixes #985.

Tested by hand.

Changeset: cea9873e7fbe793720e55043c1c7fc4aebff0bd9