admin: expect quoted ETags (#4879)

* expect quoted etags

* admin: Minor refactor of etag facilities

Co-authored-by: Matthew Holt <mholt@users.noreply.github.com>

admin.go

@@ -21,7 +21,6 @@ import (
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/base64"
-	"encoding/hex"
 	"encoding/json"
 	"errors"
 	"expvar"
@@ -901,6 +900,12 @@ func (h adminHandler) originAllowed(origin *url.URL) bool {
 // produce and verify ETags.
 func etagHasher() hash.Hash32 { return fnv.New32a() }
 
+// makeEtag returns an Etag header value (including quotes) for
+// the given config path and hash of contents at that path.
+func makeEtag(path string, hash hash.Hash) string {
+	return fmt.Sprintf(`"%s %x"`, path, hash.Sum(nil))
+}
+
 func handleConfig(w http.ResponseWriter, r *http.Request) error {
 	switch r.Method {
 	case http.MethodGet:
@@ -919,7 +924,7 @@ func handleConfig(w http.ResponseWriter, r *http.Request) error {
 
 		// we could consider setting up a sync.Pool for the summed
 		// hashes to reduce GC pressure.
-		w.Header().Set("ETag", r.URL.Path+" "+hex.EncodeToString(hash.Sum(nil)))
+		w.Header().Set("Etag", makeEtag(r.URL.Path, hash))
 
 		return nil
 

admin_test.go

@@ -15,8 +15,8 @@
 package caddy
 
 import (
-	"encoding/hex"
 	"encoding/json"
+	"fmt"
 	"net/http"
 	"reflect"
 	"sync"
@@ -168,7 +168,7 @@ func TestETags(t *testing.T) {
 	const key = "/" + rawConfigKey + "/apps/foo"
 
 	// try update the config with the wrong etag
-	err := changeConfig(http.MethodPost, key, []byte(`{"strField": "abc", "intField": 1}}`), "/"+rawConfigKey+" not_an_etag", false)
+	err := changeConfig(http.MethodPost, key, []byte(`{"strField": "abc", "intField": 1}}`), fmt.Sprintf(`"/%s not_an_etag"`, rawConfigKey), false)
 	if apiErr, ok := err.(APIError); !ok || apiErr.HTTPStatus != http.StatusPreconditionFailed {
 		t.Fatalf("expected precondition failed; got %v", err)
 	}
@@ -180,13 +180,13 @@ func TestETags(t *testing.T) {
 	}
 
 	// do the same update with the correct key
-	err = changeConfig(http.MethodPost, key, []byte(`{"strField": "abc", "intField": 1}`), key+" "+hex.EncodeToString(hash.Sum(nil)), false)
+	err = changeConfig(http.MethodPost, key, []byte(`{"strField": "abc", "intField": 1}`), makeEtag(key, hash), false)
 	if err != nil {
 		t.Fatalf("expected update to work; got %v", err)
 	}
 
 	// now try another update. The hash should no longer match and we should get precondition failed
-	err = changeConfig(http.MethodPost, key, []byte(`{"strField": "abc", "intField": 2}`), key+" "+hex.EncodeToString(hash.Sum(nil)), false)
+	err = changeConfig(http.MethodPost, key, []byte(`{"strField": "abc", "intField": 2}`), makeEtag(key, hash), false)
 	if apiErr, ok := err.(APIError); !ok || apiErr.HTTPStatus != http.StatusPreconditionFailed {
 		t.Fatalf("expected precondition failed; got %v", err)
 	}

caddy.go

@@ -145,8 +145,16 @@ func changeConfig(method, path string, input []byte, ifMatchHeader string, force
 	defer currentCfgMu.Unlock()
 
 	if ifMatchHeader != "" {
+		// expect the first and last character to be quotes
+		if len(ifMatchHeader) < 2 || ifMatchHeader[0] != '"' || ifMatchHeader[len(ifMatchHeader)-1] != '"' {
+			return APIError{
+				HTTPStatus: http.StatusBadRequest,
+				Err:        fmt.Errorf("malformed If-Match header; expect quoted string"),
+			}
+		}
+
 		// read out the parts
-		parts := strings.Fields(ifMatchHeader)
+		parts := strings.Fields(ifMatchHeader[1 : len(ifMatchHeader)-1])
 		if len(parts) != 2 {
 			return APIError{
 				HTTPStatus: http.StatusBadRequest,