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.
Bert
> + seg += strlen(root_dir);
> + proxy_request_fixup(r, master_uri, seg);
> + return OK;
> + }
> + }
> +
> + /* If this is a MERGE request or a request using a "special
> + URI", we have to doctor it a bit for proxying. */
> seg = ap_strstr(r->unparsed_uri, root_dir);
> if (seg && (r->method_number == M_MERGE ||
> - ap_strstr_c(seg, dav_svn__get_special_uri(r)))) {
> + ap_strstr_c(seg, dav_svn__get_special_uri(r)))) {
> seg += strlen(root_dir);
> -
> - r->proxyreq = PROXYREQ_REVERSE;
> - r->uri = r->unparsed_uri;
> - r->filename = apr_pstrcat(r->pool, "proxy:", master_uri,
> - "/", seg, 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);
> + proxy_request_fixup(r, master_uri, seg);
> + return OK;
> }
> }
> return OK;
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: svn-unsubscribe_at_subversion.tigris.org
> For additional commands, e-mail: svn-help_at_subversion.tigris.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe_at_subversion.tigris.org
For additional commands, e-mail: dev-help_at_subversion.tigris.org
Received on 2008-11-25 23:49:02 CET