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
