From d49f762f6d9cdc2e92e8de40f0b0e99a9d0c4fc9 Mon Sep 17 00:00:00 2001
From: Matthew Holt <mholt@users.noreply.github.com>
Date: Fri, 21 Jun 2019 14:36:26 -0600
Subject: [PATCH] Various bug fixes and minor improvements

- Fix static responder so it doesn't replace its own headers config,
  and instead replaces the actual response header values
- caddyhttp.ResponseRecorder type optionally buffers response
- Add interface guards to ensure regexp matchers get provisioned
- Use default HTTP port if one is not explicitly set
- Encode middleware writes status code 200 if not written upstream
- Templates and markdown only try to execute on text responses
- Static file server sets Content-Type based on file extension only
  (this whole thing -- MIME sniffing, etc -- needs more configurability)
---
 modules/caddyhttp/caddyhttp.go              |  7 +++-
 modules/caddyhttp/encode/encode.go          |  6 ++-
 modules/caddyhttp/fileserver/browse.go      |  1 +
 modules/caddyhttp/fileserver/staticfiles.go | 14 +++++--
 modules/caddyhttp/headers/headers.go        | 21 ++++++----
 modules/caddyhttp/markdown/markdown.go      | 14 +++++--
 modules/caddyhttp/matchers.go               |  9 +++--
 modules/caddyhttp/matchers_test.go          |  4 +-
 modules/caddyhttp/responsewriter.go         | 43 ++++++++++++++++++---
 modules/caddyhttp/server.go                 |  4 +-
 modules/caddyhttp/staticresp.go             |  5 ++-
 modules/caddyhttp/templates/templates.go    | 16 ++++++--
 12 files changed, 109 insertions(+), 35 deletions(-)

diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go
index 009e564d0..68568a984 100644
--- a/modules/caddyhttp/caddyhttp.go
+++ b/modules/caddyhttp/caddyhttp.go
@@ -265,7 +265,12 @@ func (app *App) automaticHTTPS() error {
 				if err != nil {
 					return fmt.Errorf("%s: invalid listener address: %v", srvName, addr)
 				}
-				httpRedirLnAddr := joinListenAddr(netw, host, strconv.Itoa(app.HTTPPort))
+
+				httpPort := app.HTTPPort
+				if httpPort == 0 {
+					httpPort = DefaultHTTPPort
+				}
+				httpRedirLnAddr := joinListenAddr(netw, host, strconv.Itoa(httpPort))
 				lnAddrMap[httpRedirLnAddr] = struct{}{}
 
 				if parts := strings.SplitN(port, "-", 2); len(parts) == 2 {
diff --git a/modules/caddyhttp/encode/encode.go b/modules/caddyhttp/encode/encode.go
index e20667ff3..b7ab73718 100644
--- a/modules/caddyhttp/encode/encode.go
+++ b/modules/caddyhttp/encode/encode.go
@@ -157,7 +157,11 @@ func (rw *responseWriter) init() {
 		rw.Header().Set("Content-Encoding", rw.encodingName)
 	}
 	rw.Header().Del("Accept-Ranges") // we don't know ranges for dynamically-encoded content
-	rw.ResponseWriter.WriteHeader(rw.statusCode)
+	status := rw.statusCode
+	if status == 0 {
+		status = http.StatusOK
+	}
+	rw.ResponseWriter.WriteHeader(status)
 }
 
 // Close writes any remaining buffered response and
diff --git a/modules/caddyhttp/fileserver/browse.go b/modules/caddyhttp/fileserver/browse.go
index 13295416c..5dda294c4 100644
--- a/modules/caddyhttp/fileserver/browse.go
+++ b/modules/caddyhttp/fileserver/browse.go
@@ -66,6 +66,7 @@ func (fsrv *FileServer) serveBrowse(dirPath string, w http.ResponseWriter, r *ht
 		}
 		w.Header().Set("Content-Type", "text/html; charset=utf-8")
 	}
+
 	buf.WriteTo(w)
 
 	return nil
diff --git a/modules/caddyhttp/fileserver/staticfiles.go b/modules/caddyhttp/fileserver/staticfiles.go
index 080e1a80d..49c2be446 100644
--- a/modules/caddyhttp/fileserver/staticfiles.go
+++ b/modules/caddyhttp/fileserver/staticfiles.go
@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"html/template"
 	weakrand "math/rand"
+	"mime"
 	"net/http"
 	"os"
 	"path"
@@ -185,14 +186,21 @@ func (fsrv *FileServer) ServeHTTP(w http.ResponseWriter, r *http.Request) error
 
 	// TODO: Etag
 
-	// do not allow Go to sniff the content-type
 	if w.Header().Get("Content-Type") == "" {
-		w.Header()["Content-Type"] = nil
+		mtyp := mime.TypeByExtension(filepath.Ext(filename))
+		if mtyp == "" {
+			// do not allow Go to sniff the content-type; see
+			// https://www.youtube.com/watch?v=8t8JYpt0egE
+			// TODO: Consider writing a default mime type of application/octet-stream - this is secure but violates spec
+			w.Header()["Content-Type"] = nil
+		} else {
+			w.Header().Set("Content-Type", mtyp)
+		}
 	}
 
 	// let the standard library do what it does best; note, however,
 	// that errors generated by ServeContent are written immediately
-	// to the response, so we cannot handle them (but errors here
+	// to the response, so we cannot handle them (but errors there
 	// are rare)
 	http.ServeContent(w, r, info.Name(), info.ModTime(), file)
 
diff --git a/modules/caddyhttp/headers/headers.go b/modules/caddyhttp/headers/headers.go
index 84dc453dd..d57b8114b 100644
--- a/modules/caddyhttp/headers/headers.go
+++ b/modules/caddyhttp/headers/headers.go
@@ -40,20 +40,25 @@ type RespHeaderOps struct {
 func (h Headers) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
 	repl := r.Context().Value(caddy.ReplacerCtxKey).(caddy.Replacer)
 	apply(h.Request, r.Header, repl)
-	if h.Response.Deferred || h.Response.Require != nil {
-		w = &responseWriterWrapper{
-			ResponseWriterWrapper: &caddyhttp.ResponseWriterWrapper{ResponseWriter: w},
-			replacer:              repl,
-			require:               h.Response.Require,
-			headerOps:             h.Response.HeaderOps,
+	if h.Response != nil {
+		if h.Response.Deferred || h.Response.Require != nil {
+			w = &responseWriterWrapper{
+				ResponseWriterWrapper: &caddyhttp.ResponseWriterWrapper{ResponseWriter: w},
+				replacer:              repl,
+				require:               h.Response.Require,
+				headerOps:             h.Response.HeaderOps,
+			}
+		} else {
+			apply(h.Response.HeaderOps, w.Header(), repl)
 		}
-	} else {
-		apply(h.Response.HeaderOps, w.Header(), repl)
 	}
 	return next.ServeHTTP(w, r)
 }
 
 func apply(ops *HeaderOps, hdr http.Header, repl caddy.Replacer) {
+	if ops == nil {
+		return
+	}
 	for fieldName, vals := range ops.Add {
 		fieldName = repl.ReplaceAll(fieldName, "")
 		for _, v := range vals {
diff --git a/modules/caddyhttp/markdown/markdown.go b/modules/caddyhttp/markdown/markdown.go
index 574039e55..b2292a485 100644
--- a/modules/caddyhttp/markdown/markdown.go
+++ b/modules/caddyhttp/markdown/markdown.go
@@ -4,6 +4,7 @@ import (
 	"bytes"
 	"net/http"
 	"strconv"
+	"strings"
 	"sync"
 
 	"gopkg.in/russross/blackfriday.v2"
@@ -28,12 +29,19 @@ func (m Markdown) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyht
 	buf.Reset()
 	defer bufPool.Put(buf)
 
-	rr := caddyhttp.NewResponseRecorder(w, buf)
+	shouldBuf := func(status int) bool {
+		return strings.HasPrefix(w.Header().Get("Content-Type"), "text/")
+	}
 
-	err := next.ServeHTTP(rr, r)
+	rec := caddyhttp.NewResponseRecorder(w, buf, shouldBuf)
+
+	err := next.ServeHTTP(rec, r)
 	if err != nil {
 		return err
 	}
+	if !rec.Buffered() {
+		return nil
+	}
 
 	output := blackfriday.Run(buf.Bytes())
 
@@ -43,7 +51,7 @@ func (m Markdown) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyht
 	w.Header().Del("Etag")          // don't know a way to quickly generate etag for dynamic content
 	w.Header().Del("Last-Modified") // useless for dynamic content since it's always changing
 
-	w.WriteHeader(rr.Status())
+	w.WriteHeader(rec.Status())
 	w.Write(output)
 
 	return nil
diff --git a/modules/caddyhttp/matchers.go b/modules/caddyhttp/matchers.go
index 2395e13f9..cb0820b55 100644
--- a/modules/caddyhttp/matchers.go
+++ b/modules/caddyhttp/matchers.go
@@ -226,9 +226,9 @@ func (m MatchHeaderRE) Match(r *http.Request) bool {
 }
 
 // Provision compiles m's regular expressions.
-func (m MatchHeaderRE) Provision() error {
+func (m MatchHeaderRE) Provision(ctx caddy.Context) error {
 	for _, rm := range m {
-		err := rm.Provision()
+		err := rm.Provision(ctx)
 		if err != nil {
 			return err
 		}
@@ -371,7 +371,7 @@ type MatchRegexp struct {
 }
 
 // Provision compiles the regular expression.
-func (mre *MatchRegexp) Provision() error {
+func (mre *MatchRegexp) Provision(caddy.Context) error {
 	re, err := regexp.Compile(mre.Pattern)
 	if err != nil {
 		return fmt.Errorf("compiling matcher regexp %s: %v", mre.Pattern, err)
@@ -479,14 +479,17 @@ var (
 	_ RequestMatcher    = (*MatchHost)(nil)
 	_ RequestMatcher    = (*MatchPath)(nil)
 	_ RequestMatcher    = (*MatchPathRE)(nil)
+	_ caddy.Provisioner = (*MatchPathRE)(nil)
 	_ RequestMatcher    = (*MatchMethod)(nil)
 	_ RequestMatcher    = (*MatchQuery)(nil)
 	_ RequestMatcher    = (*MatchHeader)(nil)
 	_ RequestMatcher    = (*MatchHeaderRE)(nil)
+	_ caddy.Provisioner = (*MatchHeaderRE)(nil)
 	_ RequestMatcher    = (*MatchProtocol)(nil)
 	_ RequestMatcher    = (*MatchRemoteIP)(nil)
 	_ caddy.Provisioner = (*MatchRemoteIP)(nil)
 	_ RequestMatcher    = (*MatchNegate)(nil)
 	_ caddy.Provisioner = (*MatchNegate)(nil)
 	_ RequestMatcher    = (*MatchStarlarkExpr)(nil)
+	_ caddy.Provisioner = (*MatchRegexp)(nil)
 )
diff --git a/modules/caddyhttp/matchers_test.go b/modules/caddyhttp/matchers_test.go
index 597d217c0..4a0db0bd9 100644
--- a/modules/caddyhttp/matchers_test.go
+++ b/modules/caddyhttp/matchers_test.go
@@ -219,7 +219,7 @@ func TestPathREMatcher(t *testing.T) {
 		},
 	} {
 		// compile the regexp and validate its name
-		err := tc.match.Provision()
+		err := tc.match.Provision(caddy.Context{})
 		if err != nil {
 			t.Errorf("Test %d %v: Provisioning: %v", i, tc.match, err)
 			continue
@@ -337,7 +337,7 @@ func TestHeaderREMatcher(t *testing.T) {
 		},
 	} {
 		// compile the regexp and validate its name
-		err := tc.match.Provision()
+		err := tc.match.Provision(caddy.Context{})
 		if err != nil {
 			t.Errorf("Test %d %v: Provisioning: %v", i, tc.match, err)
 			continue
diff --git a/modules/caddyhttp/responsewriter.go b/modules/caddyhttp/responsewriter.go
index 7fb45f3d8..e733db793 100644
--- a/modules/caddyhttp/responsewriter.go
+++ b/modules/caddyhttp/responsewriter.go
@@ -61,9 +61,11 @@ var ErrNotImplemented = fmt.Errorf("method not implemented")
 
 type responseRecorder struct {
 	*ResponseWriterWrapper
-	wroteHeader bool
-	statusCode  int
-	buf         *bytes.Buffer
+	wroteHeader  bool
+	statusCode   int
+	buf          *bytes.Buffer
+	shouldBuffer func(status int) bool
+	stream       bool
 }
 
 // NewResponseRecorder returns a new ResponseRecorder that can be
@@ -76,6 +78,16 @@ type responseRecorder struct {
 // responses by wrapping Write and WriteHeader methods instead of
 // buffering whole response bodies.
 //
+// Recorders optionally buffer the response. When the headers are
+// to be written, shouldBuffer will be called with the status
+// code that is being written. The rest of the headers can be read
+// from w.Header(). If shouldBuffer returns true, the response
+// will be buffered. You can know the response was buffered if
+// the Buffered() method returns true. If the response was not
+// buffered, Buffered() will return false and that means the
+// response bypassed the recorder and was written directly to the
+// underlying writer.
+//
 // Before calling this function in a middleware handler, make a
 // new buffer or obtain one from a pool (use the sync.Pool) type.
 // Using a pool is generally recommended for performance gains;
@@ -85,12 +97,13 @@ type responseRecorder struct {
 // The returned recorder can be used in place of w when calling
 // the next handler in the chain. When that handler returns, you
 // can read the status code from the recorder's Status() method.
-// The response body fills buf, and the headers are available in
-// w.Header().
-func NewResponseRecorder(w http.ResponseWriter, buf *bytes.Buffer) ResponseRecorder {
+// The response body fills buf if it was buffered, and the headers
+// are available via w.Header().
+func NewResponseRecorder(w http.ResponseWriter, buf *bytes.Buffer, shouldBuffer func(status int) bool) ResponseRecorder {
 	return &responseRecorder{
 		ResponseWriterWrapper: &ResponseWriterWrapper{ResponseWriter: w},
 		buf:                   buf,
+		shouldBuffer:          shouldBuffer,
 	}
 }
 
@@ -100,10 +113,22 @@ func (rr *responseRecorder) WriteHeader(statusCode int) {
 	}
 	rr.statusCode = statusCode
 	rr.wroteHeader = true
+
+	// decide whether we should buffer the response
+	if rr.shouldBuffer == nil {
+		return
+	}
+	rr.stream = !rr.shouldBuffer(rr.statusCode)
+	if rr.stream {
+		rr.ResponseWriterWrapper.WriteHeader(rr.statusCode)
+	}
 }
 
 func (rr *responseRecorder) Write(data []byte) (int, error) {
 	rr.WriteHeader(http.StatusOK)
+	if rr.stream {
+		return rr.ResponseWriterWrapper.Write(data)
+	}
 	return rr.buf.Write(data)
 }
 
@@ -118,12 +143,18 @@ func (rr *responseRecorder) Buffer() *bytes.Buffer {
 	return rr.buf
 }
 
+// Buffered returns whether rr has decided to buffer the response.
+func (rr *responseRecorder) Buffered() bool {
+	return !rr.stream
+}
+
 // ResponseRecorder is a http.ResponseWriter that records
 // responses instead of writing them to the client.
 type ResponseRecorder interface {
 	HTTPInterfaces
 	Status() int
 	Buffer() *bytes.Buffer
+	Buffered() bool
 }
 
 // Interface guards
diff --git a/modules/caddyhttp/server.go b/modules/caddyhttp/server.go
index 61b5631d0..94fd4d471 100644
--- a/modules/caddyhttp/server.go
+++ b/modules/caddyhttp/server.go
@@ -72,11 +72,11 @@ func (s *Server) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 			err := s.executeCompositeRoute(w, r, errStack)
 			if err != nil {
 				// TODO: what should we do if the error handler has an error?
-				log.Printf("[ERROR] handling error: %v", err)
+				log.Printf("[ERROR] [%s %s] handling error: %v", r.Method, r.RequestURI, err)
 			}
 		} else {
 			// TODO: polish the default error handling
-			log.Printf("[ERROR] Handler: %s", err)
+			log.Printf("[ERROR] [%s %s] %v", r.Method, r.RequestURI, err)
 			if handlerErr, ok := err.(HandlerError); ok {
 				w.WriteHeader(handlerErr.StatusCode)
 			} else {
diff --git a/modules/caddyhttp/staticresp.go b/modules/caddyhttp/staticresp.go
index dfb3277f2..f48fcea05 100644
--- a/modules/caddyhttp/staticresp.go
+++ b/modules/caddyhttp/staticresp.go
@@ -33,10 +33,11 @@ func (s Static) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
 	// set all headers
 	for field, vals := range s.Headers {
 		field = repl.ReplaceAll(field, "")
+		newVals := make([]string, len(vals))
 		for i := range vals {
-			vals[i] = repl.ReplaceAll(vals[i], "")
+			newVals[i] = repl.ReplaceAll(vals[i], "")
 		}
-		w.Header()[field] = vals
+		w.Header()[field] = newVals
 	}
 
 	// do not allow Go to sniff the content-type
diff --git a/modules/caddyhttp/templates/templates.go b/modules/caddyhttp/templates/templates.go
index c659f6ba9..e329e2e3e 100644
--- a/modules/caddyhttp/templates/templates.go
+++ b/modules/caddyhttp/templates/templates.go
@@ -6,6 +6,7 @@ import (
 	"io"
 	"net/http"
 	"strconv"
+	"strings"
 
 	"github.com/caddyserver/caddy"
 	"github.com/caddyserver/caddy/modules/caddyhttp"
@@ -37,14 +38,21 @@ func (t *Templates) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddy
 	buf.Reset()
 	defer bufPool.Put(buf)
 
-	rr := caddyhttp.NewResponseRecorder(w, buf)
+	shouldBuf := func(status int) bool {
+		return strings.HasPrefix(w.Header().Get("Content-Type"), "text/")
+	}
 
-	err := next.ServeHTTP(rr, r)
+	rec := caddyhttp.NewResponseRecorder(w, buf, shouldBuf)
+
+	err := next.ServeHTTP(rec, r)
 	if err != nil {
 		return err
 	}
+	if !rec.Buffered() {
+		return nil
+	}
 
-	err = t.executeTemplate(rr, r)
+	err = t.executeTemplate(rec, r)
 	if err != nil {
 		return err
 	}
@@ -54,7 +62,7 @@ func (t *Templates) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddy
 	w.Header().Del("Etag")          // don't know a way to quickly generate etag for dynamic content
 	w.Header().Del("Last-Modified") // useless for dynamic content since it's always changing
 
-	w.WriteHeader(rr.Status())
+	w.WriteHeader(rec.Status())
 	io.Copy(w, buf)
 
 	return nil