Harden Accept header parsing

lib/protocol/http/header/accept.rb

@@ -15,12 +15,12 @@ module Header
 			class Accept < Split
 				# Regular expression used to split values on commas, with optional surrounding whitespace, taking into account quoted strings.
 				SEPARATOR = /
-					(?:            # Start non-capturing group
-						"[^"\\]*"    # Match quoted strings (no escaping of quotes within)
-						|            # OR
-						[^,"]+       # Match non-quoted strings until a comma or quote
+					(?:                       # Start non-capturing group
+						"(?:[^"\\]|\\.)*"       # Match quoted strings, including quoted pairs
+						|                       # OR
+						[^,"]+                  # Match non-quoted strings until a comma or quote
 					)+
-					(?=,|\z)       # Match until a comma or end of string
+					(?=,|\z)                  # Match until a comma or end of string
 				/x
 
 				ParseError = Class.new(Error)
@@ -114,15 +114,39 @@ def parse_media_range(value)
 						type = match[:type]
 						subtype = match[:subtype]
 						parameters = {}
+						parameters_string = match[:parameters]
+						offset = 0
 
-						match[:parameters].scan(PARAMETER) do |key, value, quoted_value|
+						parameters_string.scan(PARAMETER) do |key, value, quoted_value|
+							parameter_match = Regexp.last_match
+							unless parameter_match.begin(0) == offset
+								raise ParseError, "Invalid media range parameters: #{parameters_string[offset..].inspect}"
+							end
+							offset = parameter_match.end(0)
+
+							if key.casecmp?("q")
+								if quoted_value || parameters.key?("q") || !value.match?(/\A(?:#{QVALUE})\z/)
+									raise ParseError, "Invalid quality factor: #{value.inspect}"
+								end
+
+								key = "q"
+							end
+
 							if quoted_value
 								value = QuotedString.unquote(quoted_value)
 							end
 
 							parameters[key] = value
 						end
 
+						unless offset == parameters_string.length
+							raise ParseError, "Invalid media range parameters: #{parameters_string[offset..].inspect}"
+						end
+
+						if (type.include?("*") && type != "*") || (subtype.include?("*") && subtype != "*") || (type == "*" && subtype != "*")
+							raise ParseError, "Invalid wildcards in media range: #{type}/#{subtype}"
+						end
+
 						return MediaRange.new(type, subtype, parameters)
 					else
 						raise ParseError, "Invalid media type: #{value.inspect}"

releases.md

@@ -1,5 +1,9 @@
 # Releases
 
+## Unreleased
+
+  - Improve `Accept` header parsing for quoted pairs, malformed parameters, invalid wildcards, and invalid quality factors.
+
 ## v0.64.0
 
   - Add `Protocol::HTTP::Request#rewind!` and `#retry!` for preparing requests to be sent again.

test/protocol/http/header/accept.rb

@@ -84,6 +84,72 @@
 		end
 	end
 
+	with 'foo/bar;key="A,B,C", text/plain;note="a,b; c\\"d"' do
+		it "preserves commas and escapes in quoted parameters" do
+			expect(header.length).to be == 2
+			expect(media_ranges[0].parameters).to have_keys(
+				"key" => be == "A,B,C",
+			)
+			expect(media_ranges[1].parameters).to have_keys(
+				"note" => be == 'a,b; c"d',
+			)
+		end
+	end
+
+	with "invalid media ranges" do
+		it "rejects malformed parameters" do
+			[
+				"text/html;",
+				"text/html;invalid",
+				"text/html;invalid;q=0.5",
+				"text/html;charset=",
+				'text/html;charset="unterminated',
+			].each do |value|
+				expect{subject.parse(value).media_ranges}.to raise_exception(Protocol::HTTP::Header::Accept::ParseError)
+			end
+		end
+
+		it "rejects invalid wildcard forms" do
+			[
+				"*/json",
+				"text/*+json",
+				"te*t/plain",
+			].each do |value|
+				expect{subject.parse(value).media_ranges}.to raise_exception(Protocol::HTTP::Header::Accept::ParseError)
+			end
+		end
+
+		it "rejects invalid quality factors" do
+			[
+				"text/html;q=1.1",
+				"text/html;q=0.1234",
+				"text/html;q=invalid",
+				'text/html;q="0.5"',
+				"text/html;q=0.5;q=0.4",
+			].each do |value|
+				expect{subject.parse(value).media_ranges}.to raise_exception(Protocol::HTTP::Header::Accept::ParseError)
+			end
+		end
+	end
+
+	with ".parse" do
+		it "accepts an empty string" do
+			expect(subject.parse("").media_ranges).to be == []
+		end
+
+		it "ignores empty list members" do
+			expect(subject.parse(", text/html,").media_ranges).to have_attributes(
+				length: be == 1,
+			)
+		end
+	end
+
+	with "text/html;q=0, text/plain;q=0.123, application/json;q=1.000" do
+		it "accepts standard quality factors" do
+			expect(media_ranges.collect(&:quality_factor)).to be == [1.0, 0.123, 0.0]
+		end
+	end
+
 	with ".coerce" do
 		it "coerces array to Accept" do
 			result = subject.coerce(["text/html", "application/json"])