Philip Martin wrote on Fri, Jul 08, 2011 at 19:30:02 +0100: > Daniel Shahaf <[email protected]> writes: > > > [email protected] wrote on Fri, Jul 08, 2011 at 14:02:42 -0000: > >> Author: philip > >> Date: Fri Jul 8 14:02:42 2011 > >> New Revision: 1144316 > >> > >> URL: http://svn.apache.org/viewvc?rev=1144316&view=rev > >> Log: > >> Fix issue 3953, mod_dav_svn failing to reject bogus mergeinfo on commit. > >> Move server-side mergeinfo validation so that all RA layers use it. > >> > > ... > >> @@ -222,6 +255,9 @@ svn_repos_fs_change_node_prop(svn_fs_roo > >> const svn_string_t *value, > >> apr_pool_t *pool) > >> { > >> + if (value && strcmp(name, SVN_PROP_MERGEINFO) == 0) > >> + SVN_ERR(verify_mergeinfo(value, path, pool)); > >> + > >> /* Validate the property, then call the wrapped function. */ > >> SVN_ERR(svn_repos__validate_prop(name, value, pool)); > >> return svn_fs_change_node_prop(root, path, name, value, pool); > >> > > > > Shouldn't the call to verify_mergeinfo() be made by > > svn_repos__validate_prop(), rather than here? > > Perhaps. svn_repos__validate_prop is also used by > svn_repos_fs_change_txn_props, is it appropriate to validate mergeinfo > there? >
Why not? No one should be setting svn:mergeinfo as a txnprop (or revprop). (and if they do, they shouldn't use the svn:* namespace) > We should probably split the validate function into two, one for node > props and one for revprops. > Given that we don't have any SVN_PROP_* whose semantics differ when it's set as a revprop v. when it's set as a nodeprop, I'm not sure what this would gain; I'm ±0. Unless, perhaps, you want to add verification that svn:mergeinfo isn't set as a revprop and svn:log isn't set as a nodeprop? Feel free to do so, but then I suggest you'll also teach svnsync et al to strip those (ill-set) properties to avoid breaking any repositories out there that do have svn:mergeinfo revprops and svn:log nodeprops. > -- > uberSVN: Apache Subversion Made Easy > http://www.uberSVN.com
