On 04/08/2010 09:23 PM, Philip Martin wrote:
Kamesh Jayachandran<kam...@collab.net> writes:
[issue2753] Fix issue 2753.
Let me see if I understand: The issue is that when SVNListParentPath
and AuthzSVNAccessFile are configured then GET requests for the parent
path get passed through the authz stuff. This is a bug because the
authz file doesn't control parent path.
Your patch recognises this request and avoids doing the authz check.
Yes, exactly.
Relax requests aimed at the repo Parent path from authz control.
* subversion/mod_authz_svn/mod_authz_svn.c
(req_check_access): When canonicalized 'uri' and 'root_path' are same
allow the request.
]]]
If there are no objections will commit this in next couple of days.
Thanks
With regards
Kamesh Jayachandran
Index: subversion/mod_authz_svn/mod_authz_svn.c
===================================================================
--- subversion/mod_authz_svn/mod_authz_svn.c (revision 931820)
+++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
@@ -210,6 +210,8 @@
svn_authz_t *access_conf = NULL;
svn_error_t *svn_err;
char errbuf[256];
+ const char *canonicalized_uri;
+ const char *canonicalized_root_path;
const char *username_to_authorize = get_username_to_authorize(r, conf);
switch (r->method_number)
@@ -249,6 +251,15 @@
break;
}
+ canonicalized_uri = svn_uri_canonicalize(r->uri, r->pool);
+ canonicalized_root_path = svn_uri_canonicalize(conf->base_path, r->pool);
Can conf->base_path be canonicalised once in
create_authz_svn_dir_config rather than for every request?
Yes done. Attached patch has this.
+ if (strcmp(canonicalized_uri, canonicalized_root_path) == 0)
+ {
+ /*Do no access control when root_path(as configured in<Location>) and
+ given uri are same.*/
+ return OK;
+ }
What happens if SVNParentPath is not being used? Is base_path is the
root of the repository? Does this disable authz on the root of that
repository? Perhaps you should be checking dav_svn__get_list_parentpath?
As said in my other email this works without any issue, the comment
added in the patch details the same.
I think this check would make more sense in access_checker rather than
req_check_access.
May be, I prefer req_check_access as there are 3 callers for this function.
The code needs a comment to say why no access control is neccessary in
this case.
Check my new comment in the attached patch.
The attached patch fixes one segfault(introduced in my earlier patch) also.
With regards
Kamesh Jayachandran
Index: subversion/mod_authz_svn/mod_authz_svn.c
===================================================================
--- subversion/mod_authz_svn/mod_authz_svn.c (revision 932331)
+++ subversion/mod_authz_svn/mod_authz_svn.c (working copy)
@@ -66,6 +66,9 @@
authz_svn_config_rec *conf = apr_pcalloc(p, sizeof(*conf));
conf->base_path = d;
+ if (d)
+ conf->base_path = svn_uri_canonicalize(d, p);
+
/* By default keep the fortress secure */
conf->authoritative = 1;
conf->anonymous = 1;
@@ -210,6 +213,7 @@
svn_authz_t *access_conf = NULL;
svn_error_t *svn_err;
char errbuf[256];
+ const char *canonicalized_uri;
const char *username_to_authorize = get_username_to_authorize(r, conf);
switch (r->method_number)
@@ -249,6 +253,22 @@
break;
}
+ canonicalized_uri = svn_uri_canonicalize(r->uri, r->pool);
+ if (strcmp(canonicalized_uri, conf->base_path) == 0)
+ {
+ /* Do no access control when conf->base_path(as configured in <Location>)
+ * and given uri are same. The reason for such relaxation of access
+ * control is "This module is meant to control access inside the
+ * repository path, in this case inside PATH is empty and hence
+ * dav_svn_split_uri fails saying no repository name present".
+ * One may ask it will allow access to '/' inside the repository if
+ * repository is served via SVNPath instead of SVNParentPath.
+ * It does not, The other methods(PROPFIND, MKACTIVITY) for
+ * accomplishing the operation takes care of making a request to
+ * proper URL */
+ return OK;
+ }
+
dav_err = dav_svn_split_uri(r,
r->uri,
conf->base_path,
@@ -554,7 +574,7 @@
{
authz_svn_config_rec *conf = ap_get_module_config(r->per_dir_config,
&authz_svn_module);
- const char *repos_path;
+ const char *repos_path = NULL;
const char *dest_repos_path = NULL;
int status;
@@ -611,7 +631,7 @@
{
authz_svn_config_rec *conf = ap_get_module_config(r->per_dir_config,
&authz_svn_module);
- const char *repos_path;
+ const char *repos_path = NULL;
const char *dest_repos_path = NULL;
int status;
@@ -639,7 +659,7 @@
{
authz_svn_config_rec *conf = ap_get_module_config(r->per_dir_config,
&authz_svn_module);
- const char *repos_path;
+ const char *repos_path = NULL;
const char *dest_repos_path = NULL;
int status;
[issue2753] Fix issue 2753.
Relax requests aimed at the repo Parent path from authz control.
* subversion/mod_authz_svn/mod_authz_svn.c
(create_authz_svn_dir_config): Canonicalize conf->base_path.
(req_check_access): When canonicalized 'uri' and 'conf->base_path' are same
allow the request.
(access_checker, check_user_id, auth_checker):
Initialize repos_path to 'NULL' otherwise it can point
to stray values when req_check_access relaxes certain requests without
initialising the out parameters.
