caddy.go: Check whether @id is unique

caddy.go

@@ -227,8 +227,18 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force
 	idx := make(map[string]string)
 	err = indexConfigObjects(rawCfg[rawConfigKey], "/"+rawConfigKey, idx)
 	if err != nil {
+		if len(rawCfgJSON) > 0 {
+			var oldCfg any
+			err2 := json.Unmarshal(rawCfgJSON, &oldCfg)
+			if err2 != nil {
+				err = fmt.Errorf("%v; additionally, restoring old config: %v", err, err2)
+			}
+			rawCfg[rawConfigKey] = oldCfg
+		} else {
+			rawCfg[rawConfigKey] = nil
+		}
 		return APIError{
-			HTTPStatus: http.StatusInternalServerError,
+			HTTPStatus: http.StatusBadRequest,
 			Err:        fmt.Errorf("indexing config: %v", err),
 		}
 	}
@@ -248,6 +258,8 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force
 				err = fmt.Errorf("%v; additionally, restoring old config: %v", err, err2)
 			}
 			rawCfg[rawConfigKey] = oldCfg
+		} else {
+			rawCfg[rawConfigKey] = nil
 		}
 
 		return fmt.Errorf("loading new config: %v", err)
@@ -281,14 +293,19 @@ func indexConfigObjects(ptr any, configPath string, index map[string]string) err
 	case map[string]any:
 		for k, v := range val {
 			if k == idKey {
+				var idStr string
 				switch idVal := v.(type) {
 				case string:
-					index[idVal] = configPath
+					idStr = idVal
 				case float64: // all JSON numbers decode as float64
-					index[fmt.Sprintf("%v", idVal)] = configPath
+					idStr = fmt.Sprintf("%v", idVal)
 				default:
 					return fmt.Errorf("%s: %s field must be a string or number", configPath, idKey)
 				}
+				if existingPath, ok := index[idStr]; ok {
+					return fmt.Errorf("duplicate ID '%s' found at %s and %s", idStr, existingPath, configPath)
+				}
+				index[idStr] = configPath
 				continue
 			}
 			// traverse this object property recursively

caddytest/caddytest_test.go

@@ -1,6 +1,7 @@
 package caddytest
 
 import (
+	"bytes"
 	"net/http"
 	"strings"
 	"testing"
@@ -126,3 +127,118 @@ 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"}]}]}`)
+
+	// PUT to an existing ID should fail with a 409 conflict
+	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"}`+"\n")
+
+	// POST replaces the object fully
+	tester.AssertPostResponseBody(
+		"http://localhost:2999/id/s_server",
+		headers,
+		bytes.NewBuffer(sServer1),
+		200,
+		"")
+
+	// Verify the server is running the new route
+	tester.AssertGetResponse(
+		"http://localhost:9080/",
+		200,
+		"Hello 2")
+
+	// Update the existing route to ensure IDs are handled correctly when replaced
+	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/*"]}]}]}`)
+
+	// Identical patch should succeed and return 200 (config is unchanged branch)
+	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/*"]}]}`)
+
+	// Put a new route2 object before the route1 object due to the path of /id/route1
+	// Being translated to: /config/apps/http/servers/s_server/routes/0
+	tester.AssertPutResponseBody(
+		"http://localhost:2999/id/route1",
+		headers,
+		bytes.NewBuffer(route2),
+		200,
+		"")
+
+	// Verify that the whole config looks correct, now containing both route1 and route2
+	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/*"]}]}]}}}}}`+"\n")
+
+	// Try to add another copy of route2 using POST to test duplicate ID handling
+	// Since the first route2 ended up at array index 0, and we are appending to the array, the index for the new element would be 2
+	tester.AssertPostResponseBody(
+		"http://localhost:2999/id/route2",
+		headers,
+		bytes.NewBuffer(route2),
+		400,
+		`{"error":"indexing config: duplicate ID 'route2' found at /config/apps/http/servers/s_server/routes/0 and /config/apps/http/servers/s_server/routes/2"}`+"\n")
+
+	// Use PATCH to modify an existing object successfully
+	tester.AssertPatchResponseBody(
+		"http://localhost:2999/id/route1",
+		headers,
+		bytes.NewBuffer([]byte(`{"@id":"route1","handle":[{"handler":"static_response","body":"route1"}],"match":[{"path":["/route_1/*"]}]}`)),
+		200,
+		"")
+
+	// Verify the PATCH updated the server state
+	tester.AssertGetResponse(
+		"http://localhost:9080/route_1/",
+		200,
+		"route1")
+}