Philip Martin wrote on Fri, Jul 08, 2011 at 19:30:02 +0100:
> Daniel Shahaf <d.s_at_daniel.shahaf.name> writes:
>
> > philip_at_apache.org 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
Received on 2011-07-08 20:44:11 CEST