Fix issue #1346 {path} logging {uri} and add {rewrite_uri} placeholder (#1481)

* Fixed issue with {path} actually {uri}

* Test added for path rewrite

* add in uri_escaped

* added rewrite_uri and test

* fix broken test.  Just checks for existance of rewrite header

* gitignore

* Use context to store uri value

* ignore .vscode

* tidy up, removal of comments and invalidated tests

* Remove commented out code.

* added comment as requested by lint

* fixed spelling mistake

* clarified code with variable name

* added context for uri and test

* added TODO comment to move consts

.gitignore

@@ -14,4 +14,6 @@ access.log
 /*.conf
 Caddyfile
 
-og_static/
\ No newline at end of file
+og_static/
+
+.vscode/
\ No newline at end of file

caddy.go

@@ -871,9 +871,15 @@ var (
 )
 
 // CtxKey is a value for use with context.WithValue.
+// TODO: Ideally CtxKey and consts will be moved to httpserver package.
+// currently blocked by circular import with staticfiles.
 type CtxKey string
 
 // URLPathCtxKey is a context key. It can be used in HTTP handlers with
 // context.WithValue to access the original request URI that accompanied the
 // server request. The associated value will be of type string.
 const URLPathCtxKey CtxKey = "url_path"
+
+// URIxRewriteCtxKey is a context key used to store original unrewritten
+// URI in context.WithValue
+const URIxRewriteCtxKey CtxKey = "caddy_rewrite_original_uri"

caddyhttp/fastcgi/fastcgi.go

@@ -33,11 +33,6 @@ type Handler struct {
 	ServerPort      string
 }
 
-// When a rewrite is performed, a header field of this name
-// is added to the request
-// It contains the original request URI before the rewrite.
-const internalRewriteFieldName = "Caddy-Rewrite-Original-URI"
-
 // ServeHTTP satisfies the httpserver.Handler interface.
 func (h Handler) ServeHTTP(w http.ResponseWriter, r *http.Request) (int, error) {
 	for _, rule := range h.Rules {
@@ -219,12 +214,12 @@ func (h Handler) buildEnv(r *http.Request, rule Rule, fpath string) (map[string]
 	// Strip PATH_INFO from SCRIPT_NAME
 	scriptName = strings.TrimSuffix(scriptName, pathInfo)
 
-	// Get the request URI. The request URI might be as it came in over the wire,
+	// Get the request URI from context. The request URI might be as it came in over the wire,
 	// or it might have been rewritten internally by the rewrite middleware (see issue #256).
-	// If it was rewritten, there will be a header indicating the original URL,
+	// If it was rewritten, there will be a context value with the original URL,
 	// which is needed to get the correct RequestURI value for PHP apps.
 	reqURI := r.URL.RequestURI()
-	if origURI := r.Header.Get(internalRewriteFieldName); origURI != "" {
+	if origURI, _ := r.Context().Value(caddy.URIxRewriteCtxKey).(string); origURI != "" {
 		reqURI = origURI
 	}
 
@@ -282,11 +277,8 @@ func (h Handler) buildEnv(r *http.Request, rule Rule, fpath string) (map[string]
 		env[envVar[0]] = replacer.Replace(envVar[1])
 	}
 
-	// Add all HTTP headers (except Caddy-Rewrite-Original-URI ) to env variables
+	// Add all HTTP headers to env variables
 	for field, val := range r.Header {
-		if strings.ToLower(field) == strings.ToLower(internalRewriteFieldName) {
-			continue
-		}
 		header := strings.ToUpper(field)
 		header = headerNameReplacer.Replace(header)
 		env["HTTP_"+header] = strings.Join(val, ", ")

caddyhttp/fastcgi/fastcgi_test.go

@@ -7,7 +7,6 @@ import (
 	"net/http/httptest"
 	"net/url"
 	"strconv"
-	"strings"
 	"sync"
 	"testing"
 	"time"
@@ -304,26 +303,6 @@ func TestBuildEnv(t *testing.T) {
 	envExpected["CUSTOM_URI"] = "custom_uri/fgci_test.php?test=blabla"
 	envExpected["CUSTOM_QUERY"] = "custom=true&test=blabla"
 	testBuildEnv(r, rule, fpath, envExpected)
-
-	// 6. Test Caddy-Rewrite-Original-URI header is not removed
-	r = newReq()
-	rule.EnvVars = [][2]string{
-		{"HTTP_HOST", "{host}"},
-		{"CUSTOM_URI", "custom_uri{uri}"},
-		{"CUSTOM_QUERY", "custom=true&{query}"},
-	}
-	envExpected = newEnv()
-	envExpected["HTTP_HOST"] = "localhost:2015"
-	envExpected["CUSTOM_URI"] = "custom_uri/fgci_test.php?test=blabla"
-	envExpected["CUSTOM_QUERY"] = "custom=true&test=blabla"
-	httpFieldName := strings.ToUpper(internalRewriteFieldName)
-	envExpected["HTTP_"+httpFieldName] = ""
-	r.Header.Add(internalRewriteFieldName, "/apath/torewrite/index.php")
-	testBuildEnv(r, rule, fpath, envExpected)
-	if r.Header.Get(internalRewriteFieldName) == "" {
-		t.Errorf("Error: Header Expected %v", internalRewriteFieldName)
-	}
-
 }
 
 func TestReadTimeout(t *testing.T) {

caddyhttp/httpserver/replacer.go

@@ -240,15 +240,23 @@ func (r *replacer) getSubstitution(key string) string {
 	case "{path}":
 		// if a rewrite has happened, the original URI should be used as the path
 		// rather than the rewritten URI
-		path := r.request.Header.Get("Caddy-Rewrite-Original-URI")
-		if path == "" {
+		var path string
+		origpath, _ := r.request.Context().Value(caddy.URIxRewriteCtxKey).(string)
+		if origpath == "" {
 			path = r.request.URL.Path
+		} else {
+			parsedURL, _ := url.Parse(origpath)
+			path = parsedURL.Path
 		}
 		return path
 	case "{path_escaped}":
-		path := r.request.Header.Get("Caddy-Rewrite-Original-URI")
-		if path == "" {
+		var path string
+		origpath, _ := r.request.Context().Value(caddy.URIxRewriteCtxKey).(string)
+		if origpath == "" {
 			path = r.request.URL.Path
+		} else {
+			parsedURL, _ := url.Parse(origpath)
+			path = parsedURL.Path
 		}
 		return url.QueryEscape(path)
 	case "{rewrite_path}":
@@ -276,8 +284,20 @@ func (r *replacer) getSubstitution(key string) string {
 		}
 		return port
 	case "{uri}":
-		return r.request.URL.RequestURI()
+		uri, _ := r.request.Context().Value(caddy.URIxRewriteCtxKey).(string)
+		if uri == "" {
+			uri = r.request.URL.RequestURI()
+		}
+		return uri
 	case "{uri_escaped}":
+		uri, _ := r.request.Context().Value(caddy.URIxRewriteCtxKey).(string)
+		if uri == "" {
+			uri = r.request.URL.RequestURI()
+		}
+		return url.QueryEscape(uri)
+	case "{rewrite_uri}":
+		return r.request.URL.RequestURI()
+	case "{rewrite_uri_escaped}":
 		return url.QueryEscape(r.request.URL.RequestURI())
 	case "{when}":
 		return now().Format(timeFormat)

caddyhttp/httpserver/replacer_test.go

 package httpserver
 
 import (
+	"context"
 	"net/http"
 	"net/http/httptest"
 	"os"
 	"strings"
 	"testing"
 	"time"
+
+	"github.com/mholt/caddy"
 )
 
 func TestNewReplacer(t *testing.T) {
@@ -149,6 +152,40 @@ func TestSet(t *testing.T) {
 	}
 }
 
+// Test function to test that various placeholders hold correct values after a rewrite
+// has been performed.  The NewRequest actually contains the rewritten value.
+func TestPathRewrite(t *testing.T) {
+	w := httptest.NewRecorder()
+	recordRequest := NewResponseRecorder(w)
+	reader := strings.NewReader(`{"username": "dennis"}`)
+
+	request, err := http.NewRequest("POST", "http://getcaddy.com/index.php?key=value", reader)
+	if err != nil {
+		t.Fatalf("Request Formation Failed: %s\n", err.Error())
+	}
+
+	ctx := context.WithValue(request.Context(), caddy.URIxRewriteCtxKey, "a/custom/path.php?key=value")
+	request = request.WithContext(ctx)
+
+	repl := NewReplacer(request, recordRequest, "")
+
+	if repl.Replace("This path is '{path}'") != "This path is 'a/custom/path.php'" {
+		t.Error("Expected host {path} replacement failed  (" + repl.Replace("This path is '{path}'") + ")")
+	}
+
+	if repl.Replace("This path is {rewrite_path}") != "This path is /index.php" {
+		t.Error("Expected host {rewrite_path} replacement failed (" + repl.Replace("This path is {rewrite_path}") + ")")
+	}
+	if repl.Replace("This path is '{uri}'") != "This path is 'a/custom/path.php?key=value'" {
+		t.Error("Expected host {uri} replacement failed  (" + repl.Replace("This path is '{uri}'") + ")")
+	}
+
+	if repl.Replace("This path is {rewrite_uri}") != "This path is /index.php?key=value" {
+		t.Error("Expected host {rewrite_uri} replacement failed (" + repl.Replace("This path is {rewrite_uri}") + ")")
+	}
+
+}
+
 func TestRound(t *testing.T) {
 	var tests = map[time.Duration]time.Duration{
 		// 599.935µs -> 560µs

caddyhttp/rewrite/rewrite.go

@@ -65,9 +65,6 @@ func (s SimpleRule) Match(r *http.Request) bool { return s.From == r.URL.Path }
 
 // Rewrite rewrites the internal location of the current request.
 func (s SimpleRule) Rewrite(fs http.FileSystem, r *http.Request) Result {
-	// take note of this rewrite for internal use by fastcgi
-	// all we need is the URI, not full URL
-	r.Header.Set(headerFieldName, r.URL.RequestURI())
 
 	// attempt rewrite
 	return To(fs, r, s.To, newReplacer(r))
@@ -234,8 +231,3 @@ func (r *ComplexRule) regexpMatches(rPath string) []string {
 func newReplacer(r *http.Request) httpserver.Replacer {
 	return httpserver.NewReplacer(r, nil, "")
 }
-
-// When a rewrite is performed, this header is added to the request
-// and is for internal use only, specifically the fastcgi middleware.
-// It contains the original request URI before the rewrite.
-const headerFieldName = "Caddy-Rewrite-Original-URI"

caddyhttp/rewrite/to.go

 package rewrite
 
 import (
+	"context"
 	"log"
 	"net/http"
 	"net/url"
 	"path"
 	"strings"
 
+	"github.com/mholt/caddy"
 	"github.com/mholt/caddy/caddyhttp/httpserver"
 )
 
@@ -49,7 +51,7 @@ func To(fs http.FileSystem, r *http.Request, to string, replacer httpserver.Repl
 
 	// take note of this rewrite for internal use by fastcgi
 	// all we need is the URI, not full URL
-	r.Header.Set(headerFieldName, r.URL.RequestURI())
+	*r = *r.WithContext(context.WithValue(r.Context(), caddy.URIxRewriteCtxKey, r.URL.RequestURI()))
 
 	// perform rewrite
 	r.URL.Path = u.Path