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

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