Ignore everything but path and query in requests for absolute URIs (#666). #711
Conversation
| if (requestUrlPath.Length > 0 && requestUrlPath[0] != '/') | ||
| { | ||
| Uri uri; | ||
| if (Uri.TryCreate(requestUrlPath, UriKind.Absolute, out uri)) |
There was a problem hiding this comment.
Not optimal but I don't see the need to optimize for this case - requests for absolute URIs are not expected to be the norm. So most of the time we won't be running this.
There was a problem hiding this comment.
System.Uri can't be used, it does lots of its own escaping and un-escaping. I think your three interesting cases are:
- Starts with / if it's a path
- * for OPTIONS
- starts with http:// or https:// if its absolute, skip to the first / or ? after the scheme separator
There was a problem hiding this comment.
Ok. I'll address OPTIONS * in a subsequent PR.
| @@ -804,8 +804,43 @@ protected bool TakeStartLine(SocketInput input) | |||
| QueryString = queryString; | |||
| HttpVersion = httpVersion; | |||
|
|
|||
| bool caseMatches; | |||
| if (requestUrlPath.Length > 0 && requestUrlPath[0] != '/') | |||
There was a problem hiding this comment.
Again, this is an edge case and I don't see the need to optimize this path.
There was a problem hiding this comment.
what happens for requestUrlPath.Lenght == 0? That shouldn't be valid either.
There was a problem hiding this comment.
Add a comment and RFC link for the scenario you're handling, and the fact that you're ignoring the scheme and host at the moment.
|
The URL processing looks OK. @halter73 how about the 400 response? |
| int pathIndex = requestUrlPath.IndexOf('/', hostIndex); | ||
| if (pathIndex == -1) | ||
| { | ||
| int queryIndex = requestUrlPath.IndexOf('?', hostIndex); |
There was a problem hiding this comment.
Isn't the query string already removed from requestUrlPath?
There was a problem hiding this comment.
Totally overlooked that.
There was a problem hiding this comment.
Hmm, looking at the code that brings up an interesting point. pathBegin and pathEnd are now the wrong name, it should be urlBegin or targetBegin. Similar for requestUrlPath.
There was a problem hiding this comment.
Yeah, I'm renaming those.
|
I wonder if we should take this opportunity to return a 400 for non HTTP/1.1 or HTTP/1.0 requests. |
|
Or 505 HTTP Version Not Supported ? https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html |
|
I'll file an issue for the wrong HTTP version too. |
cc @Tratcher @halter73 @sajayantony
#666