Hi Mike, I took a bit of a break on this, but I've come back and discovered another issue linked to 3445. If a directory path inside your SVN repo contains the slave location it will get incorrectly translated by the filter. For example a file with path /slave/my/svn/repo/dir/slave/file.txt will get translated to /master/my/svn/repo/dir/master/file.txt This circumvents all the added whitelisting I've done since it's in the same string as the /slave location that does need translating.
The good news is that this led me to a fix that resolves this new issue and also covers the last remaining edge case with PROPFINDs I mentioned in my previous post. The solution was to instead of searching for the string "/slave" to replace instead search for "<D:href>/slave" because all of the emitters of URLs across serf and mod_dav_svn request/response bodies always use this prefix. You might be thinking this just shifts the issue to someone having "<D:href>/slave" in their user data, but all userdata (in whitelisted filters) is escaped so on the wire it would be "<D:href>/slave" which won't trigger the translation. While I think this is now a complete fix to 3445 it's still not structurally solved. A new HTTP message type could be add to SVN for a new feature that doesn't fit the expected format of this still basic string find and replace. An XML aware proxy is the structurally sound fix, but I really don't think there's any reason to do it now, given how large a task it would be. Keeping future message type's proxy compliant should be fairly easy with the whitelists and some awareness, which hopefully the expanded proxy tests bring. This latest fix is committed at 1937142 in the proxy-move-copy-fix branch. It also includes full test coverage of every request/response that can pass through the proxy, even old V1 checkout. Hopefully this is the end of the rabbit hole, and I'm fairly confident it is with the latest fix. Let me know what you think. I will get to updating the jira ticket, just not tonight. Thanks, Jordan On 2026/07/12 19:04:55 "C. Michael Pilato" wrote: > On Fri, Jul 10, 2026 at 6:31 PM Jordan Peck <[email protected]> wrote: > > > 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. > > > > If you have access to the Jira instance, it would be awesome to have this > list ^ recorded as part of SVN-3445! > > > > 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. > > > > Would it make sense to address these two categories of fixes separately > (and get them reviewed and potentially merged ASAP)? I'd hate for > Subversion to miss out on what I suspect are some relatively > straightforward and non-controversial improvements because they are > intertwingled with some of the more complex and nuanced changes you > describe below. > > > > 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. > > > > (I'd need to look at these changes to make sense of the above, but am not > in a good spot to do so. Will try to do so later.) > > > > 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. > > > This also feels like a great contribution to the trunk/ ASAP, even if the > tests are marked as expected failures. > > > > 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. > > > > Hrm, I'd say it's not a "complete non-issue" merely because Subversion > clients might not trigger a particular codepath. For example, mod_dav_svn > tries to implement the WebDAV specification even though Subversion clients > don't speak that protocol. So if you can make something blow up using a > series of curl commands, it's still a bug ... though admittedly perhaps one > with a small blast radius. > > This has all been a much larger rabbit hole than I expected when I first > > started looking into the 3445 issue. > > > I'm so sorry. 😂 But hopefully now you can see why I was so hesitant to > just +1 your original patch. I *knew* there was some rot under the > floorboards ... it just took me a while to put my finger on where it was. > > -- Mike >
