Patch bugfix. I discovered the previous patch had a URL encoding issue that caused COPY/MOVE operations to fail for paths containing special characters (e.g., spaces).
The Destination header value is already URL-encoded by the client (e.g., "/svn/repo/file%20name.txt"). The fix now properly handles this by: 1. Encoding root_dir before comparison with the already-encoded path 2. Encoding master_path before concatenation 3. NOT re-encoding the final path (which would double-encode the remainder portion, turning %20 into %2520) This follows the same pattern used by the existing body rewrite filters (dav_svn__location_in_filter and dav_svn__location_body_filter), which also encode their search patterns because "incoming request body has it encoded already." I have attached the updated patch. Thanks, Jordan On Wed, 21 Jan 2026 at 10:30, Daniel Sahlberg <[email protected]> wrote: > Den tis 20 jan. 2026 kl 20:56 skrev Jordan Peck via dev < > [email protected]>: > >> Small update, after some testing I'm now running our company SVN server >> with this patch. Everything is working smoothly and the proxy issues are >> resolved. >> >> I've attached an updated version of the patch which fixes some code >> formatting. >> >> Thanks, >> Jordan >> > > Thank you so much for the patch and the detailed report! > > I've taken a quick look and it all seems good, but I need to study the > surrounding code in more detail to fully understand it. However I havn't > found the time yet. > > To any other takers! We have a test for this in > https://svn.apache.org/repos/asf/subversion/trunk/subversion/tests/cmdline/dav-mirror-autocheck.sh > > Updating the settings around lines 369-376 (ie, changing the > MASTER_LOCATION and SLAVE_LOCATION as indicated by the comments): > [[[ > # location directive elements for master,slave,sync > # tests currently work if master==slave,fail if different > # ** Should different locations for each work? > #MASTER_LOCATION="master" > #SLAVE_LOCATION="slave" > MASTER_LOCATION="repo" > SLAVE_LOCATION="repo" > SYNC_LOCATION="sync" > ]]] > > Without the patch, the test fails with: > [[[ > ... > dav-mirror-autocheck.sh: running svnmucc test to > http://127.0.0.1:1989/slave > r1 committed by jrandom at 2026-01-21T10:20:43.736512Z > r2 committed by jrandom at 2026-01-21T10:20:43.861355Z > svnmucc: E175015: The HTTP method 'COPY' is not allowed on > '/slave/!svn/rvr/2/trunk' > svnmucc: E160013: Can't delete node at 'branch' as it does not exist > ... > ]]] > > With the patch, the test succeeds, as it does when the MASTER_LOCATION and > SLAVE_LOCATION are the same. > > dav-mirror-autocheck.sh is run from a checkout of /trunk as follows: > $ APACHE_MPM=event ./subversion/tests/cmdline/dav-mirror-autocheck.sh > /tmp/test-work > > davautocheck is a separate target in Makefile. I'd like to add > dav-mirror-autocheck the same way (it seems to be easy enough) possibly > also making a "check it all" target for release testing. > > Cheers, > Daniel > > > >> On Mon, 19 Jan 2026 at 23:38, C. Michael Pilato <[email protected]> >> wrote: >> >>> Not really. I suspect the pool of reviewers is just small and occupied >>> with other activity. All volunteers 'round here. >>> >>> On Mon, Jan 19, 2026 at 12:48 PM Jordan Peck <[email protected]> >>> wrote: >>> >>>> Hi Mike, >>>> Thanks for having a look, is there anything else I have to do for this >>>> to be considered for merging into trunk? >>>> >>>> Thanks, >>>> Jordan >>>> >>>> On Wed, 14 Jan 2026 at 20:22, C. Michael Pilato <[email protected]> >>>> wrote: >>>> >>>>> I'm too long out of the game to offer meaningful review and testing on >>>>> this, but I definitely appreciate the thorough write-up. And given the >>>>> comments I left sprinkled in that source file (so many years ago...) about >>>>> how inefficient some of the filtering was when the master/slave repository >>>>> paths were identical, chances are pretty good that I did most of my >>>>> testing >>>>> in exactly that configuration. >>>>> >>>>> -- Mike >>>>> >>>>> On Wed, Jan 14, 2026 at 7:18 AM Jordan Peck via dev < >>>>> [email protected]> wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> I've identified and fixed a bug in mod_dav_svn's master/slave proxy >>>>>> functionality (SVNMasterURI) that causes COPY and MOVE operations to >>>>>> fail with a 400 error when the slave and master repositories are >>>>>> hosted at different paths. >>>>>> >>>>>> >>>>>> STEPS TO REPRODUCE >>>>>> ================== >>>>>> >>>>>> 1. Configure a master SVN server with the repository at /repo: >>>>>> >>>>>> # Master server (master-server.example.com) >>>>>> <Location /repo> >>>>>> DAV svn >>>>>> SVNPath /var/svn/myrepo >>>>>> </Location> >>>>>> >>>>>> 2. Configure a slave SVN server with the repository at a different >>>>>> path (/svn/repo) and SVNMasterURI pointing to the master: >>>>>> >>>>>> # Slave server (slave-server.example.com) >>>>>> <Location /svn/repo> >>>>>> DAV svn >>>>>> SVNPath /var/svn/myrepo-slave >>>>>> SVNMasterURI http://master-server.example.com/repo >>>>>> </Location> >>>>>> >>>>>> 3. Ensure the slave repository is a synced mirror of the master >>>>>> (via svnsync). >>>>>> >>>>>> 4. Attempt a COPY operation through the slave to create a branch: >>>>>> >>>>>> svn copy http://slave-server.example.com/svn/repo/trunk \ >>>>>> >>>>>> http://slave-server.example.com/svn/repo/branches/new-feature \ >>>>>> -m "Creating new feature branch" >>>>>> >>>>>> >>>>>> EXPECTED RESULT >>>>>> =============== >>>>>> >>>>>> The COPY request is proxied to the master and the branch is created >>>>>> successfully. >>>>>> >>>>>> >>>>>> ACTUAL RESULT >>>>>> ============= >>>>>> >>>>>> The operation fails with HTTP 400 Bad Request. Server logs show the >>>>>> master receiving a Destination header with path >>>>>> /svn/repo/branches/new-feature which does not exist on the master >>>>>> (the correct path would be /repo/branches/new-feature). >>>>>> >>>>>> >>>>>> ROOT CAUSE >>>>>> ========== >>>>>> >>>>>> In subversion/mod_dav_svn/mirror.c, the proxy_request_fixup() function >>>>>> correctly rewrites the Request-URI when proxying requests to the >>>>>> master. >>>>>> However, WebDAV COPY and MOVE operations specify their destination in >>>>>> the Destination HTTP header, which is not rewritten. >>>>>> >>>>>> The existing filters handle: >>>>>> - Request body rewriting (dav_svn__location_in_filter) >>>>>> - Response body rewriting (dav_svn__location_body_filter) >>>>>> - Response Location header rewriting (dav_svn__location_header_filter) >>>>>> >>>>>> But there is no rewriting for the incoming Destination request header. >>>>>> >>>>>> When paths are identical on slave and master, this bug is masked >>>>>> because the path portion extracted from the Destination header happens >>>>>> to be valid on both servers. The bug only manifests when the >>>>>> repository >>>>>> paths differ. >>>>>> >>>>>> >>>>>> FIX >>>>>> === >>>>>> >>>>>> The attached patch adds rewriting of the Destination header in >>>>>> proxy_request_fixup(). It parses the header, replaces the slave's root >>>>>> path with the master's path, and updates the header before the request >>>>>> is proxied. >>>>>> >>>>>> I have tested this fix and can confirm it fixes the issue in my >>>>>> testing. >>>>>> >>>>>> Please review the attached patch. >>>>>> >>>>>> Thanks, >>>>>> Jordan Peck >>>>>> >>>>>
move-copy-proxy-v3.patch
Description: Binary data
