Re: mod_dav_svn: Fix Destination header rewriting in master/slave proxy for COPY/MOVE

Hi Mike,
I added a test for a property change containing the slave URL and it
was silently rewritten to the master URL without errors. So this is
the worst case, because it's silent data corruption.

I asked Claude for ideas on fixing the issue, and it suggested a
simple fix that mitigates the most common cases of the bug. Instead of
having to fully parse the xml content to patch protocol URLs and
ignore repository data, it suggested only filtering the content types
that can possibly contain protocol URLs:


> Request side: dav_svn__location_in_filter() skip the filter entirely for M_PUT and M_PROPPATCH. The only client -> master URLs the proxy must translate live in the DeltaV protocol bodies (MERGE/CHECKOUT/REPORT), and those still get the filter. The condition excludes only PUT and PROPPATCH. A PUT body is svndiff (binary delta); a PROPPATCH body's payload is opaque property values. Neither ever contains a repository location the master needs rewritten.
>
> Response side: dav_svn__location_body_filter() skip the filter for non-XML Content-Types, the hrefs only exist in XML. Every response that carries hrefs needing master -> slave translation is protocol XML (multistatus, activity sets), which mod_dav_svn serves as text/xml or application/xml. A non-XML response by definition has no hrefs to translate, so skipping loses nothing.
>
> Remaining edge case: the PROPFIND multistatus residual
> The response-side filter still does a blind byte substitution over the whole XML body, and it can't tell a genuine protocol <D:href> from a dead-property value that happens to contain the master location.
>
> Why the earlier guards don't catch it: the content-type check only skips non-XML bodies. A PROPFIND multistatus is always text/xml, so it must go through the rewrite (its hrefs genuinely need translating), which means any property value sharing that body is exposed. It's the one case where data-that-must-be-rewritten and data-that-must-not coexist in the same body.
>
> Two sub-cases:
> * PROPFIND multistatus: dead-property values containing the master URL get silently rewritten alongside the real hrefs. (Main residual.)
> * text/xml-typed versioned file via proxied GET - a file whose svn:mime-type is text/xml/application/xml is misclassified as protocol XML and rewritten too.
> Reachability: Only through proxied reads of transaction-in-progress resources (/wrk/, /txn/, /txr/), committed reads are served locally and never hit the filter.
>
> Real fix: an XML-structure-aware rewrite that translates only href elements and leaves character data / property values alone, verifiable via a C-level unit test feeding a synthetic multistatus into dav_svn__location_body_filter, since the shell suite can't drive it.
>
> Severity: Silent data corruption, but narrow. Needs differing master/slave locations, a property/file value that embeds the master location, and a proxied txn read. Documented, not fixed.


I don't know enough about the SVN protocol to know if this change is
100% safe but Claude's reasoning around it seems sound. And it fixes
the previously failing tests I created for this.

It would be good to get some more people's thoughts on this fix, it
seems straight forward, and even with the edge case it's a big
improvement over the current impact of the bug.

I've attached a patch with the proposed fix from Claude.

Thanks,
Jordan


On Fri, 3 Jul 2026 at 22:24, C. Michael Pilato <[email protected]> wrote:
>
> On Fri, Jul 3, 2026 at 10:50 AM Jordan Peck <[email protected]> wrote:
> > > I don't have the setup to verify this situation today, but I
> > > suspect that Jordan does.  What happens when you repository URIs
> > > differ and a, say, a text file that contains the URL of the master
> > > repository is committed?  Does that file's content on the slave
> > > mirror repository still carry the URL of the master repository, or
> > > has it been tweaked to look like a mixture of master hostname and
> > > slave path?
> >
> > In my latest commit on the branch I added a test for this based on the
> > issue you linked and it does indeed fail, although not silently. In
> > the test case the URLs "master" and "slave" have different lengths,
> > which causes the commit to abort due to "E185004: Unexpected end of
> > svndiff input". I also tested with "master" and "slavex" which instead
> > triggers a "E200014: Checksum mismatch", so I don't think any silent
> > corruption is occurring.
>
> Thanks for verifying this.  Issue #3445 also mentions changes unwanted
> changes to properties (see
> https://issues.apache.org/jira/browse/SVN-3445?focusedCommentId=14928251&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14928251).
> I wonder if those would enjoy the same protections via checksums that
> exist for file content?
>
> > My fix to the move/copy proxy shouldn't be affected by this at all, as
> > it's only rewriting the destination header which will never contain
> > file data.
>
> Correct.  And sorry if I seemed to imply a problem with your patch.  I
> think the patch is fine.  Where I was hung up was that I read the
> patch as offering a bugfix for a situation that arises from having
> mismatched master and slave URIs, but it seems that _even with the
> patch_ we should not be recommending that folks setup their WebDAV
> proxies in that manner (unless they have other ways to mitigate the
> aforementioned risks).
>
> Of course, it's possible also that I misunderstood the full scope of
> what your patch fixes.  It's been a long time since I read Subversion
> code!
>
> -- Mike

Index: subversion/mod_dav_svn/mirror.c
===================================================================
--- subversion/mod_dav_svn/mirror.c	(revision 1935831)
+++ subversion/mod_dav_svn/mirror.c	(working copy)
@@ -120,7 +120,15 @@ static int proxy_request_fixup(request_rec *r,
 
     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);
+
+    /* PUT bodies are svndiff-encoded file content and PROPPATCH bodies carry
+       opaque property values - versioned payload, never protocol URLs.
+       Rewriting them corrupts data (issue #3445).  The hrefs we must
+       translate live in MERGE/CHECKOUT/REPORT bodies, which still get the
+       input filter. */
+    if (r->method_number != M_PUT && r->method_number != M_PROPPATCH)
+        ap_add_input_filter("IncomingRewrite", NULL, r, r->connection);
+
     return OK;
 }
 
@@ -224,12 +232,6 @@ apr_status_t dav_svn__location_in_filter(ap_filter
         return ap_get_brigade(f->next, bb, mode, block, readbytes);
     }
 
-    /* ### FIXME: While we want to fix up any locations in proxied XML
-       ### requests, we do *not* want to be futzing with versioned (or
-       ### to-be-versioned) data, such as the file contents present in
-       ### PUT requests and properties in PROPPATCH requests.
-       ### See issue #3445 for details. */
-
     /* We are url encoding the current url and the master url
        as incoming(from client) request body has it encoded already. */
     canonicalized_uri = svn_path_uri_encode(canonicalized_uri, r->pool);
@@ -315,6 +317,20 @@ apr_status_t dav_svn__location_header_filter(ap_fi
     return ap_pass_brigade(f->next, bb);
 }
 
+/* Only protocol XML (a multistatus, activity set, etc.) carries hrefs the
+   proxy must translate; anything else including a body of unidentified
+   type is versioned payload and must pass through untouched lest we
+   corrupt it (issue #3445). */
+static svn_boolean_t
+response_is_xml(const request_rec *r)
+{
+    return r->content_type
+           && (strncmp(r->content_type, "text/xml",
+                       sizeof("text/xml") - 1) == 0
+               || strncmp(r->content_type, "application/xml",
+                          sizeof("application/xml") - 1) == 0);
+}
+
 apr_status_t dav_svn__location_body_filter(ap_filter_t *f,
                                            apr_bucket_brigade *bb)
 {
@@ -342,12 +358,24 @@ apr_status_t dav_svn__location_body_filter(ap_filt
         return ap_pass_brigade(f->next, bb);
     }
 
-    /* ### FIXME: GET and PROPFIND requests that make it here must be
-       ### referring to data inside commit transactions-in-progress.
-       ### We've got to be careful not to munge the versioned data
-       ### they return in the process of trying to do URI fix-ups.
-       ### See issue #3445 for details. */
+    /* Proxied GET/PROPFIND responses that reach this filter refer to data
+       inside commit transactions-in-progress.  A non-XML body (e.g. the file
+       content of a proxied GET of a transaction resource) is versioned
+       payload, not protocol XML, and must pass through untouched.  Note that
+       a versioned file whose svn:mime-type is itself text/xml or
+       application/xml still slips past this check into the rewrite below. */
+    if (!response_is_xml(r)) {
+        ap_remove_output_filter(f);
+        return ap_pass_brigade(f->next, bb);
+    }
 
+    /* ### FIXME (SVN-3445, residual): a PROPFIND multistatus is still rewritten
+       ### wholesale below, so a dead-property *value* that happens to contain
+       ### the master location gets silently rewritten along with the genuine
+       ### <D:href>s.  Fixing that safely requires an XML-structure-aware
+       ### rewrite (translate hrefs only, leave property values alone) rather
+       ### than the blind byte substitution used here. */
+
     /* We are url encoding the current url and the master url
        as incoming(from master) request body has it encoded already. */
     canonicalized_uri = svn_path_uri_encode(canonicalized_uri, r->pool);