Skip to content
This repository was archived by the owner on Nov 20, 2018. It is now read-only.

Fix PathString over-encoding #667

Merged
merged 1 commit into from
Jul 8, 2016
Merged

Fix PathString over-encoding #667

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions src/Microsoft.AspNetCore.Http.Abstractions/Internal/PathStringHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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];
}
}
}
83 changes: 71 additions & 12 deletions src/Microsoft.AspNetCore.Http.Abstractions/PathString.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
{
Expand Down Expand Up @@ -66,25 +67,83 @@ public override string ToString()
/// <returns>The escaped path value</returns>
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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who usually calls this method and how expensive is this change? Looks like we never measured it..


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
Copy link
Member

@Tratcher Tratcher Jul 7, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

else if count > 0?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually doesn't matter, if count == 0 the following substring won't generate result. In both case the buffer needs to be returned. To save a needless append operation, I'll change it to:

  • count > 0 => append remaining unescaped chars, and then return buffer.
  • count == 0 (else) => return buffer.

{
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();
}
}

/// <summary>
Expand Down Expand Up @@ -335,7 +394,7 @@ public static implicit operator PathString(string s)
/// Implicitly calls ToString().
/// </summary>
/// <param name="path"></param>
public static implicit operator string (PathString path)
public static implicit operator string(PathString path)
{
return path.ToString();
}
Expand Down
16 changes: 16 additions & 0 deletions test/Microsoft.AspNetCore.Http.Abstractions.Tests/PathStringTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
}
}