Skip to content

[6.0] Match strangely formatted PathBase #42751 #44753

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

Merged
merged 2 commits into from
Nov 10, 2022

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Oct 26, 2022

There are unusual situations where Http.Sys, IIS, and AspNetCore disagree on the format of the root path of the site.

Description

Http.Sys and IIS support path based routing of requests between sites that are sharing the same port. It applies this matching using the normalized path (e.g. decoded, dot segments removed, etc.). The problem is that Http.Sys does some non-standard normalizations that AspNetCore doesn't like collapsing consecutive slashes, converting backslashes to forward slashes, decoding %2F, etc., so when we try to separate the PathBase later it might not match and the path gets split in the wrong place, causing errors and routing mismatches up stack.

Note there's no defined behavior for most of these unusual path constructions, they are usually mistakes made by the client when constructing the url such as consecutive slashes, backslashes, etc.. AspNetCore does less auto-correction for such mistakes, but an app can usually fix these up in middleware if it wants. This bug was worse for HttpSys because the requests failed before reaching middleware due to a malformed path after an incorrect split.

I've added extra processing to the PathBase logic to emulate Http.Sys's behaviors. E.g. /base will now match //\/%2F%5Cbase like Http.Sys does. The original PathBase //\/%2F%5Cbase will still be included in the request, but the remaining Path will be properly separated now. There's one test case (not reported by any customer) I wasn't able to fix which includes consecutive slashes and dot segments, Http.Sys collapses consecutive slashes before dot segments, but we are applying this fallback after dot segments have been removed. This case gracefully falls back to reporting no PathBase.

I have not included an app context switch since the prior behavior would fail valid requests, and I make sure to apply the old standard matching first before trying the new approach.

I did not implement this additional logic on the TryMatchLongestPrefix code path that's primarily used in delegation scenarios. That code path already gracefully falls back to reporting no PathBase when it can't match.

Contributes to #42751

Customer Impact

This was reported externally first, but then a patch was requested to unblock an internal partner's .NET 6 adoption. A limited workaround is available in the issue.

Regression?

  • Yes
  • No

Risk

  • High
  • Medium
  • Low

Using PathBase's with unusual formats is rare enough that this is the first customer to notice.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

TODO once reviewed:

  • Check IIS codepath for same issue
  • Servicing template
  • .NET 7
  • Main

@Tratcher Tratcher added this to the 6.0.x milestone Oct 26, 2022
@Tratcher Tratcher self-assigned this Oct 26, 2022
@ghost
Copy link

ghost commented Oct 26, 2022

Hi @Tratcher. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

Seems reasonable, though I'm not sure if there are any weird ordering cases with the checks you're adding here

@Tratcher Tratcher force-pushed the tratcher/release/6.0/slashslash branch from 75359b8 to 90162d0 Compare November 9, 2022 21:22
@Tratcher Tratcher requested a review from HaoK November 9, 2022 21:23
@Tratcher
Copy link
Member Author

Tratcher commented Nov 9, 2022

Updated to include IIS

@Tratcher Tratcher added the Servicing-consider Shiproom approval is required for the issue label Nov 9, 2022
@ghost
Copy link

ghost commented Nov 9, 2022

Hi @Tratcher. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@Tratcher Tratcher changed the title Match strangely formatted PathBase #42751 [6.0] Match strangely formatted PathBase #42751 Nov 9, 2022
@danmoseley
Copy link
Member

@Tratcher can you please send tactics mail for this one.

@Tratcher
Copy link
Member Author

I will as soon as I get the 7.0 port ready today.

@rbhanda rbhanda added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Nov 10, 2022
@ghost
Copy link

ghost commented Nov 10, 2022

Hi @Tratcher. This PR was just approved to be included in the upcoming servicing release. Somebody from the @dotnet/aspnet-build team will get it merged when the branches are open. Until then, please make sure all the CI checks pass and the PR is reviewed.

@rbhanda rbhanda modified the milestones: 6.0.x, 6.0.12 Nov 10, 2022
@wtgodbe
Copy link
Member

wtgodbe commented Nov 10, 2022

I will as soon as I get the 7.0 port ready today.

We just came back to this in tactics so no need - should be fine to consider the 7.0 version of this approved as well

@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants