search

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

Handle redirect http consistently with MVC RequireHttpsAttribute #210

Closed javiercn closed 6 years ago

javiercn commented 7 years ago

Per discussion with @Tratcher, the url rewrite middleware supports rewriting urls for functionality similar to require https. We should handle the scenario in a consistent way no matter what option (url rewrite or require https) we decide to use.

Tratcher commented 7 years ago

Specifically do not redirect POST or similar requests with bodies. MVC sends a 403 Forbidden response for these requests.

glennc commented 7 years ago

@Tratcher Can I get more details on what the differences between these two are?

Tratcher commented 7 years ago

Redirecting for POST requests was the big inconstancy we noticed. MVC won't redirect a POST request, it returns 403 instead. Rewrite does redirect, and this can cause clients to convert the POST to a GET.

muratg commented 7 years ago

From the triage room, @Tratcher thinks using status code 307 or 308 could be another way to fix this issue.

muratg commented 7 years ago

@Tratcher will investigate some simple things we can do in 2.1. This would technically be a breaking change, but it may be something that we can accept because of the already broken scenarios.

If simple fixes are not applicable, we'll backlog.

javiercn commented 7 years ago

@muratg @Tratcher Do we really need to do anything here? Given our plans for HTTPS in 2.1 I would avoid doing anything here. You either/or the new HTTPS stuff or the RedirectToHttps functionality. I don't think we need to support anything here, specially at the cost of a breaking change. I would like to see RedirectToHttpsAttribute vanish from existence in the future as much as posible.

Tratcher commented 7 years ago

@javiercn the main question is how correct is the implementation in this repo is. RedirectToHttpsAttribute is merely an interesting reference. 301/302 is a problem for POST requests.

javiercn commented 7 years ago

@Tratcher I understand 301/302 is a problem for POST requests, but who in real life sends 307/308 as a redirect. I'm ok if the redirect only works for get requests. I think it's better to 400 post requests as you don't want that to silently work (given that the information has already been posted through an insecure channel).

Tratcher commented 7 years ago

I don't know yet, that's why this marked as Investigate.

Tratcher commented 6 years ago

307 does look like the correct default. I've verified it works with Chrome, IE, Edge, HttpClient Core (Win) and .NET, and Curl (Win). I'll send a PR.

javiercn commented 6 years ago

@Tratcher Are you planning to 307 by default on POST requests?

Tratcher commented 6 years ago

On all requests.