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>

caddyconfig/httpcaddyfile/directives.go

@@ -103,20 +103,11 @@ func RegisterHandlerDirective(dir string, setupFunc UnmarshalHandlerFunc) {
 			return nil, h.ArgErr()
 		}
 
-		matcherSet, ok, err := h.MatcherToken()
+		matcherSet, err := h.ExtractMatcherSet()
 		if err != nil {
 			return nil, err
 		}
-		if ok {
-			// strip matcher token; we don't need to
-			// use the return value here because a
-			// new dispenser should have been made
-			// solely for this directive's tokens,
-			// with no other uses of same slice
-			h.Dispenser.Delete()
-		}
 
-		h.Dispenser.Reset() // pretend this lookahead never happened
 		val, err := setupFunc(h)
 		if err != nil {
 			return nil, err
@@ -201,7 +192,12 @@ func (h Helper) ExtractMatcherSet() (caddy.ModuleMap, error) {
 		return nil, err
 	}
 	if hasMatcher {
-		h.Dispenser.Delete() // strip matcher token
+		// strip matcher token; we don't need to
+		// use the return value here because a
+		// new dispenser should have been made
+		// solely for this directive's tokens,
+		// with no other uses of same slice
+		h.Dispenser.Delete()
 	}
 	h.Dispenser.Reset() // pretend this lookahead never happened
 	return matcherSet, nil

caddytest/integration/caddyfile_adapt/header.txt

@@ -0,0 +1,107 @@
+:80 {
+	header Denis "Ritchie"
+	header +Edsger "Dijkstra"
+	header ?John "von Neumann"
+	header -Wolfram
+	header {
+		Grace: "Hopper"  # some users habitually suffix field names with a colon
+		+Ray "Solomonoff"
+		?Tim "Berners-Lee"
+		defer
+	}
+}
+----------
+{
+	"apps": {
+		"http": {
+			"servers": {
+				"srv0": {
+					"listen": [
+						":80"
+					],
+					"routes": [
+						{
+							"handle": [
+								{
+									"handler": "headers",
+									"response": {
+										"set": {
+											"Denis": [
+												"Ritchie"
+											]
+										}
+									}
+								},
+								{
+									"handler": "headers",
+									"response": {
+										"add": {
+											"Edsger": [
+												"Dijkstra"
+											]
+										}
+									}
+								},
+								{
+									"handler": "headers",
+									"response": {
+										"require": {
+											"headers": {
+												"John": null
+											}
+										},
+										"set": {
+											"John": [
+												"von Neumann"
+											]
+										}
+									}
+								},
+								{
+									"handler": "headers",
+									"response": {
+										"deferred": true,
+										"delete": [
+											"Wolfram"
+										]
+									}
+								},
+								{
+									"handler": "headers",
+									"response": {
+										"add": {
+											"Ray": [
+												"Solomonoff"
+											]
+										},
+										"deferred": true,
+										"set": {
+											"Grace": [
+												"Hopper"
+											]
+										}
+									}
+								},
+								{
+									"handler": "headers",
+									"response": {
+										"require": {
+											"headers": {
+												"Tim": null
+											}
+										},
+										"set": {
+											"Tim": [
+												"Berners-Lee"
+											]
+										}
+									}
+								}
+							]
+						}
+					]
+				}
+			}
+		}
+	}
+}

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)
+// '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
+	}
 
-	makeResponseOps := func() {
-		if hdr.Response == nil {
-			hdr.Response = &RespHeaderOps{
-				HeaderOps: new(HeaderOps),
-			}
+	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)
-			}
-			field = strings.TrimLeft(field, "+-")
-			ops.Replace[field] = append(
-				ops.Replace[field],
-				Replacement{
-					SearchRegexp: value,
-					Replace:      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.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
 }

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)
 }

modules/caddyhttp/push/caddyfile.go

@@ -59,6 +59,8 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
 					return nil, h.ArgErr()
 				}
 				for innerNesting := h.Nesting(); h.NextBlock(innerNesting); {
+					var err error
+
 					// include current token, which we treat as an argument here
 					args := []string{h.Val()}
 					args = append(args, h.RemainingArgs()...)
@@ -66,16 +68,21 @@ func parseCaddyfile(h httpcaddyfile.Helper) (caddyhttp.MiddlewareHandler, error)
 					if handler.Headers == nil {
 						handler.Headers = new(HeaderConfig)
 					}
+
 					switch len(args) {
 					case 1:
-						headers.CaddyfileHeaderOp(&handler.Headers.HeaderOps, args[0], "", "")
+						err = headers.CaddyfileHeaderOp(&handler.Headers.HeaderOps, args[0], "", "")
 					case 2:
-						headers.CaddyfileHeaderOp(&handler.Headers.HeaderOps, args[0], args[1], "")
+						err = headers.CaddyfileHeaderOp(&handler.Headers.HeaderOps, args[0], args[1], "")
 					case 3:
-						headers.CaddyfileHeaderOp(&handler.Headers.HeaderOps, args[0], args[1], args[2])
+						err = headers.CaddyfileHeaderOp(&handler.Headers.HeaderOps, args[0], args[1], args[2])
 					default:
 						return nil, h.ArgErr()
 					}
+
+					if err != nil {
+						return nil, h.Err(err.Error())
+					}
 				}
 
 			case "GET", "HEAD":

modules/caddyhttp/reverseproxy/caddyfile.go

@@ -480,6 +480,8 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 				h.BufferRequests = true
 
 			case "header_up":
+				var err error
+
 				if h.Headers == nil {
 					h.Headers = new(headers.Handler)
 				}
@@ -487,18 +489,25 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 					h.Headers.Request = new(headers.HeaderOps)
 				}
 				args := d.RemainingArgs()
+
 				switch len(args) {
 				case 1:
-					headers.CaddyfileHeaderOp(h.Headers.Request, args[0], "", "")
+					err = headers.CaddyfileHeaderOp(h.Headers.Request, args[0], "", "")
 				case 2:
-					headers.CaddyfileHeaderOp(h.Headers.Request, args[0], args[1], "")
+					err = headers.CaddyfileHeaderOp(h.Headers.Request, args[0], args[1], "")
 				case 3:
-					headers.CaddyfileHeaderOp(h.Headers.Request, args[0], args[1], args[2])
+					err = headers.CaddyfileHeaderOp(h.Headers.Request, args[0], args[1], args[2])
 				default:
 					return d.ArgErr()
 				}
 
+				if err != nil {
+					return d.Err(err.Error())
+				}
+
 			case "header_down":
+				var err error
+
 				if h.Headers == nil {
 					h.Headers = new(headers.Handler)
 				}
@@ -510,15 +519,19 @@ func (h *Handler) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
 				args := d.RemainingArgs()
 				switch len(args) {
 				case 1:
-					headers.CaddyfileHeaderOp(h.Headers.Response.HeaderOps, args[0], "", "")
+					err = headers.CaddyfileHeaderOp(h.Headers.Response.HeaderOps, args[0], "", "")
 				case 2:
-					headers.CaddyfileHeaderOp(h.Headers.Response.HeaderOps, args[0], args[1], "")
+					err = headers.CaddyfileHeaderOp(h.Headers.Response.HeaderOps, args[0], args[1], "")
 				case 3:
-					headers.CaddyfileHeaderOp(h.Headers.Response.HeaderOps, args[0], args[1], args[2])
+					err = headers.CaddyfileHeaderOp(h.Headers.Response.HeaderOps, args[0], args[1], args[2])
 				default:
 					return d.ArgErr()
 				}
 
+				if err != nil {
+					return d.Err(err.Error())
+				}
+
 			case "transport":
 				if !d.NextArg() {
 					return d.ArgErr()