Correctly handle quoted media type parameter values

- #5349
- fix or add comments about other parsing errors and inconsistencies
 - `MediaType` did not skip whitespace before the type

nits:
- use `+=`
- `<code>` -> `<c>` since the former is not for use within a paragraph
- split tests up to remove `bool expectedResult` parameters

src/Microsoft.AspNetCore.Mvc.Core/Formatters/MediaType.cs

@@ -5,7 +5,6 @@ using System;
 using System.Globalization;
 using System.Text;
 using Microsoft.AspNetCore.Mvc.Formatters.Internal;
-using Microsoft.Extensions.Internal;
 using Microsoft.Extensions.Primitives;
 
 namespace Microsoft.AspNetCore.Mvc.Formatters
@@ -103,19 +102,20 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
                 return 0;
             }
 
-            // Parse the type, i.e. <type> in media type string "<type>/<subtype>; param1=value1; param2=value2"
+            var current = offset + HttpTokenParsingRules.GetWhitespaceLength(input, offset);
-            var typeLength = HttpTokenParsingRules.GetTokenLength(input, offset);
 
+            // Parse the type, i.e. <type> in media type string "<type>/<subtype>; param1=value1; param2=value2"
+            var typeLength = HttpTokenParsingRules.GetTokenLength(input, current);
             if (typeLength == 0)
             {
                 type = default(StringSegment);
                 return 0;
             }
 
-            type = new StringSegment(input, offset, typeLength);
+            type = new StringSegment(input, current, typeLength);
 
-            var current = offset + typeLength;
+            current += typeLength;
-            current = current + HttpTokenParsingRules.GetWhitespaceLength(input, current);
+            current += HttpTokenParsingRules.GetWhitespaceLength(input, current);
 
             return current - offset;
         }
@@ -123,6 +123,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
         private static int GetSubtypeLength(string input, int offset, out StringSegment subType)
         {
             var current = offset;
+
             // Parse the separator between type and subtype
             if (current < 0 || current >= input.Length || input[current] != '/')
             {
@@ -131,10 +132,9 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             }
 
             current++; // skip delimiter.
-            current = current + HttpTokenParsingRules.GetWhitespaceLength(input, current);
+            current +=  HttpTokenParsingRules.GetWhitespaceLength(input, current);
 
             var subtypeLength = HttpTokenParsingRules.GetTokenLength(input, current);
-
             if (subtypeLength == 0)
             {
                 subType = default(StringSegment);
@@ -143,8 +143,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
 
             subType = new StringSegment(input, current, subtypeLength);
 
-            current = current + subtypeLength;
+            current +=  subtypeLength;
-            current = current + HttpTokenParsingRules.GetWhitespaceLength(input, current);
+            current +=  HttpTokenParsingRules.GetWhitespaceLength(input, current);
 
             return current - offset;
         }
@@ -180,11 +180,12 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
         public StringSegment Charset => GetParameter("charset");
 
         /// <summary>
-        /// Determines whether the current <see cref="MediaType"/> is a subset of the <paramref name="set"/> <see cref="MediaType"/>.
+        /// Determines whether the current <see cref="MediaType"/> is a subset of the <paramref name="set"/>
+        /// <see cref="MediaType"/>.
         /// </summary>
         /// <param name="set">The set <see cref="MediaType"/>.</param>
         /// <returns>
-        /// <code>true</code> if this <see cref="MediaType"/> is a subset of <paramref name="set"/>; otherwise<code>false</code>.
+        /// <c>true</c> if this <see cref="MediaType"/> is a subset of <paramref name="set"/>; otherwise <c>false</c>.
         /// </returns>
         public bool IsSubsetOf(MediaType set)
         {
@@ -197,7 +198,10 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
         /// Gets the parameter <paramref name="parameterName"/> of the media type.
         /// </summary>
         /// <param name="parameterName">The name of the parameter to retrieve.</param>
-        /// <returns>The <see cref="StringSegment"/>for the given <paramref name="parameterName"/> if found; otherwise<code>null</code>.</returns>
+        /// <returns>
+        /// The <see cref="StringSegment"/>for the given <paramref name="parameterName"/> if found; otherwise
+        /// <c>null</c>.
+        /// </returns>
         public StringSegment GetParameter(string parameterName)
         {
             return GetParameter(new StringSegment(parameterName));
@@ -207,7 +211,10 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
         /// Gets the parameter <paramref name="parameterName"/> of the media type.
         /// </summary>
         /// <param name="parameterName">The name of the parameter to retrieve.</param>
-        /// <returns>The <see cref="StringSegment"/>for the given <paramref name="parameterName"/> if found; otherwise<code>null</code>.</returns>
+        /// <returns>
+        /// The <see cref="StringSegment"/>for the given <paramref name="parameterName"/> if found; otherwise
+        /// <c>null</c>.
+        /// </returns>
         public StringSegment GetParameter(StringSegment parameterName)
         {
             var parametersParser = _parameterParser;
@@ -229,7 +236,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
         /// <paramref name="encoding"/>.
         /// </summary>
         /// <param name="mediaType">The media type whose encoding will be replaced.</param>
-        /// <param name="encoding">The encoding that will replace the encoding in the <paramref name="mediaType"/></param>
+        /// <param name="encoding">The encoding that will replace the encoding in the <paramref name="mediaType"/>.
+        /// </param>
         /// <returns>A media type with the replaced encoding.</returns>
         public static string ReplaceEncoding(string mediaType, Encoding encoding)
         {
@@ -241,15 +249,15 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
         /// <paramref name="encoding"/>.
         /// </summary>
         /// <param name="mediaType">The media type whose encoding will be replaced.</param>
-        /// <param name="encoding">The encoding that will replace the encoding in the <paramref name="mediaType"/></param>
+        /// <param name="encoding">The encoding that will replace the encoding in the <paramref name="mediaType"/>.
+        /// </param>
         /// <returns>A media type with the replaced encoding.</returns>
         public static string ReplaceEncoding(StringSegment mediaType, Encoding encoding)
         {
             var parsedMediaType = new MediaType(mediaType);
             var charset = parsedMediaType.GetParameter("charset");
 
-            if (charset.HasValue &&
+            if (charset.HasValue && charset.Equals(encoding.WebName, StringComparison.OrdinalIgnoreCase))
-                charset.Equals(encoding.WebName, StringComparison.OrdinalIgnoreCase))
             {
                 return mediaType.Value;
             }
@@ -292,6 +300,9 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
         public static MediaTypeSegmentWithQuality CreateMediaTypeSegmentWithQuality(string mediaType, int start)
         {
             var parsedMediaType = new MediaType(mediaType, start, length: null);
+
+            // Short-circuit use of the MediaTypeParameterParser if constructor detected an invalid type or subtype.
+            // Parser would set ParsingFailed==true in this case. But, we handle invalid parameters as a separate case.
             if (parsedMediaType.Type.Equals(default(StringSegment)) ||
                 parsedMediaType.SubType.Equals(default(StringSegment)))
             {
@@ -300,12 +311,13 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
 
             var parser = parsedMediaType._parameterParser;
 
-            double quality = 1.0d;
+            var quality = 1.0d;
             MediaTypeParameter parameter;
             while (parser.ParseNextParameter(out parameter))
             {
                 if (parameter.HasName(QualityParameter))
                 {
+                    // If media type contains two `q` values i.e. it's invalid in an uncommon way, pick last value.
                     quality = double.Parse(
                         parameter.Value.Value, NumberStyles.AllowDecimalPoint,
                         NumberFormatInfo.InvariantInfo);
@@ -313,17 +325,15 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             }
 
             // We check if the parsed media type has a value at this stage when we have iterated
-            // over all the parameters and we know if the parsing was sucessful.
+            // over all the parameters and we know if the parsing was successful.
-            if (!parser.ParsingFailed)
+            if (parser.ParsingFailed)
-            {
-                return new MediaTypeSegmentWithQuality(
-                    new StringSegment(mediaType, start, parser.CurrentOffset - start),
-                    quality);
-            }
-            else
             {
                 return default(MediaTypeSegmentWithQuality);
             }
+
+            return new MediaTypeSegmentWithQuality(
+                new StringSegment(mediaType, start, parser.CurrentOffset - start),
+                quality);
         }
 
         private static Encoding GetEncodingFromCharset(StringSegment charset)
@@ -407,7 +417,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
                 }
 
                 var parameterLength = GetParameterLength(_mediaTypeBuffer, CurrentOffset, out result);
-                CurrentOffset = CurrentOffset + parameterLength;
+                CurrentOffset +=  parameterLength;
 
                 if (parameterLength == 0)
                 {
@@ -420,8 +430,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
 
             private static int GetParameterLength(string input, int startIndex, out MediaTypeParameter parsedValue)
             {
-                if (OffsetIsOutOfRange(startIndex, input.Length) ||
+                if (OffsetIsOutOfRange(startIndex, input.Length) || input[startIndex] != ';')
-                    input[startIndex] != ';')
                 {
                     parsedValue = default(MediaTypeParameter);
                     return 0;
@@ -442,8 +451,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
                 var valueLength = GetValueLength(input, current, out value);
 
                 parsedValue = new MediaTypeParameter(name, value);
+                current +=  valueLength;
 
-                current = current + valueLength;
                 return current - startIndex;
             }
 
@@ -452,10 +461,9 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
                 var current = startIndex;
 
                 current++; // skip ';'
-                current = current + HttpTokenParsingRules.GetWhitespaceLength(input, current);
+                current +=  HttpTokenParsingRules.GetWhitespaceLength(input, current);
 
                 var nameLength = HttpTokenParsingRules.GetTokenLength(input, current);
-
                 if (nameLength == 0)
                 {
                     name = default(StringSegment);
@@ -464,8 +472,9 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
 
                 name = new StringSegment(input, current, nameLength);
 
-                current = current + nameLength;
+                current +=  nameLength;
-                current = current + HttpTokenParsingRules.GetWhitespaceLength(input, current);
+                current +=  HttpTokenParsingRules.GetWhitespaceLength(input, current);
+
                 return current - startIndex;
             }
 
@@ -474,25 +483,31 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
                 var current = startIndex;
 
                 current++; // skip '='.
-                current = current + HttpTokenParsingRules.GetWhitespaceLength(input, current);
+                current +=  HttpTokenParsingRules.GetWhitespaceLength(input, current);
 
                 var valueLength = HttpTokenParsingRules.GetTokenLength(input, current);
 
                 if (valueLength == 0)
                 {
                     // A value can either be a token or a quoted string. Check if it is a quoted string.
-                    if (HttpTokenParsingRules.GetQuotedStringLength(input, current, out valueLength) != HttpParseResult.Parsed)
+                    var result = HttpTokenParsingRules.GetQuotedStringLength(input, current, out valueLength);
+                    if (result != HttpParseResult.Parsed)
                     {
                         // We have an invalid value. Reset the name and return.
                         value = default(StringSegment);
                         return 0;
                     }
+
+                    // Quotation marks are not part of a quoted parameter value.
+                    value = new StringSegment(input, current + 1, valueLength - 2);
+                }
+                else
+                {
+                    value = new StringSegment(input, current, valueLength);
                 }
 
-                value = new StringSegment(input, current, valueLength);
+                current +=  valueLength;
-
+                current +=  HttpTokenParsingRules.GetWhitespaceLength(input, current);
-                current = current + valueLength;
-                current = current + HttpTokenParsingRules.GetWhitespaceLength(input, current);
 
                 return current - startIndex;
             }
@@ -530,27 +545,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
 
             public bool Equals(MediaTypeParameter other)
             {
-                return HasName(other.Name) &&
+                return HasName(other.Name) && Value.Equals(other.Value, StringComparison.OrdinalIgnoreCase);
-                    Value.Equals(other.Value, StringComparison.OrdinalIgnoreCase);
-            }
-
-            /// <inheritdoc />
-            public override bool Equals(object obj)
-            {
-                if (ReferenceEquals(null, obj))
-                {
-                    return false;
-                }
-
-                return obj is MediaTypeParameter && Equals((MediaTypeParameter)obj);
-            }
-
-            public override int GetHashCode()
-            {
-                HashCodeCombiner hashCode = HashCodeCombiner.Start();
-                hashCode.Add(Name.Value);
-                hashCode.Add(Value.Value);
-                return hashCode;
             }
 
             public override string ToString() => $"{Name}={Value}";

test/Microsoft.AspNetCore.Mvc.Core.Test/Formatters/MediaTypeTest.cs

@@ -1,6 +1,7 @@
 // Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System.Text;
 using Microsoft.Extensions.Primitives;
 using Xunit;
 
@@ -11,7 +12,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
         [Theory]
         [InlineData("application/json")]
         [InlineData("application /json")]
-        public void CanParse_ParameterlessMediaTypes(string mediaType)
+        [InlineData(" application / json ")]
+        public void Constructor_CanParseParameterlessMediaTypes(string mediaType)
         {
             // Arrange & Act
             var result = new MediaType(mediaType, 0, mediaType.Length);
@@ -21,16 +23,30 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             Assert.Equal(new StringSegment("json"), result.SubType);
         }
 
+        public static TheoryData<string> MediaTypesWithParameters
+        {
+            get
+            {
+                return new TheoryData<string>
+                {
+                    "application/json;format=pretty;charset=utf-8;q=0.8",
+                    "application/json;format=pretty;charset=\"utf-8\";q=0.8",
+                    "application/json;format=pretty;charset=utf-8; q=0.8 ",
+                    "application/json;format=pretty;charset=utf-8 ; q=0.8 ",
+                    "application/json;format=pretty; charset=utf-8 ; q=0.8 ",
+                    "application/json;format=pretty ; charset=utf-8 ; q=0.8 ",
+                    "application/json; format=pretty ; charset=utf-8 ; q=0.8 ",
+                    "application/json; format=pretty ; charset=utf-8 ; q=  0.8 ",
+                    "application/json; format=pretty ; charset=utf-8 ; q  =  0.8 ",
+                    " application /  json; format =  pretty ; charset = utf-8 ; q  =  0.8 ",
+                    " application /  json; format =  \"pretty\" ; charset = \"utf-8\" ; q  =  \"0.8\" ",
+                };
+            }
+        }
+
         [Theory]
-        [InlineData("application/json;format=pretty;charset=utf-8;q=0.8")]
+        [MemberData(nameof(MediaTypesWithParameters))]
-        [InlineData("application/json;format=pretty;charset=utf-8; q=0.8 ")]
+        public void Constructor_CanParseMediaTypesWithParameters(string mediaType)
-        [InlineData("application/json;format=pretty;charset=utf-8 ; q=0.8 ")]
-        [InlineData("application/json;format=pretty; charset=utf-8 ; q=0.8 ")]
-        [InlineData("application/json;format=pretty ; charset=utf-8 ; q=0.8 ")]
-        [InlineData("application/json; format=pretty ; charset=utf-8 ; q=0.8 ")]
-        [InlineData("application/json; format=pretty ; charset=utf-8 ; q=  0.8 ")]
-        [InlineData("application/json; format=pretty ; charset=utf-8 ; q  =  0.8 ")]
-        public void CanParse_MediaTypesWithParameters(string mediaType)
         {
             // Arrange & Act
             var result = new MediaType(mediaType, 0, mediaType.Length);
@@ -43,6 +59,21 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             Assert.Equal(new StringSegment("utf-8"), result.GetParameter("charset"));
         }
 
+        [Theory]
+        [MemberData(nameof(MediaTypesWithParameters))]
+        public void ReplaceEncoding_ReturnsExpectedMediaType(string mediaType)
+        {
+            // Arrange
+            var encoding = Encoding.GetEncoding("iso-8859-1");
+            var expectedMediaType = mediaType.Replace("utf-8", "iso-8859-1");
+
+            // Act
+            var result = MediaType.ReplaceEncoding(mediaType, encoding);
+
+            // Assert
+            Assert.Equal(expectedMediaType, result);
+        }
+
         [Theory]
         [InlineData("application/json;charset=utf-8")]
         [InlineData("application/json;format=indent;q=0.8;charset=utf-8")]
@@ -52,89 +83,15 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
         {
             // Arrange
             var expectedParameter = new StringSegment("utf-8");
-
             var parsedMediaType = new MediaType(mediaType, 0, mediaType.Length);
 
             // Act
             var result = parsedMediaType.GetParameter("charset");
 
             // Assert
-            Assert.NotNull(result);
             Assert.Equal(expectedParameter, result);
         }
 
-        [Fact]
-        public void GetParameter_IsCaseInsensitive()
-        {
-            // Arrange
-            var mediaType = "application/json;charset=utf-8";
-            var expectedParameter = new StringSegment("utf-8");
-
-            var parsedMediaType = new MediaType(mediaType);
-
-            // Act
-            var result = parsedMediaType.GetParameter("CHARSET");
-
-            // Assert
-            Assert.NotNull(result);
-            Assert.Equal(expectedParameter, result);
-        }
-
-        [Theory]
-        [InlineData("application/json", "application/json", true)]
-        [InlineData("application/json", "application/json;charset=utf-8", true)]
-        [InlineData("application/json;charset=utf-8", "application/json", false)]
-        [InlineData("application/json;q=0.8", "application/json;q=0.9", true)]
-        [InlineData("application/json;q=0.8;charset=utf-7", "application/json;charset=utf-8;q=0.9", true)]
-        [InlineData("application/json;format=indent;charset=utf-8", "application/json", false)]
-        [InlineData("application/json", "application/json;format=indent;charset=utf-8", true)]
-        [InlineData("application/json;format=indent;charset=utf-8", "application/json;format=indent;charset=utf-8", true)]
-        [InlineData("application/json;charset=utf-8;format=indent", "application/json;format=indent;charset=utf-8", true)]
-        public void IsSubsetOf(string set, string subset, bool expectedResult)
-        {
-            // Arrange
-            var setMediaType = new MediaType(set);
-            var subSetMediaType = new MediaType(subset);
-
-            // Act
-            var result = subSetMediaType.IsSubsetOf(setMediaType);
-
-            // Assert
-            Assert.Equal(expectedResult, result);
-        }
-
-        [Theory]
-        [InlineData("*/*", true)]
-        [InlineData("text/*", false)]
-        [InlineData("text/plain", false)]
-        public void MatchesAllTypes(string value, bool expectedResult)
-        {
-            // Arrange
-            var mediaType = new MediaType(value);
-
-            // Act
-            var result = mediaType.MatchesAllTypes;
-
-            // Assert
-            Assert.Equal(expectedResult, result);
-        }
-
-        [Theory]
-        [InlineData("*/*", true)]
-        [InlineData("text/*", true)]
-        [InlineData("text/plain", false)]
-        public void MatchesAllSubtypes(string value, bool expectedResult)
-        {
-            // Arrange
-            var mediaType = new MediaType(value);
-
-            // Act
-            var result = mediaType.MatchesAllSubTypes;
-
-            // Assert
-            Assert.Equal(expectedResult, result);
-        }
-
         [Fact]
         public void GetParameter_ReturnsNull_IfParameterIsNotInMediaType()
         {
@@ -148,5 +105,128 @@ namespace Microsoft.AspNetCore.Mvc.Formatters
             // Assert
             Assert.False(result.HasValue);
         }
+
+        [Fact]
+        public void GetParameter_IsCaseInsensitive()
+        {
+            // Arrange
+            var mediaType = "application/json;charset=utf-8";
+            var expectedParameter = new StringSegment("utf-8");
+
+            var parsedMediaType = new MediaType(mediaType);
+
+            // Act
+            var result = parsedMediaType.GetParameter("CHARSET");
+
+            // Assert
+            Assert.Equal(expectedParameter, result);
+        }
+
+        [Theory]
+        [InlineData("application/json", "application/json")]
+        [InlineData("application/json", "application/json;charset=utf-8")]
+        [InlineData("application/json;q=0.8", "application/json;q=0.9")]
+        [InlineData("application/json;q=0.8;charset=utf-7", "application/json;charset=utf-8;q=0.9")]
+        [InlineData("application/json", "application/json;format=indent;charset=utf-8")]
+        [InlineData("application/json;format=indent;charset=utf-8", "application/json;format=indent;charset=utf-8")]
+        [InlineData("application/json;charset=utf-8;format=indent", "application/json;format=indent;charset=utf-8")]
+        public void IsSubsetOf_ReturnsTrueWhenExpected(string set, string subset)
+        {
+            // Arrange
+            var setMediaType = new MediaType(set);
+            var subSetMediaType = new MediaType(subset);
+
+            // Act
+            var result = subSetMediaType.IsSubsetOf(setMediaType);
+
+            // Assert
+            Assert.True(result);
+        }
+
+        [Theory]
+        [InlineData("application/json;charset=utf-8", "application/json")]
+        [InlineData("application/json;format=indent;charset=utf-8", "application/json")]
+        [InlineData("application/json;format=indent;charset=utf-8", "application/json;charset=utf-8")]
+        public void IsSubsetOf_ReturnsFalseWhenExpected(string set, string subset)
+        {
+            // Arrange
+            var setMediaType = new MediaType(set);
+            var subSetMediaType = new MediaType(subset);
+
+            // Act
+            var result = subSetMediaType.IsSubsetOf(setMediaType);
+
+            // Assert
+            Assert.False(result);
+        }
+
+        [Fact]
+        public void MatchesAllTypes_ReturnsTrueWhenExpected()
+        {
+            // Arrange
+            var mediaType = new MediaType("*/*");
+
+            // Act
+            var result = mediaType.MatchesAllTypes;
+
+            // Assert
+            Assert.True(result);
+        }
+
+        [Theory]
+        [InlineData("text/*")]
+        [InlineData("text/plain")]
+        public void MatchesAllTypes_ReturnsFalseWhenExpected(string value)
+        {
+            // Arrange
+            var mediaType = new MediaType(value);
+
+            // Act
+            var result = mediaType.MatchesAllTypes;
+
+            // Assert
+            Assert.False(result);
+        }
+
+        [Theory]
+        [InlineData("*/*")]
+        [InlineData("text/*")]
+        public void MatchesAllSubtypes_ReturnsTrueWhenExpected(string value)
+        {
+            // Arrange
+            var mediaType = new MediaType(value);
+
+            // Act
+            var result = mediaType.MatchesAllSubTypes;
+
+            // Assert
+            Assert.True(result);
+        }
+
+        [Fact]
+        public void MatchesAllSubtypes_ReturnsFalseWhenExpected()
+        {
+            // Arrange
+            var mediaType = new MediaType("text/plain");
+
+            // Act
+            var result = mediaType.MatchesAllSubTypes;
+
+            // Assert
+            Assert.False(result);
+        }
+
+        [Theory]
+        [MemberData(nameof(MediaTypesWithParameters))]
+        [InlineData("application/json;format=pretty;q=0.9;charset=utf-8;q=0.8")]
+        [InlineData("application/json;format=pretty;q=0.9;charset=utf-8;q=0.8;version=3")]
+        public void CreateMediaTypeSegmentWithQuality_FindsQValue(string value)
+        {
+            // Arrange & Act
+            var mediaTypeSegment = MediaType.CreateMediaTypeSegmentWithQuality(value, start: 0);
+
+            // Assert
+            Assert.Equal(0.8d, mediaTypeSegment.Quality);
+        }
     }
 }