mod_proxy_fcgi: Handle CGI Local Redirect Responses

changes-entries/proxy-fcgi-local-redirect.txt

@@ -0,0 +1,3 @@
+  *) mod_proxy_fcgi: Handle CGI Local Redirect Responses according to
+     RFC 3875 section 6.2.2. PR 61430.
+     [Arturo Bernal]

docs/log-message-tags/next-number

@@ -1 +1 @@
-10635
+10636

modules/proxy/mod_proxy_fcgi.c

@@ -29,6 +29,7 @@ typedef struct {
 
 typedef struct {
     char *dirwalk_uri_path;
+    const char *local_redirect; /* URI path of a CGI local redirect response */
 } fcgi_req_config_t;
 
 /* We will assume FPM, but still differentiate */
@@ -593,6 +594,48 @@ static int handle_headers(request_rec *r, int *state,
     return 0;
 }
 
+/* What the CGI response headers did to the request, see local_redirect(). */
+typedef struct {
+    int nheaders;
+    int nerr_headers;
+    const char *content_type;
+    const char *status_line;
+} resp_snapshot;
+
+static void snapshot_response(request_rec *r, resp_snapshot *snap)
+{
+    snap->nheaders = apr_table_elts(r->headers_out)->nelts;
+    snap->nerr_headers = apr_table_elts(r->err_headers_out)->nelts;
+    snap->content_type = r->content_type;
+    snap->status_line = r->status_line;
+}
+
+/*
+ * Returns the local URI path of a CGI Local Redirect Response (RFC 3875,
+ * 6.2.2), which is a response without a status or a content type and with
+ * nothing but a Location of a URI path in its headers.
+ */
+static const char *local_redirect(request_rec *r, const resp_snapshot *before)
+{
+    resp_snapshot now;
+    const char *location;
+
+    snapshot_response(r, &now);
+    if (r->status != HTTP_OK
+            || now.status_line != before->status_line
+            || now.content_type != before->content_type
+            || now.nheaders != before->nheaders + 1
+            || now.nerr_headers != before->nerr_headers) {
+        return NULL;
+    }
+
+    location = apr_table_get(r->headers_out, "Location");
+    if (location && location[0] == '/' && location[1] != '/') {
+        return location;
+    }
+    return NULL;
+}
+
 static apr_status_t dispatch(proxy_conn_rec *conn, proxy_dir_conf *conf,
                              request_rec *r, apr_pool_t *setaside_pool,
                              apr_uint16_t request_id, const char **err,
@@ -601,6 +644,7 @@ static apr_status_t dispatch(proxy_conn_rec *conn, proxy_dir_conf *conf,
 {
     apr_bucket_brigade *ib, *ob;
     int seen_end_of_headers = 0, done = 0, ignore_body = 0;
+    int redirected = 0;
     apr_status_t rv = APR_SUCCESS;
     int script_error_status = HTTP_OK;
     conn_rec *c = r->connection;
@@ -828,11 +872,35 @@ static apr_status_t dispatch(proxy_conn_rec *conn, proxy_dir_conf *conf,
 
                         if (st == 1) {
                             int status;
+                            const char *location;
+                            resp_snapshot before;
                             seen_end_of_headers = 1;
 
+                            snapshot_response(r, &before);
                             status = ap_scan_script_header_err_brigade_ex(r, ob,
                                 NULL, APLOG_MODULE_INDEX);
 
+                            /* A local redirect is done by fcgi_request_status()
+                             * once the response is complete and the connection
+                             * to the backend is released, so read and drop the
+                             * rest of it like the body of a 304. */
+                            if (status == OK
+                                    && (location = local_redirect(r, &before))) {
+                                fcgi_req_config_t *rconf =
+                                    ap_get_module_config(r->request_config,
+                                                         &proxy_fcgi_module);
+
+                                if (rconf == NULL) {
+                                    rconf = apr_pcalloc(r->pool, sizeof(*rconf));
+                                    ap_set_module_config(r->request_config,
+                                                         &proxy_fcgi_module,
+                                                         rconf);
+                                }
+                                rconf->local_redirect = location;
+                                apr_table_unset(r->headers_out, "Location");
+                                redirected = ignore_body = 1;
+                            }
+
                             /* FCGI has its own body framing mechanism which we don't
                              * match against any provided Content-Length, so let the
                              * core determine C-L vs T-E based on what's actually sent.
@@ -942,7 +1010,7 @@ static apr_status_t dispatch(proxy_conn_rec *conn, proxy_dir_conf *conf,
                 } else {
                     /* XXX what if we haven't seen end of the headers yet? */
 
-                    if (script_error_status == HTTP_OK) {
+                    if (script_error_status == HTTP_OK && !redirected) {
                         b = apr_bucket_eos_create(c->bucket_alloc);
                         APR_BRIGADE_INSERT_TAIL(ob, b);
 
@@ -1384,10 +1452,42 @@ static const char *cmd_setenv(cmd_parms *cmd, void *in_dconf,
 
     return NULL;
 }
+
+/*
+ * Does the local redirect found by dispatch() once the request to the backend
+ * is complete, on the request_status hook.
+ */
+static int fcgi_request_status(int *status, request_rec *r)
+{
+    fcgi_req_config_t *rconf = ap_get_module_config(r->request_config,
+                                                    &proxy_fcgi_module);
+
+    if (*status == OK && rconf && rconf->local_redirect) {
+        const char *location = rconf->local_redirect;
+
+        ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(10635)
+                      "Internal redirect to %s", location);
+
+        r->status_line = NULL;
+        if (r->method_number != M_GET) {
+            /* keep HEAD, which is passed around as M_GET, too */
+            r->method = "GET";
+            r->method_number = M_GET;
+        }
+        apr_table_unset(r->headers_in, "Content-Length");
+        ap_internal_redirect_handler(location, r);
+        return OK;
+    }
+
+    return DECLINED;
+}
+
 static void register_hooks(apr_pool_t *p)
 {
     proxy_hook_scheme_handler(proxy_fcgi_handler, NULL, NULL, APR_HOOK_FIRST);
     proxy_hook_canon_handler(proxy_fcgi_canon, NULL, NULL, APR_HOOK_FIRST);
+    APR_OPTIONAL_HOOK(proxy, request_status, fcgi_request_status, NULL, NULL,
+                      APR_HOOK_MIDDLE);
 }
 
 static const command_rec command_table[] = {

test/modules/proxy/env.py

@@ -71,7 +71,8 @@ def __init__(self, env: 'HttpdTestEnv'):
         super().__init__(env=env)
         self.add_source_dir(os.path.dirname(inspect.getfile(ProxyTestSetup)))
         self.add_modules(["proxy", "proxy_http", "proxy_ajp", "proxy_balancer",
-                          "proxy_connect", "proxy_uwsgi", "lbmethod_byrequests",
+                          "proxy_connect", "proxy_uwsgi", "proxy_fcgi",
+                          "lbmethod_byrequests",
                           "remoteip"])
 
 
@@ -86,8 +87,8 @@ def __init__(self, pytestconfig=None):
         self._d_mixed = f"mixed.{self.http_tld}"
 
         self.add_httpd_log_modules(
-            ["proxy", "proxy_http", "proxy_balancer", "lbmethod_byrequests",
-             "ssl"])
+            ["proxy", "proxy_http", "proxy_fcgi", "proxy_balancer",
+             "lbmethod_byrequests", "ssl"])
         self.add_cert_specs([
             CertificateSpec(domains=[
                 self._d_forward, self._d_reverse, self._d_mixed

test/modules/proxy/fcgi_faker.py

@@ -0,0 +1,154 @@
+"""
+Minimal FastCGI Responder for testing mod_proxy_fcgi.
+
+Listens on a TCP port and answers every request with the CGI response which
+the test registered for its SCRIPT_NAME.  A connection is kept open for as
+many requests as the client sends on it, so that the tests can tell whether
+mod_proxy_fcgi reused a connection and whether it left it in a usable state.
+"""
+import socket
+import struct
+import threading
+
+FCGI_BEGIN_REQUEST = 1
+FCGI_END_REQUEST = 3
+FCGI_PARAMS = 4
+FCGI_STDIN = 5
+FCGI_STDOUT = 6
+
+MAX_CONTENT = 65535
+
+
+def _read_exact(sock, n):
+    data = b""
+    while len(data) < n:
+        chunk = sock.recv(n - len(data))
+        if not chunk:
+            return None
+        data += chunk
+    return data
+
+
+def _parse_params(data):
+    params = {}
+    pos = 0
+
+    def length():
+        nonlocal pos
+        if data[pos] >> 7:
+            n = struct.unpack_from(">I", data, pos)[0] & 0x7fffffff
+            pos += 4
+        else:
+            n = data[pos]
+            pos += 1
+        return n
+
+    while pos < len(data):
+        nlen = length()
+        vlen = length()
+        name = data[pos:pos + nlen].decode("latin-1")
+        pos += nlen
+        params[name] = data[pos:pos + vlen].decode("latin-1")
+        pos += vlen
+    return params
+
+
+def _record(rtype, request_id, content=b""):
+    return struct.pack(">BBHHBx", 1, rtype, request_id, len(content), 0) + content
+
+
+class FcgiFaker:
+
+    def __init__(self, host, port):
+        self._host = host
+        self._port = port
+        self._socket = None
+        self._thread = None
+        self._done = False
+        self._lock = threading.Lock()
+        self._conn_count = 0
+        # path -> exact CGI response (headers and body) sent over FCGI_STDOUT
+        self.routes = {}
+        # what the backend has seen, in order: one dict per request
+        self.requests = []
+
+    def start(self):
+        self._socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+        self._socket.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1)
+        self._socket.bind((self._host, self._port))
+        self._socket.listen(8)
+        self._socket.settimeout(0.5)
+        self._thread = threading.Thread(target=self._accept, daemon=True)
+        self._thread.start()
+
+    def stop(self):
+        self._done = True
+        self._thread.join(timeout=5)
+        self._socket.close()
+
+    def reset(self):
+        with self._lock:
+            self.requests = []
+
+    def _accept(self):
+        while not self._done:
+            try:
+                conn, _ = self._socket.accept()
+            except socket.timeout:
+                continue
+            except OSError:
+                return
+            with self._lock:
+                self._conn_count += 1
+                conn_id = self._conn_count
+            threading.Thread(target=self._serve, args=(conn, conn_id),
+                             daemon=True).start()
+
+    def _serve(self, conn, conn_id):
+        conn.settimeout(30)
+        request_id = 0
+        params = b""
+        stdin = b""
+        try:
+            while True:
+                header = _read_exact(conn, 8)
+                if header is None:
+                    return
+                _, rtype, request_id_, clen, plen = struct.unpack(">BBHHBx", header)
+                content = _read_exact(conn, clen) if clen else b""
+                if plen:
+                    _read_exact(conn, plen)
+                if rtype == FCGI_BEGIN_REQUEST:
+                    request_id = request_id_
+                    params = b""
+                    stdin = b""
+                elif rtype == FCGI_PARAMS:
+                    params += content
+                elif rtype == FCGI_STDIN:
+                    stdin += content
+                    if clen == 0:
+                        self._respond(conn, conn_id, request_id,
+                                      _parse_params(params), stdin)
+        except OSError:
+            return
+        finally:
+            conn.close()
+
+    def _respond(self, conn, conn_id, request_id, params, stdin):
+        # SCRIPT_NAME is the path of the request, REQUEST_URI stays the one
+        # the client sent when the server redirects internally.
+        path = params.get("SCRIPT_NAME", "")
+        with self._lock:
+            self.requests.append({"conn": conn_id, "uri": path,
+                                  "params": params, "stdin": stdin})
+        if path in self.routes:
+            out = self.routes[path]
+            if callable(out):
+                out = out(params, stdin)
+        else:
+            out = b"Status: 404 Not Found\r\nContent-Type: text/plain\r\n\r\nno route\n"
+        for i in range(0, len(out), MAX_CONTENT):
+            conn.sendall(_record(FCGI_STDOUT, request_id, out[i:i + MAX_CONTENT]))
+        conn.sendall(_record(FCGI_STDOUT, request_id))
+        conn.sendall(_record(FCGI_END_REQUEST, request_id,
+                             struct.pack(">IBxxx", 0, 0)))