From 957980630688898d8cc6de357f658d98231b5e35 Mon Sep 17 00:00:00 2001 From: jacalvar Date: Mon, 3 Oct 2016 14:57:45 -0700 Subject: [PATCH] [Fixes #5150] parsing issue on asp.net when request quality factor is specified --- .../Formatters/MediaType.cs | 11 +++- .../Internal/AcceptHeaderParser.cs | 35 ++++++++---- .../Internal/AcceptHeaderParserTest.cs | 53 ++++++++++++++++--- .../ContentNegotiationTest.cs | 20 +++++++ 4 files changed, 101 insertions(+), 18 deletions(-) diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Formatters/MediaType.cs b/src/Microsoft.AspNetCore.Mvc.Core/Formatters/MediaType.cs index 0d20e5be36..e30b52408e 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Formatters/MediaType.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Formatters/MediaType.cs @@ -292,6 +292,12 @@ namespace Microsoft.AspNetCore.Mvc.Formatters public static MediaTypeSegmentWithQuality CreateMediaTypeSegmentWithQuality(string mediaType, int start) { var parsedMediaType = new MediaType(mediaType, start, length: null); + if (parsedMediaType.Type.Equals(default(StringSegment)) || + parsedMediaType.SubType.Equals(default(StringSegment))) + { + return default(MediaTypeSegmentWithQuality); + } + var parser = parsedMediaType._parameterParser; double quality = 1.0d; @@ -306,9 +312,9 @@ namespace Microsoft.AspNetCore.Mvc.Formatters } } - // We check if the parsed media type has value at this stage when we have iterated + // 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. - if (!parser.ParsingFailed && parser.CurrentOffset >= start) + if (!parser.ParsingFailed) { return new MediaTypeSegmentWithQuality( new StringSegment(mediaType, start, parser.CurrentOffset - start), @@ -395,6 +401,7 @@ namespace Microsoft.AspNetCore.Mvc.Formatters { if (_mediaTypeBuffer == null) { + ParsingFailed = true; result = default(MediaTypeParameter); return false; } diff --git a/src/Microsoft.AspNetCore.Mvc.Core/Internal/AcceptHeaderParser.cs b/src/Microsoft.AspNetCore.Mvc.Core/Internal/AcceptHeaderParser.cs index 33eae84fdd..45e1800067 100644 --- a/src/Microsoft.AspNetCore.Mvc.Core/Internal/AcceptHeaderParser.cs +++ b/src/Microsoft.AspNetCore.Mvc.Core/Internal/AcceptHeaderParser.cs @@ -29,7 +29,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Internal { throw new ArgumentNullException(nameof(parsedValues)); } - for (var i = 0; i < acceptHeaders.Count; i++) { var charIndex = 0; @@ -46,13 +45,6 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Internal parsedValues.Add(output); } } - else - { - var invalidValuesError = Resources.FormatAcceptHeaderParser_ParseAcceptHeader_InvalidValues( - value.Substring(charIndex)); - - throw new FormatException(invalidValuesError); - } } } } @@ -80,11 +72,34 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Internal return true; } - MediaTypeSegmentWithQuality result; - var length = GetMediaTypeWithQualityLength(value, currentIndex, out result); + // We deliberately want to ignore media types that we are not capable of parsing. + // This is due to the fact that some browsers will send invalid media types like + // ; q=0.9 or */;q=0.2, etc. + // In this scenario, our recovery action consists of advancing the pointer to the + // next separator and moving on. + // In case we don't find the next separator, we simply advance the cursor to the + // end of the string to signal that we are done parsing. + var result = default(MediaTypeSegmentWithQuality); + var length = 0; + try + { + length = GetMediaTypeWithQualityLength(value, currentIndex, out result); + } + catch + { + length = 0; + } if (length == 0) { + // The parsing failed. + currentIndex = value.IndexOf(',', currentIndex); + if (currentIndex == -1) + { + index = value.Length; + return false; + } + index = currentIndex; return false; } diff --git a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/AcceptHeaderParserTest.cs b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/AcceptHeaderParserTest.cs index b8a470cff6..9b86759578 100644 --- a/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/AcceptHeaderParserTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.Core.Test/Internal/AcceptHeaderParserTest.cs @@ -3,6 +3,7 @@ using System; using System.Collections.Generic; +using System.Linq; using Microsoft.Extensions.Primitives; using Xunit; @@ -54,14 +55,54 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Internal { // Arrange var header = "application/json, application/xml,;q=0.8"; - var expectedException = "\"Invalid values ';q=0.8'.\""; + var expectedMediaTypes = new List + { + new MediaTypeSegmentWithQuality(new StringSegment("application/json"),1.0), + new MediaTypeSegmentWithQuality(new StringSegment("application/xml"),1.0), + }; // Act - var ex = Assert.Throws( - () => { AcceptHeaderParser.ParseAcceptHeader(new List { header }); }); + var mediaTypes = AcceptHeaderParser.ParseAcceptHeader(new List { header }); // Assert - Assert.Equal(expectedException, ex.Message); + Assert.Equal(expectedMediaTypes, mediaTypes); + } + + public static TheoryData ParseAcceptHeaderWithInvalidMediaTypesData => + new TheoryData + { + { new [] { ";q=0.9" }, new string[] { } }, + { new [] { "/" }, new string[] { } }, + { new [] { "*/" }, new string[] { } }, + { new [] { "/*" }, new string[] { } }, + { new [] { "/;q=0.9" }, new string[] { } }, + { new [] { "*/;q=0.9" }, new string[] { } }, + { new [] { "/*;q=0.9" }, new string[] { } }, + { new [] { "/;q=0.9,text/html" }, new string[] { "text/html" } }, + { new [] { "*/;q=0.9,text/html" }, new string[] { "text/html" } }, + { new [] { "/*;q=0.9,text/html" }, new string[] { "text/html" } }, + { new [] { "img/png,/;q=0.9,text/html" }, new string[] { "img/png", "text/html" } }, + { new [] { "img/png,*/;q=0.9,text/html" }, new string[] { "img/png", "text/html" } }, + { new [] { "img/png,/*;q=0.9,text/html" }, new string[] { "img/png", "text/html" } }, + { new [] { "img/png, /;q=0.9" }, new string[] { "img/png", } }, + { new [] { "img/png, */;q=0.9" }, new string[] { "img/png", } }, + { new [] { "img/png;q=1.0, /*;q=0.9" }, new string[] { "img/png;q=1.0", } }, + }; + + [Theory] + [MemberData(nameof(ParseAcceptHeaderWithInvalidMediaTypesData))] + public void ParseAcceptHeader_GracefullyRecoversFromInvalidMediaTypeValues_AndReturnsValidMediaTypes( + string[] acceptHeader, + string[] expected) + { + // Arrange + var expectedMediaTypes = expected.Select(e => new MediaTypeSegmentWithQuality(new StringSegment(e), 1.0)).ToList(); + + // Act + var parsed = AcceptHeaderParser.ParseAcceptHeader(acceptHeader); + + // Assert + Assert.Equal(expectedMediaTypes, parsed); } [Fact] @@ -70,8 +111,8 @@ namespace Microsoft.AspNetCore.Mvc.Formatters.Internal // Arrange var expected = new List { - new MediaTypeSegmentWithQuality(new StringSegment("application/json"),1.0), - new MediaTypeSegmentWithQuality(new StringSegment("application/xml;q=0.8"),0.8) + new MediaTypeSegmentWithQuality(new StringSegment("application/json"), 1.0), + new MediaTypeSegmentWithQuality(new StringSegment("application/xml;q=0.8"), 0.8) }; // Act diff --git a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ContentNegotiationTest.cs b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ContentNegotiationTest.cs index c94f6047de..41cd535e49 100644 --- a/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ContentNegotiationTest.cs +++ b/test/Microsoft.AspNetCore.Mvc.FunctionalTests/ContentNegotiationTest.cs @@ -85,6 +85,26 @@ namespace Microsoft.AspNetCore.Mvc.FunctionalTests Assert.Equal(expectedContentType, response.Content.Headers.ContentType); } + [Theory] + [InlineData("/;q=0.9")] + [InlineData("/;q=0.9, invalid;q=0.5;application/json;q=0.1")] + [InlineData("/invalid;q=0.9, application/json;q=0.1,invalid;q=0.5")] + [InlineData("text/html, application/json, image/jpeg, *; q=.2, */*; q=.2")] + public async Task ContentNegotiationWithPartiallyValidAcceptHeader_SkipsInvalidEntries(string acceptHeader) + { + // Arrange + var expectedContentType = MediaTypeHeaderValue.Parse("application/json;charset=utf-8"); + var request = new HttpRequestMessage(HttpMethod.Get, "http://localhost/ContentNegotiation/UserInfo_ProducesWithTypeOnly"); + request.Headers.TryAddWithoutValidation("Accept", acceptHeader); + + // Act + var response = await Client.SendAsync(request); + + // Assert + Assert.Equal(HttpStatusCode.OK, response.StatusCode); + Assert.Equal(expectedContentType, response.Content.Headers.ContentType); + } + [Fact] public async Task ProducesAttributeWithTypeOnly_RunsRegularContentNegotiation() {