From 150b4708f158c0cdcc2d49f2ec72f3e82c6c64e4 Mon Sep 17 00:00:00 2001 From: Troy Dai Date: Thu, 7 Jul 2016 10:06:30 -0700 Subject: [PATCH] Fix PathString over-encoding Base on RFC 3986, ensure following characters are not encoded alpha, digit, "-", "_", ".", "~", "@", ":", "/", "!", "$", ";", "=", "'", "(", ")", "*", "+", "," --- .../Internal/PathStringHelper.cs | 32 +++++++ .../PathString.cs | 83 ++++++++++++++++--- .../PathStringTests.cs | 16 ++++ 3 files changed, 119 insertions(+), 12 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Http.Abstractions/Internal/PathStringHelper.cs diff --git a/src/Microsoft.AspNetCore.Http.Abstractions/Internal/PathStringHelper.cs b/src/Microsoft.AspNetCore.Http.Abstractions/Internal/PathStringHelper.cs new file mode 100644 index 00000000..45b44ab1 --- /dev/null +++ b/src/Microsoft.AspNetCore.Http.Abstractions/Internal/PathStringHelper.cs @@ -0,0 +1,32 @@ +// Copyright (c) .NET Foundation. All rights reserved. +// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. + +namespace Microsoft.AspNetCore.Http.Internal +{ + internal class PathStringHelper + { + private static bool[] ValidPathChars = { + false, false, false, false, false, false, false, false, // 0x00 - 0x07 + false, false, false, false, false, false, false, false, // 0x08 - 0x0F + false, false, false, false, false, false, false, false, // 0x10 - 0x17 + false, false, false, false, false, false, false, false, // 0x18 - 0x1F + false, true, false, false, true, false, true, true, // 0x20 - 0x27 + true, true, true, true, true, true, true, true, // 0x28 - 0x2F + true, true, true, true, true, true, true, true, // 0x30 - 0x37 + true, true, true, true, false, true, false, false, // 0x38 - 0x3F + true, true, true, true, true, true, true, true, // 0x40 - 0x47 + true, true, true, true, true, true, true, true, // 0x48 - 0x4F + true, true, true, true, true, true, true, true, // 0x50 - 0x57 + true, true, true, false, false, false, false, true, // 0x58 - 0x5F + false, true, true, true, true, true, true, true, // 0x60 - 0x67 + true, true, true, true, true, true, true, true, // 0x68 - 0x6F + true, true, true, true, true, true, true, true, // 0x70 - 0x77 + true, true, true, false, false, false, true, false, // 0x78 - 0x7F + }; + + public static bool IsValidPathChar(char c) + { + return c < ValidPathChars.Length && ValidPathChars[c]; + } + } +} diff --git a/src/Microsoft.AspNetCore.Http.Abstractions/PathString.cs b/src/Microsoft.AspNetCore.Http.Abstractions/PathString.cs index 72e8d959..0b56e462 100644 --- a/src/Microsoft.AspNetCore.Http.Abstractions/PathString.cs +++ b/src/Microsoft.AspNetCore.Http.Abstractions/PathString.cs @@ -2,8 +2,9 @@ // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. using System; -using System.Text.Encodings.Web; +using System.Text; using Microsoft.AspNetCore.Http.Abstractions; +using Microsoft.AspNetCore.Http.Internal; namespace Microsoft.AspNetCore.Http { @@ -66,25 +67,83 @@ public override string ToString() /// The escaped path value public string ToUriComponent() { - // TODO: Measure the cost of this escaping and consider optimizing. if (!HasValue) { return string.Empty; } - var values = _value.Split(splitChar); - var changed = false; - for (var i = 0; i < values.Length; i++) - { - var value = values[i]; - values[i] = UrlEncoder.Default.Encode(value); - if (!changed && value != values[i]) + StringBuilder buffer = null; + + var start = 0; + var count = 0; + var requiresEscaping = false; + for (int i = 0; i < _value.Length; ++i) + { + if (PathStringHelper.IsValidPathChar(_value[i])) { - changed = true; + if (requiresEscaping) + { + // the current segment requires escape + if (buffer == null) + { + buffer = new StringBuilder(_value.Length * 3); + } + + buffer.Append(Uri.EscapeDataString(_value.Substring(start, count))); + + requiresEscaping = false; + start = i; + count = 0; + } + + count++; } + else + { + if (!requiresEscaping) + { + // the current segument doesn't require escape + if (buffer == null) + { + buffer = new StringBuilder(_value.Length * 3); + } + + buffer.Append(_value, start, count); + + requiresEscaping = true; + start = i; + count = 0; + } + + count++; + } + } + + if (count == _value.Length && !requiresEscaping) + { + return _value; } + else + { + if (count > 0) + { + if (buffer == null) + { + buffer = new StringBuilder(_value.Length * 3); + } + + if (requiresEscaping) + { + buffer.Append(Uri.EscapeDataString(_value.Substring(start, count))); + } + else + { + buffer.Append(_value, start, count); + } + } - return changed ? string.Join("/", values) : _value; + return buffer.ToString(); + } } /// @@ -335,7 +394,7 @@ public override int GetHashCode() /// Implicitly calls ToString(). /// /// - public static implicit operator string (PathString path) + public static implicit operator string(PathString path) { return path.ToString(); } diff --git a/test/Microsoft.AspNetCore.Http.Abstractions.Tests/PathStringTests.cs b/test/Microsoft.AspNetCore.Http.Abstractions.Tests/PathStringTests.cs index 6aa15216..b9019445 100644 --- a/test/Microsoft.AspNetCore.Http.Abstractions.Tests/PathStringTests.cs +++ b/test/Microsoft.AspNetCore.Http.Abstractions.Tests/PathStringTests.cs @@ -185,5 +185,21 @@ public void StartsWithSegmentsWithRemainder_DoesMatchUsingSpecifiedComparison(st Assert.Equal(expectedResult, result); } + + [Theory] + [InlineData("unreserved", "/abc123.-_~", "/abc123.-_~")] + [InlineData("colon", "/:", "/:")] + [InlineData("at", "/@", "/@")] + [InlineData("sub-delims", "/!$&'()*+,;=", "/!$&'()*+,;=")] + [InlineData("reserved", "/?#[]", "/%3F%23%5B%5D")] + [InlineData("pct-encoding", "/单行道", "/%E5%8D%95%E8%A1%8C%E9%81%93")] + [InlineData("mixed1", "/index/单行道=(x*y)[abc]", "/index/%E5%8D%95%E8%A1%8C%E9%81%93=(x*y)%5Babc%5D")] + [InlineData("mixed2", "/index/单行道=(x*y)[abc]_", "/index/%E5%8D%95%E8%A1%8C%E9%81%93=(x*y)%5Babc%5D_")] + public void ToUriComponentEscapeCorrectly(string category, string input, string expected) + { + var path = new PathString(input); + + Assert.Equal(expected, path.ToUriComponent()); + } } }