fix(gw): duplicate blocks and range etags (#486)

Co-authored-by: Henrique Dias <hacdias@gmail.com>

gateway/gateway.go

@@ -174,7 +174,7 @@ const (
 )
 
 // DuplicateBlocksPolicy represents the content type parameter 'dups' (IPIP-412)
-type DuplicateBlocksPolicy int
+type DuplicateBlocksPolicy uint8
 
 const (
 	DuplicateBlocksUnspecified DuplicateBlocksPolicy = iota // 0 - implicit default
@@ -183,14 +183,16 @@ const (
 )
 
 // NewDuplicateBlocksPolicy returns DuplicateBlocksPolicy based on the content type parameter 'dups' (IPIP-412)
-func NewDuplicateBlocksPolicy(dupsValue string) DuplicateBlocksPolicy {
+func NewDuplicateBlocksPolicy(dupsValue string) (DuplicateBlocksPolicy, error) {
 	switch dupsValue {
 	case "y":
-		return DuplicateBlocksIncluded
+		return DuplicateBlocksIncluded, nil
 	case "n":
-		return DuplicateBlocksExcluded
+		return DuplicateBlocksExcluded, nil
+	case "":
+		return DuplicateBlocksUnspecified, nil
 	}
-	return DuplicateBlocksUnspecified
+	return 0, fmt.Errorf("unsupported application/vnd.ipld.car content type dups parameter: %q", dupsValue)
 }
 
 func (d DuplicateBlocksPolicy) Bool() bool {

gateway/handler_car.go

@@ -2,6 +2,7 @@ package gateway
 
 import (
 	"context"
+	"encoding/binary"
 	"fmt"
 	"io"
 	"net/http"
@@ -166,20 +167,18 @@ func buildCarParams(r *http.Request, contentTypeParams map[string]string) (CarPa
 	}
 
 	// optional dups from IPIP-412
-	if dups := NewDuplicateBlocksPolicy(contentTypeParams["dups"]); dups != DuplicateBlocksUnspecified {
-		switch dups {
-		case DuplicateBlocksExcluded, DuplicateBlocksIncluded:
-			params.Duplicates = dups
-		default:
-			return CarParams{}, fmt.Errorf("unsupported application/vnd.ipld.car content type dups parameter:  %q", dups)
-		}
-	} else {
+	dups, err := NewDuplicateBlocksPolicy(contentTypeParams["dups"])
+	if err != nil {
+		return CarParams{}, err
+	}
+	if dups == DuplicateBlocksUnspecified {
 		// when duplicate block preference is not specified, we set it to
 		// false, as this has always been the default behavior, we should
 		// not break legacy clients, and responses to requests made via ?format=car
 		// should benefit from block deduplication
-		params.Duplicates = DuplicateBlocksExcluded
+		dups = DuplicateBlocksExcluded
 	}
+	params.Duplicates = dups
 
 	return params, nil
 }
@@ -226,31 +225,43 @@ func getCarRootCidAndLastSegment(imPath path.ImmutablePath) (cid.Cid, string, er
 }
 
 func getCarEtag(imPath path.ImmutablePath, params CarParams, rootCid cid.Cid) string {
-	data := imPath.String()
+	h := xxhash.New()
+	h.WriteString(imPath.String())
+	// be careful with hashes here, we need boundaries and per entry salt, we don't want a request that has:
+	//   - scope = dfs
+	// and:
+	//   - order = dfs
+	// to result in the same hash because if we just do hash(scope + order) they would both yield hash("dfs").
 	if params.Scope != DagScopeAll {
-		data += string(params.Scope)
+		h.WriteString("\x00scope=")
+		h.WriteString(string(params.Scope))
 	}
 
 	// 'order' from IPIP-412 impact Etag only if set to something else
 	// than DFS (which is the implicit default)
 	if params.Order != DagOrderDFS {
-		data += string(params.Order)
+		h.WriteString("\x00order=")
+		h.WriteString(string(params.Order))
 	}
 
 	// 'dups' from IPIP-412 impact Etag only if 'y'
-	if dups := params.Duplicates.String(); dups == "y" {
-		data += dups
+	if dups := params.Duplicates; dups == DuplicateBlocksIncluded {
+		h.WriteString("\x00dups=y")
 	}
 
 	if params.Range != nil {
 		if params.Range.From != 0 || params.Range.To != nil {
-			data += strconv.FormatInt(params.Range.From, 10)
+			h.WriteString("\x00range=")
+			var b [8]byte
+			binary.LittleEndian.PutUint64(b[:], uint64(params.Range.From))
+			h.Write(b[:])
 			if params.Range.To != nil {
-				data += strconv.FormatInt(*params.Range.To, 10)
+				binary.LittleEndian.PutUint64(b[:], uint64(*params.Range.To))
+				h.Write(b[:])
 			}
 		}
 	}
 
-	suffix := strconv.FormatUint(xxhash.Sum64([]byte(data)), 32)
+	suffix := strconv.FormatUint(h.Sum64(), 32)
 	return `W/"` + rootCid.String() + ".car." + suffix + `"`
 }

gateway/handler_car_test.go

@@ -110,6 +110,30 @@ func TestCarParams(t *testing.T) {
 			require.Equal(t, test.expectedDuplicates.String(), params.Duplicates.String())
 		}
 	})
+
+	t.Run("buildCarParams from Accept header: order and dups parsing (invalid)", func(t *testing.T) {
+		t.Parallel()
+
+		// below ensure the implicit default (DFS and no duplicates) is correctly inferred
+		// from the value read from Accept header
+		tests := []string{
+			"application/vnd.ipld.car; dups=invalid",
+			"application/vnd.ipld.car; order=invalid",
+			"application/vnd.ipld.car; order=dfs; dups=invalid",
+			"application/vnd.ipld.car; order=invalid; dups=y",
+		}
+		for _, test := range tests {
+			r := mustNewRequest(t, http.MethodGet, "http://example.com/", nil)
+			r.Header.Set("Accept", test)
+
+			mediaType, formatParams, err := customResponseFormat(r)
+			assert.NoError(t, err)
+			assert.Equal(t, carResponseFormat, mediaType)
+
+			_, err = buildCarParams(r, formatParams)
+			assert.ErrorContains(t, err, "unsupported application/vnd.ipld.car content type")
+		}
+	})
 }
 
 func TestContentTypeFromCarParams(t *testing.T) {