accept: Improve tokenization (#296)

- Support parameter values with commas (inside quotes)
- Add a few relevant tests for new support

Uses a tokenization algorithm similar to that of spring-projects, which is also
under the Apache 2.0 license:
  https://github.com/spring-projects/spring-framework/

Author: mattwomple

packager/accept/accept.go

@@ -1,7 +1,9 @@
 package accept
 
 import (
+	"log"
 	"mime"
+	"strings"
 
 	"github.com/WICG/webpackage/go/signedexchange/version"
 	"github.com/ampproject/amppackager/packager/util"
@@ -18,23 +20,61 @@ const SxgContentType = "application/signed-exchange;v=" + AcceptedSxgVersion
 // signedexchange library.
 var SxgVersion = version.Version1b3
 
+// Tokenize a comma-separated string of accept patterns into a slice
+func tokenize(accept string) []string {
+	var tokens []string
+	acceptLen := len(accept)
+	if acceptLen == 0 {
+		return tokens
+	}
+
+	inQuotes := false
+	startIndex := 0
+	for i := 0; i < acceptLen; i++ {
+		char := accept[i]
+		switch char {
+		case '"':
+			inQuotes = !inQuotes
+		case ',':
+			if !inQuotes {
+				tokens = append(tokens, util.TrimHeaderValue(accept[startIndex:i]))
+				startIndex = i + 1
+			}
+		case '\\':
+			if !inQuotes {
+				log.Printf("unable to parse Accept header: %s", accept)
+				return []string{}
+			}
+			i++
+		}
+	}
+	tokens = append(tokens, util.TrimHeaderValue(accept[startIndex:]))
+	return tokens
+}
+
+// Determine whether a version specified by the accept header matches the
+// version of signed exchange output by the packager
+func hasMatchingSxgVersion(versions []string) bool {
+	for _, version := range versions {
+		if version == AcceptedSxgVersion {
+			return true
+		}
+	}
+	return false
+}
+
 // True if the given Accept header is one that the packager can satisfy. It
 // must contain application/signed-exchange;v=$V so that the packager knows
 // whether or not it can supply the correct version. "" and "*/*" are not
 // satisfiable, for this reason.
 func CanSatisfy(accept string) bool {
-	// There is an edge case on which this comma-splitting fails:
-	//   Accept: application/signed-exchange;junk="some,thing";v=b2
-	// However, in practice, browsers don't send media types with quoted
-	// commas in them:
-	//   https://developer.mozilla.org/en-US/docs/Web/HTTP/Content_negotiation/List_of_default_Accept_values
-	// So we'll live with this deficiency for the sake of not forking
-	// mime.ParseMediaType.
-	types := util.Comma.Split(accept, -1)
+	types := tokenize(accept)
 	for _, mediaRange := range types {
 		mediatype, params, err := mime.ParseMediaType(mediaRange)
-		if err == nil && mediatype == "application/signed-exchange" && params["v"] == AcceptedSxgVersion {
-			return true
+		if err == nil && mediatype == "application/signed-exchange" {
+			if hasMatchingSxgVersion(strings.Split(params["v"], ",")) {
+				return true
+			}
 		}
 	}
 	return false

packager/accept/accept_test.go

@@ -11,16 +11,17 @@ func TestCanSatisfy(t *testing.T) {
 	assert.False(t, CanSatisfy("*/*"))
 	assert.False(t, CanSatisfy("image/jpeg;v=b3"))
 	assert.False(t, CanSatisfy(`application/signed-exchange;v=b2`))
-	// This is a bug that will be triggered when a UA starts supporting multiple SXG versions:
-	assert.False(t, CanSatisfy(`application/signed-exchange;x="a,b";v="b3"`))
+	assert.False(t, CanSatisfy(`application/signed-exchange;v="b1,b2"`))
+	assert.False(t, CanSatisfy(`application/signed-exchange;x="y,application/signed-exchange;v=b3,z";v=b1`))
 
 	assert.True(t, CanSatisfy(`application/signed-exchange;v=b3`))
 	assert.True(t, CanSatisfy(`application/signed-exchange;v="b3"`))
+	assert.True(t, CanSatisfy(`application/signed-exchange;v="b2,b3,b4"`))
 	assert.True(t, CanSatisfy(`application/signed-exchange;v=b3;q=0.8`))
+	assert.True(t, CanSatisfy(`application/signed-exchange;v=b2;q=0.9,application/signed-exchange;v="b3,b4";q=0.8`))
 	assert.True(t, CanSatisfy(`application/signed-exchange;v=b1,application/signed-exchange;v=b3`))
 	assert.True(t, CanSatisfy(`application/signed-exchange;x="v=b1";v="b3"`))
 	assert.True(t, CanSatisfy("*/*, application/signed-exchange;v=b3"))
 	assert.True(t, CanSatisfy("*/* \t,\t application/signed-exchange;v=b3"))
-	// This is the same bug, though one which won't occur in practice:
-	assert.True(t, CanSatisfy(`application/signed-exchange;x="y,application/signed-exchange;v=b3,z";v=b1`))
+	assert.True(t, CanSatisfy(`application/signed-exchange;x="a,b";v="b3"`))
 }

packager/util/http.go

@@ -4,13 +4,23 @@ import (
 	"fmt"
 	"regexp"
 	"net/http"
+	"strings"
 )
 
 // A comma, as defined in https://tools.ietf.org/html/rfc7230#section-7, with
 // OWS defined in https://tools.ietf.org/html/rfc7230#appendix-B. This is
 // commonly used as a separator in header field value definitions.
 var Comma *regexp.Regexp = regexp.MustCompile(`[ \t]*,[ \t]*`)
 
+// Trim optional whitespace from a header value, adhering to
+// https://tools.ietf.org/html/rfc7230#section-7 with OWS defined in
+// https://tools.ietf.org/html/rfc7230#appendix-B.
+func TrimHeaderValue(s string) string {
+	return strings.TrimFunc(s, func(r rune) bool {
+		return r == ' ' || r == '\t'
+	})
+}
+
 // Conditional request headers that ServeHTTP may receive and need to be sent with fetchURL.
 // https://developer.mozilla.org/en-US/docs/Web/HTTP/Conditional_requests#Conditional_headers
 var ConditionalRequestHeaders = map[string]bool{