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

Looping Redirect #236

Closed casaba-dan closed 6 years ago

casaba-dan commented 7 years ago

Title

Looping redirect

Functional impact

This bug causes a user to be redirected in a loop until the redirect request reaches the max URL size.

Minimal repro steps

  1. Enable ReWriting in your server
  2. Add a redirect rule /path/one/segment1 --> path/two/segment1
  3. Send a request with the url /path/one/://a
  4. Follow the redirects until error condition is reached.
  5. Observe the long path with path/://a prepended repeatedly.

Expected result

/path/two/://a

Actual result

The request is continually redirected with the path base prepended repeatedly.

jkotalik commented 7 years ago

The bug is that we do IndexOf("://" .. ) to differentiate between the string containing the http:// or not. Specifically here in the redirect. https://github.com/aspnet/BasicMiddleware/blob/dev/src/Microsoft.AspNetCore.Rewrite/Internal/RedirectRule.cs#L65

@Tratcher @mikaelm12 What would be the right plan of action here? May need a more advanced check.

mikaelm12 commented 7 years ago

There's already a pr out for this 😄 https://github.com/aspnet/BasicMiddleware/pull/238 But yeah. We needed to make it a "starts with" check

But @Tratcher Should it also check for ws:// and wss:// ?

Tratcher commented 7 years ago

No, ws:// and wss:// are only ever used on the client side. They aren't used on servers. ws translates to an http handshake on the server.

rjmooney commented 7 years ago

I'm not able to reproduce this. I've sent an e-mail to @casaba-dan asking for clarification.

muratg commented 7 years ago

@rjmooney did you guys find out what this was about? Is there an actual bug still?

rjmooney commented 7 years ago

@muratg I wasn't able to reproduce the issue. I tried numerous permutations of the redirect rules based on what I read here and conversations with @casaba-dan. Based on my understanding, testing, and code review, I didn't see a way for this to trigger. (Dan has since left the company.)

muratg commented 7 years ago

@rjmooney OK, thanks for the update!

muratg commented 6 years ago

:+1: Closing after a (almost) a year.