[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: mod_dav_svn changing the request filename - interaction with mod_rewrite

From: Ben Reser <ben_at_reser.org>
Date: Wed, 11 Dec 2013 13:22:54 -0800

On 12/11/13 10:45 AM, Ben Reser wrote:
> Hmm this is going to be a pain to fix (possibly impossible). Because what
> mod_rewrite is doing is really hackish. When you use the PT (PassThrough) flag
> mod_rewrite puts passthrough:/my/new/URL into r->filename. Then eventually it
> moves it to r->uri, but not until after it goes through our translate_name
> hook. So we can't reliably know if the request is going to be served by us or
> not by asking for the per_dir config.

Okay this isn't strictly true. r->uri is set before it gets to us. It's just
that mod_rewrite doesn't trigger a new location walk (that happens after the
map_to_storage hooks run which is the next step after the translate_name hook).
 In order for mod_rewrite to work transparently the location_walk would need to
be triggered again.

We could do that on our side with something like this:
[[[
Index: subversion/mod_dav_svn/mod_dav_svn.c
===================================================================
--- subversion/mod_dav_svn/mod_dav_svn.c (revision 1549924)
+++ subversion/mod_dav_svn/mod_dav_svn.c (working copy)
@@ -1121,8 +1121,21 @@
   const char *fs_path, *repos_basename, *repos_path;
   const char *ignore_cleaned_uri, *ignore_relative_path;
   int ignore_had_slash;
- dir_conf_t *conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
+ dir_conf_t *conf;

+ /* rewalk the location blocks if mod_rewrite rewrote something */
+ if (apr_table_get(r->notes, "mod_rewrite_rewritten"))
+ {
+ r->per_dir_config = r->server->lookup_defaults;
+ ap_location_walk(r);
+#if AP_MODULE_MAGIC_AT_LEAST(20110117,0)
+ ap_if_walk(r);
+#endif
+ }
+
+ /* Now that we have an accurate per_dir_config retrieve our config */
+ conf = ap_get_module_config(r->per_dir_config, &dav_svn_module);
+
   /* module is not configured, bail out early */
   if (!conf->fs_path && !conf->fs_parent_path)
     return DECLINED;
]]]

But I really don't think we should for several reasons.

First of all it starts messing with httpd internals that I don't think we
should really need to be doing. Note that I had to include the ap_if_walk()
for 2.3.x+. This sort of fix would have to forever be kept in sync with the
code in ap_process_request_internal(). Making it rather brittle.

This changes the conf available to all the other modules that follow us in the
translate_name hooks. Since we have to do this before we can tell if we're
configured, this impacts all traffic not just us. We could go to the effort of
storing the state and then putting things back if we DECLINED, but that's not
as simple as it seems. There is a cache for the location and if walks that I'm
not sure if we have to restore that.

It seems to me that this is something that mod_rewrite should be doing and if
since it isn't there's probably a really good reason for it not to do this.

So at this point I think this needs to be taken to the dev_at_httpd.apache.org
list. I'll start a new thread over there.
Received on 2013-12-11 22:23:26 CET

This is an archived mail posted to the Subversion Dev mailing list.