Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider having a configuration setting to allow YARP use raw target for the forwarded requests #2702

Open
IlyaBiryukov opened this issue Jan 3, 2025 · 5 comments
Labels
Type: Idea This issue is a high-level idea for discussion.

Comments

@IlyaBiryukov
Copy link

Consider having a configuration setting to allow YARP use raw target for the forwarded request path, query and fragment.

When YARP runs as middleware in ASP.NET, it gets request path & query after they have been unescaped from the original raw target. This makes forwarded request path & query not matching the original ones. Escaping may be lost or different.

It was reported by codespaces in dev tunnels in microsoft/dev-tunnels#492

The following may be a workaround:

  1. Add a custom request transform that extracts raw target from IHttpRequestFeature.RawTarget property on the incoming request and saves it to ProxyRequest.Options like this:
new RequestFuncTransform((transformContext) =>
    {
        var requestFeature = transformContext.HttpContext.Features.Get<IHttpRequestFeature>();
        if (requestFeature != null &&
            !string.IsNullOrEmpty(requestFeature.RawTarget))
        {
            transformContext.ProxyRequest.Options.Set(OptionsKey, requestFeature.RawTarget);
        }

        return ValueTask.CompletedTask;
    });

// Where OptionsKey is declared like this:
public static HttpRequestOptionsKey<string> OptionsKey { get; } =
    new HttpRequestOptionsKey<string>(nameof(IHttpRequestFeature.RawTarget));
  1. Add a custom DelegatingHandler to HTTP message handler chain that gets raw target from request.Options and updates the RequestUri like this:
public sealed class RawTargetHttpMessageHandler : DelegatingHandler
{
    protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request, CancellationToken cancellationToken)
    {
        if (request.Options.TryGetValue(OptionsKey, out string? rawTarget))
        {
            request.RequestUri = ApplyRawTargetToRequestUri(request.RequestUri, rawTarget);
        }

        return base.SendAsync(request, cancellationToken);
    }
}

// Where ApplyRawTargetToRequestUri is something like this:
public static Uri? ApplyRawTargetToRequestUri(Uri? requestUri, string? rawTarget)
    {
        if (requestUri == null || string.IsNullOrEmpty(rawTarget))
        {
            return requestUri;
        }

        var uriBuilder = new UriBuilder(requestUri);
        var queryIndex = rawTarget.IndexOf('?');
        if (queryIndex >= 0)
        {
            uriBuilder.Path = rawTarget.Substring(0, queryIndex);
            var fragmentIndex = rawTarget.IndexOf('#', queryIndex);
            if (fragmentIndex >= 0)
            {
                uriBuilder.Fragment = rawTarget.Substring(fragmentIndex);
                uriBuilder.Query = rawTarget.Substring(queryIndex, fragmentIndex - queryIndex);
            }
            else
            {
                uriBuilder.Query = rawTarget.Substring(queryIndex);
            }
        }
        else
        {
            var fragmentIndex = rawTarget.IndexOf('#');
            if (fragmentIndex >= 0)
            {
                uriBuilder.Fragment = rawTarget.Substring(fragmentIndex);
                uriBuilder.Path = rawTarget.Substring(0, fragmentIndex);
            }
            else
            {
                uriBuilder.Path = rawTarget;
            }
        }

        return uriBuilder.Uri;
    }
  1. In YARP forwarder's SendAsync use HttpMessageInvoker with this handler.
@IlyaBiryukov IlyaBiryukov added the Type: Idea This issue is a high-level idea for discussion. label Jan 3, 2025
@MihaZupan
Copy link
Member

I think the underlying issue you're hitting is #1419.
Besides this case (escaped forward slash), did you see other problematic cases?

Re: workaround, you shouldn't need the transform/handler split, you can set the Uri in the transform directly.

@IlyaBiryukov
Copy link
Author

In the workaround I did try setting transformContext.Path to raw target path, but that still causes unescaping. Setting transformContext.ProxyRequest.RequestUri is also not an option because RequestUri is not initialized yet, and the transform doesn't have enough context to set full request Uri. Anyway, I'd rather YARP support this out-of-the-box :)

@MihaZupan
Copy link
Member

MihaZupan commented Jan 6, 2025

If you ensure that your transform is the last one in the list you have all the context, the Uri can be constructed in the transform like so:

context.ProxyRequest.RequestUri = RequestUtilities.MakeDestinationAddress(context.DestinationPrefix, context.Path, context.Query.QueryString);

followed by any modifications you wish to make. The code snippet is exactly the same thing that YARP would later do for you if RequestUri hasn't been set yet.
Note that you likely don't have to deal with splitting of things like fragment since those aren't sent over the wire anyway (only the path and query are).

Besides this case (escaped forward slash), did you see other problematic cases?

@IlyaBiryukov
Copy link
Author

Thanks for tip on request uri transform!

Besides this case (escaped forward slash), did you see other problematic cases?

Well, in general, unescaping loses fidelity of forwarded requests. E.g. if a customer wanted to test how their server deals with escaping while using a dev tunnel to forward the requests, they'd want to get the request path as is, with all %xx unescaped.

Arguably, it's a niche use case.

@MihaZupan
Copy link
Member

There are some URL transformations YARP (or rather, System.Uri) will perform. For example if your URL contains escaped characters where that escaping is pointless, we'll unescape it for your (e.g. %41 will be transformed into A).
In the vast majority of cases that doesn't matter, because clients normally don't generate such requests in the first place.

The escaped slash bug is unfortunate, but it will hopefully be fixed at some point. If there are other problematic cases where YARP is modifying things unnecessarily we would of course want to look into that. If they come up we could consider adding options to control YARP behavior, but I'm not aware of any at the moment.

For cases where you need absolute control over exactly what YARP sends out on the other side, we've exposed a UriCreationOptions.DangerousDisablePathAndQueryCanonicalization API. As the name suggests, it's unsafe, and you need to know exactly what you're doing, but it's an escape hatch if you need different behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Idea This issue is a high-level idea for discussion.
Projects
None yet
Development

No branches or pull requests

2 participants