headers: Support default header values in Caddyfile with '?' (#3807)

* implement default values for header directive

closes #3804

* remove `set_default` header op and rely on "require" handler instead

This has the following advantages over the previous attempt:

- It does not introduce a new operation for headers, but rather nicely
  extends over an existing feature in the header handler.
- It removes the need to specify the header as "deferred" because it is
  already implicitely deferred by the use of the require handler. This
  should be less confusing to the user.

* add integration test for header directive in caddyfile

* bubble up errors when parsing caddyfile header directive

* don't export unnecessarily and don't canonicalize headers unnecessarily

* fix response headers not passed in blocks

* caddyfile: fix clash when using default header in block

Each header is now set in a separate handler so that it doesn't clash
with other headers set/added/deleted in the same block.

* caddyhttp: New idle_timeout default of 5m

* reverseproxy: fix random hangs on http/2 requests with server push (#3875)

see https://github.com/golang/go/issues/42534

* Refactor and cleanup with improvements

* More specific link

Co-authored-by: Matthew Holt <mholt@users.noreply.github.com>
Co-authored-by: Денис Телюх <telyukh.denis@gmail.com>

2 files changed, 323 insertions, 51 deletions

diff --git a/modules/caddyhttp/headers/caddyfile.go b/modules/caddyhttp/headers/caddyfile.go
index d893cab..75498b2 100644
--- a/modules/caddyhttp/headers/caddyfile.go
+++ b/modules/caddyhttp/headers/caddyfile.go
@@ -15,7 +15,9 @@
 package headers
 
 import (
+	"fmt"
 	"net/http"
+	"reflect"
 	"strings"
 
 	"github.com/caddyserver/caddy/v2/caddyconfig/httpcaddyfile"
@@ -23,15 +25,16 @@ import (
 )
 
 func init() {
-	httpcaddyfile.RegisterHandlerDirective("header", parseCaddyfile)
-	httpcaddyfile.RegisterHandlerDirective("request_header", parseReqHdrCaddyfile)
+	httpcaddyfile.RegisterDirective("header", parseCaddyfile)
+	httpcaddyfile.RegisterDirective("request_header", parseReqHdrCaddyfile)
 }
 
 // parseCaddyfile sets up the handler for response headers from
 // Caddyfile tokens. Syntax:
 //
-//     header [<matcher>] [[+|-]<field> [<value|regexp>] [<replacement>]] {
+//     header [<matcher>] [[+|-|?]<field> [<value|regexp>] [<replacement>]] {
 //         [+]<field> [<value|regexp> [<replacement>]]
+//         ?<field> <default_value>
 //         -<field>
 //         [defer]
 //     }
@@ -39,17 +42,23 @@ func init() {
 // Either a block can be opened or a single header field can be configured
 // in the first line, but not both in the same directive. Header operations
 // are deferred to write-time if any headers are being deleted or if the
-// 'defer' subdirective is used.
-func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) {
-	hdr := new(Handler)
-
-	makeResponseOps := func() {
-		if hdr.Response == nil {
-			hdr.Response = &RespHeaderOps{
-				HeaderOps: new(HeaderOps),
-			}
+// 'defer' subdirective is used. + appends a header value, - deletes a field,
+// and ? conditionally sets a value only if the header field is not already
+// set.
+func parseCaddyfile(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) {
+	matcherSet, err := h.ExtractMatcherSet()
+	if err != nil {
+		return nil, err
+	}
+
+	makeHandler := func() Handler {
+		return Handler{
+			Response: &RespHeaderOps{
+				HeaderOps: &HeaderOps{},
+			},
 		}
 	}
+	handler, handlerWithRequire := makeHandler(), makeHandler()
 
 	for h.Next() {
 		// first see if headers are in the initial line
@@ -64,10 +73,18 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
 			if h.NextArg() {
 				replacement = h.Val()
 			}
-			makeResponseOps()
-			CaddyfileHeaderOp(hdr.Response.HeaderOps, field, value, replacement)
-			if len(hdr.Response.HeaderOps.Delete) > 0 {
-				hdr.Response.Deferred = true
+			err := applyHeaderOp(
+				handler.Response.HeaderOps,
+				handler.Response,
+				field,
+				value,
+				replacement,
+			)
+			if err != nil {
+				return nil, h.Err(err.Error())
+			}
+			if len(handler.Response.HeaderOps.Delete) > 0 {
+				handler.Response.Deferred = true
 			}
 		}
 
@@ -75,12 +92,18 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
 		for h.NextBlock(0) {
 			field := h.Val()
 			if field == "defer" {
-				hdr.Response.Deferred = true
+				handler.Response.Deferred = true
 				continue
 			}
 			if hasArgs {
-				return nil, h.Err("cannot specify headers in both arguments and block")
+				return nil, h.Err("cannot specify headers in both arguments and block") // because it would be weird
 			}
+
+			// sometimes it is habitual for users to suffix a field name with a colon,
+			// as if they were writing a curl command or something; see
+			// https://caddy.community/t/v2-reverse-proxy-please-add-cors-example-to-the-docs/7349/19
+			field = strings.TrimSuffix(field, ":")
+
 			var value, replacement string
 			if h.NextArg() {
 				value = h.Val()
@@ -88,15 +111,34 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
 			if h.NextArg() {
 				replacement = h.Val()
 			}
-			makeResponseOps()
-			CaddyfileHeaderOp(hdr.Response.HeaderOps, field, value, replacement)
-			if len(hdr.Response.HeaderOps.Delete) > 0 {
-				hdr.Response.Deferred = true
+
+			handlerToUse := handler
+			if strings.HasPrefix(field, "?") {
+				handlerToUse = handlerWithRequire
+			}
+
+			err := applyHeaderOp(
+				handlerToUse.Response.HeaderOps,
+				handlerToUse.Response,
+				field,
+				value,
+				replacement,
+			)
+			if err != nil {
+				return nil, h.Err(err.Error())
 			}
 		}
 	}
 
-	return hdr, nil
+	var configValues []httpcaddyfile.ConfigValue
+	if !reflect.DeepEqual(handler, makeHandler()) {
+		configValues = append(configValues, h.NewRoute(matcherSet, handler)...)
+	}
+	if !reflect.DeepEqual(handlerWithRequire, makeHandler()) {
+		configValues = append(configValues, h.NewRoute(matcherSet, handlerWithRequire)...)
+	}
+
+	return configValues, nil
 }
 
 // parseReqHdrCaddyfile sets up the handler for request headers
@@ -104,17 +146,27 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
 //
 //     request_header [<matcher>] [[+|-]<field> [<value|regexp>] [<replacement>]]
 //
-func parseReqHdrCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error) {
-	hdr := new(Handler)
+func parseReqHdrCaddyfile(h httpcaddyfile.Helper) ([]httpcaddyfile.ConfigValue, error) {
+	matcherSet, err := h.ExtractMatcherSet()
+	if err != nil {
+		return nil, err
+	}
+
+	configValues := []httpcaddyfile.ConfigValue{}
+
 	for h.Next() {
 		if !h.NextArg() {
 			return nil, h.ArgErr()
 		}
 		field := h.Val()
 
+		hdr := Handler{
+			Request: &HeaderOps{},
+		}
+
 		// sometimes it is habitual for users to suffix a field name with a colon,
 		// as if they were writing a curl command or something; see
-		// https://caddy.community/t/v2-reverse-proxy-please-add-cors-example-to-the-docs/7349
+		// https://caddy.community/t/v2-reverse-proxy-please-add-cors-example-to-the-docs/7349/19
 		field = strings.TrimSuffix(field, ":")
 
 		var value, replacement string
@@ -131,13 +183,17 @@ func parseReqHdrCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler,
 		if hdr.Request == nil {
 			hdr.Request = new(HeaderOps)
 		}
-		CaddyfileHeaderOp(hdr.Request, field, value, replacement)
+		if err := CaddyfileHeaderOp(hdr.Request, field, value, replacement); err != nil {
+			return nil, h.Err(err.Error())
+		}
+
+		configValues = append(configValues, h.NewRoute(matcherSet, hdr)...)
 
 		if h.NextArg() {
 			return nil, h.ArgErr()
 		}
 	}
-	return hdr, nil
+	return configValues, nil
 }
 
 // CaddyfileHeaderOp applies a new header operation according to
@@ -148,32 +204,59 @@ func parseReqHdrCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler,
 // will be used to search and then replacement will be used to
 // complete the substring replacement; in that case, any + or -
 // prefix to field will be ignored.
-func CaddyfileHeaderOp(ops *HeaderOps, field, value, replacement string) {
-	if strings.HasPrefix(field, "+") {
+func CaddyfileHeaderOp(ops *HeaderOps, field, value, replacement string) error {
+	return applyHeaderOp(ops, nil, field, value, replacement)
+}
+
+func applyHeaderOp(ops *HeaderOps, respHeaderOps *RespHeaderOps, field, value, replacement string) error {
+	switch {
+	case strings.HasPrefix(field, "+"): // append
 		if ops.Add == nil {
 			ops.Add = make(http.Header)
 		}
 		ops.Add.Set(field[1:], value)
-	} else if strings.HasPrefix(field, "-") {
+
+	case strings.HasPrefix(field, "-"): // delete
 		ops.Delete = append(ops.Delete, field[1:])
-	} else {
-		if replacement == "" {
-			if ops.Set == nil {
-				ops.Set = make(http.Header)
-			}
-			ops.Set.Set(field, value)
-		} else {
-			if ops.Replace == nil {
-				ops.Replace = make(map[string][]Replacement)
+		if respHeaderOps != nil {
+			respHeaderOps.Deferred = true
+		}
+
+	case strings.HasPrefix(field, "?"): // default (conditional on not existing) - response headers only
+		if respHeaderOps == nil {
+			return fmt.Errorf("%v: the default header modifier ('?') can only be used on response headers; for conditional manipulation of request headers, use matchers", field)
+		}
+		if respHeaderOps.Require == nil {
+			respHeaderOps.Require = &caddyhttp.ResponseMatcher{
+				Headers: make(http.Header),
 			}
-			field = strings.TrimLeft(field, "+-")
-			ops.Replace[field] = append(
-				ops.Replace[field],
-				Replacement{
-					SearchRegexp: value,
-					Replace:      replacement,
-				},
-			)
 		}
+		field = strings.TrimPrefix(field, "?")
+		respHeaderOps.Require.Headers[field] = nil
+		if respHeaderOps.Set == nil {
+			respHeaderOps.Set = make(http.Header)
+		}
+		respHeaderOps.Set.Set(field, value)
+
+	case replacement != "": // replace
+		if ops.Replace == nil {
+			ops.Replace = make(map[string][]Replacement)
+		}
+		field = strings.TrimLeft(field, "+-?")
+		ops.Replace[field] = append(
+			ops.Replace[field],
+			Replacement{
+				SearchRegexp: value,
+				Replace:      replacement,
+			},
+		)
+
+	default: // set (overwrite)
+		if ops.Set == nil {
+			ops.Set = make(http.Header)
+		}
+		ops.Set.Set(field, value)
 	}
+
+	return nil
 }
diff --git a/modules/caddyhttp/headers/headers_test.go b/modules/caddyhttp/headers/headers_test.go
index e4f03ad..11bdb0d 100644
--- a/modules/caddyhttp/headers/headers_test.go
+++ b/modules/caddyhttp/headers/headers_test.go
@@ -14,8 +14,197 @@
 
 package headers
 
-import "testing"
+import (
+	"context"
+	"fmt"
+	"net/http"
+	"net/http/httptest"
+	"reflect"
+	"testing"
 
-func TestReqHeaders(t *testing.T) {
-	// TODO: write tests
+	"github.com/caddyserver/caddy/v2"
+	"github.com/caddyserver/caddy/v2/modules/caddyhttp"
+)
+
+func TestHandler(t *testing.T) {
+	for i, tc := range []struct {
+		handler            Handler
+		reqHeader          http.Header
+		respHeader         http.Header
+		respStatusCode     int
+		expectedReqHeader  http.Header
+		expectedRespHeader http.Header
+	}{
+		{
+			handler: Handler{
+				Request: &HeaderOps{
+					Add: http.Header{
+						"Expose-Secrets": []string{"always"},
+					},
+				},
+			},
+			reqHeader: http.Header{
+				"Expose-Secrets": []string{"i'm serious"},
+			},
+			expectedReqHeader: http.Header{
+				"Expose-Secrets": []string{"i'm serious", "always"},
+			},
+		},
+		{
+			handler: Handler{
+				Request: &HeaderOps{
+					Set: http.Header{
+						"Who-Wins": []string{"batman"},
+					},
+				},
+			},
+			reqHeader: http.Header{
+				"Who-Wins": []string{"joker"},
+			},
+			expectedReqHeader: http.Header{
+				"Who-Wins": []string{"batman"},
+			},
+		},
+		{
+			handler: Handler{
+				Request: &HeaderOps{
+					Delete: []string{"Kick-Me"},
+				},
+			},
+			reqHeader: http.Header{
+				"Kick-Me": []string{"if you can"},
+				"Keep-Me": []string{"i swear i'm innocent"},
+			},
+			expectedReqHeader: http.Header{
+				"Keep-Me": []string{"i swear i'm innocent"},
+			},
+		},
+		{
+			handler: Handler{
+				Request: &HeaderOps{
+					Replace: map[string][]Replacement{
+						"Best-Server": {
+							Replacement{
+								Search:  "NGINX",
+								Replace: "the Caddy web server",
+							},
+							Replacement{
+								SearchRegexp: `Apache(\d+)`,
+								Replace:      "Caddy",
+							},
+						},
+					},
+				},
+			},
+			reqHeader: http.Header{
+				"Best-Server": []string{"it's NGINX, undoubtedly", "I love Apache2"},
+			},
+			expectedReqHeader: http.Header{
+				"Best-Server": []string{"it's the Caddy web server, undoubtedly", "I love Caddy"},
+			},
+		},
+		{
+			handler: Handler{
+				Response: &RespHeaderOps{
+					Require: &caddyhttp.ResponseMatcher{
+						Headers: http.Header{
+							"Cache-Control": nil,
+						},
+					},
+					HeaderOps: &HeaderOps{
+						Add: http.Header{
+							"Cache-Control": []string{"no-cache"},
+						},
+					},
+				},
+			},
+			respHeader: http.Header{},
+			expectedRespHeader: http.Header{
+				"Cache-Control": []string{"no-cache"},
+			},
+		},
+		{
+			handler: Handler{
+				Response: &RespHeaderOps{
+					Require: &caddyhttp.ResponseMatcher{
+						Headers: http.Header{
+							"Cache-Control": []string{"no-cache"},
+						},
+					},
+					HeaderOps: &HeaderOps{
+						Delete: []string{"Cache-Control"},
+					},
+				},
+			},
+			respHeader: http.Header{
+				"Cache-Control": []string{"no-cache"},
+			},
+			expectedRespHeader: http.Header{},
+		},
+		{
+			handler: Handler{
+				Response: &RespHeaderOps{
+					Require: &caddyhttp.ResponseMatcher{
+						StatusCode: []int{5},
+					},
+					HeaderOps: &HeaderOps{
+						Add: http.Header{
+							"Fail-5xx": []string{"true"},
+						},
+					},
+				},
+			},
+			respStatusCode: 503,
+			respHeader:     http.Header{},
+			expectedRespHeader: http.Header{
+				"Fail-5xx": []string{"true"},
+			},
+		},
+	} {
+		rr := httptest.NewRecorder()
+
+		req := &http.Request{Header: tc.reqHeader}
+		repl := caddy.NewReplacer()
+		ctx := context.WithValue(req.Context(), caddy.ReplacerCtxKey, repl)
+		req = req.WithContext(ctx)
+
+		tc.handler.Provision(caddy.Context{})
+
+		next := nextHandler(func(w http.ResponseWriter, r *http.Request) error {
+			for k, hdrs := range tc.respHeader {
+				for _, v := range hdrs {
+					w.Header().Add(k, v)
+				}
+			}
+
+			status := 200
+			if tc.respStatusCode != 0 {
+				status = tc.respStatusCode
+			}
+			w.WriteHeader(status)
+
+			if tc.expectedReqHeader != nil && !reflect.DeepEqual(r.Header, tc.expectedReqHeader) {
+				return fmt.Errorf("expected request header %v, got %v", tc.expectedReqHeader, r.Header)
+			}
+
+			return nil
+		})
+
+		if err := tc.handler.ServeHTTP(rr, req, next); err != nil {
+			t.Errorf("Test %d: %w", i, err)
+			continue
+		}
+
+		actual := rr.Header()
+		if tc.expectedRespHeader != nil && !reflect.DeepEqual(actual, tc.expectedRespHeader) {
+			t.Errorf("Test %d: expected response header %v, got %v", i, tc.expectedRespHeader, actual)
+			continue
+		}
+	}
+}
+
+type nextHandler func(http.ResponseWriter, *http.Request) error
+
+func (f nextHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) error {
+	return f(w, r)
 }