From 64f8b557b1b1d4ca869ee7e0cba91f09e86883ef Mon Sep 17 00:00:00 2001 From: "Y.Horie" Date: Sun, 17 Oct 2021 00:09:16 +0900 Subject: [PATCH 1/3] fileserver: Fix compression breaks using httpInclude (#4352) (#4358) --- modules/caddyhttp/templates/tplcontext.go | 1 + .../caddyhttp/templates/tplcontext_test.go | 98 +++++++++++++------ 2 files changed, 70 insertions(+), 29 deletions(-) diff --git a/modules/caddyhttp/templates/tplcontext.go b/modules/caddyhttp/templates/tplcontext.go index 71886c9a0c5..b1646c12736 100644 --- a/modules/caddyhttp/templates/tplcontext.go +++ b/modules/caddyhttp/templates/tplcontext.go @@ -160,6 +160,7 @@ func (c TemplateContext) funcHTTPInclude(uri string) (string, error) { } virtReq.Host = c.Req.Host virtReq.Header = c.Req.Header.Clone() + virtReq.Header.Set("Accept-Encoding", "identity") // https://github.com/caddyserver/caddy/issues/4352 virtReq.Trailer = c.Req.Trailer.Clone() virtReq.Header.Set(recursionPreventionHeader, strconv.Itoa(recursionCount)) diff --git a/modules/caddyhttp/templates/tplcontext_test.go b/modules/caddyhttp/templates/tplcontext_test.go index c2fe61f4cc5..ddc8b99ba47 100644 --- a/modules/caddyhttp/templates/tplcontext_test.go +++ b/modules/caddyhttp/templates/tplcontext_test.go @@ -16,6 +16,7 @@ package templates import ( "bytes" + "context" "fmt" "net/http" "os" @@ -25,10 +26,49 @@ import ( "strings" "testing" "time" + + "github.com/caddyserver/caddy/v2/modules/caddyhttp" ) +type handle struct { +} + +func (h *handle) ServeHTTP(w http.ResponseWriter, r *http.Request) { + if r.Header.Get("Accept-Encoding") == "identity" { + w.Write([]byte("good contents")) + } else { + w.Write([]byte("bad cause Accept-Encoding: " + r.Header.Get("Accept-Encoding"))) + } +} + +func TestHTTPInclude(t *testing.T) { + tplContext := getContextOrFail(t) + for i, test := range []struct { + uri string + handler *handle + expect string + }{ + { + uri: "https://example.com/foo/bar", + handler: &handle{}, + expect: "good contents", + }, + } { + ctx := context.WithValue(tplContext.Req.Context(), caddyhttp.ServerCtxKey, test.handler) + tplContext.Req = tplContext.Req.WithContext(ctx) + tplContext.Req.Header.Add("Accept-Encoding", "gzip") + result, err := tplContext.funcHTTPInclude(test.uri) + if result != test.expect { + t.Errorf("Test %d: expected '%s' but got '%s'", i, test.expect, result) + } + if err != nil { + t.Errorf("Test %d: got error: %v", i, result) + } + } +} + func TestMarkdown(t *testing.T) { - context := getContextOrFail(t) + tplContext := getContextOrFail(t) for i, test := range []struct { body string @@ -39,7 +79,7 @@ func TestMarkdown(t *testing.T) { expect: "\n", }, } { - result, err := context.funcMarkdown(test.body) + result, err := tplContext.funcMarkdown(test.body) if result != test.expect { t.Errorf("Test %d: expected '%s' but got '%s'", i, test.expect, result) } @@ -80,9 +120,9 @@ func TestCookie(t *testing.T) { expect: "cookieValue", }, } { - context := getContextOrFail(t) - context.Req.AddCookie(test.cookie) - actual := context.Cookie(test.cookieName) + tplContext := getContextOrFail(t) + tplContext.Req.AddCookie(test.cookie) + actual := tplContext.Cookie(test.cookieName) if actual != test.expect { t.Errorf("Test %d: Expected cookie value '%s' but got '%s' for cookie with name '%s'", i, test.expect, actual, test.cookieName) @@ -111,12 +151,12 @@ func TestImport(t *testing.T) { shouldErr: true, }, } { - context := getContextOrFail(t) + tplContext := getContextOrFail(t) var absFilePath string // create files for test case if test.fileName != "" { - absFilePath := filepath.Join(fmt.Sprintf("%s", context.Root), test.fileName) + absFilePath := filepath.Join(fmt.Sprintf("%s", tplContext.Root), test.fileName) if err := os.WriteFile(absFilePath, []byte(test.fileContent), os.ModePerm); err != nil { os.Remove(absFilePath) t.Fatalf("Test %d: Expected no error creating file, got: '%s'", i, err.Error()) @@ -124,9 +164,9 @@ func TestImport(t *testing.T) { } // perform test - context.NewTemplate("parent") - actual, err := context.funcImport(test.fileName) - templateWasDefined := strings.Contains(context.tpl.DefinedTemplates(), test.expect) + tplContext.NewTemplate("parent") + actual, err := tplContext.funcImport(test.fileName) + templateWasDefined := strings.Contains(tplContext.tpl.DefinedTemplates(), test.expect) if err != nil { if !test.shouldErr { t.Errorf("Test %d: Expected no error, got: '%s'", i, err) @@ -135,7 +175,7 @@ func TestImport(t *testing.T) { t.Errorf("Test %d: Expected error but had none", i) } else if !templateWasDefined && actual != "" { // template should be defined, return value should be an empty string - t.Errorf("Test %d: Expected template %s to be define but got %s", i, test.expect, context.tpl.DefinedTemplates()) + t.Errorf("Test %d: Expected template %s to be define but got %s", i, test.expect, tplContext.tpl.DefinedTemplates()) } @@ -191,12 +231,12 @@ func TestInclude(t *testing.T) { args: "text", }, } { - context := getContextOrFail(t) + tplContext := getContextOrFail(t) var absFilePath string // create files for test case if test.fileName != "" { - absFilePath := filepath.Join(fmt.Sprintf("%s", context.Root), test.fileName) + absFilePath := filepath.Join(fmt.Sprintf("%s", tplContext.Root), test.fileName) if err := os.WriteFile(absFilePath, []byte(test.fileContent), os.ModePerm); err != nil { os.Remove(absFilePath) t.Fatalf("Test %d: Expected no error creating file, got: '%s'", i, err.Error()) @@ -204,7 +244,7 @@ func TestInclude(t *testing.T) { } // perform test - actual, err := context.funcInclude(test.fileName, test.args) + actual, err := tplContext.funcInclude(test.fileName, test.args) if err != nil { if !test.shouldErr { t.Errorf("Test %d: Expected no error, got: '%s'", i, err) @@ -225,12 +265,12 @@ func TestInclude(t *testing.T) { } func TestCookieMultipleCookies(t *testing.T) { - context := getContextOrFail(t) + tplContext := getContextOrFail(t) cookieNameBase, cookieValueBase := "cookieName", "cookieValue" for i := 0; i < 10; i++ { - context.Req.AddCookie(&http.Cookie{ + tplContext.Req.AddCookie(&http.Cookie{ Name: fmt.Sprintf("%s%d", cookieNameBase, i), Value: fmt.Sprintf("%s%d", cookieValueBase, i), }) @@ -238,7 +278,7 @@ func TestCookieMultipleCookies(t *testing.T) { for i := 0; i < 10; i++ { expectedCookieVal := fmt.Sprintf("%s%d", cookieValueBase, i) - actualCookieVal := context.Cookie(fmt.Sprintf("%s%d", cookieNameBase, i)) + actualCookieVal := tplContext.Cookie(fmt.Sprintf("%s%d", cookieNameBase, i)) if actualCookieVal != expectedCookieVal { t.Errorf("Expected cookie value %s, found %s", expectedCookieVal, actualCookieVal) } @@ -246,7 +286,7 @@ func TestCookieMultipleCookies(t *testing.T) { } func TestIP(t *testing.T) { - context := getContextOrFail(t) + tplContext := getContextOrFail(t) for i, test := range []struct { inputRemoteAddr string expect string @@ -257,15 +297,15 @@ func TestIP(t *testing.T) { {"[2001:db8:a0b:12f0::1]", "[2001:db8:a0b:12f0::1]"}, {`[fe80:1::3%eth0]:44`, `fe80:1::3%eth0`}, } { - context.Req.RemoteAddr = test.inputRemoteAddr - if actual := context.RemoteIP(); actual != test.expect { + tplContext.Req.RemoteAddr = test.inputRemoteAddr + if actual := tplContext.RemoteIP(); actual != test.expect { t.Errorf("Test %d: Expected %s but got %s", i, test.expect, actual) } } } func TestStripHTML(t *testing.T) { - context := getContextOrFail(t) + tplContext := getContextOrFail(t) for i, test := range []struct { input string @@ -302,7 +342,7 @@ func TestStripHTML(t *testing.T) { expect: ` Date: Mon, 18 Oct 2021 14:00:43 -0400 Subject: [PATCH 2/3] reverseproxy: Prevent copying the response if a response handler ran (#4388) --- .../caddyhttp/reverseproxy/reverseproxy.go | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go index 360b018e0ca..e626962ec00 100644 --- a/modules/caddyhttp/reverseproxy/reverseproxy.go +++ b/modules/caddyhttp/reverseproxy/reverseproxy.go @@ -614,6 +614,11 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, repl * res.Body = h.bufferedBody(res.Body) } + // the response body may get closed by a response handler, + // and we need to keep track to make sure we don't try to copy + // the response if it was already closed + bodyClosed := false + // see if any response handler is configured for this response from the backend for i, rh := range h.HandleResponse { if rh.Match != nil && !rh.Match.Match(res.StatusCode, res.Header) { @@ -638,8 +643,6 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, repl * continue } - res.Body.Close() - // set up the replacer so that parts of the original response can be // used for routing decisions for field, value := range res.Header { @@ -649,7 +652,17 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, repl * repl.Set("http.reverse_proxy.status_text", res.Status) h.logger.Debug("handling response", zap.Int("handler", i)) - if routeErr := rh.Routes.Compile(next).ServeHTTP(rw, req); routeErr != nil { + + // pass the request through the response handler routes + routeErr := rh.Routes.Compile(next).ServeHTTP(rw, req) + + // always close the response body afterwards since it's expected + // that the response handler routes will have written to the + // response writer with a new body + res.Body.Close() + bodyClosed = true + + if routeErr != nil { // wrap error in roundtripSucceeded so caller knows that // the roundtrip was successful and to not retry return roundtripSucceeded{routeErr} @@ -690,15 +703,17 @@ func (h *Handler) reverseProxy(rw http.ResponseWriter, req *http.Request, repl * } rw.WriteHeader(res.StatusCode) - err = h.copyResponse(rw, res.Body, h.flushInterval(req, res)) - res.Body.Close() // close now, instead of defer, to populate res.Trailer - if err != nil { - // we're streaming the response and we've already written headers, so - // there's nothing an error handler can do to recover at this point; - // the standard lib's proxy panics at this point, but we'll just log - // the error and abort the stream here - h.logger.Error("aborting with incomplete response", zap.Error(err)) - return nil + if !bodyClosed { + err = h.copyResponse(rw, res.Body, h.flushInterval(req, res)) + res.Body.Close() // close now, instead of defer, to populate res.Trailer + if err != nil { + // we're streaming the response and we've already written headers, so + // there's nothing an error handler can do to recover at this point; + // the standard lib's proxy panics at this point, but we'll just log + // the error and abort the stream here + h.logger.Error("aborting with incomplete response", zap.Error(err)) + return nil + } } if len(res.Trailer) > 0 { From 062657d0d80aa1c2e3af51414db634f49c9a03a0 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Mon, 18 Oct 2021 14:19:04 -0400 Subject: [PATCH 3/3] caddycmd: Add `--skip-standard` to `list-modules` command, quieter output (#4386) * caddycmd: Add --skip-standard to list-modules command, quieter output * caddycmd: Also quiet `caddy upgrade` output, redundant information --- cmd/commandfuncs.go | 20 ++++++++++++++------ cmd/commands.go | 1 + cmd/packagesfuncs.go | 2 +- 3 files changed, 16 insertions(+), 7 deletions(-) diff --git a/cmd/commandfuncs.go b/cmd/commandfuncs.go index cc3f65c70a9..bc62f28145b 100644 --- a/cmd/commandfuncs.go +++ b/cmd/commandfuncs.go @@ -360,6 +360,7 @@ func cmdBuildInfo(fl Flags) (int, error) { func cmdListModules(fl Flags) (int, error) { packages := fl.Bool("packages") versions := fl.Bool("versions") + skipStandard := fl.Bool("skip-standard") printModuleInfo := func(mi moduleInfo) { fmt.Print(mi.caddyModuleID) @@ -388,14 +389,19 @@ func cmdListModules(fl Flags) (int, error) { return caddy.ExitCodeSuccess, nil } - if len(standard) > 0 { - for _, mod := range standard { - printModuleInfo(mod) + // Standard modules (always shipped with Caddy) + if !skipStandard { + if len(standard) > 0 { + for _, mod := range standard { + printModuleInfo(mod) + } } + fmt.Printf("\n Standard modules: %d\n", len(standard)) } - fmt.Printf("\n Standard modules: %d\n", len(standard)) + + // Non-standard modules (third party plugins) if len(nonstandard) > 0 { - if len(standard) > 0 { + if len(standard) > 0 && !skipStandard { fmt.Println() } for _, mod := range nonstandard { @@ -403,8 +409,10 @@ func cmdListModules(fl Flags) (int, error) { } } fmt.Printf("\n Non-standard modules: %d\n", len(nonstandard)) + + // Unknown modules (couldn't get Caddy module info) if len(unknown) > 0 { - if len(standard) > 0 || len(nonstandard) > 0 { + if (len(standard) > 0 && !skipStandard) || len(nonstandard) > 0 { fmt.Println() } for _, mod := range unknown { diff --git a/cmd/commands.go b/cmd/commands.go index 7b184fd79b9..f562430456d 100644 --- a/cmd/commands.go +++ b/cmd/commands.go @@ -208,6 +208,7 @@ config file; otherwise the default is assumed.`, fs := flag.NewFlagSet("list-modules", flag.ExitOnError) fs.Bool("packages", false, "Print package paths") fs.Bool("versions", false, "Print version information") + fs.Bool("skip-standard", false, "Skip printing standard modules") return fs }(), }) diff --git a/cmd/packagesfuncs.go b/cmd/packagesfuncs.go index 6aaf52bfbfb..c4f41eaa212 100644 --- a/cmd/packagesfuncs.go +++ b/cmd/packagesfuncs.go @@ -220,7 +220,7 @@ func getModules() (standard, nonstandard, unknown []moduleInfo, err error) { } func listModules(path string) error { - cmd := exec.Command(path, "list-modules", "--versions") + cmd := exec.Command(path, "list-modules", "--versions", "--skip-standard") cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr err := cmd.Run()