Hi Mike, I've done some more work on this, added some more tests to dav-mirror-autocheck.sh and changed the proxy config to test more edge cases. The fixes that claude proposed in my previous email do fix some of the new tests, but there were a couple more edge cases that it missed that I've now resolved.
First off this is the list of things I've found that break due to the 3445 issue: - File content via commit containing the slave url would silently get rewritten to the master url if their paths were the same length. If the path length differs the commit would instead fail. - Property edits would also silently rewrite a slave url to master - Commit log messages containing the slave url would cause the commit to fail. - Lock comments get a slave URL silently rewritten, and also translated back when reading it through the proxy. So you need to read from the master to see the corruption. Not 3445 related but I also found that some of the proxy code treated the <Location> path as decoded when it's not. So if it contained an escapable character none of the path rewriting took place and most requests would fail when reaching the master server. Then I also found a non-proxy bug with the new tests, any mod_dav_svn repository whose <Location> contains an escapable character will fail commits with an http 400 code, no proxy involved. I found the cause, 3 callers of get_resource() passed an encoded path when it expected a decoded one. You can see my fix for this in r1936013. For the initial 3445 related fixes I committed the patch proposed in my previous email (r1936010), this fixed the worst of the issues such as the file content munging and property edit munging. Then with the follow up fixes (r1936015 & r1936031) I added a whitelist to the response filtering and converted the blacklist for requests (added in the initial fix) into a whitelist too. I decided that having the filters whitelist based on method type instead of blacklist was preferable for a couple reasons: - There are only 2 method types that actually need filtering in each path, a blacklist would need to contain 4~ - If a method that requires a path rewrite isn't in the whitelist, it will fail loudly when proxied to the master. A blacklist with the opposite case will potentially be silently munging data. My most recent commit r1936032 added many new proxy tests in dav-mirror-autocheck.sh that I've been using to discover all these issues. It now uses master/slave locations with spaces to test URI-encoding and full coverage of every operation that can pass through the proxy. There is also coverage of the one remaining edge case bug, which is property values containing the master url are rewritten inside proxied PROPFIND responses on a transaction in progress (/!svn/txr/). It doesn't affect regular PROPFINDs because these are served by the slave locally and not proxied to the master. The test for this is also somewhat convoluted because there are no SVN operations that leave an open transaction in progress, so a raw curl POST is used to set this up. The PROPFIND to retrieve a property on this is then what triggers the master URL munging, this also has to be via curl since no SVN op triggers a PROPFIND on a transaction in progress. So you could consider this remaining edge case a complete non-issue. This has all been a much larger rabbit hole than I expected when I first started looking into the 3445 issue. But I think the fixes now in the branch put 99% of the fears of data munging/corruption to bed, and it feels like it wouldn't be worth the effort of implementing your idea of passing the slave URL to the master in the header and having the master handle the rewrite just to fix this edge case. Although you could argue adding it for some more robustness over the current proxy setup. Let me know your thoughts. Thanks, Jordan On 2026/07/06 20:57:54 Jordan Peck via dev wrote: > Hi Mike, > I did see your idea, and thinking about it again now it could work > quite well alongside the fix that claude proposed. > > With your idea I don't think you'd want to remove the filters > completely given that the master server being proxied to might be on > an older version and ignore a new path filter header. However, I think > you'd still want filtering on the request side of the proxy. Having > the master server filter using a client provided string (on a path it > will read/write) seems like a security nightmare. Also it wouldn't > work for path based authorisation. > > With the claude fix of more selective filtering on the request side > there is in theory no potential for user data munging anymore. > > For the response filtering I think your idea of passing a filter URL > in the header to the master server side so it can do the filtering on > the structured data could work well. There's little security concern > because it would only be filtering data that is already on its way > out, and we can hopefully resolve the last remaining edge cases for > data munging. > > The only clash from the 2 fixes is that if the master server handles > the filtering, we shouldn't filter on the response side. This is fine > if the master server runs the same version and supports server-side > filtering. But I think we'd need to selectively enable the > response-side filtering if it's from an older master server that isn't > performing any filtering. > > I don't know how big of a task it would be to do the master > server-side filtering using a new header. I imagine it's a reasonable > amount of work, but I think it's a better option than the other > potential fix, performing full XML parsing and filtering on the proxy. > If we want it to be free of edge cases. > > Thanks, > Jordan > > On Mon, 6 Jul 2026 at 18:28, C. Michael Pilato <[email protected]> wrote: > > > > On Sat, Jul 4, 2026 at 4:21 PM Jordan Peck <[email protected]> wrote: > >> > >> 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. > > > > > > Okay, so this is what I feared with issue SVN-3445. (Thanks for whipping > > up these tests, by the way!) > > > > I notice that I (softly) proposed the following in that same issue[1]: > > > >> Have the slave server drop a header into the proxied request that the > >> master > >> looks for. The header tells the master server what the slave's > >> "localpath" > >> is. The master, upon seeing that header, then uses the slave's > >> localpath > >> instead of its own paths when generating responses. Is a header which > >> can > >> essentially cause a server to lie about its locations a bad/insecure > >> thing? > > > > > > ... but I don't see any serious discussion about it in the issue itself. > > The idea here being (if I can try to tap into my 17-years-ago-headstate...) > > that as the slave proxies a request through to the master, it tells the > > master "pretend, while generating protocol paths in your responses, that > > you're me". I think that with such a setup, we don't need the content > > filters at all? > > > > -- Mike > > > > [1] > > https://issues.apache.org/jira/browse/SVN-3445?focusedCommentId=14928254&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-14928254 >
