Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Commit 61c968d

Browse files
authored
Ensure System.Uri consistently uses the correct RFC 3986 reserved character set (#26006)
Resolve an issue with System.Uri where certain percent encoded characters that should be left as-is are decoded, but only when the URI also contains Unicode or an encoded non-reserved symbol. This issue was left over after the URI reserved character set was upgraded from RFC 2396 to RFC 3986. In several places the upgrade was missed, resulting in certain code paths still using the old reserved character set. This fix updates places where the RFC 2396 set was still being used, and adds tests to ensure this behavior is consistent. It also retains the code used for quirking in .Net Framework, but always enables the new behavior instead of checking an AppContextSwitch. This fix has been ported from .NET Framework.
1 parent 5c00c63 commit 61c968d

File tree

8 files changed

+243
-50
lines changed

8 files changed

+243
-50
lines changed

src/System.Private.Uri/src/System/IriHelper.cs

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -95,11 +95,12 @@ internal static bool CheckIsReserved(char ch, UriComponents component)
9595
{
9696
return (component == (UriComponents)0) ? UriHelper.IsGenDelim(ch) : false;
9797
}
98-
else
98+
else if (UriParser.DontEnableStrictRFC3986ReservedCharacterSets)
9999
{
100+
// Since we aren't enabling strict RFC 3986 reserved sets, we stick with the old behavior
101+
// (for app-compat) which was a broken mix of RFCs 2396 and 3986.
100102
switch (component)
101103
{
102-
// Reserved chars according to RFC 3987
103104
case UriComponents.UserInfo:
104105
if (ch == '/' || ch == '?' || ch == '#' || ch == '[' || ch == ']' || ch == '@')
105106
return true;
@@ -125,6 +126,10 @@ internal static bool CheckIsReserved(char ch, UriComponents component)
125126
}
126127
return false;
127128
}
129+
else
130+
{
131+
return (UriHelper.RFC3986ReservedMarks.IndexOf(ch) >= 0);
132+
}
128133
}
129134

130135
//

src/System.Private.Uri/src/System/UriHelper.cs

Lines changed: 29 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -657,29 +657,44 @@ internal static char EscapedAscii(char digit, char next)
657657
+ 10)));
658658
}
659659

660-
// Do not unescape these in safe mode:
661-
// 1) reserved = ";" | "/" | "?" | ":" | "@" | "&" | "=" | "+" | "$" | ","
662-
// 2) excluded = control | "#" | "%" | "\"
660+
internal const string RFC3986ReservedMarks = @";/?:@&=+$,#[]!'()*";
661+
private const string RFC2396ReservedMarks = @";/?:@&=+$,";
662+
private const string RFC3986UnreservedMarks = @"-_.~";
663+
private const string RFC2396UnreservedMarks = @"-_.~*'()!";
664+
private const string AdditionalUnsafeToUnescape = @"%\#";// While not specified as reserved, these are still unsafe to unescape.
665+
666+
// When unescaping in safe mode, do not unescape the RFC 3986 reserved set:
667+
// gen-delims = ":" / "/" / "?" / "#" / "[" / "]" / "@"
668+
// sub-delims = "!" / "$" / "&" / "'" / "(" / ")"
669+
// / "*" / "+" / "," / ";" / "="
663670
//
664-
// That will still give plenty characters unescaped by SafeUnesced mode such as
665-
// 1) Unicode characters
666-
// 2) Unreserved = alphanum | "-" | "_" | "." | "!" | "~" | "*" | "'" | "(" | ")"
667-
// 3) DelimitersAndUnwise = "<" | ">" | <"> | "{" | "}" | "|" | "^" | "[" | "]" | "`"
671+
// In addition, do not unescape the following unsafe characters:
672+
// excluded = "%" / "\"
673+
//
674+
// This implementation used to use the following variant of the RFC 2396 reserved set.
675+
// That behavior is now disabled by default, and is controlled by a UriSyntax property.
676+
// reserved = ";" | "/" | "?" | "@" | "&" | "=" | "+" | "$" | ","
677+
// excluded = control | "#" | "%" | "\"
668678
internal static bool IsNotSafeForUnescape(char ch)
669679
{
670680
if (ch <= '\x1F' || (ch >= '\x7F' && ch <= '\x9F'))
681+
{
671682
return true;
672-
else if ((ch >= ';' && ch <= '@' && (ch | '\x2') != '>') ||
673-
(ch >= '#' && ch <= '&') ||
674-
ch == '+' || ch == ',' || ch == '/' || ch == '\\')
683+
}
684+
else if (UriParser.DontEnableStrictRFC3986ReservedCharacterSets)
685+
{
686+
if ((ch != ':' && (RFC2396ReservedMarks.IndexOf(ch) >= 0) || (AdditionalUnsafeToUnescape.IndexOf(ch) >= 0)))
687+
{
688+
return true;
689+
}
690+
}
691+
else if ((RFC3986ReservedMarks.IndexOf(ch) >= 0) || (AdditionalUnsafeToUnescape.IndexOf(ch) >= 0))
692+
{
675693
return true;
676-
694+
}
677695
return false;
678696
}
679697

680-
private const string RFC3986ReservedMarks = @":/?#[]@!$&'()*+,;=";
681-
private const string RFC3986UnreservedMarks = @"-._~";
682-
683698
private static unsafe bool IsReservedUnreservedOrHash(char c)
684699
{
685700
if (IsUnreserved(c))

src/System.Private.Uri/src/System/UriSyntax.cs

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -100,6 +100,15 @@ public abstract partial class UriParser
100100

101101
internal static UriParser VsMacrosUri;
102102

103+
internal static bool DontEnableStrictRFC3986ReservedCharacterSets
104+
{
105+
// In .NET Framework this would test against an AppContextSwitch. Since this is a potentially
106+
// breaking change, we'll leave in the system used to disable it.
107+
get
108+
{
109+
return false;
110+
}
111+
}
103112

104113
static UriParser()
105114
{
Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
// See the LICENSE file in the project root for more information.
4+
5+
using System.Reflection;
6+
using System.Text;
7+
8+
using Xunit;
9+
10+
namespace System.PrivateUri.Tests
11+
{
12+
public class IriEncodingDecodingTest
13+
{
14+
// This array contains potentially problematic URI data strings and their canonical encoding.
15+
private static string[,] RFC3986CompliantDecoding =
16+
{
17+
{ "%3B%2F%3F%3A%40%26%3D%2B%24%2C", "%3B%2F%3F%3A%40%26%3D%2B%24%2C" }, // Encoded RFC 2396 Reserved Marks.
18+
{ @"-_.~", @"-_.~" }, // RFC3986 Unreserved Marks.
19+
{ "%2D%5F%2E%7E", @"-_.~" }, // Encoded RFC 3986 Unreserved Marks.
20+
{ "%2F%3F%3A%40%23%5B%5D", "%2F%3F%3A%40%23%5B%5D" }, // Encoded RFC 3986 Gen Delims.
21+
{ @";&=+$,!'()*", @";&=+$,!'()*" }, // RFC 3986 Sub Delims.
22+
{ "%3B%26%3D%2B%24%2C%21%27%28%29%2A", "%3B%26%3D%2B%24%2C%21%27%28%29%2A" } // Encoded RFC3986 Sub Delims.
23+
};
24+
25+
[Fact]
26+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Requires fix shipping in .NET 4.7.2")]
27+
public void AbsoluteUri_DangerousPathSymbols_RFC3986CompliantAbsoluteUri()
28+
{
29+
Uri uri;
30+
string baseUri = "http://a/%C3%88/";
31+
for (int i = 0; i < RFC3986CompliantDecoding.GetLength(0); i++)
32+
{
33+
string sourceStr = baseUri + RFC3986CompliantDecoding[i, 0];
34+
uri = new Uri(sourceStr);
35+
Assert.Equal(baseUri + RFC3986CompliantDecoding[i, 1], uri.AbsoluteUri);
36+
}
37+
}
38+
39+
[Fact]
40+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Requires fix shipping in .NET 4.7.2")]
41+
public void AbsolutePath_DangerousPathSymbols_RFC3986CompliantAbsolutePath()
42+
{
43+
Uri uri;
44+
string host = "http://a";
45+
string path = "/%C3%88/";
46+
for (int i = 0; i < RFC3986CompliantDecoding.GetLength(0); i++)
47+
{
48+
string sourceStr = host + path + RFC3986CompliantDecoding[i, 0];
49+
uri = new Uri(sourceStr);
50+
Assert.Equal(path + RFC3986CompliantDecoding[i, 1], uri.AbsolutePath);
51+
}
52+
}
53+
54+
[Fact]
55+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Requires fix shipping in .NET 4.7.2")]
56+
public void Segments_DangerousPathSymbols_RFC3986CompliantPathSegments()
57+
{
58+
Uri uri;
59+
string host = "http://a";
60+
string path = "/%C3%88/";
61+
for (int i = 0; i < RFC3986CompliantDecoding.GetLength(0); i++)
62+
{
63+
string sourceStr = host + path + RFC3986CompliantDecoding[i, 0];
64+
uri = new Uri(sourceStr);
65+
Assert.Equal(path + RFC3986CompliantDecoding[i, 1], String.Join(String.Empty, uri.Segments));
66+
}
67+
}
68+
69+
[Fact]
70+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Requires fix shipping in .NET 4.7.2")]
71+
public void Equality_DangerousPathSymbols_RFC3986CompliantEquality()
72+
{
73+
string baseUri = "http://a/%C3%88/";
74+
for (int i = 0; i < RFC3986CompliantDecoding.GetLength(0); i++)
75+
{
76+
Uri uri1 = new Uri(baseUri + RFC3986CompliantDecoding[i, 0]);
77+
Uri uri2 = new Uri(baseUri + RFC3986CompliantDecoding[i, 1]);
78+
Assert.Equal(uri1, uri2);
79+
}
80+
}
81+
82+
[Fact]
83+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Requires fix shipping in .NET 4.7.2")]
84+
public void PathAndQuery_DangerousQuerySymbols_RFC3986CompliantPathAndQuery()
85+
{
86+
Uri uri;
87+
string host = "http://a";
88+
string path = "/%C3%88/";
89+
for (int i = 0; i < RFC3986CompliantDecoding.GetLength(0); i++)
90+
{
91+
string sourceStr = host + path + "?" + RFC3986CompliantDecoding[i, 0];
92+
uri = new Uri(sourceStr);
93+
Assert.Equal(path + "?" + RFC3986CompliantDecoding[i, 1], uri.PathAndQuery);
94+
}
95+
}
96+
97+
[Fact]
98+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Requires fix shipping in .NET 4.7.2")]
99+
public void Query_DangerousQuerySymbols_RFC3986CompliantQuery()
100+
{
101+
Uri uri;
102+
string baseUri = "http://a/%C3%88/";
103+
for (int i = 0; i < RFC3986CompliantDecoding.GetLength(0); i++)
104+
{
105+
string sourceStr = baseUri + "?" + RFC3986CompliantDecoding[i, 0];
106+
uri = new Uri(sourceStr);
107+
Assert.Equal("?" + RFC3986CompliantDecoding[i, 1], uri.Query);
108+
}
109+
}
110+
111+
[Fact]
112+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Requires fix shipping in .NET 4.7.2")]
113+
public void Equality_DangerousQuerySymbols_RFC3986CompliantEquality()
114+
{
115+
string baseUri = "http://a/%C3%88/";
116+
for (int i = 0; i < RFC3986CompliantDecoding.GetLength(0); i++)
117+
{
118+
Uri uriTest = new Uri(baseUri + "?" + RFC3986CompliantDecoding[i, 0]);
119+
Uri uriTest1 = new Uri(baseUri + "?" + RFC3986CompliantDecoding[i, 1]);
120+
Assert.Equal(uriTest, uriTest1);
121+
}
122+
}
123+
124+
[Fact]
125+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Requires fix shipping in .NET 4.7.2")]
126+
public void Fragment_DangerousFragmentSymbols_RFC3986CompliantFragment()
127+
{
128+
Uri uri;
129+
string baseUri = "http://a/%C3%88/";
130+
for (int i = 0; i < RFC3986CompliantDecoding.GetLength(0); i++)
131+
{
132+
string sourceStr = baseUri + "#" + RFC3986CompliantDecoding[i, 0];
133+
uri = new Uri(sourceStr);
134+
Assert.Equal("#" + RFC3986CompliantDecoding[i, 1], uri.Fragment);
135+
}
136+
}
137+
138+
[Fact]
139+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Requires fix shipping in .NET 4.7.2")]
140+
public void UserInfo_DangerousUserInfoSymbols_RFC3986CompliantUserInfo()
141+
{
142+
Uri uri;
143+
string baseUri = "http://";
144+
string host = "a";
145+
string path = "/%C3%88/";
146+
for (int i = 0; i < RFC3986CompliantDecoding.GetLength(0); i++)
147+
{
148+
string sourceStr = baseUri + RFC3986CompliantDecoding[i, 0] + "@" + host + path;
149+
uri = new Uri(sourceStr);
150+
Assert.Equal(RFC3986CompliantDecoding[i, 1], uri.UserInfo);
151+
}
152+
}
153+
}
154+
}

src/System.Private.Uri/tests/FunctionalTests/IriTest.cs

Lines changed: 28 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -403,54 +403,55 @@ private string EscapeUnescapeTestComponent(string uriInput, UriComponents compon
403403

404404
/// <summary>
405405
/// First column contains input characters found to be potential issues with the current implementation.
406-
/// The second column contains the current (.Net 4.5.2) Uri behavior for Uri normalization.
406+
/// The second column contains the current (.Net Core 2.1/Framework 4.7.2) Uri behavior for Uri normalization.
407407
/// </summary>
408408
private static string[,] s_checkIsReservedEscapingStrings =
409409
{
410-
// : [ ] in host
410+
// : [ ] in host.
411411
{"http://user@ser%3Aver.srv:123/path/path/resource.ext?query=expression#fragment", null},
412412
{"http://user@server.srv%3A999/path/path/resource.ext?query=expression#fragment", null},
413413
{"http://user@server.%5Bsrv:123/path/path/resource.ext?query=expression#fragment", null},
414414
{"http://user@ser%5Dver.srv:123/path/path/resource.ext?query=expression#fragment", null},
415415

416-
// [ ] in userinfo
417-
{"http://us%5Ber@server.srv:123/path/path/resource.ext?query=expression#fragment",
416+
// [ ] in userinfo.
417+
{"http://us%5Ber@server.srv:123/path/path/resource.ext?query=expression#fragment",
418418
"http://us%5Ber@server.srv:123/path/path/resource.ext?query=expression#fragment"},
419-
{"http://u%5Dser@server.srv:123/path/path/resource.ext?query=expression#fragment",
419+
{"http://u%5Dser@server.srv:123/path/path/resource.ext?query=expression#fragment",
420420
"http://u%5Dser@server.srv:123/path/path/resource.ext?query=expression#fragment"},
421-
{"http://us%5B%5Der@server.srv:123/path/path/resource.ext?query=expression#fragment",
421+
{"http://us%5B%5Der@server.srv:123/path/path/resource.ext?query=expression#fragment",
422422
"http://us%5B%5Der@server.srv:123/path/path/resource.ext?query=expression#fragment"},
423423

424-
// [ ] in path
425-
{"http://user@server.srv:123/path/pa%5Bth/resource.ext?query=expression#fragment",
426-
"http://user@server.srv:123/path/pa[th/resource.ext?query=expression#fragment"},
427-
{"http://user@server.srv:123/pa%5Dth/path%5D/resource.ext?query=expression#fragment",
428-
"http://user@server.srv:123/pa]th/path]/resource.ext?query=expression#fragment"},
429-
{"http://user@server.srv:123/path/p%5Ba%5Dth/resource.ext?query=expression#fragment",
430-
"http://user@server.srv:123/path/p[a]th/resource.ext?query=expression#fragment"},
424+
// [ ] : ' in path.
425+
{"http://user@server.srv:123/path/pa%5B%3A%27th/resource.ext?query=expression#fragment",
426+
"http://user@server.srv:123/path/pa%5B%3A%27th/resource.ext?query=expression#fragment"},
427+
{"http://user@server.srv:123/pa%5D%3A%27th/path%5D%3A%27/resource.ext?query=expression#fragment",
428+
"http://user@server.srv:123/pa%5D%3A%27th/path%5D%3A%27/resource.ext?query=expression#fragment"},
429+
{"http://user@server.srv:123/path/p%5B%3A%27a%5D%3A%27th/resource.ext?query=expression#fragment",
430+
"http://user@server.srv:123/path/p%5B%3A%27a%5D%3A%27th/resource.ext?query=expression#fragment"},
431431

432-
// [ ] in query
433-
{"http://user@server.srv:123/path/path/resource.ext?que%5Bry=expression#fragment",
434-
"http://user@server.srv:123/path/path/resource.ext?que[ry=expression#fragment"},
435-
{"http://user@server.srv:123/path/path/resource.ext?query=exp%5Dression#fragment",
436-
"http://user@server.srv:123/path/path/resource.ext?query=exp]ression#fragment"},
437-
{"http://user@server.srv:123/path/path/resource.ext?que%5Bry=exp%5Dression#fragment",
438-
"http://user@server.srv:123/path/path/resource.ext?que[ry=exp]ression#fragment"},
432+
// [ ] : ' in query.
433+
{"http://user@server.srv:123/path/path/resource.ext?que%5B%3A%27ry=expression#fragment",
434+
"http://user@server.srv:123/path/path/resource.ext?que%5B%3A%27ry=expression#fragment"},
435+
{"http://user@server.srv:123/path/path/resource.ext?query=exp%5D%3A%27ression#fragment",
436+
"http://user@server.srv:123/path/path/resource.ext?query=exp%5D%3A%27ression#fragment"},
437+
{"http://user@server.srv:123/path/path/resource.ext?que%5B%3A%27ry=exp%5D%3A%27ression#fragment",
438+
"http://user@server.srv:123/path/path/resource.ext?que%5B%3A%27ry=exp%5D%3A%27ression#fragment"},
439439

440-
// [ ] in fragment
441-
{"http://user@server.srv:123/path/path/resource.ext?query=expression#fr%5Bagment",
442-
"http://user@server.srv:123/path/path/resource.ext?query=expression#fr[agment"},
443-
{"http://user@server.srv:123/path/path/resource.ext?query=expression#fragment%5D",
444-
"http://user@server.srv:123/path/path/resource.ext?query=expression#fragment]"},
445-
{"http://user@server.srv:123/path/path/resource.ext?query=expression#fr%5Bagment%5D",
446-
"http://user@server.srv:123/path/path/resource.ext?query=expression#fr[agment]"}
440+
// [ ] : ' in fragment.
441+
{"http://user@server.srv:123/path/path/resource.ext?query=expression#fr%5B%3A%27agment",
442+
"http://user@server.srv:123/path/path/resource.ext?query=expression#fr%5B%3A%27agment"},
443+
{"http://user@server.srv:123/path/path/resource.ext?query=expression#fragment%5D%3A%27",
444+
"http://user@server.srv:123/path/path/resource.ext?query=expression#fragment%5D%3A%27"},
445+
{"http://user@server.srv:123/path/path/resource.ext?query=expression#fr%5B%3A%27agment%5D%3A%27",
446+
"http://user@server.srv:123/path/path/resource.ext?query=expression#fr%5B%3A%27agment%5D%3A%27"}
447447
};
448448

449449
/// <summary>
450450
/// This test validates behavior differences caused by the incorrect conditional expression found in function
451451
/// CheckIsReserved().
452452
/// </summary>
453453
[Fact]
454+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Requires fix shipping in .NET 4.7.2")]
454455
public void Iri_CheckIsReserved_EscapingBehavior()
455456
{
456457
for (int i = 0; i < s_checkIsReservedEscapingStrings.GetLength(0); i++)

src/System.Private.Uri/tests/FunctionalTests/System.Private.Uri.Functional.Tests.csproj

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
<Compile Include="IdnCheckHostNameTest.cs" />
1212
<Compile Include="IdnDnsSafeHostTest.cs" />
1313
<Compile Include="IdnHostNameValidationTest.cs" />
14+
<Compile Include="IriEncodingDecodingTests.cs" />
1415
<Compile Include="IriRelativeFileResolutionTest.cs" />
1516
<Compile Include="IriTest.cs" />
1617
<Compile Include="UriBuilderParameterTest.cs" />

src/System.Private.Uri/tests/FunctionalTests/UriEscapingTest.cs

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -321,13 +321,14 @@ public void UriAbsoluteEscaping_RFC2396Unreserved_NoEscaping()
321321
}
322322

323323
[Fact]
324-
public void UriAbsoluteUnEscaping_RFC2396UnreservedEscaped_AllUnescaped()
324+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Requires fix shipping in .NET 4.7.2")]
325+
public void UriAbsoluteUnEscaping_RFC3986UnreservedEscaped_AllUnescaped()
325326
{
326-
string escaped = Escape(RFC2396Unreserved);
327+
string escaped = Escape(RFC3986Unreserved);
327328
string input = "http://" + AlphaNumeric.ToLowerInvariant() + "/" + escaped
328329
+ "?" + escaped + "#" + escaped;
329-
string expectedOutput = "http://" + AlphaNumeric.ToLowerInvariant() + "/" + RFC2396Unreserved
330-
+ "?" + RFC2396Unreserved + "#" + RFC2396Unreserved;
330+
string expectedOutput = "http://" + AlphaNumeric.ToLowerInvariant() + "/" + RFC3986Unreserved
331+
+ "?" + RFC3986Unreserved + "#" + RFC3986Unreserved;
331332

332333
Uri testUri = new Uri(input);
333334
Assert.Equal(expectedOutput, testUri.AbsoluteUri);
@@ -398,12 +399,12 @@ public void UriAbsoluteEscaping_FullIPv6Uri_NothingEscaped()
398399
}
399400

400401
[Fact]
402+
[SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Requires fix shipping in .NET 4.7.2")]
401403
public void UriAbsoluteEscaping_SurrogatePair_LocaleIndependent()
402404
{
403405
string uriString = "http://contosotest.conto.soco.ntosoco.com/surrgtest()?$filter=";
404-
string expectedString = uriString + "%E6%95%B0%E6%8D%AE%20eq%20" +
405-
"'%F0%A0%80%80%F0%A0%80%81%F0%A0%80%82%F0%A0%80%83%F0%AA%9B%91%F0%AA%9B" +
406-
"%92%F0%AA%9B%93%F0%AA%9B%94%F0%AA%9B%95%F0%AA%9B%96'";
406+
string expectedString = uriString + "%E6%95%B0%E6%8D%AE%20eq%20%27%F0%A0%80%80%F0%A0%80%81%F0%A0%80%82%F0%A0%80%83%F0" +
407+
"%AA%9B%91%F0%AA%9B%92%F0%AA%9B%93%F0%AA%9B%94%F0%AA%9B%95%F0%AA%9B%96%27";
407408

408409
using (ThreadCultureChange iriHelper = new ThreadCultureChange())
409410
{

0 commit comments

Comments
 (0)