You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@subversion.apache.org by ph...@apache.org on 2011/11/18 10:09:56 UTC
svn commit: r1203546 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Author: philip
Date: Fri Nov 18 09:09:56 2011
New Revision: 1203546
URL: http://svn.apache.org/viewvc?rev=1203546&view=rev
Log:
Nested Locations (which are not used very often and only make sense
inside SVNParentPath) have inherited the parent's root_dir since the
code was written in r862059. I can see no sense in using the
parent's value and the few features that that rely on root_dir don't
appear to work when the nested Locations are used.
This change fixes v2 POST and write-through mirroring when nested
Locations are used.
Also, a nested Location inside an SVNPath Location is ambiguous so
prevent access and log the problem.
* subversion/mod_dav_svn/mod_dav_svn.c
(merge_dir_config): Prefer the child root_dir, detect/log invalid
nesting, prevent access to invalid nesting repositories.
Modified:
subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Modified: subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
URL: http://svn.apache.org/viewvc/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c?rev=1203546&r1=1203545&r2=1203546&view=diff
==============================================================================
--- subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c (original)
+++ subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c Fri Nov 18 09:09:56 2011
@@ -215,8 +215,18 @@ merge_dir_config(apr_pool_t *p, void *ba
newconf->list_parentpath = INHERIT_VALUE(parent, child, list_parentpath);
newconf->txdelta_cache = INHERIT_VALUE(parent, child, txdelta_cache);
newconf->fulltext_cache = INHERIT_VALUE(parent, child, fulltext_cache);
- /* Prefer our parent's value over our new one - hence the swap. */
- newconf->root_dir = INHERIT_VALUE(child, parent, root_dir);
+ newconf->root_dir = INHERIT_VALUE(parent, child, root_dir);
+
+ if (parent->fs_path)
+ {
+ /* Nesting inside SVNPath is ambiguous so prevent access. */
+ newconf->fs_path = newconf->fs_parent_path = NULL;
+
+ ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL,
+ "mod_dav_svn: invalid nested Location '%s' inside "
+ "SVNPath Location '%s'",
+ child->root_dir, parent->root_dir);
+ }
return newconf;
}
Re: svn commit: r1203546 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Nov 18, 2011 at 03:07:11PM +0100, Stefan Sperling wrote:
> On Fri, Nov 18, 2011 at 02:52:44PM +0100, Stefan Sperling wrote:
> > Suggestion (not tested);
> >
> > ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
> > "mod_dav_svn: nested Location '%s' inside "
> > "SVNPath Location '%s'; this hinders access to a file "
> > "or directory '%s' inside the repository '%s'",
> > child->root_dir, parent->root_dir,
> > svn_dirent_basename(child->root_dir, scratch_pool),
> > parent->fs_path);
>
> Better suggestion.
And a new patch, based on your r1203651 commit:
[[[
Follow-up to r1203651:
* subversion/mod_dav_svn/mod_dav_svn.c
(merge_dir_config): Use svn__fspath instead of svn_dirent for URL-paths.
Correctly print Locations nested more than 1 level deep.
]]]
Index: subversion/mod_dav_svn/mod_dav_svn.c
===================================================================
--- subversion/mod_dav_svn/mod_dav_svn.c (revision 1203651)
+++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
@@ -221,7 +221,8 @@ merge_dir_config(apr_pool_t *p, void *base, void *
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
"mod_dav_svn: nested Location '%s' hinders access to '%s' "
"in SVNPath Location '%s'",
- child->root_dir, svn_dirent_basename(child->root_dir, p),
+ child->root_dir,
+ svn_fspath__skip_ancestor(parent->root_dir, child->root_dir),
parent->root_dir);
return newconf;
Re: svn commit: r1203546 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Nov 18, 2011 at 03:07:11PM +0100, Stefan Sperling wrote:
> On Fri, Nov 18, 2011 at 02:52:44PM +0100, Stefan Sperling wrote:
> > Suggestion (not tested);
> >
> > ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
> > "mod_dav_svn: nested Location '%s' inside "
> > "SVNPath Location '%s'; this hinders access to a file "
> > "or directory '%s' inside the repository '%s'",
> > child->root_dir, parent->root_dir,
> > svn_dirent_basename(child->root_dir, scratch_pool),
> > parent->fs_path);
>
> Better suggestion.
And a new patch, based on your r1203651 commit:
[[[
Follow-up to r1203651:
* subversion/mod_dav_svn/mod_dav_svn.c
(merge_dir_config): Use svn__fspath instead of svn_dirent for URL-paths.
Correctly print Locations nested more than 1 level deep.
]]]
Index: subversion/mod_dav_svn/mod_dav_svn.c
===================================================================
--- subversion/mod_dav_svn/mod_dav_svn.c (revision 1203651)
+++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
@@ -221,7 +221,8 @@ merge_dir_config(apr_pool_t *p, void *base, void *
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
"mod_dav_svn: nested Location '%s' hinders access to '%s' "
"in SVNPath Location '%s'",
- child->root_dir, svn_dirent_basename(child->root_dir, p),
+ child->root_dir,
+ svn_fspath__skip_ancestor(parent->root_dir, child->root_dir),
parent->root_dir);
return newconf;
Re: svn commit: r1203546 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Nov 18, 2011 at 02:52:44PM +0100, Stefan Sperling wrote:
> Suggestion (not tested);
>
> ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
> "mod_dav_svn: nested Location '%s' inside "
> "SVNPath Location '%s'; this hinders access to a file "
> "or directory '%s' inside the repository '%s'",
> child->root_dir, parent->root_dir,
> svn_dirent_basename(child->root_dir, scratch_pool),
> parent->fs_path);
Better suggestion.
[[[
Follow-up to r1203546:
* subversion/mod_dav_svn/mod_dav_svn.c
(merge_dir_config): Only log a warning about nested SVNPath locations,
but still allow access. Also improve the message being logged.
]]]
Index: subversion/mod_dav_svn/mod_dav_svn.c
===================================================================
--- subversion/mod_dav_svn/mod_dav_svn.c (revision 1203650)
+++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
@@ -218,16 +218,14 @@ merge_dir_config(apr_pool_t *p, void *base, void *
newconf->root_dir = INHERIT_VALUE(parent, child, root_dir);
if (parent->fs_path)
- {
- /* Nesting inside SVNPath is ambiguous so prevent access. */
- newconf->fs_path = newconf->fs_parent_path = NULL;
+ ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
+ "mod_dav_svn: nested Location '%s' inside "
+ "SVNPath Location '%s'; this hinders access to a "
+ "file or directory '%s' inside of repository '%s'",
+ child->root_dir, parent->root_dir,
+ svn_fspath__skip_ancestor(parent->root_dir, child->root_dir),
+ parent->fs_path);
- ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL,
- "mod_dav_svn: invalid nested Location '%s' inside "
- "SVNPath Location '%s'",
- child->root_dir, parent->root_dir);
- }
-
return newconf;
}
Re: svn commit: r1203546 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Nov 18, 2011 at 02:52:44PM +0100, Stefan Sperling wrote:
> Suggestion (not tested);
>
> ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
> "mod_dav_svn: nested Location '%s' inside "
> "SVNPath Location '%s'; this hinders access to a file "
> "or directory '%s' inside the repository '%s'",
> child->root_dir, parent->root_dir,
> svn_dirent_basename(child->root_dir, scratch_pool),
> parent->fs_path);
Better suggestion.
[[[
Follow-up to r1203546:
* subversion/mod_dav_svn/mod_dav_svn.c
(merge_dir_config): Only log a warning about nested SVNPath locations,
but still allow access. Also improve the message being logged.
]]]
Index: subversion/mod_dav_svn/mod_dav_svn.c
===================================================================
--- subversion/mod_dav_svn/mod_dav_svn.c (revision 1203650)
+++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
@@ -218,16 +218,14 @@ merge_dir_config(apr_pool_t *p, void *base, void *
newconf->root_dir = INHERIT_VALUE(parent, child, root_dir);
if (parent->fs_path)
- {
- /* Nesting inside SVNPath is ambiguous so prevent access. */
- newconf->fs_path = newconf->fs_parent_path = NULL;
+ ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
+ "mod_dav_svn: nested Location '%s' inside "
+ "SVNPath Location '%s'; this hinders access to a "
+ "file or directory '%s' inside of repository '%s'",
+ child->root_dir, parent->root_dir,
+ svn_fspath__skip_ancestor(parent->root_dir, child->root_dir),
+ parent->fs_path);
- ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL,
- "mod_dav_svn: invalid nested Location '%s' inside "
- "SVNPath Location '%s'",
- child->root_dir, parent->root_dir);
- }
-
return newconf;
}
Re: svn commit: r1203546 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Nov 18, 2011 at 01:37:24PM +0000, Philip Martin wrote:
> philip@apache.org writes:
>
> > Author: philip
> > Date: Fri Nov 18 09:09:56 2011
> > New Revision: 1203546
>
> > Also, a nested Location inside an SVNPath Location is ambiguous so
> > prevent access and log the problem.
>
> > + if (parent->fs_path)
> > + {
> > + /* Nesting inside SVNPath is ambiguous so prevent access. */
> > + newconf->fs_path = newconf->fs_parent_path = NULL;
> > +
> > + ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL,
> > + "mod_dav_svn: invalid nested Location '%s' inside "
> > + "SVNPath Location '%s'",
> > + child->root_dir, parent->root_dir);
> > + }
>
> I'm having second thoughts about this bit. Given
>
> <Location /foo>
> ...
> SVNPath /some/foo
> </Location>
> <Location /foo/bar>
> ...
> SVNPath /some/bar
> </Location>
>
> there is obvious ambiguity about the URL /foo/bar. Is it the root of
> the repository at /foo/bar or is it /bar in the repository at /foo?
> However provided I'm not trying to use /bar inside the /foo repository
> then checkouts, updates and commits work for both repositories. (Some
> operations even work on /bar inside /foo, although not enough to be
> useful.)
>
> What that means is that some people may be using this setup, either
> intentionally or accidentally. Should we continue to support them? If
> we choose not to support them in 1.8 can we make the change in 1.7.2 as
> well?
I didn't notice this when approving the fix but we should really be using
APLOG_WARNING instead of APLOG_ERR. It's not supposed to be an error,
just a warning.
I think logging a warning is entirely appropriate.
There is a potential problem here, and admins might not be aware of it.
We might want to rephrase the message to make its intention clear.
Suggestion (not tested);
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
"mod_dav_svn: nested Location '%s' inside "
"SVNPath Location '%s'; this hinders access to a file "
"or directory '%s' inside the repository '%s'",
child->root_dir, parent->root_dir,
svn_dirent_basename(child->root_dir, scratch_pool),
parent->fs_path);
Re: svn commit: r1203546 -
/subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Stefan Sperling <st...@elego.de>.
On Fri, Nov 18, 2011 at 01:37:24PM +0000, Philip Martin wrote:
> philip@apache.org writes:
>
> > Author: philip
> > Date: Fri Nov 18 09:09:56 2011
> > New Revision: 1203546
>
> > Also, a nested Location inside an SVNPath Location is ambiguous so
> > prevent access and log the problem.
>
> > + if (parent->fs_path)
> > + {
> > + /* Nesting inside SVNPath is ambiguous so prevent access. */
> > + newconf->fs_path = newconf->fs_parent_path = NULL;
> > +
> > + ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL,
> > + "mod_dav_svn: invalid nested Location '%s' inside "
> > + "SVNPath Location '%s'",
> > + child->root_dir, parent->root_dir);
> > + }
>
> I'm having second thoughts about this bit. Given
>
> <Location /foo>
> ...
> SVNPath /some/foo
> </Location>
> <Location /foo/bar>
> ...
> SVNPath /some/bar
> </Location>
>
> there is obvious ambiguity about the URL /foo/bar. Is it the root of
> the repository at /foo/bar or is it /bar in the repository at /foo?
> However provided I'm not trying to use /bar inside the /foo repository
> then checkouts, updates and commits work for both repositories. (Some
> operations even work on /bar inside /foo, although not enough to be
> useful.)
>
> What that means is that some people may be using this setup, either
> intentionally or accidentally. Should we continue to support them? If
> we choose not to support them in 1.8 can we make the change in 1.7.2 as
> well?
I didn't notice this when approving the fix but we should really be using
APLOG_WARNING instead of APLOG_ERR. It's not supposed to be an error,
just a warning.
I think logging a warning is entirely appropriate.
There is a potential problem here, and admins might not be aware of it.
We might want to rephrase the message to make its intention clear.
Suggestion (not tested);
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, NULL,
"mod_dav_svn: nested Location '%s' inside "
"SVNPath Location '%s'; this hinders access to a file "
"or directory '%s' inside the repository '%s'",
child->root_dir, parent->root_dir,
svn_dirent_basename(child->root_dir, scratch_pool),
parent->fs_path);
Re: svn commit: r1203546 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Philip Martin <ph...@wandisco.com>.
philip@apache.org writes:
> Author: philip
> Date: Fri Nov 18 09:09:56 2011
> New Revision: 1203546
> Also, a nested Location inside an SVNPath Location is ambiguous so
> prevent access and log the problem.
> + if (parent->fs_path)
> + {
> + /* Nesting inside SVNPath is ambiguous so prevent access. */
> + newconf->fs_path = newconf->fs_parent_path = NULL;
> +
> + ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL,
> + "mod_dav_svn: invalid nested Location '%s' inside "
> + "SVNPath Location '%s'",
> + child->root_dir, parent->root_dir);
> + }
I'm having second thoughts about this bit. Given
<Location /foo>
...
SVNPath /some/foo
</Location>
<Location /foo/bar>
...
SVNPath /some/bar
</Location>
there is obvious ambiguity about the URL /foo/bar. Is it the root of
the repository at /foo/bar or is it /bar in the repository at /foo?
However provided I'm not trying to use /bar inside the /foo repository
then checkouts, updates and commits work for both repositories. (Some
operations even work on /bar inside /foo, although not enough to be
useful.)
What that means is that some people may be using this setup, either
intentionally or accidentally. Should we continue to support them? If
we choose not to support them in 1.8 can we make the change in 1.7.2 as
well?
--
Philip
Re: svn commit: r1203546 - /subversion/trunk/subversion/mod_dav_svn/mod_dav_svn.c
Posted by Philip Martin <ph...@wandisco.com>.
philip@apache.org writes:
> Author: philip
> Date: Fri Nov 18 09:09:56 2011
> New Revision: 1203546
> Also, a nested Location inside an SVNPath Location is ambiguous so
> prevent access and log the problem.
> + if (parent->fs_path)
> + {
> + /* Nesting inside SVNPath is ambiguous so prevent access. */
> + newconf->fs_path = newconf->fs_parent_path = NULL;
> +
> + ap_log_error(APLOG_MARK, APLOG_ERR, 0, NULL,
> + "mod_dav_svn: invalid nested Location '%s' inside "
> + "SVNPath Location '%s'",
> + child->root_dir, parent->root_dir);
> + }
I'm having second thoughts about this bit. Given
<Location /foo>
...
SVNPath /some/foo
</Location>
<Location /foo/bar>
...
SVNPath /some/bar
</Location>
there is obvious ambiguity about the URL /foo/bar. Is it the root of
the repository at /foo/bar or is it /bar in the repository at /foo?
However provided I'm not trying to use /bar inside the /foo repository
then checkouts, updates and commits work for both repositories. (Some
operations even work on /bar inside /foo, although not enough to be
useful.)
What that means is that some people may be using this setup, either
intentionally or accidentally. Should we continue to support them? If
we choose not to support them in 1.8 can we make the change in 1.7.2 as
well?
--
Philip