caddy.go: Check whether @id is unique(#6991)

caddy.go

@@ -224,6 +224,10 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force
 	idx := make(map[string]string)
 	err = indexConfigObjects(rawCfg[rawConfigKey], "/"+rawConfigKey, idx)
 	if err != nil {
+		err2 := rollbackRawCfg()
+		if err2 != nil {
+			err = errors.Join(err, err2)
+		}
 		return APIError{
 			HTTPStatus: http.StatusInternalServerError,
 			Err:        fmt.Errorf("indexing config: %v", err),
@@ -239,12 +243,10 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force
 			// with what caddy is still running; we need to
 			// unmarshal it again because it's likely that
 			// pointers deep in our rawCfg map were modified
-			var oldCfg any
+			err2 := rollbackRawCfg()
-			err2 := json.Unmarshal(rawCfgJSON, &oldCfg)
 			if err2 != nil {
 				err = fmt.Errorf("%v; additionally, restoring old config: %v", err, err2)
 			}
-			rawCfg[rawConfigKey] = oldCfg
 		}
 
 		return fmt.Errorf("loading new config: %v", err)
@@ -261,6 +263,20 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force
 	return nil
 }
 
+func rollbackRawCfg() error {
+	var oldCfg any
+	if len(rawCfgJSON) != 0 {
+		err := json.Unmarshal(rawCfgJSON, &oldCfg)
+		if err != nil {
+			return err
+		}
+		rawCfg[rawConfigKey] = oldCfg
+	} else {
+		rawCfg[rawConfigKey] = nil
+	}
+	return nil
+}
+
 // readConfig traverses the current config to path
 // and writes its JSON encoding to out.
 func readConfig(path string, out io.Writer) error {
@@ -268,6 +284,14 @@ func readConfig(path string, out io.Writer) error {
 	defer rawCfgMu.RUnlock()
 	return unsyncedConfigAccess(http.MethodGet, path, nil, out)
 }
+func writeToIdIndex(index map[string]string, key, val string) error {
+	_, found := index[key]
+	if found {
+		return errors.New("multiple keys found: " + key)
+	}
+	index[key] = val
+	return nil
+}
 
 // indexConfigObjects recursively searches ptr for object fields named
 // "@id" and maps that ID value to the full configPath in the index.
@@ -280,9 +304,15 @@ func indexConfigObjects(ptr any, configPath string, index map[string]string) err
 			if k == idKey {
 				switch idVal := v.(type) {
 				case string:
-					index[idVal] = configPath
+					err := writeToIdIndex(index, idVal, configPath)
+					if err != nil {
+						return err
+					}
 				case float64: // all JSON numbers decode as float64
-					index[fmt.Sprintf("%v", idVal)] = configPath
+					err := writeToIdIndex(index, fmt.Sprintf("%v", idVal), configPath)
+					if err != nil {
+						return err
+					}
 				default:
 					return fmt.Errorf("%s: %s field must be a string or number", configPath, idKey)
 				}

caddytest/caddytest_test.go

@@ -1,6 +1,7 @@
 package caddytest
 
 import (
+	"bytes"
 	"net/http"
 	"strings"
 	"testing"
@@ -126,3 +127,77 @@ func TestLoadUnorderedJSON(t *testing.T) {
 	}
 	tester.AssertResponseCode(req, 200)
 }
+
+func TestCheckID(t *testing.T) {
+	tester := NewTester(t)
+	tester.InitServer(`{
+		"admin": {
+			"listen": "localhost:2999"
+		},
+		"apps": {
+			"http": {
+				"http_port": 9080,
+				"servers": {
+					"s_server": {
+						"@id": "s_server",
+						"listen": [
+							":9080"
+						],
+						"routes": [
+							{
+								"handle": [
+									{
+										"handler": "static_response",
+										"body": "Hello"
+									}
+								]
+							}
+						]
+					}
+				}
+			}
+		}
+	}
+  `, "json")
+	headers := []string{"Content-Type:application/json"}
+	sServer1 := []byte(
+		`{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route1","handle":[{"handler":"static_response","body":"Hello 2"}]}]}`)
+	tester.AssertPutResponseBody(
+		"http://localhost:2999/id/s_server",
+		headers, bytes.NewBuffer(sServer1), 409, `{"error":"[/config/apps/http/servers/s_server] key already exists: s_server"}
+`)
+	tester.AssertPostResponseBody("http://localhost:2999/id/s_server", headers, bytes.NewBuffer(sServer1), 200, "")
+
+	tester.AssertGetResponse("http://localhost:9080/", 200, "Hello 2")
+	tester.AssertPostResponseBody(
+		"http://localhost:2999/id/s_server",
+		headers, bytes.NewBuffer([]byte(
+			`{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route1","handle":[
+  {"handler":"static_response","body":"Hello2"}],"match":[{"path":["/route_1/*"]}]}]}`)), 200, "")
+
+	sServer2 := []byte(`
+{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route1","handle":[
+  {"handler":"static_response","body":"Hello2"}],"match":[{"path":["/route_1/*"]}]}]}`)
+	tester.AssertPatchResponseBody(
+		"http://localhost:2999/id/s_server",
+		headers, bytes.NewBuffer(sServer2), 200, "")
+	route2 := []byte(
+		`{"@id":"route2","handle": [{"handler": "static_response","body": "route2"}],"match":[{"path":["/route_2/*"]}]}`)
+	tester.AssertPutResponseBody(
+		"http://localhost:2999/id/route1",
+		headers, bytes.NewBuffer(route2), 200, "")
+	tester.AssertGetResponse("http://localhost:2999/config", 200,
+		`{"admin":{"listen":"localhost:2999"},"apps":{"http":{"http_port":9080,"servers":{"s_server":{"@id":"s_server","listen":[":9080"],"routes":[{"@id":"route2","handle":[{"body":"route2","handler":"static_response"}],"match":[{"path":["/route_2/*"]}]},{"@id":"route1","handle":[{"body":"Hello2","handler":"static_response"}],"match":[{"path":["/route_1/*"]}]}]}}}}}
+`)
+	// use post or put to add one
+	tester.AssertPostResponseBody(
+		"http://localhost:2999/id/route2",
+		headers, bytes.NewBuffer(route2), 500, `{"error":"indexing config: multiple keys found: route2"}
+`)
+	// use patch to update
+	tester.AssertPatchResponseBody(
+		"http://localhost:2999/id/route1",
+		headers, bytes.NewBuffer([]byte(`{"@id":"route1","handle":[{"handler":"static_response","body":"route1"}],"match":[{"path":["/route_1/*"]}]}`)),
+		200, "")
+	tester.AssertGetResponse("http://localhost:9080/route_1/", 200, "route1")
+}