aspnet / BasicMiddleware

[Archived] Basic middleware components for ASP.NET Core. Project moved to https://github.com/aspnet/AspNetCore
Apache License 2.0
169 stars 84 forks source link

Add method to RewriteOptions to modify www to non-www and vice-versa. #194

Closed jkotalik closed 6 years ago

jkotalik commented 7 years ago

Seems like a use case that should be inherently supported. Though most rewriters do not support regex match on host name, if we can support it with a method, that'd be great!

muratg commented 7 years ago

Sounds like a good feature! We'll assess this in the future based on the customer feedback.

muratg commented 7 years ago

http://stackoverflow.com/questions/41205435/asp-net-core-1-1-url-rewriting-www-to-non-www

guardrex commented 7 years ago

An option for matching on the entire URL would be nice here, including scheme, port, and query.

HarelM commented 7 years ago

See also #220, rewrite entire URL including parameters. This is essential in order to be able to serve facebook crawler the right details when using SPA. I have copy-paste the code from the rewrite rule on my site but I don't think it's the right approach. the following is the rule I would like to remove from my web.config: Current code can't do it.

<rewrite>           
   <rules>            
     <rule name="opengraph" patternSyntax="ECMAScript" stopProcessing="true">   
       <match url=".*" />                 
       <action type="Rewrite" url="/api/opengraph/{C:1}" logRewrittenUrl="true" />            
       <conditions logicalGrouping="MatchAny" trackAllCaptures="true">               
         <add input="{QUERY_STRING}" pattern=".*escaped_fragment_=%2F%3Fs%3D(.*)" />                  
       </conditions>                 
     </rule>             
   </rules>       
</rewrite>
HarelM commented 7 years ago

Any updates on this? Is there a plan to support this request?

jkotalik commented 7 years ago

@muratg Could this be something to add in 2.1?

HarelM commented 7 years ago

Any updates on this?

alvipeo commented 7 years ago

Yeah, it's been a while...

HarelM commented 7 years ago

Pinging... I still have a piece of code I would like to get rid of and am waiting for this to be solved so remove it.

muratg commented 7 years ago

@glennc Do you think this is something we'd consider in 2.1?

@HarelM @alvipeo -- would one of you guys be interested in sending a PR?

HarelM commented 7 years ago

sure, any guidelines? Tests locating? Preferred implementation method?

jkotalik commented 7 years ago

I'd say we can do something similar to what we did for Redirection to Https: https://github.com/aspnet/BasicMiddleware/blob/dev/src/Microsoft.AspNetCore.Rewrite/Internal/RedirectToHttpsRule.cs We would need to add tests and doc comments too. Thanks for the help!

HarelM commented 7 years ago

I couldn't find unit tests to RewriteRule did I miss anything? Also the same fix should be applied to both RedirectRule right? I'm hoping to solve #196, #193 and #220 on the same pull request...

HarelM commented 7 years ago

Well, I tried... downloaded the project, ran ./build.cmd /t:restore and got it to compile outside visual studio. Couldn't make it compile from visual studio. Added a test to simulate the issue, it failed and I don't know how to debug it... Sorry guys, too complex for the time I have to invest in this. I don't want to send a pull request without a test that demonstrate the issue I want to fix. The changes I wanted to add are relatively small but I'm not sure they cover everything. Here they are:

    public class RewriteRule : IRule
    {
    ...
        public virtual void ApplyRule(RewriteContext context)
        {
            var pathWithQuery = context.HttpContext.Request.Path + context.HttpContext.Request.QueryString;
            var initMatchResults = InitialMatch.Match(pathWithQuery == PathString.Empty
                ? pathWithQuery
                : pathWithQuery.Substring(1));

instead of what was formally written:

        public virtual void ApplyRule(RewriteContext context)
        {
            var path = context.HttpContext.Request.Path;
            Match initMatchResults;
            if (path == PathString.Empty)
            {
                initMatchResults = InitialMatch.Match(path.ToString());
            }
            else
            {
                initMatchResults = InitialMatch.Match(path.ToString().Substring(1));
            }

Maybe the pathWithQuery should also contain host and basebase for this issue as well, same as the example of RedirectToHttpsRule. This probably needs to be applied to RedirectRule and not just Rewrite. Sorry. Hope you'll decide to fix it without my PR...

jkotalik commented 7 years ago

Thanks for the starting point. Considering the feedback, I think we should add this options for 2.1. Are you using the nightly bits of AspNetCore? If not, then you wouldn't see these changes until 2.1-preview anyways. @muratg I can put this in my backlog for 2.1?

muratg commented 7 years ago

@HarelM, thank you for giving it a shot!

@jkotalik Assigned to you in 2.1, but this is lower priority than the other items in your queue, I think.

alexsorokoletov commented 6 years ago

I'd love to have that! Thank you!

muratg commented 6 years ago

Backlogging this, but we'll accept contributions.

jkotalik commented 6 years ago

@muratg this was already done by an external contributor with #297

muratg commented 6 years ago

Duh, right.

alexsorokoletov commented 6 years ago

Out of curiosity, the BasicMiddleware repo does not contain any information on how do I install latest version of that repo from the NuGet. Maybe it's just me don't knowing to which NuGet this repo corresponds.

Readme is almost blank

muratg commented 6 years ago

@alexsorokoletov an RTM build with this has not been released yet. If you're interested in using unsupported nightly builds, you can use our myget feed. Detailed instructions are here: https://github.com/aspnet/home/#builds

alexsorokoletov commented 6 years ago

Thank you @muratg. I would appreciate if that info (builds) and mapping to packages was included somewhere in README. When do you plan to release the RTM?

muratg commented 6 years ago

@alexsorokoletov RTM dates are still TBD.

Re README, there's a link to home repo's root (README file). We can look into changing it to look more discoverable if you have any recommendations. For mapping the package names, I recommend checking the source folder structure under src folder.

Thank you for your feedback!

HarelM commented 6 years ago

I'm not sure this issue can be closed since as far as I can understand it groups the following issues:

196, #193, #220

Which were closed in favor of this issue, am I missing something? non-www to www is just one aspect of missing rewrite/redirect...

muratg commented 6 years ago

@HarelM Could you file another issue with your proposal? Thanks.

HarelM commented 6 years ago

Already did, see #220. Feel free to reopen it.

HarelM commented 6 years ago

@muratg Thanks for taking the time and reviewing my comments, please also consider opening #193 and #196 as they too were closed in favor of this issue.