Author: jpeck
Date: Wed Jul 8 20:23:32 2026
New Revision: 1936011
Log:
Fix URI-encoded vs non-encoded comparison of stored root dir
dav_svn__get_root_dir() returns the <Location> path canonicalized and
URI-encoded (create_dir_config() runs it through
svn_urlpath__canonicalize(), which normalizes by decode-then-encode),
but mirror.c treated it as decoded throughout. For any location path
containing a URI-escapable character this made the write-through proxy
silently non-functional:
- request routing searched the decoded r->uri for the encoded root,
never matching, so write requests were never proxied and failed on
the read-only slave;
- both body-rewrite filters URI-encoded the already-encoded root and
master paths a second time before compiling the search pattern
('%' is itself escaped, so "/svn%20repo" became "/svn%2520repo"),
a byte sequence that can never occur in real protocol bodies
- the COPY/MOVE Destination rewrite double-encoded the root the same
way, so the ancestor comparison against the (encoded, normalized)
Destination path always failed.
All latent for plain-ASCII locations, where the extra encode is a
no-op. Regression coverage coming in a follow-up commit.
* subversion/mod_dav_svn/mirror.c
(proxy_request_fixup_destination): Use the stored root dir directly
and correct the comment that misdescribed it as decoded.
(dav_svn__proxy_request_fixup): Decode the root once and match the
decoded form against the decoded r->uri.
(dav_svn__location_in_filter, dav_svn__location_body_filter): Drop
the double-encode of the pattern and replacement strings; both are
already in the wire domain.
Modified:
subversion/branches/proxy-move-copy-fix/subversion/mod_dav_svn/mirror.c
Modified:
subversion/branches/proxy-move-copy-fix/subversion/mod_dav_svn/mirror.c
==============================================================================
--- subversion/branches/proxy-move-copy-fix/subversion/mod_dav_svn/mirror.c
Wed Jul 8 20:16:06 2026 (r1936010)
+++ subversion/branches/proxy-move-copy-fix/subversion/mod_dav_svn/mirror.c
Wed Jul 8 20:23:32 2026 (r1936011)
@@ -53,14 +53,15 @@ proxy_request_fixup_destination(request_
return;
}
- /* apr_uri_parse() leaves dest_uri.path URI-encoded but
- dav_svn__get_root_dir() is decoded. Encode the root so the comparison
- and the rebuilt header both stay in the encoded domain. rel is then
- already encoded and must not be re-encoded, only master_uri is, just
- as dav_svn__location_header_filter() encodes it on the way back. */
+ /* apr_uri_parse() leaves dest_uri.path URI-encoded, and
+ dav_svn__get_root_dir() is also stored URI-encoded and canonical
+ (create_dir_config() runs it through svn_urlpath__canonicalize(),
+ which encodes; see mod_dav_svn.c). Canonicalizing DEST_PATH
+ normalizes its hex encoding to the same rules, so the ancestor
+ check compares like with like. REL comes out still encoded and
+ must not be re-encoded. */
dest_path = svn_urlpath__canonicalize(dest_uri.path, r->pool);
- root_dir = svn_path_uri_encode(dav_svn__get_root_dir(r), r->pool);
- root_dir = svn_urlpath__canonicalize(root_dir, r->pool);
+ root_dir = dav_svn__get_root_dir(r);
rel = svn_urlpath__skip_ancestor(root_dir, dest_path);
if (rel == NULL)
@@ -143,6 +144,11 @@ int dav_svn__proxy_request_fixup(request
if (root_dir && master_uri) {
const char *seg;
+ const char *root_dir_decoded;
+
+ /* ROOT_DIR is stored canonical and URI-encoded, but at fixup time
+ R->URI is the decoded path, compare like with like. */
+ root_dir_decoded = svn_path_uri_decode(root_dir, r->pool);
/* We know we can always safely handle these. */
if (r->method_number == M_REPORT ||
@@ -157,7 +163,7 @@ int dav_svn__proxy_request_fixup(request
transaction tree resources. */
if (r->method_number == M_PROPFIND ||
r->method_number == M_GET) {
- if ((seg = ap_strstr(r->uri, root_dir))) {
+ if ((seg = ap_strstr(r->uri, root_dir_decoded))) {
if (ap_strstr_c(seg, apr_pstrcat(r->pool, special_uri,
"/wrk/", SVN_VA_NULL))
|| ap_strstr_c(seg, apr_pstrcat(r->pool, special_uri,
@@ -165,7 +171,7 @@ int dav_svn__proxy_request_fixup(request
|| ap_strstr_c(seg, apr_pstrcat(r->pool, special_uri,
"/txr/", SVN_VA_NULL))) {
int rv;
- seg += strlen(root_dir);
+ seg += strlen(root_dir_decoded);
rv = proxy_request_fixup(r, master_uri, seg);
if (rv) return rv;
}
@@ -176,13 +182,13 @@ int dav_svn__proxy_request_fixup(request
/* If this is a write request aimed at a public URI (such as
MERGE, LOCK, UNLOCK, etc.) or any as-yet-unhandled request
using a "special URI", we have to doctor it a bit for proxying. */
- seg = ap_strstr(r->uri, root_dir);
+ seg = ap_strstr(r->uri, root_dir_decoded);
if (seg && (r->method_number == M_MERGE ||
r->method_number == M_LOCK ||
r->method_number == M_UNLOCK ||
ap_strstr_c(seg, special_uri))) {
int rv;
- seg += strlen(root_dir);
+ seg += strlen(root_dir_decoded);
rv = proxy_request_fixup(r, master_uri, seg);
if (rv) return rv;
return OK;
@@ -232,10 +238,10 @@ apr_status_t dav_svn__location_in_filter
return ap_get_brigade(f->next, bb, mode, block, readbytes);
}
- /* We are url encoding the current url and the master url
- as incoming(from client) request body has it encoded already. */
- canonicalized_uri = svn_path_uri_encode(canonicalized_uri, r->pool);
- root_dir = svn_path_uri_encode(root_dir, r->pool);
+ /* Both CANONICALIZED_URI and ROOT_DIR are already canonical and
+ URI-encoded (svn_urlpath__canonicalize() output and the stored
+ <Location> path, respectively), which is the same domain the
+ protocol bodies use on the wire. */
if (!f->ctx) {
ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
ctx->remotepath = canonicalized_uri;
@@ -375,10 +381,10 @@ apr_status_t dav_svn__location_body_filt
### rewrite (translate hrefs only, leave property values alone) rather
### than the blind byte substitution used here. */
- /* We are url encoding the current url and the master url
- as incoming(from master) request body has it encoded already. */
- canonicalized_uri = svn_path_uri_encode(canonicalized_uri, r->pool);
- root_dir = svn_path_uri_encode(root_dir, r->pool);
+ /* Both CANONICALIZED_URI and ROOT_DIR are already canonical and
+ URI-encoded (svn_urlpath__canonicalize() output and the stored
+ <Location> path, respectively), which is the same domain the
+ protocol bodies use on the wire. */
if (!f->ctx) {
ctx = f->ctx = apr_pcalloc(r->pool, sizeof(*ctx));
ctx->remotepath = canonicalized_uri;
