Issue #1141 - When Accept header or Accept-Charset header has invalid QualityFactor we throw.

Fix: Imitating the same behavior as it is in WebApi. We ignore the entire header even if one part of it is invalid.

src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/HeaderParsingHelpers.cs

@@ -2,9 +2,8 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System.Collections.Generic;
-using Microsoft.AspNet.Mvc.HeaderValueAbstractions;
 
-namespace Microsoft.AspNet.Mvc
+namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
 {
     public static class HeaderParsingHelpers
     {
@@ -18,7 +17,14 @@ namespace Microsoft.AspNet.Mvc
             var acceptHeaderCollection = new List<MediaTypeWithQualityHeaderValue>();
             foreach (var item in acceptHeader.Split(','))
             {
-                acceptHeaderCollection.Add(MediaTypeWithQualityHeaderValue.Parse(item));
+                MediaTypeWithQualityHeaderValue parsedAcceptHeader;
+                // If we are unable to parse any of the Accept Headers, we ignore them completely.
+                if (!MediaTypeWithQualityHeaderValue.TryParse(item, out parsedAcceptHeader))
+                {
+                    return null;
+                }
+
+                acceptHeaderCollection.Add(parsedAcceptHeader);
             }
 
             return acceptHeaderCollection;
@@ -34,7 +40,14 @@ namespace Microsoft.AspNet.Mvc
             var acceptCharsetHeaderCollection = new List<StringWithQualityHeaderValue>();
             foreach (var item in acceptCharsetHeader.Split(','))
             {
-                acceptCharsetHeaderCollection.Add(StringWithQualityHeaderValue.Parse(item));
+                StringWithQualityHeaderValue parsedAcceptCharsetHeader;
+                // If we are unable to parse any of the Accept-Charset Headers, we ignore them completely.
+                if (!StringWithQualityHeaderValue.TryParse(item, out parsedAcceptCharsetHeader))
+                {
+                    return null;
+                }
+
+                acceptCharsetHeaderCollection.Add(parsedAcceptCharsetHeader);
             }
 
             return acceptCharsetHeaderCollection;

src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/MediaTypeWithQualityHeaderValue.cs

@@ -2,7 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
-using System.Collections.Generic;
+using System.Globalization;
 
 namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
 {
@@ -10,26 +10,32 @@ namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
     {
         public double? Quality { get; private set; }
 
-        public static new MediaTypeWithQualityHeaderValue Parse(string input)
+        public static bool TryParse(string input, out MediaTypeWithQualityHeaderValue headerValue)
         {
             MediaTypeHeaderValue mediaTypeHeaderValue;
             if (!MediaTypeHeaderValue.TryParse(input, out mediaTypeHeaderValue))
             {
-                return null;
+                headerValue = null;
+                return false;
             }
 
             var quality = HttpHeaderUtilitites.Match;
-            string qualityStringValue = null;
+            string qualityStringValue;
             if (mediaTypeHeaderValue.Parameters.TryGetValue("q", out qualityStringValue))
             {
-                if (!Double.TryParse(qualityStringValue, out quality))
+                if (!double.TryParse(
+                        qualityStringValue,
+                        NumberStyles.AllowLeadingWhite | NumberStyles.AllowDecimalPoint |
+                            NumberStyles.AllowTrailingWhite,
+                        NumberFormatInfo.InvariantInfo,
+                        out quality))
                 {
-                    return null;
+                    headerValue = null;
+                    return false;
                 }
             }
 
-            return
+            headerValue = new MediaTypeWithQualityHeaderValue()
-                new MediaTypeWithQualityHeaderValue()
                 {
                     MediaType = mediaTypeHeaderValue.MediaType,
                     MediaSubType = mediaTypeHeaderValue.MediaSubType,
@@ -38,6 +44,19 @@ namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
                     Parameters = mediaTypeHeaderValue.Parameters,
                     Quality = quality,
                 };
-        } 
+
+            return true;
+        }
+
+        public static new MediaTypeWithQualityHeaderValue Parse(string input)
+        {
+            MediaTypeWithQualityHeaderValue headerValue = null;
+            if (!MediaTypeWithQualityHeaderValue.TryParse(input, out headerValue))
+            {
+                throw new ArgumentException(Resources.FormatInvalidAcceptHeader(input));
+            }
+
+            return headerValue;
+        }
     }
 }

src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/Properties/Resources.Designer.cs

@@ -10,6 +10,38 @@ namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
         private static readonly ResourceManager _resourceManager
             = new ResourceManager("Microsoft.AspNet.Mvc.HeaderValueAbstractions.Resources", typeof(Resources).GetTypeInfo().Assembly);
 
+        /// <summary>
+        /// Invalid Argument. Accept Charset '{0}' could not be parsed.
+        /// </summary>
+        internal static string InvalidAcceptCharset
+        {
+            get { return GetString("InvalidAcceptCharset"); }
+        }
+
+        /// <summary>
+        /// Invalid Argument. Accept Charset '{0}' could not be parsed.
+        /// </summary>
+        internal static string FormatInvalidAcceptCharset(object p0)
+        {
+            return string.Format(CultureInfo.CurrentCulture, GetString("InvalidAcceptCharset"), p0);
+        }
+
+        /// <summary>
+        /// Invalid Argument. Accept Header '{0}' could not be parsed.
+        /// </summary>
+        internal static string InvalidAcceptHeader
+        {
+            get { return GetString("InvalidAcceptHeader"); }
+        }
+
+        /// <summary>
+        /// Invalid Argument. Accept Header '{0}' could not be parsed.
+        /// </summary>
+        internal static string FormatInvalidAcceptHeader(object p0)
+        {
+            return string.Format(CultureInfo.CurrentCulture, GetString("InvalidAcceptHeader"), p0);
+        }
+
         /// <summary>
         /// Invalid Argument. Content type '{0}' could not be parsed.
         /// </summary>

src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/Resources.resx

@@ -117,6 +117,12 @@
   <resheader name="writer">
     <value>System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089</value>
   </resheader>
+  <data name="InvalidAcceptCharset" xml:space="preserve">
+    <value>Invalid Argument. Accept Charset '{0}' could not be parsed.</value>
+  </data>
+  <data name="InvalidAcceptHeader" xml:space="preserve">
+    <value>Invalid Argument. Accept Header '{0}' could not be parsed.</value>
+  </data>
   <data name="InvalidContentType" xml:space="preserve">
     <value>Invalid Argument. Content type '{0}' could not be parsed.</value>
   </data>

src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/StringWithQualityHeaderValue.cs

@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Globalization;
 
 namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
 {
@@ -13,7 +14,7 @@ namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
 
         public string Value { get; set; }
 
-        public static StringWithQualityHeaderValue Parse(string input)
+        public static bool TryParse(string input, out StringWithQualityHeaderValue headerValue)
         {
             var inputArray = input.Split(new[] { ';' }, 2);
             var value = inputArray[0].Trim();
@@ -27,9 +28,15 @@ namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
                 if (nameValuePair.Length > 1 && nameValuePair[0].Trim().Equals("q"))
                 {
                     // TODO: all extraneous parameters are ignored. Throw/return null if that is the case.
-                    if (!Double.TryParse(nameValuePair[1].Trim(), out quality))
+                    if (!double.TryParse(
+                            nameValuePair[1],
+                            NumberStyles.AllowLeadingWhite | NumberStyles.AllowDecimalPoint |
+                                NumberStyles.AllowTrailingWhite,
+                            NumberFormatInfo.InvariantInfo,
+                            out quality))
                     {
-                        return null;
+                        headerValue = null;
+                        return false;
                     }
                 }
             }
@@ -41,7 +48,19 @@ namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
                 RawValue = input
             };
 
-            return stringWithQualityHeader;
+            headerValue = stringWithQualityHeader;
+            return true;
+        }
+
+        public static StringWithQualityHeaderValue Parse(string input)
+        {
+            StringWithQualityHeaderValue headerValue;
+            if(!TryParse(input, out headerValue))
+            {
+                throw new ArgumentException(Resources.FormatInvalidAcceptCharset(input));
+            }
+
+            return headerValue;
         }
     }
 }

test/Microsoft.AspNet.Mvc.Core.Test/ActionResults/ObjectResultTests.cs

@@ -6,10 +6,9 @@ using System.Collections.Generic;
 using System.IO;
 using System.Linq;
 using System.Text;
-using System.Threading;
 using System.Threading.Tasks;
-using Microsoft.AspNet.Mvc.HeaderValueAbstractions;
 using Microsoft.AspNet.Http;
+using Microsoft.AspNet.Mvc.HeaderValueAbstractions;
 using Microsoft.AspNet.Routing;
 using Microsoft.Framework.DependencyInjection;
 using Microsoft.Framework.DependencyInjection.Fallback;
@@ -288,10 +287,10 @@ namespace Microsoft.AspNet.Mvc.Core.Test.ActionResults
             // For each additional accept header, it is called once. 
             // Arrange
             var acceptHeaderCollection = string.IsNullOrEmpty(acceptHeader) ? 
-                                         null :
+                null :
-                                         acceptHeader?.Split(',')
+                acceptHeader?.Split(',')
-                                                      .Select(header => MediaTypeWithQualityHeaderValue.Parse(header))
+                             .Select(header => MediaTypeWithQualityHeaderValue.Parse(header))
-                                                      .ToArray();
+                             .ToArray();
             var stream = new MemoryStream();
             var httpResponse = new Mock<HttpResponse>();
             httpResponse.SetupProperty<string>(o => o.ContentType);

test/Microsoft.AspNet.Mvc.Core.Test/MediaTypeWithQualityHeaderValueTests.cs

@@ -65,11 +65,13 @@ namespace Microsoft.AspNet.Mvc.Core.Test
         public void SortMediaTypeWithQualityHeaderValuesByQFactor_SortsCorrectly(IEnumerable<string> unsorted, IEnumerable<string> expectedSorted)
         {
             // Arrange
-            var unsortedValues =
+            var unsortedValues = 
-                new List<MediaTypeWithQualityHeaderValue>(unsorted.Select(u => MediaTypeWithQualityHeaderValue.Parse(u)));
+                new List<MediaTypeWithQualityHeaderValue>(
+                    unsorted.Select(u => MediaTypeWithQualityHeaderValue.Parse(u)));
 
             var expectedSortedValues =
-                new List<MediaTypeWithQualityHeaderValue>(expectedSorted.Select(u => MediaTypeWithQualityHeaderValue.Parse(u)));
+                new List<MediaTypeWithQualityHeaderValue>(
+                    expectedSorted.Select(u => MediaTypeWithQualityHeaderValue.Parse(u)));
 
             // Act
             var actualSorted = unsortedValues.OrderByDescending(m => m, MediaTypeWithQualityHeaderValueComparer.QualityComparer).ToArray();

test/Microsoft.AspNet.Mvc.HeaderValueAbstractions.Test/HeaderParsingHelpersTests.cs

@@ -0,0 +1,90 @@
+// Copyright (c) Microsoft Open Technologies, Inc. All rights reserved.
+// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
+
+using System.Collections.Generic;
+using Xunit;
+
+namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
+{
+    public class HeaderParsingHelpersTests
+    {
+        //Accept Headers
+        [Fact]
+        public void GetAcceptHeaders_ReturnsParsedHeaders()
+        {
+            // Arrange & Act
+            var headers = HeaderParsingHelpers.GetAcceptHeaders("application/xml;q=0.4, application/xhtml;q=0.9");
+
+            // Assert
+            Assert.Equal(2, headers.Count);
+            Assert.Equal("application", headers[0].MediaType);
+            Assert.Equal("xml", headers[0].MediaSubType);
+            Assert.Equal(0.4, headers[0].Quality);
+            Assert.Equal("application", headers[1].MediaType);
+            Assert.Equal("xhtml", headers[1].MediaSubType);
+            Assert.Equal(0.9, headers[1].Quality);
+        }
+
+        public static IEnumerable<object[]> CasesWhereGetAcceptHeadersReturnsNull
+        {
+            get
+            {
+                yield return new object[] { "" };
+                yield return new object[] { "application/xml;q=0.4, application/xhtml;q=0,9" };
+                yield return new object[] { "application/xml;q=0.4, application/xhtml;q=-0.4" };
+                yield return new object[] { "application/xml;q=0 4" };
+                yield return new object[] { "application/xml;q=0*4" };
+                yield return new object[] { "application/xml;q=1^4" };
+            }
+        }
+
+        [Theory]
+        [MemberData(nameof(CasesWhereGetAcceptHeadersReturnsNull))]
+        public void GetAcceptHeaders_ReturnsNull(string acceptHeader)
+        {
+            // Arrange & Act
+            var headers = HeaderParsingHelpers.GetAcceptHeaders(acceptHeader);
+
+            // Assert
+            Assert.Null(headers);
+        }
+
+        // Charset Headers
+        [Fact]
+        public void GetAcceptCharsetHeaders_ReturnsParsedHeaders()
+        {
+            // Arrange & Act
+            var headers = HeaderParsingHelpers.GetAcceptCharsetHeaders("utf-8;q=0.7,gzip;q=0.3");
+
+            // Assert
+            Assert.Equal(2, headers.Count);
+            Assert.Equal("utf-8", headers[0].Value);
+            Assert.Equal(0.7, headers[0].Quality);
+            Assert.Equal("gzip", headers[1].Value);
+            Assert.Equal(0.3, headers[1].Quality);
+        }
+
+        public static IEnumerable<object[]> CasesWhereGetCharsetHeadersReturnsNull
+        {
+            get
+            {
+                yield return new object[] { "" };
+                yield return new object[] { "utf-8;q=0,7,gzip;q=-1" };
+                yield return new object[] { "utf-8;q=0*7,gzip;q=1.0" };
+                yield return new object[] { "utf-8;q=0 7" };
+                yield return new object[] { "utf-8;q=0^7" };
+            }
+        }
+
+        [Theory]
+        [MemberData(nameof(CasesWhereGetCharsetHeadersReturnsNull))]
+        public void GetAcceptCharsetHeaders_ReturnsNull_IfAcceptHeaderIsEmpty(string charsetHeader)
+        {
+            // Arrange & Act
+            var headers = HeaderParsingHelpers.GetAcceptCharsetHeaders(charsetHeader);
+
+            // Assert
+            Assert.Null(headers);
+        }
+    }
+}

test/Microsoft.AspNet.Mvc.HeaderValueAbstractions.Test/MediaTypeHeaderValueParsingTests.cs

@@ -57,6 +57,7 @@ namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
                                          double quality,
                                          string rawValue)
         {
+            // Arrange
             var parsedValue = MediaTypeWithQualityHeaderValue.Parse(rawValue);
             // Act and Assert
             Assert.Equal(rawValue, parsedValue.RawValue);
@@ -64,9 +65,35 @@ namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
             Assert.Equal(mediaSubType, parsedValue.MediaSubType);
             Assert.Equal(charset, parsedValue.Charset);
             Assert.Equal(range, parsedValue.MediaTypeRange);
+            Assert.Equal(quality, parsedValue.Quality);
             ValidateParametes(parameters, parsedValue.Parameters);
         }
 
+        [Theory]
+        [InlineData(false, "text", "plain", null, "text /plain;q=1,9")]
+        [InlineData(true, "text", "plain", 0.9, "text/plain;q=0.9")]
+        public void MediaTypeWithQualityHeaderValue_TryParse_ReturnsApproperiateResults(
+            bool result,
+            string mediaType,
+            string mediaSubType,
+            double quality,
+            string rawValue)
+        {
+            // Arrange
+            MediaTypeWithQualityHeaderValue parsedValue;
+            var isValid = MediaTypeWithQualityHeaderValue.TryParse(rawValue, out parsedValue);
+
+            // Act and Assert
+            Assert.Equal(result, isValid);
+            if(result)
+            {
+                Assert.Equal(rawValue, parsedValue.RawValue);
+                Assert.Equal(mediaType, parsedValue.MediaType);
+                Assert.Equal(mediaSubType, parsedValue.MediaSubType);
+                Assert.Equal(quality, parsedValue.Quality);
+            }
+        }
+
         [Theory]
         [InlineData("*/*;", "*/*", true)]
         [InlineData("text/*;", "text/*", true)]
@@ -88,6 +115,7 @@ namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
                                                         string mediaType2,
                                                         bool isMediaType1Subset)
         {
+            // Arrange
             var parsedMediaType1 = MediaTypeWithQualityHeaderValue.Parse(mediaType1);
             var parsedMediaType2 = MediaTypeWithQualityHeaderValue.Parse(mediaType2);
 

test/Microsoft.AspNet.Mvc.HeaderValueAbstractions.Test/StringWithQualityHeaderValueParsingTests.cs

@@ -19,11 +19,38 @@ namespace Microsoft.AspNet.Mvc.HeaderValueAbstractions
                                          double quality,
                                          string rawValue)
         {
+            // Arrange
             var parsedValue = StringWithQualityHeaderValue.Parse(rawValue);
+
             // Act and Assert
             Assert.Equal(rawValue, parsedValue.RawValue);
             Assert.Equal(value, parsedValue.Value);
             Assert.Equal(quality, parsedValue.Quality);
         }
+
+        [Theory]
+        [InlineData("*", true, 0.7, "*;q=.7")]
+        [InlineData("iso-8859-5", true, HttpHeaderUtilitites.Match, "iso-8859-5")]
+        [InlineData("unicode-1-1", true, 0.8, "unicode-1-1;q=0.8")]
+        [InlineData("unicode-1-1", false, 0.8, "unicode-1-1;q=-")]
+        public void StringWithQualityHeaderValue_TryParse_ReturnsApproperiateResults(
+            string value,
+            bool result,
+            double quality,
+            string rawValue)
+        {
+            // Arrange
+            StringWithQualityHeaderValue parsedValue;
+            var isValid = StringWithQualityHeaderValue.TryParse(rawValue, out parsedValue);
+
+            // Act and Assert
+            Assert.Equal(result, isValid);
+            if(result == true)
+            {
+                Assert.Equal(rawValue, parsedValue.RawValue);
+                Assert.Equal(value, parsedValue.Value);
+                Assert.Equal(quality, parsedValue.Quality);
+            }
+        }
     }
 }