error for invalid media type should not be completely swallowed (#10186)

* error for invalid media type should not be completely swallowed

Signed-off-by: Matheus Pimenta <matheuscscp@gmail.com>

model/textparse/interface.go

@@ -61,12 +61,19 @@ type Parser interface {
 }
 
 // New returns a new parser of the byte slice.
-func New(b []byte, contentType string) Parser {
+//
+// This function always returns a valid parser, but might additionally
+// return an error if the content type cannot be parsed.
+func New(b []byte, contentType string) (p Parser, warning error) {
+	if contentType == "" {
+		return NewPromParser(b), nil
+	}
+
 	mediaType, _, err := mime.ParseMediaType(contentType)
 	if err == nil && mediaType == "application/openmetrics-text" {
-		return NewOpenMetricsParser(b)
+		return NewOpenMetricsParser(b), nil
 	}
-	return NewPromParser(b)
+	return NewPromParser(b), err
 }
 
 // Entry represents the type of a parsed entry.

model/textparse/interface_test.go

@@ -0,0 +1,112 @@
+// Copyright 2022 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package textparse
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestNewParser(t *testing.T) {
+	t.Parallel()
+
+	requirePromParser := func(t *testing.T, p Parser) {
+		require.NotNil(t, p)
+		_, ok := p.(*PromParser)
+		require.True(t, ok)
+	}
+
+	requireOpenMetricsParser := func(t *testing.T, p Parser) {
+		require.NotNil(t, p)
+		_, ok := p.(*OpenMetricsParser)
+		require.True(t, ok)
+	}
+
+	for name, tt := range map[string]*struct {
+		contentType    string
+		validateParser func(*testing.T, Parser)
+		warning        string
+	}{
+		"empty-string": {
+			contentType:    "",
+			validateParser: requirePromParser,
+			warning:        "",
+		},
+		"invalid-content-type-1": {
+			contentType:    "invalid/",
+			validateParser: requirePromParser,
+			warning:        "expected token after slash",
+		},
+		"invalid-content-type-2": {
+			contentType:    "invalid/invalid/invalid",
+			validateParser: requirePromParser,
+			warning:        "unexpected content after media subtype",
+		},
+		"invalid-content-type-3": {
+			contentType:    "/",
+			validateParser: requirePromParser,
+			warning:        "no media type",
+		},
+		"invalid-content-type-4": {
+			contentType:    "application/openmetrics-text; charset=UTF-8; charset=utf-8",
+			validateParser: requirePromParser,
+			warning:        "duplicate parameter name",
+		},
+		"openmetrics": {
+			contentType:    "application/openmetrics-text",
+			validateParser: requireOpenMetricsParser,
+			warning:        "",
+		},
+		"openmetrics-with-charset": {
+			contentType:    "application/openmetrics-text; charset=utf-8",
+			validateParser: requireOpenMetricsParser,
+			warning:        "",
+		},
+		"openmetrics-with-charset-and-version": {
+			contentType:    "application/openmetrics-text; version=1.0.0; charset=utf-8",
+			validateParser: requireOpenMetricsParser,
+			warning:        "",
+		},
+		"plain-text": {
+			contentType:    "text/plain",
+			validateParser: requirePromParser,
+			warning:        "",
+		},
+		"plain-text-with-version": {
+			contentType:    "text/plain; version=0.0.4",
+			validateParser: requirePromParser,
+			warning:        "",
+		},
+		"some-other-valid-content-type": {
+			contentType:    "text/html",
+			validateParser: requirePromParser,
+			warning:        "",
+		},
+	} {
+		t.Run(name, func(t *testing.T) {
+			tt := tt // Copy to local variable before going parallel.
+			t.Parallel()
+
+			p, err := New([]byte{}, tt.contentType)
+			tt.validateParser(t, p)
+			if tt.warning == "" {
+				require.NoError(t, err)
+			} else {
+				require.Error(t, err)
+				require.Contains(t, err.Error(), tt.warning)
+			}
+		})
+	}
+}

promql/fuzz.go

@@ -57,7 +57,13 @@ const (
 )
 
 func fuzzParseMetricWithContentType(in []byte, contentType string) int {
-	p := textparse.New(in, contentType)
+	p, warning := textparse.New(in, contentType)
+	if warning != nil {
+		// An invalid content type is being passed, which should not happen
+		// in this context.
+		panic(warning)
+	}
+
 	var err error
 	for {
 		_, err = p.Next()

promql/fuzz_test.go

@@ -0,0 +1,39 @@
+// Copyright 2022 The Prometheus Authors
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+// Only build when go-fuzz is in use
+//go:build gofuzz
+// +build gofuzz
+
+package promql
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+func TestfuzzParseMetricWithContentTypePanicOnInvalid(t *testing.T) {
+	defer func() {
+		if p := recover(); p == nil {
+			t.Error("invalid content type should panic")
+		} else {
+			err, ok := p.(error)
+			require.True(t, ok)
+			require.Contains(t, err.Error(), "duplicate parameter name")
+		}
+	}()
+
+	const invalidContentType = "application/openmetrics-text; charset=UTF-8; charset=utf-8"
+	fuzzParseMetricWithContentType([]byte{}, invalidContentType)
+}

scrape/scrape.go

@@ -1418,8 +1418,16 @@ type appendErrors struct {
 }
 
 func (sl *scrapeLoop) append(app storage.Appender, b []byte, contentType string, ts time.Time) (total, added, seriesAdded int, err error) {
+	p, warning := textparse.New(b, contentType)
+	if warning != nil {
+		level.Debug(sl.l).Log(
+			"msg", "Invalid content type on scrape, using prometheus parser as fallback",
+			"content-type", contentType,
+			"err", warning,
+		)
+	}
+
 	var (
-		p              = textparse.New(b, contentType)
 		defTime        = timestamp.FromTime(ts)
 		appErrs        = appendErrors{}
 		sampleLimitErr error

scrape/scrape_test.go

@@ -1479,7 +1479,8 @@ func TestScrapeLoopAppendCacheEntryButErrNotFound(t *testing.T) {
 	fakeRef := storage.SeriesRef(1)
 	expValue := float64(1)
 	metric := `metric{n="1"} 1`
-	p := textparse.New([]byte(metric), "")
+	p, warning := textparse.New([]byte(metric), "")
+	require.NoError(t, warning)
 
 	var lset labels.Labels
 	p.Next()