From d49f762f6d9cdc2e92e8de40f0b0e99a9d0c4fc9 Mon Sep 17 00:00:00 2001 From: Matthew Holt Date: Fri, 21 Jun 2019 14:36:26 -0600 Subject: 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 009e564..68568a9 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 e20667f..b7ab737 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 1329541..5dda294 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 080e1a8..49c2be4 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 84dc453..d57b811 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 574039e..b2292a4 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/") + } + + rec := caddyhttp.NewResponseRecorder(w, buf, shouldBuf) - err := next.ServeHTTP(rr, r) + 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 2395e13..cb0820b 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 597d217..4a0db0b 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 7fb45f3..e733db7 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 61b5631..94fd4d4 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 dfb3277..f48fcea 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 c659f6b..e329e2e 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/") + } + + rec := caddyhttp.NewResponseRecorder(w, buf, shouldBuf) - err := next.ServeHTTP(rr, r) + 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 -- cgit v1.2.3