diff --git a/admin.go b/admin.go index 5ceb3daeb2d..9d74b201618 100644 --- a/admin.go +++ b/admin.go @@ -1142,6 +1142,20 @@ func handleStop(w http.ResponseWriter, r *http.Request) error { return nil } +func parseCanonicalArrayIndex(idx string) (int, error) { + if idx == "" { + return 0, fmt.Errorf("empty index") + } + i, err := strconv.Atoi(idx) + if err != nil { + return 0, err + } + if strconv.Itoa(i) != idx { + return 0, fmt.Errorf("non-canonical array index") + } + return i, nil +} + // unsyncedConfigAccess traverses into the current config and performs // the operation at path according to method, using body and out as // needed. This is a low-level, unsynchronized function; most callers @@ -1203,11 +1217,12 @@ traverseLoop: var idx int if method != http.MethodPost { idxStr := parts[len(parts)-1] - idx, err = strconv.Atoi(idxStr) + idx, err = parseCanonicalArrayIndex(idxStr) if err != nil { return fmt.Errorf("[%s] invalid array index '%s': %v", path, idxStr, err) } + if idx < 0 || (method != http.MethodPut && idx >= len(arr)) || idx > len(arr) { return fmt.Errorf("[%s] array index out of bounds: %s", path, idxStr) } @@ -1307,7 +1322,7 @@ traverseLoop: } case []any: - partInt, err := strconv.Atoi(part) + partInt, err := parseCanonicalArrayIndex(part) if err != nil { return fmt.Errorf("[/%s] invalid array index '%s': %v", strings.Join(parts[:i+1], "/"), part, err) diff --git a/admin_test.go b/admin_test.go index 97dc76f4db4..c51f6af189e 100644 --- a/admin_test.go +++ b/admin_test.go @@ -15,6 +15,7 @@ package caddy import ( + "bytes" "context" "crypto/x509" "encoding/json" @@ -956,3 +957,47 @@ MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQDRS0LmTwUT0iwP }) } } + +func TestUnsyncedConfigAccessCanonicalArrayIndices(t *testing.T) { + rawCfg = map[string]any{ + rawConfigKey: map[string]any{ + "list": []any{"zero", "one", "two", "three", "four", "five", "six", "seven", "eight", "nine", "ten"}, + }, + } + + tests := []struct { + name string + path string + wantOutput string + wantErr bool + }{ + {name: "allow zero", path: "/" + rawConfigKey + "/list/0", wantOutput: "\"zero\"\n"}, + {name: "allow one", path: "/" + rawConfigKey + "/list/1", wantOutput: "\"one\"\n"}, + {name: "allow ten", path: "/" + rawConfigKey + "/list/10", wantOutput: "\"ten\"\n"}, + {name: "reject leading zero", path: "/" + rawConfigKey + "/list/01", wantErr: true}, + {name: "reject multiple leading zeros", path: "/" + rawConfigKey + "/list/002", wantErr: true}, + {name: "reject plus sign", path: "/" + rawConfigKey + "/list/+1", wantErr: true}, + {name: "reject negative zero", path: "/" + rawConfigKey + "/list/-0", wantErr: true}, + } + + for i, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + var gotOutput bytes.Buffer + err := unsyncedConfigAccess(http.MethodGet, tc.path, nil, &gotOutput) + + if tc.wantErr { + if err == nil { + t.Errorf("test %d (%s): input path %q: expected error, got nil with output %q", i, tc.name, tc.path, gotOutput.String()) + } + return + } + + if err != nil { + t.Errorf("test %d (%s): input path %q: expected no error with output %q, got error %v with output %q", i, tc.name, tc.path, tc.wantOutput, err, gotOutput.String()) + } + if gotOutput.String() != tc.wantOutput { + t.Errorf("test %d (%s): input path %q: expected output %q, got %q", i, tc.name, tc.path, tc.wantOutput, gotOutput.String()) + } + }) + } +}