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
proxy-fix-3445.patch
Description: Binary data
