Double percent encoding of forward slashes (%2F) in paths

[DanielFischer79]

Describe the bug

we use yarp in front of an nexus server which hosting node packages. Node packages urls can contains %2f instead of slash as part of the path. yarp encode the percent again to %25 so the resulting path was wrongly modifed.

To Reproduce

GET https://nexus.domain.com/repository/npm-proxy/@types%2fcors

YARP convert the url to, but its wrong: https://nexus.domain.com/repository/npm-proxy/@types%252fcors

YARP should not modify the path by default. If there is a special use case for duing that, it should be done by a feature flag

Further technical details

We tested it with new version 1.0.0 / .net 5.0

We analysed the issue in detail and found the reason for this issue: # By fixing the issue #1219 a major change was done with path encoding. The char '%' was not longer a valid path character for yarp. (Class RequestUtilities, Method: IsValidPathChar)

We temporary fix the issue by creating a custom request transformer and set the uri like RequestUtilities did it before.

[Tratcher]

There's a fundamental issue in aspnetcore with %2F and %252F that makes it impossible for YARP to correctly round trip both values (https://github.com/dotnet/aspnetcore/issues/30655). #1219 defaulted to the safer of the two misunderstandings since double decoding %252F could be dangerous. I don't think we'll be able to sort this out properly until .NET 7.

[karelz]

Triage: We depend on ASP.NET issue https://github.com/dotnet/aspnetcore/issues/30655 to be fixed first. Then we may have to react. Moving to Backlog for now.

[mcrio]

Not sure if I may be doing something differently than @DanielFischer79 but I am observing the following:

• Original path: /path/value%2F123
• Yarp transforms path to: /path/value/123

In my case Yarp seems to be url-decoding the path.

Yarp 1.0.0 on Kestrel

EDIT: actually it seems like Daniel said it does encode it to /path/value%252F123. Then I assume the destination application which is an ASP core (Kestrel) application decodes that internally to /path/value/123 :/

[mcrio]

Conclusion for my case. It is the combination of YARP > DAPR > ASP CORE that converts /path/value%2F123 to /path/value/123

Steps:

• Original path: /path/value%2F123
• Yarp requests path as DAPR method invoke (PathPrefix transform): /v1.0/invoke/myservice/method/path/value%252F123
• Dapr seems to be doing some path magic...
• ASP core incoming path: /path/value/123

Dapr seems to be doing some magic as well because when I make a request to ASP Core with path /path/value%252F123 it resolves it to /path/value%2F123... while when making the request through Dapr it resolves to /path/value/123

[karelz]

Triage: Depends solely on ASP.NET changes, no YARP work -- see https://github.com/microsoft/reverse-proxy/issues/1419#issuecomment-984871819, will be (hopefully) part of 7.0.

[Schoch85]

It tried to use YARP as a ReverseProxy for GitLab, but it didn't worked because of the %252F problem. I tried everything to circumvent the problem, but with no success. In the end I modified the YARP function "IsValidPathChar" in the class "RequestUtilities" as described below. I think this problem should be addressed as soon as possible, because it prevents the usage of YARP for different important applications.

[MethodImpl(MethodImplOptions.AggressiveInlining)]
    internal static bool IsValidPathChar(char c)
    {
        // Add this if condition to avoid encoding %
        if (c == 37)
            return true;

        // Use local array and uint .Length compare to elide the bounds check on array access
        var validChars = ValidPathChars;
        var i = (int)c;

        // Array is in chunks of 32 bits, so get offset by dividing by 32
        var offset = i >> 5; // i / 32;
        // Significant bit position is the remainder of the above calc; i % 32 => i & 31
        var significantBit = 1u << (i & 31);

        // Check offset in bounds and check if significant bit set
        return (uint)offset < (uint)validChars.Length &&
            ((validChars[offset] & significantBit) != 0);
    }

[giotab]

It tried to use YARP as a ReverseProxy for GitLab, but it didn't worked because of the %252F problem. I tried everything to circumvent the problem, but with no success. In the end I modified the YARP function "IsValidPathChar" in the class "RequestUtilities" as described below. I think this problem should be addressed as soon as possible, because it prevents the usage of YARP for different important applications.

[MethodImpl(MethodImplOptions.AggressiveInlining)]
    internal static bool IsValidPathChar(char c)
    {
        // Add this if condition to avoid encoding %
        if (c == 37)
            return true;

        // Use local array and uint .Length compare to elide the bounds check on array access
        var validChars = ValidPathChars;
        var i = (int)c;

        // Array is in chunks of 32 bits, so get offset by dividing by 32
        var offset = i >> 5; // i / 32;
        // Significant bit position is the remainder of the above calc; i % 32 => i & 31
        var significantBit = 1u << (i & 31);

        // Check offset in bounds and check if significant bit set
        return (uint)offset < (uint)validChars.Length &&
            ((validChars[offset] & significantBit) != 0);
    }

Can you elaborate where did you insert this code, and how did you use it? Did you encounter any other issues in GitLab after introducing this "fix"?

[Schoch85]

I changed my approach to not affect other routes.

So what I did is I changed the method "MakeDestinationAddress" in the class "RequestUtilities" as follows:

public static Uri MakeDestinationAddress(string destinationPrefix, PathString path, QueryString query)
{
    ReadOnlySpan<char> prefixSpan = destinationPrefix;

    if (path.HasValue && destinationPrefix.EndsWith('/'))
    {
        // When PathString has a value it always starts with a '/'. Avoid double slashes when concatenating.
        prefixSpan = prefixSpan[0..^1];
    }

    // Do not encode Path's for GitLab
    string targetAddress;
    if (String.Compare(destinationPrefix, "https://gitlab.example.ch", StringComparison.OrdinalIgnoreCase) != 0)
    {
        targetAddress = string.Concat(prefixSpan, EncodePath(path), query.ToUriComponent());
    }
    else
    {
        targetAddress = string.Concat(prefixSpan, path.Value!, query.ToUriComponent());
    }

    return new Uri(targetAddress, UriKind.Absolute);
}

So the destinationPrefix is hardcoded. It is not a nice solution, but it works for GitLab, no problems anymore occurred.

But in my opinion it should be possible for every route to set a flag if we want to encode the path or not.

[Schoch85]

I forgot to mention, when I started to use the source code of YARP instead the Nuget package I got the following error message sometimes, independent from GitLab:

"The request cannot be forwarded, the response has already started"

So what I did it commented out the following lines in the "HttpForwarder" class:

    //if (RequestUtilities.IsResponseSet(context.Response))
    //{
    //    throw new InvalidOperationException("The request cannot be forwarded, the response has already started");
    //}