Skip to content

Fix behavior of catch-all routes in 3.1 #21114

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

Closed
rynowak opened this issue Apr 22, 2020 · 4 comments · Fixed by #21200
Closed

Fix behavior of catch-all routes in 3.1 #21114

rynowak opened this issue Apr 22, 2020 · 4 comments · Fixed by #21200
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares
Milestone

Comments

@rynowak
Copy link
Member

rynowak commented Apr 22, 2020

Describe the bug

A number of routing features related to catch-all routes don't behave correctly in 3.1 due to a bug in how the routing DFA is built. This was fixed for 5.0 as part of #20801

See: #18677 and #16579 for examples of cases that are broken. Since this is a bug in how the DFA is built, there are many many cases that can reproduce this bug, but they generally involved a catch all route.

To Reproduce

See: #18677

How do I know if I'm impacted by this bug?

  • Do you have a catch-all route? {**foo} or similar
  • Is your catch all route failing to match requests it should match?
  • Does removing other routes make your catch-all start working?

If you answered yes to these questions, you are affected by this bug.

There are no known workarounds for this issue other than using a combination of routes that does not trigger the problem.

If it works for your scenario you could try configuring the order of a catch all route to come after other routes (higher numeric value of order). This may not be a good fit for your use case, and may not work if you have multiple routes.

@rynowak
Copy link
Member Author

rynowak commented Apr 22, 2020

This is a risky bug for us to take for 3.1 because many users could have deployed applications that work despite the bug. Any time we make a change it carries some risk, and especially if it goes from one non-exceptional behavior to another.

There are definitely cases where fixing this bug will result in a different endpoint being matched as a result of the change.

As a mitigating factor, catch all routes are usually used as a slug route or a route of least resort, and are inherently use when you want a route with lower priority than others. This means that an application using a catch-all route is more likely to see an endpoint matched instead of a 404 than they are to see an endpoint matched instead of a different endpoint.

An example:

/{controller=Home}/{action=Index}/{id?} - order: 0
/{**slug} - order: 1
/SomeOtherThing - order: 2

In this case today a request to /SomeOtherThing will match the 3rd route - this is the broken behavior - it should match the second route. If we fix this bug it will start to match the second route.

This is clearly a change in behavior from one working state to another working state. Someone who understands routing's behavior would have known that the 3.1 behavior is a bug, but I'm sure plenty of capable users might have not noticed, or deployed anyway if it did what they need in the cases they tried.


We need to make a decision about whether to fix this by default in a patch, and make the fix out-out via a switch, or to make the fix opt-in via a switch.

We know that OData is broken by this in some cases, as well as proxy and anyone using lots of slug routes.

@rynowak
Copy link
Member Author

rynowak commented Apr 30, 2020

The decision from ship room was to make this opt-in with an AppContext switch. So the fix looks like:

AppContext.SetSwitch("Microsoft.AspNetCore.Routing.UseCorrectCatchAllBehavior", true);

Put this in Program.Main as the first statement.

@Rick-Anderson
Copy link
Contributor

@rynowak see Feature request for 5.0: Run analyzers in debug mode #18791
The analyzer could flag catchall routes and and have a fwlink pointing to the problem/solution.

@Pilchie Pilchie modified the milestones: 3.1.x, 3.1.5 May 4, 2020
@mkArtakMSFT
Copy link
Contributor

Closing per @rynowak 's comment, as the follow up here is in docs, which is now tracked separately.

@ghost ghost locked as resolved and limited conversation to collaborators Jun 17, 2020
@amcasey amcasey added area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares and removed area-runtime labels Jun 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-middleware Includes: URL rewrite, redirect, response cache/compression, session, and other general middlewares
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants