Re: svn commit: r34082 - trunk/subversion/mod_dav_svn

Bert Huijben wrote:
> Hi,
>
> Reviewing this for the 1.5.x backport nomination
>
> 2008/11/6 <cmpilato_at_tigris.org>:
>> Author: cmpilato
>> Date: Thu Nov 6 12:44:19 2008
>> New Revision: 34082
>>
>> Log:
>> Fix issue #3275 by teaching the mod_dav_svn proxy logic to hand off
>> GET and PROPFIND requests that are aimed at working resource URIs to
>> the proxy instead of trying to field them in the slave server.
>>
>> WARNING: I'm relying on my compiler and the testimony of the
>> attributed tester to determine the legitimacy of this patch.
>> Developers familiar with the proxy code are encouraged to
>> review this commit!
>>
>> * subversion/mod_dav_svn/mirror.c
>> (proxy_request_fixup): New helper function, cored from
>> dav_svn__proxy_merge_fixup().
>> (dav_svn__proxy_merge_fixup): Look for GET and PROPFIND requests
>> aimed at working resource URIs, and proxy those away, too.
>>
>> Tested by: Kylo Ginsberg <kylo{_AT_}kylo.net>
>>
>> Modified:
>> trunk/subversion/mod_dav_svn/mirror.c
>>
>> Modified: trunk/subversion/mod_dav_svn/mirror.c
>> URL: http://svn.collab.net/viewvc/svn/trunk/subversion/mod_dav_svn/mirror.c?pathrev=34082&r1=34081&r2=34082
>> ==============================================================================
>> --- trunk/subversion/mod_dav_svn/mirror.c Thu Nov 6 12:37:35 2008 (r34081)
>> +++ trunk/subversion/mod_dav_svn/mirror.c Thu Nov 6 12:44:19 2008 (r34082)
>> @@ -23,6 +23,22 @@
>>
>> #include "dav_svn.h"
>>
>> +
>> +static void proxy_request_fixup(request_rec *r,
>> + const char *master_uri,
>> + const char *uri_segment)
>> +{
>> + r->proxyreq = PROXYREQ_REVERSE;
>> + r->uri = r->unparsed_uri;
>> + r->filename = apr_pstrcat(r->pool, "proxy:", master_uri,
>> + "/", uri_segment, NULL);
>> + r->handler = "proxy-server";
>> + ap_add_output_filter("LocationRewrite", NULL, r, r->connection);
>> + ap_add_output_filter("ReposRewrite", NULL, r, r->connection);
>> + ap_add_input_filter("IncomingRewrite", NULL, r, r->connection);
>> +}
>> +
>> +
>> int dav_svn__proxy_merge_fixup(request_rec *r)
>> {
>> const char *root_dir, *master_uri;
>> @@ -34,26 +50,37 @@ int dav_svn__proxy_merge_fixup(request_r
>> const char *seg;
>>
>> /* We know we can always safely handle these. */
>> - if (r->method_number == M_PROPFIND ||
>> - r->method_number == M_GET ||
>> - r->method_number == M_REPORT ||
>> + if (r->method_number == M_REPORT ||
>> r->method_number == M_OPTIONS) {
>> return OK;
>> }
>>
>> + /* These are read-only requests -- the kind we like to handle
>> + ourselves -- but we need to make sure they aren't aimed at
>> + working resource URIs before trying to field them. Why?
>> + Because working resource URIs are modeled in Subversion
>> + using uncommitted Subversion transactions -- stuff our copy
>> + of the repository isn't guaranteed to have on hand. */
>> + if (r->method_number == M_PROPFIND ||
>> + r->method_number == M_GET) {
>> + seg = ap_strstr(r->unparsed_uri, root_dir);
>> + if (seg && ap_strstr_c(seg, apr_pstrcat(r->pool, root_dir,
>> + dav_svn__get_master_uri(r),
>> + "/wrk/", NULL))) {
>
> These two instances of ap_strstr look like some kind of hack to me.
>
> The root_dir should be at a specific point in the uri, not just on any location.
>
> Without further changes this code might have some security
> implications. If we don't apply a stricter check here we might forward
> parts of the url that aren't validated to the master server, or
> something like that.
>
>
> The rest of the patch looks good.

Thanks for that. I agree about the root_dir bit, but in my defense I merely
copied other code doing the same thing. Both instances should be changed,
though.

Now, if only I could figure out why folks testing the patch say it doesn't
work (see comments in issue #3275)...

-- 
C. Michael Pilato <cmpilato_at_collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand