mod_proxy: Fix canonicalisation and FCGI env (PATH_INFO, SCRIPT_NAME) for

           "balancer:" URLs set via SetHandler, also allowing for "unix:"
           sockets with BalancerMember(s).  PR 69168.

* modules/proxy/proxy_util.h, modules/proxy/proxy_util.c:
  Move proxy_interpolate() from mod_proxy.c to ap_proxy_interpolate(),
  exported locally only (non public).
  Move proxy_fixup() from mod_proxy.c to ap_proxy_canon_url(), exported
  locally only too (non public).
  Rollback ap_proxy_fixup_uds_filename() to a local fixup_uds_filename()
  usable from proxy_util.c only. The public function will be removed in
  a following commit.

* modules/proxy/mod_proxy.h:
  Note that ap_proxy_fixup_uds_filename() is deprecated.

* modules/proxy/mod_proxy.c:
  Just use ap_proxy_canon_url() from proxy_fixup() and proxy_handler()
  for SetHandler URLs.

* modules/proxy/mod_proxy_balancer.c:
  Do not canonicalize the path from proxy_balancer_canon() anymore but
  rather from balancer_fixup() where the balancer URL is rewritten to
  the BalancerMember URL.



git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1919022 13f79535-47bb-0310-9956-ffa450edef68

6 files changed, 176 insertions, 181 deletions

diff --git a/changes-entries/pr69168.txt b/changes-entries/pr69168.txt
new file mode 100644
index 0000000000..c5304ec424
--- /dev/null
+++ b/changes-entries/pr69168.txt
@@ -0,0 +1,3 @@
+  *) mod_proxy: Fix canonicalisation and FCGI env (PATH_INFO, SCRIPT_NAME) for
+     "balancer:" URLs set via SetHandler, also allowing for "unix:" sockets
+     with BalancerMember(s).  PR 69168.  [Yann Ylavic]
diff --git a/modules/proxy/mod_proxy.c b/modules/proxy/mod_proxy.c
index f3e1a0909a..f0b73fa1e0 100644
--- a/modules/proxy/mod_proxy.c
+++ b/modules/proxy/mod_proxy.c
@@ -828,60 +828,6 @@ static int proxy_detect(request_rec *r)
     return DECLINED;
 }
 
-static const char *proxy_interpolate(request_rec *r, const char *str)
-{
-    /* Interpolate an env str in a configuration string
-     * Syntax ${var} --> value_of(var)
-     * Method: replace one var, and recurse on remainder of string
-     * Nothing clever here, and crap like nested vars may do silly things
-     * but we'll at least avoid sending the unwary into a loop
-     */
-    const char *start;
-    const char *end;
-    const char *var;
-    const char *val;
-    const char *firstpart;
-
-    start = ap_strstr_c(str, "${");
-    if (start == NULL) {
-        return str;
-    }
-    end = ap_strchr_c(start+2, '}');
-    if (end == NULL) {
-        return str;
-    }
-    /* OK, this is syntax we want to interpolate.  Is there such a var ? */
-    var = apr_pstrmemdup(r->pool, start+2, end-(start+2));
-    val = apr_table_get(r->subprocess_env, var);
-    firstpart = apr_pstrmemdup(r->pool, str, (start-str));
-
-    if (val == NULL) {
-        return apr_pstrcat(r->pool, firstpart,
-                           proxy_interpolate(r, end+1), NULL);
-    }
-    else {
-        return apr_pstrcat(r->pool, firstpart, val,
-                           proxy_interpolate(r, end+1), NULL);
-    }
-}
-static apr_array_header_t *proxy_vars(request_rec *r,
-                                      apr_array_header_t *hdr)
-{
-    int i;
-    apr_array_header_t *ret = apr_array_make(r->pool, hdr->nelts,
-                                             sizeof (struct proxy_alias));
-    struct proxy_alias *old = (struct proxy_alias *) hdr->elts;
-
-    for (i = 0; i < hdr->nelts; ++i) {
-        struct proxy_alias *newcopy = apr_array_push(ret);
-        newcopy->fake = (old[i].flags & PROXYPASS_INTERPOLATE)
-                        ? proxy_interpolate(r, old[i].fake) : old[i].fake;
-        newcopy->real = (old[i].flags & PROXYPASS_INTERPOLATE)
-                        ? proxy_interpolate(r, old[i].real) : old[i].real;
-    }
-    return ret;
-}
-
 PROXY_DECLARE(int) ap_proxy_trans_match(request_rec *r, struct proxy_alias *ent,
                                         proxy_dir_conf *dconf)
 {
@@ -897,8 +843,8 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_rec *r, struct proxy_alias *ent,
     const char *servlet_uri = NULL;
 
     if (dconf && (dconf->interpolate_env == 1) && (ent->flags & PROXYPASS_INTERPOLATE)) {
-        fake = proxy_interpolate(r, ent->fake);
-        real = proxy_interpolate(r, ent->real);
+        fake = ap_proxy_interpolate(r, ent->fake);
+        real = ap_proxy_interpolate(r, ent->real);
     }
     else {
         fake = ent->fake;
@@ -1218,38 +1164,12 @@ static int proxy_map_location(request_rec *r)
  */
 static int proxy_fixup(request_rec *r)
 {
-    char *url, *p;
-    int access_status;
-    proxy_dir_conf *dconf = ap_get_module_config(r->per_dir_config,
-                                                 &proxy_module);
-
     if (!r->proxyreq || !r->filename || strncmp(r->filename, "proxy:", 6) != 0)
         return DECLINED;
 
     /* XXX: Shouldn't we try this before we run the proxy_walk? */
-    url = &r->filename[6];
 
-    if ((dconf->interpolate_env == 1) && (r->proxyreq == PROXYREQ_REVERSE)) {
-        /* create per-request copy of reverse proxy conf,
-         * and interpolate vars in it
-         */
-        proxy_req_conf *rconf = apr_palloc(r->pool, sizeof(proxy_req_conf));
-        ap_set_module_config(r->request_config, &proxy_module, rconf);
-        rconf->raliases = proxy_vars(r, dconf->raliases);
-        rconf->cookie_paths = proxy_vars(r, dconf->cookie_paths);
-        rconf->cookie_domains = proxy_vars(r, dconf->cookie_domains);
-    }
-
-    /* canonicalise each specific scheme */
-    if ((access_status = proxy_run_canon_handler(r, url))) {
-        return access_status;
-    }
-
-    p = strchr(url, ':');
-    if (p == NULL || p == url)
-        return HTTP_BAD_REQUEST;
-
-    return OK;      /* otherwise; we've done the best we can */
+    return ap_proxy_canon_url(r);
 }
 
 /* Send a redirection if the request contains a hostname which is not */
@@ -1327,11 +1247,8 @@ static int proxy_handler(request_rec *r)
         r->proxyreq = PROXYREQ_REVERSE;
         r->filename = apr_pstrcat(r->pool, r->handler, r->filename, NULL);
 
-        /* Still need to fixup/canonicalize r->filename */
-        rc = ap_proxy_fixup_uds_filename(r);
-        if (rc <= OK) {
-            rc = proxy_fixup(r);
-        }
+        /* Still need to canonicalize r->filename */
+        rc = ap_proxy_canon_url(r);
         if (rc != OK) {
             r->filename = old_filename;
             r->proxyreq = 0;
@@ -1344,6 +1261,15 @@ static int proxy_handler(request_rec *r)
         return rc;
     }
 
+    uri = r->filename + 6;
+    p = strchr(uri, ':');
+    if (p == NULL) {
+        ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01141)
+                      "proxy_handler no URL in %s", r->filename);
+        return HTTP_BAD_REQUEST;
+    }
+    scheme = apr_pstrmemdup(r->pool, uri, p - uri);
+
     /* handle max-forwards / OPTIONS / TRACE */
     if ((str = apr_table_get(r->headers_in, "Max-Forwards"))) {
         char *end;
@@ -1423,14 +1349,6 @@ static int proxy_handler(request_rec *r)
         }
     }
 
-    uri = r->filename + 6;
-    p = strchr(uri, ':');
-    if (p == NULL) {
-        ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(01141)
-                      "proxy_handler no URL in %s", r->filename);
-        return HTTP_BAD_REQUEST;
-    }
-
     /* If the host doesn't have a domain name, add one and redirect. */
     if (conf->domain != NULL) {
         rc = proxy_needsdomain(r, uri, conf->domain);
@@ -1438,7 +1356,6 @@ static int proxy_handler(request_rec *r)
             return HTTP_MOVED_PERMANENTLY;
     }
 
-    scheme = apr_pstrmemdup(r->pool, uri, p - uri);
     /* Check URI's destination host against NoProxy hosts */
     /* Bypass ProxyRemote server lookup if configured as NoProxy */
     for (direct_connect = i = 0; i < conf->dirconn->nelts &&
diff --git a/modules/proxy/mod_proxy.h b/modules/proxy/mod_proxy.h
index 316313d098..cf6d5057df 100644
--- a/modules/proxy/mod_proxy.h
+++ b/modules/proxy/mod_proxy.h
@@ -1034,6 +1034,7 @@ PROXY_DECLARE(proxy_balancer_shared *) ap_proxy_find_balancershm(ap_slotmem_prov
  * r->notes ("uds_path")
  * @param r        current request
  * @return         OK if fixed up, DECLINED if not UDS, or an HTTP_XXX error
+ * @remark Deprecated (for internal use only)
  */
 PROXY_DECLARE(int) ap_proxy_fixup_uds_filename(request_rec *r);
 
diff --git a/modules/proxy/mod_proxy_balancer.c b/modules/proxy/mod_proxy_balancer.c
index 400b723ee9..38ff05c7f2 100644
--- a/modules/proxy/mod_proxy_balancer.c
+++ b/modules/proxy/mod_proxy_balancer.c
@@ -70,23 +70,21 @@ extern void proxy_update_members(proxy_balancer **balancer, request_rec *r,
 
 static int proxy_balancer_canon(request_rec *r, char *url)
 {
-    char *host, *path;
-    char *search = NULL;
-    const char *err;
+    char *host;
     apr_port_t port = 0;
+    const char *err;
 
     /* TODO: offset of BALANCER_PREFIX ?? */
     if (ap_cstr_casecmpn(url, "balancer:", 9) == 0) {
+        ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, "canonicalising URL %s", url);
         url += 9;
     }
     else {
         return DECLINED;
     }
 
-    ap_log_rerror(APLOG_MARK, APLOG_TRACE1, 0, r, "canonicalising URL %s", url);
-
     /* do syntatic check.
-     * We break the URL into host, port, path, search
+     * We break the URL into host, port, path
      */
     err = ap_proxy_canon_netloc(r->pool, &url, NULL, NULL, &host, &port);
     if (err) {
@@ -95,50 +93,12 @@ static int proxy_balancer_canon(request_rec *r, char *url)
                       url, err);
         return HTTP_BAD_REQUEST;
     }
-    /*
-     * now parse path/search args, according to rfc1738:
-     * process the path. With proxy-noncanon set (by
-     * mod_proxy) we use the raw, unparsed uri
-     */
-    if (apr_table_get(r->notes, "proxy-nocanon")) {
-        path = url;   /* this is the raw path */
-    }
-    else if (apr_table_get(r->notes, "proxy-noencode")) {
-        path = url;   /* this is the encoded path already */
-        search = r->args;
-    }
-    else {
-        core_dir_config *d = ap_get_core_module_config(r->per_dir_config);
-        int flags = d->allow_encoded_slashes && !d->decode_encoded_slashes ? PROXY_CANONENC_NOENCODEDSLASHENCODING : 0;
 
-        path = ap_proxy_canonenc_ex(r->pool, url, strlen(url), enc_path, flags,
-                                    r->proxyreq);
-        if (!path) {
-            return HTTP_BAD_REQUEST;
-        }
-        search = r->args;
-    }
-    /*
-     * If we have a raw control character or a ' ' in nocanon path or
-     * r->args, correct encoding was missed.
+    /* The canon_handler hooks are run per the BalancerMember in
+     * balancer_fixup(), keep the original/raw path for now.
      */
-    if (path == url && *ap_scan_vchar_obstext(path)) {
-        ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10416)
-                      "To be forwarded path contains control "
-                      "characters or spaces");
-        return HTTP_FORBIDDEN;
-    }
-    if (search && *ap_scan_vchar_obstext(search)) {
-         ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(10407)
-                       "To be forwarded query string contains control "
-                       "characters or spaces");
-         return HTTP_FORBIDDEN;
-    }
-
-    r->filename = apr_pstrcat(r->pool, "proxy:" BALANCER_PREFIX, host,
-            "/", path, (search) ? "?" : "", (search) ? search : "", NULL);
-
-    r->path_info = apr_pstrcat(r->pool, "/", path, NULL);
+    r->filename = apr_pstrcat(r->pool, "proxy:" BALANCER_PREFIX,
+                              host, "/", url, NULL);
 
     return OK;
 }
@@ -428,25 +388,25 @@ static proxy_worker *find_best_worker(proxy_balancer *balancer,
 
 }
 
-static int rewrite_url(request_rec *r, proxy_worker *worker,
-                        char **url)
+static int balancer_fixup(request_rec *r, proxy_worker *worker, char **url)
 {
-    const char *scheme = strstr(*url, "://");
-    const char *path = NULL;
+    const char *path;
+    int rc;
 
-    if (scheme)
-        path = ap_strchr_c(scheme + 3, '/');
-
-    /* we break the URL into host, port, uri */
-    if (!worker) {
-        return ap_proxyerror(r, HTTP_BAD_REQUEST, apr_pstrcat(r->pool,
-                             "missing worker. URI cannot be parsed: ", *url,
-                             NULL));
+    /* Build the proxy URL from the worker URL and the actual path */
+    path = strstr(*url, "://");
+    if (path) {
+        path = ap_strchr_c(path + 3, '/');
     }
+    r->filename = apr_pstrcat(r->pool, "proxy:", worker->s->name, path, NULL);
 
-    *url = apr_pstrcat(r->pool, worker->s->name, path, NULL);
-
-    return OK;
+    /* Canonicalize r->filename per the worker scheme's canon_handler hook */
+    rc = ap_proxy_canon_url(r);
+    if (rc == OK) {
+        AP_DEBUG_ASSERT(strncmp(r->filename, "proxy:", 6) == 0);
+        *url = apr_pstrdup(r->pool, r->filename + 6);
+    }
+    return rc;
 }
 
 static void force_recovery(proxy_balancer *balancer, server_rec *s)
@@ -503,7 +463,8 @@ static int proxy_balancer_pre_request(proxy_worker **worker,
      * for balancer, because this is failover attempt.
      */
     if (!*balancer &&
-        !(*balancer = ap_proxy_get_balancer(r->pool, conf, *url, 1)))
+        (ap_cstr_casecmpn(*url, BALANCER_PREFIX, sizeof(BALANCER_PREFIX) - 1)
+         || !(*balancer = ap_proxy_get_balancer(r->pool, conf, *url, 1))))
         return DECLINED;
 
     /* Step 2: Lock the LoadBalancer
@@ -637,10 +598,12 @@ static int proxy_balancer_pre_request(proxy_worker **worker,
 
     /* Rewrite the url from 'balancer://url'
      * to the 'worker_scheme://worker_hostname[:worker_port]/url'
-     * This replaces the balancers fictional name with the
-     * real hostname of the elected worker.
+     * This replaces the balancers fictional name with the real
+     * hostname of the elected worker and canonicalizes according
+     * to the worker scheme (calls canon_handler hooks).
      */
-    access_status = rewrite_url(r, *worker, url);
+    access_status = balancer_fixup(r, *worker, url);
+
     /* Add the session route to request notes if present */
     if (route) {
         apr_table_setn(r->notes, "session-sticky", sticky);
diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c
index 4ba3bfe639..a8619197c4 100644
--- a/modules/proxy/proxy_util.c
+++ b/modules/proxy/proxy_util.c
@@ -2425,7 +2425,7 @@ static int ap_proxy_retry_worker(const char *proxy_function, proxy_worker *worke
  * were passed a UDS url (eg: from mod_proxy) and adjust uds_path
  * as required.  
  */
-PROXY_DECLARE(int) ap_proxy_fixup_uds_filename(request_rec *r) 
+static int fixup_uds_filename(request_rec *r) 
 {
     char *uds_url = r->filename + 6, *origin_url;
 
@@ -2477,9 +2477,115 @@ PROXY_DECLARE(int) ap_proxy_fixup_uds_filename(request_rec *r)
         return OK;
     }
 
+    apr_table_unset(r->notes, "uds_path");
     return DECLINED;
 }
 
+/* Deprecated (unused upstream) */
+PROXY_DECLARE(int) ap_proxy_fixup_uds_filename(request_rec *r)
+{
+    return fixup_uds_filename(r);
+}
+
+PROXY_DECLARE(const char *) ap_proxy_interpolate(request_rec *r,
+                                                 const char *str)
+{
+    /* Interpolate an env str in a configuration string
+     * Syntax ${var} --> value_of(var)
+     * Method: replace one var, and recurse on remainder of string
+     * Nothing clever here, and crap like nested vars may do silly things
+     * but we'll at least avoid sending the unwary into a loop
+     */
+    const char *start;
+    const char *end;
+    const char *var;
+    const char *val;
+    const char *firstpart;
+
+    start = ap_strstr_c(str, "${");
+    if (start == NULL) {
+        return str;
+    }
+    end = ap_strchr_c(start+2, '}');
+    if (end == NULL) {
+        return str;
+    }
+    /* OK, this is syntax we want to interpolate.  Is there such a var ? */
+    var = apr_pstrmemdup(r->pool, start+2, end-(start+2));
+    val = apr_table_get(r->subprocess_env, var);
+    firstpart = apr_pstrmemdup(r->pool, str, (start-str));
+
+    if (val == NULL) {
+        return apr_pstrcat(r->pool, firstpart,
+                           ap_proxy_interpolate(r, end+1), NULL);
+    }
+    else {
+        return apr_pstrcat(r->pool, firstpart, val,
+                           ap_proxy_interpolate(r, end+1), NULL);
+    }
+}
+
+static apr_array_header_t *proxy_vars(request_rec *r, apr_array_header_t *hdr)
+{
+    int i;
+    apr_array_header_t *ret = apr_array_make(r->pool, hdr->nelts,
+                                             sizeof (struct proxy_alias));
+    struct proxy_alias *old = (struct proxy_alias *) hdr->elts;
+
+    for (i = 0; i < hdr->nelts; ++i) {
+        struct proxy_alias *newcopy = apr_array_push(ret);
+        newcopy->fake = (old[i].flags & PROXYPASS_INTERPOLATE)
+                        ? ap_proxy_interpolate(r, old[i].fake) : old[i].fake;
+        newcopy->real = (old[i].flags & PROXYPASS_INTERPOLATE)
+                        ? ap_proxy_interpolate(r, old[i].real) : old[i].real;
+    }
+    return ret;
+}
+
+PROXY_DECLARE(int) ap_proxy_canon_url(request_rec *r)
+{
+    char *url, *p;
+    int access_status;
+    proxy_dir_conf *dconf = ap_get_module_config(r->per_dir_config,
+                                                 &proxy_module);
+
+    if (!r->proxyreq || !r->filename || strncmp(r->filename, "proxy:", 6) != 0)
+        return DECLINED;
+
+    /* Put the UDS path appart if any (and not already stripped) */
+    if (r->proxyreq == PROXYREQ_REVERSE) {
+        access_status = fixup_uds_filename(r);
+        if (ap_is_HTTP_ERROR(access_status)) {
+            return access_status;
+        }
+    }
+
+    /* Keep this after fixup_uds_filename() */
+    url = &r->filename[6];
+
+    if ((dconf->interpolate_env == 1) && (r->proxyreq == PROXYREQ_REVERSE)) {
+        /* create per-request copy of reverse proxy conf,
+         * and interpolate vars in it
+         */
+        proxy_req_conf *rconf = apr_palloc(r->pool, sizeof(proxy_req_conf));
+        ap_set_module_config(r->request_config, &proxy_module, rconf);
+        rconf->raliases = proxy_vars(r, dconf->raliases);
+        rconf->cookie_paths = proxy_vars(r, dconf->cookie_paths);
+        rconf->cookie_domains = proxy_vars(r, dconf->cookie_domains);
+    }
+
+    /* canonicalise each specific scheme */
+    if ((access_status = proxy_run_canon_handler(r, url))) {
+        return access_status;
+    }
+
+    p = strchr(url, ':');
+    if (p == NULL || p == url)
+        return HTTP_BAD_REQUEST;
+
+    return OK;      /* otherwise; we've done the best we can */
+}
+
 PROXY_DECLARE(int) ap_proxy_pre_request(proxy_worker **worker,
                                         proxy_balancer **balancer,
                                         request_rec *r,
@@ -2489,16 +2595,16 @@ PROXY_DECLARE(int) ap_proxy_pre_request(proxy_worker **worker,
 
     access_status = proxy_run_pre_request(worker, balancer, r, conf, url);
     if (access_status == DECLINED && *balancer == NULL) {
-        const int forward = (r->proxyreq == PROXYREQ_PROXY);
+        /* UDS path stripped from *url by proxy_fixup() already */
         *worker = ap_proxy_get_worker_ex(r->pool, NULL, conf, *url,
-                                         forward ? AP_PROXY_WORKER_NO_UDS : 0);
+                                         AP_PROXY_WORKER_NO_UDS);
         if (*worker) {
             ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
                           "%s: found worker %s for %s",
                           (*worker)->s->scheme, (*worker)->s->name, *url);
             access_status = OK;
         }
-        else if (forward) {
+        else if (r->proxyreq == PROXYREQ_PROXY) {
             if (conf->forward) {
                 ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
                               "*: found forward proxy worker for %s", *url);
@@ -2535,19 +2641,6 @@ PROXY_DECLARE(int) ap_proxy_pre_request(proxy_worker **worker,
         access_status = HTTP_SERVICE_UNAVAILABLE;
     }
 
-    if (access_status == OK && r->proxyreq == PROXYREQ_REVERSE) {
-        int rc = ap_proxy_fixup_uds_filename(r);
-        if (ap_is_HTTP_ERROR(rc)) {
-            return rc;
-        }
-        /* If the URL has changed in r->filename, take everything after
-         * the "proxy:" prefix.
-         */
-        if (rc == OK) {
-            *url = apr_pstrdup(r->pool, r->filename + 6);
-        }
-    }
-
     return access_status;
 }
 
diff --git a/modules/proxy/proxy_util.h b/modules/proxy/proxy_util.h
index 42d0f89811..9602dcb067 100644
--- a/modules/proxy/proxy_util.h
+++ b/modules/proxy/proxy_util.h
@@ -75,6 +75,24 @@ PROXY_DECLARE(apr_status_t) ap_proxy_decrement_busy_count(void *worker_);
  */
 PROXY_DECLARE(void) ap_proxy_increment_busy_count(proxy_worker *worker);
 
+
+/*
+ * interpolate an env str in a configuration string
+ *
+ * @param r    current request
+ * @param str  the string to interpolcate
+ * @return     the interpolated string
+ */
+PROXY_DECLARE(const char *) ap_proxy_interpolate(request_rec *r,
+                                                 const char *str);
+
+/*
+ * Canonicalize the URL in r->filename
+ * @param r           current request
+ * @return            OK or an HTTP_XXX error
+ */
+PROXY_DECLARE(int) ap_proxy_canon_url(request_rec *r);
+
 /** @} */
 
 #endif /* PROXY_UTIL_H_ */