caddyfile: Reject directives in the place of site addresses (#6104)

Co-authored-by: Francis Lavoie <lavofr@gmail.com>

caddyconfig/caddyfile/parse.go

@@ -160,14 +160,14 @@ func (p *parser) begin() error {
 	}
 
 	if ok, name := p.isNamedRoute(); ok {
-		// named routes only have one key, the route name
-		p.block.Keys = []string{name}
-		p.block.IsNamedRoute = true
-
 		// we just need a dummy leading token to ease parsing later
 		nameToken := p.Token()
 		nameToken.Text = name
 
+		// named routes only have one key, the route name
+		p.block.Keys = []Token{nameToken}
+		p.block.IsNamedRoute = true
+
 		// get all the tokens from the block, including the braces
 		tokens, err := p.blockTokens(true)
 		if err != nil {
@@ -211,10 +211,11 @@ func (p *parser) addresses() error {
 	var expectingAnother bool
 
 	for {
-		tkn := p.Val()
+		value := p.Val()
+		token := p.Token()
 
 		// special case: import directive replaces tokens during parse-time
-		if tkn == "import" && p.isNewLine() {
+		if value == "import" && p.isNewLine() {
 			err := p.doImport(0)
 			if err != nil {
 				return err
@@ -223,9 +224,9 @@ func (p *parser) addresses() error {
 		}
 
 		// Open brace definitely indicates end of addresses
-		if tkn == "{" {
+		if value == "{" {
 			if expectingAnother {
-				return p.Errf("Expected another address but had '%s' - check for extra comma", tkn)
+				return p.Errf("Expected another address but had '%s' - check for extra comma", value)
 			}
 			// Mark this server block as being defined with braces.
 			// This is used to provide a better error message when
@@ -237,15 +238,15 @@ func (p *parser) addresses() error {
 		}
 
 		// Users commonly forget to place a space between the address and the '{'
-		if strings.HasSuffix(tkn, "{") {
+		if strings.HasSuffix(value, "{") {
-			return p.Errf("Site addresses cannot end with a curly brace: '%s' - put a space between the token and the brace", tkn)
+			return p.Errf("Site addresses cannot end with a curly brace: '%s' - put a space between the token and the brace", value)
 		}
 
-		if tkn != "" { // empty token possible if user typed ""
+		if value != "" { // empty token possible if user typed ""
 			// Trailing comma indicates another address will follow, which
 			// may possibly be on the next line
-			if tkn[len(tkn)-1] == ',' {
+			if value[len(value)-1] == ',' {
-				tkn = tkn[:len(tkn)-1]
+				value = value[:len(value)-1]
 				expectingAnother = true
 			} else {
 				expectingAnother = false // but we may still see another one on this line
@@ -254,11 +255,12 @@ func (p *parser) addresses() error {
 			// If there's a comma here, it's probably because they didn't use a space
 			// between their two domains, e.g. "foo.com,bar.com", which would not be
 			// parsed as two separate site addresses.
-			if strings.Contains(tkn, ",") {
+			if strings.Contains(value, ",") {
-				return p.Errf("Site addresses cannot contain a comma ',': '%s' - put a space after the comma to separate site addresses", tkn)
+				return p.Errf("Site addresses cannot contain a comma ',': '%s' - put a space after the comma to separate site addresses", value)
 			}
 
-			p.block.Keys = append(p.block.Keys, tkn)
+			token.Text = value
+			p.block.Keys = append(p.block.Keys, token)
 		}
 
 		// Advance token and possibly break out of loop or return error
@@ -637,8 +639,8 @@ func (p *parser) closeCurlyBrace() error {
 func (p *parser) isNamedRoute() (bool, string) {
 	keys := p.block.Keys
 	// A named route block is a single key with parens, prefixed with &.
-	if len(keys) == 1 && strings.HasPrefix(keys[0], "&(") && strings.HasSuffix(keys[0], ")") {
+	if len(keys) == 1 && strings.HasPrefix(keys[0].Text, "&(") && strings.HasSuffix(keys[0].Text, ")") {
-		return true, strings.TrimSuffix(keys[0][2:], ")")
+		return true, strings.TrimSuffix(keys[0].Text[2:], ")")
 	}
 	return false, ""
 }
@@ -646,8 +648,8 @@ func (p *parser) isNamedRoute() (bool, string) {
 func (p *parser) isSnippet() (bool, string) {
 	keys := p.block.Keys
 	// A snippet block is a single key with parens. Nothing else qualifies.
-	if len(keys) == 1 && strings.HasPrefix(keys[0], "(") && strings.HasSuffix(keys[0], ")") {
+	if len(keys) == 1 && strings.HasPrefix(keys[0].Text, "(") && strings.HasSuffix(keys[0].Text, ")") {
-		return true, strings.TrimSuffix(keys[0][1:], ")")
+		return true, strings.TrimSuffix(keys[0].Text[1:], ")")
 	}
 	return false, ""
 }
@@ -691,11 +693,19 @@ func (p *parser) blockTokens(retainCurlies bool) ([]Token, error) {
 // grouped by segments.
 type ServerBlock struct {
 	HasBraces    bool
-	Keys         []string
+	Keys         []Token
 	Segments     []Segment
 	IsNamedRoute bool
 }
 
+func (sb ServerBlock) GetKeysText() []string {
+	res := []string{}
+	for _, k := range sb.Keys {
+		res = append(res, k.Text)
+	}
+	return res
+}
+
 // DispenseDirective returns a dispenser that contains
 // all the tokens in the server block.
 func (sb ServerBlock) DispenseDirective(dir string) *Dispenser {

caddyconfig/caddyfile/parse_test.go

@@ -347,7 +347,7 @@ func TestParseOneAndImport(t *testing.T) {
 				i, len(test.keys), len(result.Keys))
 			continue
 		}
-		for j, addr := range result.Keys {
+		for j, addr := range result.GetKeysText() {
 			if addr != test.keys[j] {
 				t.Errorf("Test %d, key %d: Expected '%s', but was '%s'",
 					i, j, test.keys[j], addr)
@@ -379,8 +379,9 @@ func TestRecursiveImport(t *testing.T) {
 	}
 
 	isExpected := func(got ServerBlock) bool {
-		if len(got.Keys) != 1 || got.Keys[0] != "localhost" {
+		textKeys := got.GetKeysText()
-			t.Errorf("got keys unexpected: expect localhost, got %v", got.Keys)
+		if len(textKeys) != 1 || textKeys[0] != "localhost" {
+			t.Errorf("got keys unexpected: expect localhost, got %v", textKeys)
 			return false
 		}
 		if len(got.Segments) != 2 {
@@ -474,8 +475,9 @@ func TestDirectiveImport(t *testing.T) {
 	}
 
 	isExpected := func(got ServerBlock) bool {
-		if len(got.Keys) != 1 || got.Keys[0] != "localhost" {
+		textKeys := got.GetKeysText()
-			t.Errorf("got keys unexpected: expect localhost, got %v", got.Keys)
+		if len(textKeys) != 1 || textKeys[0] != "localhost" {
+			t.Errorf("got keys unexpected: expect localhost, got %v", textKeys)
 			return false
 		}
 		if len(got.Segments) != 2 {
@@ -616,7 +618,7 @@ func TestParseAll(t *testing.T) {
 					i, len(test.keys[j]), j, len(block.Keys))
 				continue
 			}
-			for k, addr := range block.Keys {
+			for k, addr := range block.GetKeysText() {
 				if addr != test.keys[j][k] {
 					t.Errorf("Test %d, block %d, key %d: Expected '%s', but got '%s'",
 						i, j, k, test.keys[j][k], addr)
@@ -769,7 +771,7 @@ func TestSnippets(t *testing.T) {
 	if len(blocks) != 1 {
 		t.Fatalf("Expect exactly one server block. Got %d.", len(blocks))
 	}
-	if actual, expected := blocks[0].Keys[0], "http://example.com"; expected != actual {
+	if actual, expected := blocks[0].GetKeysText()[0], "http://example.com"; expected != actual {
 		t.Errorf("Expected server name to be '%s' but was '%s'", expected, actual)
 	}
 	if len(blocks[0].Segments) != 2 {
@@ -844,7 +846,7 @@ func TestSnippetAcrossMultipleFiles(t *testing.T) {
 	if len(blocks) != 1 {
 		t.Fatalf("Expect exactly one server block. Got %d.", len(blocks))
 	}
-	if actual, expected := blocks[0].Keys[0], "http://example.com"; expected != actual {
+	if actual, expected := blocks[0].GetKeysText()[0], "http://example.com"; expected != actual {
 		t.Errorf("Expected server name to be '%s' but was '%s'", expected, actual)
 	}
 	if len(blocks[0].Segments) != 1 {

caddyconfig/httpcaddyfile/addresses.go

@@ -88,15 +88,15 @@ func (st *ServerType) mapAddressToServerBlocks(originalServerBlocks []serverBloc
 		// will be served by them; this has the effect of treating each
 		// key of a server block as its own, but without having to repeat its
 		// contents in cases where multiple keys really can be served together
-		addrToKeys := make(map[string][]string)
+		addrToKeys := make(map[string][]caddyfile.Token)
 		for j, key := range sblock.block.Keys {
 			// a key can have multiple listener addresses if there are multiple
 			// arguments to the 'bind' directive (although they will all have
 			// the same port, since the port is defined by the key or is implicit
 			// through automatic HTTPS)
-			addrs, err := st.listenerAddrsForServerBlockKey(sblock, key, options)
+			addrs, err := st.listenerAddrsForServerBlockKey(sblock, key.Text, options)
 			if err != nil {
-				return nil, fmt.Errorf("server block %d, key %d (%s): determining listener address: %v", i, j, key, err)
+				return nil, fmt.Errorf("server block %d, key %d (%s): determining listener address: %v", i, j, key.Text, err)
 			}
 
 			// associate this key with each listener address it is served on
@@ -122,9 +122,9 @@ func (st *ServerType) mapAddressToServerBlocks(originalServerBlocks []serverBloc
 			// parse keys so that we only have to do it once
 			parsedKeys := make([]Address, 0, len(keys))
 			for _, key := range keys {
-				addr, err := ParseAddress(key)
+				addr, err := ParseAddress(key.Text)
 				if err != nil {
-					return nil, fmt.Errorf("parsing key '%s': %v", key, err)
+					return nil, fmt.Errorf("parsing key '%s': %v", key.Text, err)
 				}
 				parsedKeys = append(parsedKeys, addr.Normalize())
 			}

caddyconfig/httpcaddyfile/httptype.go

@@ -65,8 +65,11 @@ func (st ServerType) Setup(
 	originalServerBlocks := make([]serverBlock, 0, len(inputServerBlocks))
 	for _, sblock := range inputServerBlocks {
 		for j, k := range sblock.Keys {
-			if j == 0 && strings.HasPrefix(k, "@") {
+			if j == 0 && strings.HasPrefix(k.Text, "@") {
-				return nil, warnings, fmt.Errorf("cannot define a matcher outside of a site block: '%s'", k)
+				return nil, warnings, fmt.Errorf("%s:%d: cannot define a matcher outside of a site block: '%s'", k.File, k.Line, k.Text)
+			}
+			if _, ok := registeredDirectives[k.Text]; ok {
+				return nil, warnings, fmt.Errorf("%s:%d: parsed '%s' as a site address, but it is a known directive; directives must appear in a site block", k.File, k.Line, k.Text)
 			}
 		}
 		originalServerBlocks = append(originalServerBlocks, serverBlock{
@@ -490,7 +493,7 @@ func (ServerType) extractNamedRoutes(
 			route.HandlersRaw = []json.RawMessage{caddyconfig.JSONModuleObject(handler, "handler", subroute.CaddyModule().ID.Name(), h.warnings)}
 		}
 
-		namedRoutes[sb.block.Keys[0]] = &route
+		namedRoutes[sb.block.GetKeysText()[0]] = &route
 	}
 	options["named_routes"] = namedRoutes
 
@@ -528,12 +531,12 @@ func (st *ServerType) serversFromPairings(
 		// address), otherwise their routes will improperly be added
 		// to the same server (see issue #4635)
 		for j, sblock1 := range p.serverBlocks {
-			for _, key := range sblock1.block.Keys {
+			for _, key := range sblock1.block.GetKeysText() {
 				for k, sblock2 := range p.serverBlocks {
 					if k == j {
 						continue
 					}
-					if sliceContains(sblock2.block.Keys, key) {
+					if sliceContains(sblock2.block.GetKeysText(), key) {
 						return nil, fmt.Errorf("ambiguous site definition: %s", key)
 					}
 				}

caddytest/integration/acme_test.go

@@ -23,7 +23,7 @@ import (
 	"go.uber.org/zap"
 )
 
-const acmeChallengePort = 8080
+const acmeChallengePort = 9081
 
 // Test the basic functionality of Caddy's ACME server
 func TestACMEServerWithDefaults(t *testing.T) {

caddytest/integration/caddyfile_adapt/http_valid_directive_like_site_address.txt

@@ -0,0 +1,46 @@
+http://handle {
+	file_server
+}
+----------
+{
+	"apps": {
+		"http": {
+			"servers": {
+				"srv0": {
+					"listen": [
+						":80"
+					],
+					"routes": [
+						{
+							"match": [
+								{
+									"host": [
+										"handle"
+									]
+								}
+							],
+							"handle": [
+								{
+									"handler": "subroute",
+									"routes": [
+										{
+											"handle": [
+												{
+													"handler": "file_server",
+													"hide": [
+														"./Caddyfile"
+													]
+												}
+											]
+										}
+									]
+								}
+							],
+							"terminal": true
+						}
+					]
+				}
+			}
+		}
+	}
+}

caddytest/integration/caddyfile_test.go

@@ -496,6 +496,7 @@ func TestUriReplace(t *testing.T) {
 
 	tester.AssertGetResponse("http://localhost:9080/endpoint?test={%20content%20}", 200, "test=%7B%20content%20%7D")
 }
+
 func TestHandleErrorSimpleCodes(t *testing.T) {
 	tester := caddytest.NewTester(t)
 	tester.InitServer(`{
@@ -584,3 +585,30 @@ func TestHandleErrorRangeAndCodes(t *testing.T) {
 	tester.AssertGetResponse("http://localhost:9080/threehundred", 301, "Error code is equal to 500 or in the [300..399] range")
 	tester.AssertGetResponse("http://localhost:9080/private", 410, "Error in the [400 .. 499] range")
 }
+
+func TestInvalidSiteAddressesAsDirectives(t *testing.T) {
+	type testCase struct {
+		config, expectedError string
+	}
+
+	failureCases := []testCase{
+		{
+			config: `
+			handle {
+				file_server
+			}`,
+			expectedError: `Caddyfile:2: parsed 'handle' as a site address, but it is a known directive; directives must appear in a site block`,
+		},
+		{
+			config: `
+			reverse_proxy localhost:9000 localhost:9001 {
+				file_server
+			}`,
+			expectedError: `Caddyfile:2: parsed 'reverse_proxy' as a site address, but it is a known directive; directives must appear in a site block`,
+		},
+	}
+
+	for _, failureCase := range failureCases {
+		caddytest.AssertLoadError(t, failureCase.config, "caddyfile", failureCase.expectedError)
+	}
+}

caddytest/integration/pki_test.go

@@ -9,6 +9,9 @@ import (
 func TestLeafCertLifetimeLessThanIntermediate(t *testing.T) {
 	caddytest.AssertLoadError(t, `
     {
+      "admin": {
+        "disabled": true
+      },
       "apps": {
         "http": {
           "servers": {
@@ -56,6 +59,9 @@ func TestLeafCertLifetimeLessThanIntermediate(t *testing.T) {
 func TestIntermediateLifetimeLessThanRoot(t *testing.T) {
 	caddytest.AssertLoadError(t, `
     {
+      "admin": {
+        "disabled": true
+      },
       "apps": {
         "http": {
           "servers": {