Support for X-Forwarded-Prefix in ForwardedHeadersMiddleware #49249

andwi · 2023-07-06T19:05:38Z

Support for X-Forwarded-Prefix in ForwardedHeadersMiddleware

You've read the Contributor Guide and Code of Conduct.
You've included unit or integration tests for your change, where applicable.
You've included inline docs for your change, where applicable.
There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

Adds the option to change PathBase based on the X-Forwarded-Prefix header.

Fixes #23263

ghost · 2023-07-06T19:05:47Z

Thanks for your PR, @andwi. Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

andwi · 2023-07-06T19:15:00Z

@dotnet-policy-service agree

src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs

adityamandaleeka · 2023-07-07T21:20:10Z

Thanks for the contribution @andwi! Since this change affects public API, it will need to go through the API review process (but in this case, the API change is so small and follows the existing pattern so it will be super easy!).

To get that kicked off, can you please add a comment on the issue (#23263) with the proposed API change, following the template here? https://github.com/dotnet/aspnetcore/issues/new?assignees=&labels=api-suggestion&template=30_api_proposal.md&title=

Tag me once you've done that and we can mark the issue as being ready for us to review during the next API review session.

BTW the CI failure was unrelated (message is below and I hadn't seen this one before... seems like something went wrong when building the installers @wtgodbe?) so I reran the failing leg.

src\Installers\Windows\WindowsHostingBundle\SharedFramework.wxs(39,0): error LGHT0103: (NETCORE_ENGINEERING_TELEMETRY=Build) The system cannot find the file 'D:\a\_work\1\s\artifacts\installers\Release\\aspnetcore-runtime-8.0.0-ci-win-x64.exe'

wtgodbe · 2023-07-07T21:26:53Z

BTW the CI failure was unrelated (message is below and I hadn't seen this one before... seems like something went wrong when building the installers @wtgodbe?) so I reran the failing leg.

Looks like an ordering issue - I don't think I've seen that one before, I'll keep an eye out for it in future builds

andwi · 2023-07-08T06:58:47Z

Updated the logic for setting the X-Original-Prefix header to make sure empty string is replaced with /

ghost · 2023-07-21T15:01:02Z

Looks like this PR hasn't been active for some time and the codebase could have been changed in the meantime.
To make sure no conflicting changes have occurred, please rerun validation before merging. You can do this by leaving an /azp run comment here (requires commit rights), or by simply closing and reopening.

adityamandaleeka · 2023-07-21T17:05:31Z

Still waiting on API review for this one.

Tratcher · 2023-07-24T20:38:55Z

src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs

+                requestHeaders[_options.OriginalPrefixHeaderName] =
+                    request.PathBase.HasValue ? request.PathBase.ToString() : "/";


"/" and "" are not equivalent paths, especially for PathBase. PathBase is commonly empty. I understand that's awkward to represent in this collection. It might be better to skip adding the original value when empty.

Changed so that X-Original-Prefix is only added if PathBase has value

Tratcher · 2023-07-24T20:41:03Z

src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs

+                    requestHeaders.Remove(_options.ForwardedPrefixHeaderName);
+                }
+
+                request.PathBase = PathString.FromUriComponent(currentValues.Prefix.TrimEnd('/'));


TrimEnd shouldn't be needed, pass the given value through. PathString has built in logic to avoid duplicate slashes when combined with another PathString (PathBase + Path).

aspnetcore/src/Http/Http.Abstractions/src/PathString.cs

Lines 318 to 327 in a02b94e

public PathString Add(PathString other)

{

if (HasValue &&

other.HasValue &&

Value[^1] == '/')

{

// If the path string has a trailing slash and the other string has a leading slash, we need

// to trim one of them.

var combined = string.Concat(Value.AsSpan(), other.Value.AsSpan(1));

return new PathString(combined);

Tratcher · 2023-07-24T20:43:01Z

src/Middleware/HttpOverrides/test/ForwardedHeadersMiddlewareTest.cs

@@ -1116,4 +1121,137 @@ public async Task ForwardersWithDirectOptionsRunsTwice(int limit, string header,
        Assert.Equal(expectedScheme, context.Request.Scheme);
        Assert.Equal(remainingHeader, context.Request.Headers["X-Forwarded-Proto"].ToString());
    }
+
+    [Theory]
+    [InlineData("", "/foo", "/foo", "/")]


Add /foo/bar as a multi-segment test case.

Added multi-segment test case

adityamandaleeka · 2023-07-25T20:36:09Z

@andwi The API has been approved #23263 (comment)

@Tratcher Looks like your comments have been addressed. Any more feedback or is this ready for merge?

davidfowl · 2023-07-26T01:33:49Z

src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs

+                if (forwardedPrefix!.Length > entriesConsumed)
+                {
+                    // Truncate the consumed header values
+                    requestHeaders[_options.ForwardedPrefixHeaderName] = forwardedPrefix.Take(forwardedPrefix.Length - entriesConsumed).ToArray();


Why is this using LINQ? Let's aim to avoid all temporary allocations.

Why is this using LINQ?

Just followed the existing pattern in the class.

Have pushed new commits that change all the cases where header values are truncated to use a new helper method that uses Array.Copy instead of LINQ.

amcasey · 2023-08-01T17:36:15Z

Looks like API review signed off: #23263. Good to go?

adityamandaleeka · 2023-08-01T21:01:02Z

Thanks @andwi

X-Forwarded-Prefix

6f07b50

andwi requested review from a team, Tratcher and BrennanConroy as code owners July 6, 2023 19:05

ghost added area-runtime community-contribution Indicates that the PR has been added by a community member labels Jul 6, 2023

PublicAPI.Unshipped.txt

eaff1af

mitchdenny reviewed Jul 7, 2023

View reviewed changes

src/Middleware/HttpOverrides/src/ForwardedHeadersMiddleware.cs Show resolved Hide resolved

XForwardedPrefixParameterCountMismatch test

0c9e0e1

mkArtakMSFT assigned adityamandaleeka Jul 7, 2023

andwi mentioned this pull request Jul 8, 2023

Support for x-forwarded-prefix #23263

Closed

andwi added 3 commits July 8, 2023 06:23

Update test XForwardedPrefixInvalidPath

87ce3a7

Set X-Original-Prefix to / instead of empty string

9a77df8

Test that X-Original-Prefix is escaped

cf93819

ghost added the pending-ci-rerun When assigned to a PR indicates that the CI checks should be rerun label Jul 21, 2023

Tratcher reviewed Jul 24, 2023

View reviewed changes

Revert slash behaviour

2705132

davidfowl reviewed Jul 26, 2023

View reviewed changes

andwi added 3 commits July 26, 2023 07:32

XForwardedPrefixTruncateConsumedValues test

a069b53

Use Array.Copy to truncate consumed prefixes

3550f14

Use TruncateConsumedHeaderValues for all cases

9c9948c

Tratcher approved these changes Jul 26, 2023

View reviewed changes

adityamandaleeka approved these changes Aug 1, 2023

View reviewed changes

adityamandaleeka merged commit dc3d37f into dotnet:main Aug 1, 2023

ghost added this to the 8.0-rc1 milestone Aug 1, 2023

amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Aug 25, 2023

Tratcher mentioned this pull request Oct 26, 2023

app.UsePathBase(pathBase) add support to read pathbase from request header to handle multitenant application #51669

Closed

1 task

Algorithman mentioned this pull request Feb 6, 2024

How to run Smartstore in a subfolder? smartstore/Smartstore#998

Closed

joegoldman2 mentioned this pull request Apr 11, 2024

Add doc about X-Forwarded-Prefix/X-Original-Prefix dotnet/AspNetCore.Docs#32295

Merged

Rick-Anderson mentioned this pull request Apr 15, 2024

Update Add doc about X-Forwarded-Prefix/X-Original-Prefix dotnet/AspNetCore.Docs#32316

Closed

		requestHeaders[_options.OriginalPrefixHeaderName] =
		request.PathBase.HasValue ? request.PathBase.ToString() : "/";

	public PathString Add(PathString other)
	{
	if (HasValue &&
	other.HasValue &&
	Value[^1] == '/')
	{
	// If the path string has a trailing slash and the other string has a leading slash, we need
	// to trim one of them.
	var combined = string.Concat(Value.AsSpan(), other.Value.AsSpan(1));
	return new PathString(combined);

Support for X-Forwarded-Prefix in ForwardedHeadersMiddleware #49249

Support for X-Forwarded-Prefix in ForwardedHeadersMiddleware #49249

Uh oh!

Conversation

andwi commented Jul 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Support for X-Forwarded-Prefix in ForwardedHeadersMiddleware

Uh oh!

ghost commented Jul 6, 2023

Uh oh!

andwi commented Jul 6, 2023

Uh oh!

Uh oh!

adityamandaleeka commented Jul 7, 2023

Uh oh!

wtgodbe commented Jul 7, 2023

Uh oh!

andwi commented Jul 8, 2023

Uh oh!

ghost commented Jul 21, 2023

Uh oh!

adityamandaleeka commented Jul 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adityamandaleeka commented Jul 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amcasey commented Aug 1, 2023

Uh oh!

adityamandaleeka commented Aug 1, 2023

Uh oh!

Uh oh!

andwi commented Jul 6, 2023 •

edited

Loading