From a5ee158109d5f54e1d4a959f0911ac5485895453 Mon Sep 17 00:00:00 2001 From: Zoltan Barna Date: Wed, 16 Nov 2022 17:19:13 +0100 Subject: [PATCH 01/10] Remove supportsMultipleValues parameter from CookieHeaderParserShared.TryParseValues() method to prevent parsing strings wrong which contain separator characters. Reported by this issue: #45014 --- src/Http/Headers/test/SetCookieHeaderValueTest.cs | 4 +++- .../Http/src/Internal/RequestCookieCollection.cs | 2 +- .../Http/test/RequestCookiesCollectionTests.cs | 14 ++++++++++++++ src/Http/Shared/CookieHeaderParserShared.cs | 4 ++-- 4 files changed, 20 insertions(+), 4 deletions(-) diff --git a/src/Http/Headers/test/SetCookieHeaderValueTest.cs b/src/Http/Headers/test/SetCookieHeaderValueTest.cs index 8ab14433519b..a9fa17660d0e 100644 --- a/src/Http/Headers/test/SetCookieHeaderValueTest.cs +++ b/src/Http/Headers/test/SetCookieHeaderValueTest.cs @@ -261,10 +261,12 @@ public static TheoryData InvalidCookieValues }; var invalidString3 = "ipt={\"v\":{\"L\":3},\"pt\":{\"d:3},\"ct\":{},\"_t\":44,\"_v\":\"2\"}; domain=domain1; expires=Sun, 06 Nov 1994 08:49:37 GMT"; + var invalidString4 = "dd,:(\"sa;"; + dataset.Add(null, new[] { invalidString1 }); dataset.Add(new[] { invalidHeader2a, invalidHeader2b }.ToList(), new[] { invalidString2 }); dataset.Add(new[] { invalidHeader3 }.ToList(), new[] { invalidString3 }); - dataset.Add(new[] { header1 }.ToList(), new[] { string1, invalidString1 }); + dataset.Add(new[] { header1 }.ToList(), new[] { string1, invalidString1, invalidString4 }); dataset.Add(new[] { header1 }.ToList(), new[] { invalidString1, null, "", " ", ",", " , ", string1 }); dataset.Add(new[] { header1 }.ToList(), new[] { string1 + ", " + invalidString1 }); dataset.Add(new[] { header1 }.ToList(), new[] { invalidString1 + ", " + string1 }); diff --git a/src/Http/Http/src/Internal/RequestCookieCollection.cs b/src/Http/Http/src/Internal/RequestCookieCollection.cs index 8ed1de311c9d..22582a720ed3 100644 --- a/src/Http/Http/src/Internal/RequestCookieCollection.cs +++ b/src/Http/Http/src/Internal/RequestCookieCollection.cs @@ -69,7 +69,7 @@ public static RequestCookieCollection Parse(StringValues values) var collection = new RequestCookieCollection(); var store = collection.Store!; - if (CookieHeaderParserShared.TryParseValues(values, store, supportsMultipleValues: true)) + if (CookieHeaderParserShared.TryParseValues(values, store)) { if (store.Count == 0) { diff --git a/src/Http/Http/test/RequestCookiesCollectionTests.cs b/src/Http/Http/test/RequestCookiesCollectionTests.cs index d2648435af6d..93dd13e502cc 100644 --- a/src/Http/Http/test/RequestCookiesCollectionTests.cs +++ b/src/Http/Http/test/RequestCookiesCollectionTests.cs @@ -29,4 +29,18 @@ public void ParseManyCookies() Assert.Equal(12, cookies.Count); } + + + [Fact] + public void ParseInvalidCookies() + { + var cookies = RequestCookieCollection.Parse(new StringValues(new[] + { + "er=dd,cc,bb", + "errorcookie=dd,:(\"sa;", + "s;" + })); + + Assert.Empty(cookies); + } } diff --git a/src/Http/Shared/CookieHeaderParserShared.cs b/src/Http/Shared/CookieHeaderParserShared.cs index 2e2b3d67beac..6beb39665a1c 100644 --- a/src/Http/Shared/CookieHeaderParserShared.cs +++ b/src/Http/Shared/CookieHeaderParserShared.cs @@ -9,7 +9,7 @@ namespace Microsoft.Net.Http.Headers; internal static class CookieHeaderParserShared { - public static bool TryParseValues(StringValues values, IDictionary store, bool supportsMultipleValues) + public static bool TryParseValues(StringValues values, IDictionary store) { // If a parser returns an empty list, it means there was no value, but that's valid (e.g. "Accept: "). The caller // can ignore the value. @@ -26,7 +26,7 @@ public static bool TryParseValues(StringValues values, IDictionary Date: Thu, 17 Nov 2022 18:36:56 +0100 Subject: [PATCH 02/10] Revert "Remove supportsMultipleValues parameter from CookieHeaderParserShared.TryParseValues() method to prevent parsing strings wrong which contain separator characters. Reported by this issue: #45014" This reverts commit a5ee158109d5f54e1d4a959f0911ac5485895453. --- src/Http/Headers/test/SetCookieHeaderValueTest.cs | 4 +--- .../Http/src/Internal/RequestCookieCollection.cs | 2 +- .../Http/test/RequestCookiesCollectionTests.cs | 14 -------------- src/Http/Shared/CookieHeaderParserShared.cs | 4 ++-- 4 files changed, 4 insertions(+), 20 deletions(-) diff --git a/src/Http/Headers/test/SetCookieHeaderValueTest.cs b/src/Http/Headers/test/SetCookieHeaderValueTest.cs index a9fa17660d0e..8ab14433519b 100644 --- a/src/Http/Headers/test/SetCookieHeaderValueTest.cs +++ b/src/Http/Headers/test/SetCookieHeaderValueTest.cs @@ -261,12 +261,10 @@ public static TheoryData InvalidCookieValues }; var invalidString3 = "ipt={\"v\":{\"L\":3},\"pt\":{\"d:3},\"ct\":{},\"_t\":44,\"_v\":\"2\"}; domain=domain1; expires=Sun, 06 Nov 1994 08:49:37 GMT"; - var invalidString4 = "dd,:(\"sa;"; - dataset.Add(null, new[] { invalidString1 }); dataset.Add(new[] { invalidHeader2a, invalidHeader2b }.ToList(), new[] { invalidString2 }); dataset.Add(new[] { invalidHeader3 }.ToList(), new[] { invalidString3 }); - dataset.Add(new[] { header1 }.ToList(), new[] { string1, invalidString1, invalidString4 }); + dataset.Add(new[] { header1 }.ToList(), new[] { string1, invalidString1 }); dataset.Add(new[] { header1 }.ToList(), new[] { invalidString1, null, "", " ", ",", " , ", string1 }); dataset.Add(new[] { header1 }.ToList(), new[] { string1 + ", " + invalidString1 }); dataset.Add(new[] { header1 }.ToList(), new[] { invalidString1 + ", " + string1 }); diff --git a/src/Http/Http/src/Internal/RequestCookieCollection.cs b/src/Http/Http/src/Internal/RequestCookieCollection.cs index 22582a720ed3..8ed1de311c9d 100644 --- a/src/Http/Http/src/Internal/RequestCookieCollection.cs +++ b/src/Http/Http/src/Internal/RequestCookieCollection.cs @@ -69,7 +69,7 @@ public static RequestCookieCollection Parse(StringValues values) var collection = new RequestCookieCollection(); var store = collection.Store!; - if (CookieHeaderParserShared.TryParseValues(values, store)) + if (CookieHeaderParserShared.TryParseValues(values, store, supportsMultipleValues: true)) { if (store.Count == 0) { diff --git a/src/Http/Http/test/RequestCookiesCollectionTests.cs b/src/Http/Http/test/RequestCookiesCollectionTests.cs index 93dd13e502cc..d2648435af6d 100644 --- a/src/Http/Http/test/RequestCookiesCollectionTests.cs +++ b/src/Http/Http/test/RequestCookiesCollectionTests.cs @@ -29,18 +29,4 @@ public void ParseManyCookies() Assert.Equal(12, cookies.Count); } - - - [Fact] - public void ParseInvalidCookies() - { - var cookies = RequestCookieCollection.Parse(new StringValues(new[] - { - "er=dd,cc,bb", - "errorcookie=dd,:(\"sa;", - "s;" - })); - - Assert.Empty(cookies); - } } diff --git a/src/Http/Shared/CookieHeaderParserShared.cs b/src/Http/Shared/CookieHeaderParserShared.cs index 6beb39665a1c..2e2b3d67beac 100644 --- a/src/Http/Shared/CookieHeaderParserShared.cs +++ b/src/Http/Shared/CookieHeaderParserShared.cs @@ -9,7 +9,7 @@ namespace Microsoft.Net.Http.Headers; internal static class CookieHeaderParserShared { - public static bool TryParseValues(StringValues values, IDictionary store) + public static bool TryParseValues(StringValues values, IDictionary store, bool supportsMultipleValues) { // If a parser returns an empty list, it means there was no value, but that's valid (e.g. "Accept: "). The caller // can ignore the value. @@ -26,7 +26,7 @@ public static bool TryParseValues(StringValues values, IDictionary Date: Thu, 17 Nov 2022 19:03:23 +0100 Subject: [PATCH 03/10] Ignore cookie separators at the end of the string --- .../test/RequestCookiesCollectionTests.cs | 32 +++++++++++++++++++ src/Http/Shared/CookieHeaderParserShared.cs | 7 ++++ 2 files changed, 39 insertions(+) diff --git a/src/Http/Http/test/RequestCookiesCollectionTests.cs b/src/Http/Http/test/RequestCookiesCollectionTests.cs index d2648435af6d..e0599584fe28 100644 --- a/src/Http/Http/test/RequestCookiesCollectionTests.cs +++ b/src/Http/Http/test/RequestCookiesCollectionTests.cs @@ -29,4 +29,36 @@ public void ParseManyCookies() Assert.Equal(12, cookies.Count); } + + [Theory] + [InlineData("er=dd,cc,bb", new[] { "dd" })] + [InlineData("er=dd,err=cc,errr=bb", new[] { "dd", "cc", "bb" })] + [InlineData("errorcookie=dd,:(\"sa;", new[] { "dd" })] + [InlineData("s;", null)] + public void ParseInvalidCookies(string cookieToParse, string[] expectedCookieValues) + { + //var cookies = RequestCookieCollection.Parse(new StringValues(new[] + //{ + // "er=dd,cc,bb", + // "er=dd,err=cc,errr=bb", + // "errorcookie=dd,:(\"sa;", + // "s;" + //})); + + var cookies = RequestCookieCollection.Parse(new StringValues(new[] { cookieToParse })); + + if(expectedCookieValues == null) + { + Assert.Equal(0, cookies.Count); + return; + } + + Assert.Equal(expectedCookieValues.Length, cookies.Count); + + for (int i = 0; i < expectedCookieValues.Length; i++) + { + var value = expectedCookieValues[i]; + Assert.Equal(value, cookies.ElementAt(i).Value); + } + } } diff --git a/src/Http/Shared/CookieHeaderParserShared.cs b/src/Http/Shared/CookieHeaderParserShared.cs index 2e2b3d67beac..b1056f0bfac8 100644 --- a/src/Http/Shared/CookieHeaderParserShared.cs +++ b/src/Http/Shared/CookieHeaderParserShared.cs @@ -107,6 +107,13 @@ private static int GetNextNonEmptyOrWhitespaceIndex(StringSegment input, int sta // If we have a separator, skip the separator and all following whitespaces. If we support // empty values, continue until the current character is neither a separator nor a whitespace. separatorFound = true; + + // We accept single separator, but separator at the end of a value is ignored. + if (input.Length > 1 && current + 1 == input.Length) + { + return current; + } + current++; // skip delimiter. current = current + HttpRuleParser.GetWhitespaceLength(input, current); From 1e2860bd2197c6b271264f8e5833ec373e0a0283 Mon Sep 17 00:00:00 2001 From: Zoltan Barna Date: Thu, 17 Nov 2022 19:04:25 +0100 Subject: [PATCH 04/10] Remove comments from ParseInvalidCookies test --- src/Http/Http/test/RequestCookiesCollectionTests.cs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/Http/Http/test/RequestCookiesCollectionTests.cs b/src/Http/Http/test/RequestCookiesCollectionTests.cs index e0599584fe28..21a3ae17b9ca 100644 --- a/src/Http/Http/test/RequestCookiesCollectionTests.cs +++ b/src/Http/Http/test/RequestCookiesCollectionTests.cs @@ -37,14 +37,6 @@ public void ParseManyCookies() [InlineData("s;", null)] public void ParseInvalidCookies(string cookieToParse, string[] expectedCookieValues) { - //var cookies = RequestCookieCollection.Parse(new StringValues(new[] - //{ - // "er=dd,cc,bb", - // "er=dd,err=cc,errr=bb", - // "errorcookie=dd,:(\"sa;", - // "s;" - //})); - var cookies = RequestCookieCollection.Parse(new StringValues(new[] { cookieToParse })); if(expectedCookieValues == null) @@ -54,7 +46,6 @@ public void ParseInvalidCookies(string cookieToParse, string[] expectedCookieVal } Assert.Equal(expectedCookieValues.Length, cookies.Count); - for (int i = 0; i < expectedCookieValues.Length; i++) { var value = expectedCookieValues[i]; From 1457ddc805cd98e8f236e3b3b6efb4dac98008bc Mon Sep 17 00:00:00 2001 From: Zoltan Barna Date: Mon, 28 Nov 2022 18:57:19 +0100 Subject: [PATCH 05/10] Filter out single separators from CookieHeaderParsing --- src/Http/Http/test/RequestCookiesCollectionTests.cs | 2 ++ src/Http/Shared/CookieHeaderParserShared.cs | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Http/Http/test/RequestCookiesCollectionTests.cs b/src/Http/Http/test/RequestCookiesCollectionTests.cs index 21a3ae17b9ca..fa3fb6d67f74 100644 --- a/src/Http/Http/test/RequestCookiesCollectionTests.cs +++ b/src/Http/Http/test/RequestCookiesCollectionTests.cs @@ -31,6 +31,8 @@ public void ParseManyCookies() } [Theory] + [InlineData(",", null)] + [InlineData(";", null)] [InlineData("er=dd,cc,bb", new[] { "dd" })] [InlineData("er=dd,err=cc,errr=bb", new[] { "dd", "cc", "bb" })] [InlineData("errorcookie=dd,:(\"sa;", new[] { "dd" })] diff --git a/src/Http/Shared/CookieHeaderParserShared.cs b/src/Http/Shared/CookieHeaderParserShared.cs index b1056f0bfac8..8985714f0615 100644 --- a/src/Http/Shared/CookieHeaderParserShared.cs +++ b/src/Http/Shared/CookieHeaderParserShared.cs @@ -109,7 +109,7 @@ private static int GetNextNonEmptyOrWhitespaceIndex(StringSegment input, int sta separatorFound = true; // We accept single separator, but separator at the end of a value is ignored. - if (input.Length > 1 && current + 1 == input.Length) + if (input.Length >= 1 && current + 1 == input.Length) { return current; } From cf14c8cba1ef5550ab2abebc5fbdfd2883cec33d Mon Sep 17 00:00:00 2001 From: Zoltan Barna Date: Mon, 28 Nov 2022 21:22:29 +0100 Subject: [PATCH 06/10] Guard against null values in case of parsed separator --- src/Http/Shared/CookieHeaderParserShared.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/Http/Shared/CookieHeaderParserShared.cs b/src/Http/Shared/CookieHeaderParserShared.cs index 8985714f0615..0b2b3cd13fe6 100644 --- a/src/Http/Shared/CookieHeaderParserShared.cs +++ b/src/Http/Shared/CookieHeaderParserShared.cs @@ -28,8 +28,15 @@ public static bool TryParseValues(StringValues values, IDictionary= 1 && current + 1 == input.Length) + //We accept single separator, but separator at the end of a value is ignored. + if (input.Length > 1 && current + 1 == input.Length) { return current; } From 462b4e178374a347ba89c14e7037aa9c03170bac Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Barna=20Zolt=C3=A1n?= Date: Thu, 1 Dec 2022 06:18:12 +0100 Subject: [PATCH 07/10] Remove additional logic from CookieHeaderParserShared.cs Co-authored-by: Chris Ross --- src/Http/Shared/CookieHeaderParserShared.cs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/Http/Shared/CookieHeaderParserShared.cs b/src/Http/Shared/CookieHeaderParserShared.cs index 0b2b3cd13fe6..7c188a1cbf99 100644 --- a/src/Http/Shared/CookieHeaderParserShared.cs +++ b/src/Http/Shared/CookieHeaderParserShared.cs @@ -114,13 +114,6 @@ private static int GetNextNonEmptyOrWhitespaceIndex(StringSegment input, int sta // If we have a separator, skip the separator and all following whitespaces. If we support // empty values, continue until the current character is neither a separator nor a whitespace. separatorFound = true; - - //We accept single separator, but separator at the end of a value is ignored. - if (input.Length > 1 && current + 1 == input.Length) - { - return current; - } - current++; // skip delimiter. current = current + HttpRuleParser.GetWhitespaceLength(input, current); From 11d5dee15ee5ec9a5c882c7c960733b1015d9355 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Barna=20Zolt=C3=A1n?= Date: Thu, 1 Dec 2022 06:19:39 +0100 Subject: [PATCH 08/10] Apply suggested change to TryParseValues in CookieHeaderParserShared Co-authored-by: Chris Ross --- src/Http/Shared/CookieHeaderParserShared.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/Shared/CookieHeaderParserShared.cs b/src/Http/Shared/CookieHeaderParserShared.cs index 7c188a1cbf99..ce71f420648a 100644 --- a/src/Http/Shared/CookieHeaderParserShared.cs +++ b/src/Http/Shared/CookieHeaderParserShared.cs @@ -28,7 +28,7 @@ public static bool TryParseValues(StringValues values, IDictionary Date: Thu, 1 Dec 2022 06:08:33 -0800 Subject: [PATCH 09/10] Update src/Http/Shared/CookieHeaderParserShared.cs --- src/Http/Shared/CookieHeaderParserShared.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Http/Shared/CookieHeaderParserShared.cs b/src/Http/Shared/CookieHeaderParserShared.cs index ce71f420648a..485db30ada1a 100644 --- a/src/Http/Shared/CookieHeaderParserShared.cs +++ b/src/Http/Shared/CookieHeaderParserShared.cs @@ -28,8 +28,8 @@ public static bool TryParseValues(StringValues values, IDictionary Date: Thu, 1 Dec 2022 09:30:38 -0800 Subject: [PATCH 10/10] Update src/Http/Shared/CookieHeaderParserShared.cs --- src/Http/Shared/CookieHeaderParserShared.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Http/Shared/CookieHeaderParserShared.cs b/src/Http/Shared/CookieHeaderParserShared.cs index 485db30ada1a..e558ec1e4dc4 100644 --- a/src/Http/Shared/CookieHeaderParserShared.cs +++ b/src/Http/Shared/CookieHeaderParserShared.cs @@ -36,7 +36,7 @@ public static bool TryParseValues(StringValues values, IDictionary