From 0d8a7368d941b8d20c0de1e6de831737c6293d52 Mon Sep 17 00:00:00 2001 From: sornaks Date: Sun, 28 Sep 2014 12:01:40 -0700 Subject: [PATCH] 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. --- .../HeaderParsingHelpers.cs | 21 ++++- .../MediaTypeWithQualityHeaderValue.cs | 37 ++++++-- .../Properties/Resources.Designer.cs | 32 +++++++ .../Resources.resx | 6 ++ .../StringWithQualityHeaderValue.cs | 27 +++++- .../ActionResults/ObjectResultTests.cs | 11 ++- .../MediaTypeWithQualityHeaderValueTests.cs | 8 +- .../HeaderParsingHelpersTests.cs | 90 +++++++++++++++++++ .../MediaTypeHeaderValueParsingTests.cs | 28 ++++++ ...tringWithQualityHeaderValueParsingTests.cs | 27 ++++++ 10 files changed, 261 insertions(+), 26 deletions(-) rename src/{Microsoft.AspNet.Mvc.Core/Formatters => Microsoft.AspNet.Mvc.HeaderValueAbstractions}/HeaderParsingHelpers.cs (58%) create mode 100644 test/Microsoft.AspNet.Mvc.HeaderValueAbstractions.Test/HeaderParsingHelpersTests.cs diff --git a/src/Microsoft.AspNet.Mvc.Core/Formatters/HeaderParsingHelpers.cs b/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/HeaderParsingHelpers.cs similarity index 58% rename from src/Microsoft.AspNet.Mvc.Core/Formatters/HeaderParsingHelpers.cs rename to src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/HeaderParsingHelpers.cs index 551d940f2a..aaf22cf357 100644 --- a/src/Microsoft.AspNet.Mvc.Core/Formatters/HeaderParsingHelpers.cs +++ b/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(); 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(); 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; diff --git a/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/MediaTypeWithQualityHeaderValue.cs b/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/MediaTypeWithQualityHeaderValue.cs index 89fa195332..44903df4b3 100644 --- a/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/MediaTypeWithQualityHeaderValue.cs +++ b/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 - new MediaTypeWithQualityHeaderValue() + headerValue = 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; + } } } diff --git a/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/Properties/Resources.Designer.cs b/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/Properties/Resources.Designer.cs index 2299e4a0fd..076b72da3b 100644 --- a/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/Properties/Resources.Designer.cs +++ b/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); + /// + /// Invalid Argument. Accept Charset '{0}' could not be parsed. + /// + internal static string InvalidAcceptCharset + { + get { return GetString("InvalidAcceptCharset"); } + } + + /// + /// Invalid Argument. Accept Charset '{0}' could not be parsed. + /// + internal static string FormatInvalidAcceptCharset(object p0) + { + return string.Format(CultureInfo.CurrentCulture, GetString("InvalidAcceptCharset"), p0); + } + + /// + /// Invalid Argument. Accept Header '{0}' could not be parsed. + /// + internal static string InvalidAcceptHeader + { + get { return GetString("InvalidAcceptHeader"); } + } + + /// + /// Invalid Argument. Accept Header '{0}' could not be parsed. + /// + internal static string FormatInvalidAcceptHeader(object p0) + { + return string.Format(CultureInfo.CurrentCulture, GetString("InvalidAcceptHeader"), p0); + } + /// /// Invalid Argument. Content type '{0}' could not be parsed. /// diff --git a/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/Resources.resx b/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/Resources.resx index d1a33eedd0..0f3e3bbab5 100644 --- a/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/Resources.resx +++ b/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/Resources.resx @@ -117,6 +117,12 @@ System.Resources.ResXResourceWriter, System.Windows.Forms, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089 + + Invalid Argument. Accept Charset '{0}' could not be parsed. + + + Invalid Argument. Accept Header '{0}' could not be parsed. + Invalid Argument. Content type '{0}' could not be parsed. diff --git a/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/StringWithQualityHeaderValue.cs b/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/StringWithQualityHeaderValue.cs index 548b902a5e..16645c2477 100644 --- a/src/Microsoft.AspNet.Mvc.HeaderValueAbstractions/StringWithQualityHeaderValue.cs +++ b/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; } } } diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/ActionResults/ObjectResultTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/ActionResults/ObjectResultTests.cs index df04df1908..d79051437a 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/ActionResults/ObjectResultTests.cs +++ b/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 : - acceptHeader?.Split(',') - .Select(header => MediaTypeWithQualityHeaderValue.Parse(header)) - .ToArray(); + null : + acceptHeader?.Split(',') + .Select(header => MediaTypeWithQualityHeaderValue.Parse(header)) + .ToArray(); var stream = new MemoryStream(); var httpResponse = new Mock(); httpResponse.SetupProperty(o => o.ContentType); diff --git a/test/Microsoft.AspNet.Mvc.Core.Test/MediaTypeWithQualityHeaderValueTests.cs b/test/Microsoft.AspNet.Mvc.Core.Test/MediaTypeWithQualityHeaderValueTests.cs index c58db9ad9c..6941c7d3c6 100644 --- a/test/Microsoft.AspNet.Mvc.Core.Test/MediaTypeWithQualityHeaderValueTests.cs +++ b/test/Microsoft.AspNet.Mvc.Core.Test/MediaTypeWithQualityHeaderValueTests.cs @@ -65,11 +65,13 @@ namespace Microsoft.AspNet.Mvc.Core.Test public void SortMediaTypeWithQualityHeaderValuesByQFactor_SortsCorrectly(IEnumerable unsorted, IEnumerable expectedSorted) { // Arrange - var unsortedValues = - new List(unsorted.Select(u => MediaTypeWithQualityHeaderValue.Parse(u))); + var unsortedValues = + new List( + unsorted.Select(u => MediaTypeWithQualityHeaderValue.Parse(u))); var expectedSortedValues = - new List(expectedSorted.Select(u => MediaTypeWithQualityHeaderValue.Parse(u))); + new List( + expectedSorted.Select(u => MediaTypeWithQualityHeaderValue.Parse(u))); // Act var actualSorted = unsortedValues.OrderByDescending(m => m, MediaTypeWithQualityHeaderValueComparer.QualityComparer).ToArray(); diff --git a/test/Microsoft.AspNet.Mvc.HeaderValueAbstractions.Test/HeaderParsingHelpersTests.cs b/test/Microsoft.AspNet.Mvc.HeaderValueAbstractions.Test/HeaderParsingHelpersTests.cs new file mode 100644 index 0000000000..fada25120e --- /dev/null +++ b/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 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 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); + } + } +} \ No newline at end of file diff --git a/test/Microsoft.AspNet.Mvc.HeaderValueAbstractions.Test/MediaTypeHeaderValueParsingTests.cs b/test/Microsoft.AspNet.Mvc.HeaderValueAbstractions.Test/MediaTypeHeaderValueParsingTests.cs index dd2559a2fa..4e138a5725 100644 --- a/test/Microsoft.AspNet.Mvc.HeaderValueAbstractions.Test/MediaTypeHeaderValueParsingTests.cs +++ b/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); diff --git a/test/Microsoft.AspNet.Mvc.HeaderValueAbstractions.Test/StringWithQualityHeaderValueParsingTests.cs b/test/Microsoft.AspNet.Mvc.HeaderValueAbstractions.Test/StringWithQualityHeaderValueParsingTests.cs index fa0c884441..bc726e9366 100644 --- a/test/Microsoft.AspNet.Mvc.HeaderValueAbstractions.Test/StringWithQualityHeaderValueParsingTests.cs +++ b/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); + } + } } }