Apply a request rewrite to what the server is sent

A rewrite only reached the copy of the request kept for logging. On a
direct connection the server is sent the request line held in the header
buffer, which was parsed and reduced to its path before the filters ran, so
the rewrite showed up in the log and nowhere else. Through an HTTP parent
the rewritten copy is what goes out, which is why it appeared to work
there.

Splice the new path back into that buffer. The destination was chosen, and
the access rules applied to it, before the rewrite happened, so a rewrite
that changes the method or the authority is left alone: acting on it would
send the request somewhere the rules never saw. Say so in the manual.

The copy needed to notice a rewrite is only taken when a request filter
exists, so a proxy without one does no extra work.

man/3proxy.cfg.5

@@ -1485,6 +1485,12 @@ REGEXP - PCRE (Perl) regular expression. Use * if no regexp matching is required
 REWRITE_EXPRESSION - substitution string. May contain Perl-style substrings
 $1, $2, etc. $0 means the whole matched string. \er and \en may be used
 to insert new lines; the string may be empty ("").
+.br
+A rewritten request is what the server receives. The destination is chosen,
+and the access rules are applied to it, before the filters run, so a rewrite
+that names another host or changes the method is logged but not acted on:
+the request is still sent where the access rules allowed. Rewriting the path
+or the query works on a direct connection and through a parent alike.
 
 ACE - access control entry (user names, source IPs, destination IPs, ports, etc.),
 identical to allow/deny/bandlimin commands. The regular expression is only

src/proxy.c

@@ -156,6 +156,26 @@ static void freeptr(void *p){
 	if(*pp) { free(*pp); *pp = NULL; }
 }
 
+#ifndef WITHMAIN
+/* Point at the path in a request line and report the authority it names.
+   Returns NULL if the line is not one we can put back together. */
+static unsigned char * reqpath(unsigned char *line, unsigned char **host, int *hostlen)
+{
+	unsigned char *sp, *p;
+
+	*host = NULL;
+	*hostlen = 0;
+	if(!line || !(sp = (unsigned char *)strchr((char *)line, ' '))) return NULL;
+	while(*sp == ' ') sp++;
+	if(*sp == '/') return sp;
+	if(strncasecmp((char *)sp, "http://", 7)) return NULL;
+	*host = p = sp + 7;
+	while(*p && *p != '/' && *p != ' ') p++;
+	*hostlen = (int)(p - *host);
+	return (*p == '/')? p : NULL;
+}
+#endif
+
 static void logurl(struct clientparam * param, char * buf, char * req, int ftp){
  char *sb;
  char *se;
@@ -254,6 +274,7 @@ void * proxychild(struct clientparam* param) {
  int sleeptime = 0;
 #ifndef WITHMAIN
  int reqsize, reqbufsize;
+ unsigned char *origreq = NULL;
 #endif
  int authenticate;
  struct pollfd fds[2];
@@ -577,11 +598,51 @@ for(;;){
 
 #ifndef WITHMAIN
 
+ /* Only worth keeping a copy when something can rewrite it. */
+ if(param->nreqfilters) origreq = (unsigned char *)strdup((char *)req);
  action = handlereqfilters(param, &req, &reqbufsize, 0, &reqsize);
  if(action == HANDLED){
+	freeptr(&origreq);
 	RETURN(0);
  }
- if(action != PASS) RETURN(517);
+ if(action != PASS){
+	freeptr(&origreq);
+	RETURN(517);
+ }
+
+ /* Only the copy in req was rewritten. On a direct connection the server is
+    sent the request line held in buf, which was parsed and reduced to its
+    path before the filters ran, so put the new path there as well.
+
+    The destination was chosen, and the access rules applied to it, before
+    the rewrite happened. A rewrite that changes the method or the authority
+    is therefore left alone: acting on it would send the request somewhere
+    the rules never saw. */
+ if(origreq && !isconnect && !ftp && strcmp((char *)req, (char *)origreq)){
+	unsigned char *oldhost, *newhost, *oldpath, *newpath;
+	int oldhostlen, newhostlen, methodlen;
+
+	methodlen = (int)(strchr((char *)origreq, ' ') - (char *)origreq);
+	oldpath = reqpath(origreq, &oldhost, &oldhostlen);
+	newpath = reqpath(req, &newhost, &newhostlen);
+	if(oldpath && newpath
+	   && methodlen > 0 && !strncmp((char *)req, (char *)origreq, methodlen)
+	   && req[methodlen] == ' '
+	   && oldhostlen == newhostlen
+	   && (!oldhostlen || !strncasecmp((char *)oldhost, (char *)newhost, oldhostlen))){
+		int newlen = (int)strlen((char *)newpath);
+		int delta = newlen - ((int)reqlen - ssoff);
+
+		if(ssoff > 0 && (int)reqlen >= ssoff && inbuf + delta < bufsize - 1){
+			memmove(buf + ssoff + newlen, buf + reqlen, inbuf - reqlen + 1);
+			memcpy(buf + ssoff, newpath, newlen);
+			inbuf += delta;
+			reqlen += delta;
+			buf[inbuf] = 0;
+		}
+	}
+ }
+ freeptr(&origreq);
  action = handlehdrfilterscli(param, &buf, &bufsize, 0, &inbuf);
  if(action == HANDLED){
 	RETURN(0);

tests/cases/pcre.py

@@ -1,9 +1,10 @@
 """PCRE filtering: matching, rewriting, options and rule scope.
 
-Request rewriting only reaches the wire through an HTTP parent. On a direct
+A request rewrite is applied to the buffer the server is sent, so it works
-connection the request has already been parsed and converted to origin form
+on a direct connection as well as through a parent. The destination was
-by the time the filter runs, so the rewrite shows up in the log and nowhere
+chosen, and the access rules applied to it, before the filter ran, so a
-else; that path is left alone here rather than pinned down as correct.
+rewrite that moves the request to another host or changes the method is
+ignored rather than acted on.
 """
 
 
@@ -97,7 +98,67 @@ def run(t):
     t.contains(r, "PEER.ADDR", "reply data can be rewritten")
     t.not_contains(r, "peer.addr", "the original text is gone")
 
-    # --- rewriting the request, which needs an HTTP parent -------------------
+    # --- rewriting the request ------------------------------------------------
+    p = proxy_with("rewrite_req", 'pcre_rewrite request dunno "/echo/old" "/echo/new"')
+    r = t.http(url + "/echo/old", proxy=p)
+    t.eq(200, r.status, "a rewritten request still arrives")
+    t.contains(r, "path=/echo/new", "the origin sees the rewritten path")
+
+    # the replacement may be longer or shorter than what it replaces
+    p = proxy_with("rewrite_long", 'pcre_rewrite request dunno "/echo/x" "/echo/deeper/still"')
+    t.contains(t.http(url + "/echo/x", proxy=p), "path=/echo/deeper/still",
+               "a longer replacement is spliced in")
+    p = proxy_with("rewrite_short", 'pcre_rewrite request dunno "/echo/aaaaaaaaaa" "/echo/b"')
+    t.contains(t.http(url + "/echo/aaaaaaaaaa", proxy=p), "path=/echo/b",
+               "a shorter replacement is spliced in")
+
+    p = proxy_with("rewrite_query", 'pcre_rewrite request dunno "token=old" "token=new"')
+    t.contains(t.http(url + "/echo?token=old", proxy=p), "query=token=new",
+               "the query can be rewritten")
+
+    p = proxy_with("rewrite_none", 'pcre_rewrite request dunno "/nothing" "/else"')
+    t.contains(t.http(url + "/echo/keep", proxy=p), "path=/echo/keep",
+               "a request that does not match is left alone")
+
+    # what follows the request line has to survive the splice
+    p = proxy_with("rewrite_post", 'pcre_rewrite request dunno "/echo/old" "/echo/new"')
+    r = t.http(url + "/echo/old", proxy=p, method="POST", body="hello",
+               headers={"Content-Type": "text/plain"})
+    t.contains(r, "path=/echo/new", "a POST is rewritten too")
+    t.contains(r, "content.length=5", "its body is still described correctly")
+
+    conn = t.connection("127.0.0.1", origin, proxy=p)
+    try:
+        first = t.http(url + "/echo/old", proxy=p, conn=conn)
+        second = t.http(url + "/echo/old", proxy=p, conn=conn)
+        t.contains(first, "path=/echo/new", "the first of two on a connection is rewritten")
+        t.contains(second, "path=/echo/new", "and so is the second")
+    finally:
+        conn.close()
+
+    # --- rewrites that would change where the request goes --------------------
+    elsewhere = t.free_port()
+    t.start("pcre_elsewhere", f"""
+        log
+        flush
+        auth iponly
+        allow *
+        http * /echo* echo
+        httpsrv -p{elsewhere}
+    """, ports=[elsewhere])
+
+    p = proxy_with("rewrite_host",
+                   f'pcre_rewrite request dunno "127.0.0.1:{origin}" "127.0.0.1:{elsewhere}"')
+    r = t.http(url + "/echo", proxy=p)
+    t.eq(200, r.status, "a rewrite naming another host still answers")
+    t.contains(r, f"host=127.0.0.1:{origin}",
+               "but the request goes where the access rules allowed")
+
+    p = proxy_with("rewrite_method", 'pcre_rewrite request dunno "^GET" "HEAD"')
+    t.contains(t.http(url + "/echo", proxy=p), "method=GET",
+               "a rewrite of the method is ignored")
+
+    # --- and the same rewrite through an HTTP parent --------------------------
     parent = t.free_port()
     t.start("pcre_parent", f"""
         log
@@ -106,8 +167,8 @@ def run(t):
         allow *
         proxy -p{parent}
     """, ports=[parent])
-    p = proxy_with("rewrite_req", 'pcre_rewrite request dunno "/echo/old" "/echo/new"',
+    p = proxy_with("rewrite_parent", 'pcre_rewrite request dunno "/echo/old" "/echo/new"',
                    f"parent 1000 http 127.0.0.1 {parent}")
     r = t.http(url + "/echo/old", proxy=p)
-    t.eq(200, r.status, "a rewritten request still arrives")
+    t.eq(200, r.status, "a rewritten request through a parent arrives")
-    t.contains(r, "path=/echo/new", "the origin sees the rewritten request")
+    t.contains(r, "path=/echo/new", "the origin sees the rewritten path through a parent")