Skip to content
This repository was archived by the owner on Dec 18, 2018. It is now read-only.

Ignore everything but path and query in requests for absolute URIs (#666). #711

Closed
wants to merge 2 commits into from
Closed

Ignore everything but path and query in requests for absolute URIs (#666). #711

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06433cd
Ignore everything but path and query in requests for absolute URIs (#…
Mar 23, 2016
d1840ec
Address PR feedback.
Mar 23, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 36 additions & 1 deletion src/Microsoft.AspNetCore.Server.Kestrel/Http/Frame.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,8 +804,43 @@ protected bool TakeStartLine(SocketInput input)
QueryString = queryString;
HttpVersion = httpVersion;

bool caseMatches;
if (requestUrlPath.Length > 0 && requestUrlPath[0] != '/')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this is an edge case and I don't see the need to optimize this path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens for requestUrlPath.Lenght == 0? That shouldn't be valid either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
int hostIndex;
if (requestUrlPath.StartsWith("http://"))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be interested in the allocation profile on this. Just curious if we could just operate on the bytes before materializing to a string or all the operations here non-allocating?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing the normal path is a relative URI this path might not even showup. Is that correct?

In reply to: 57374946 [](ancestors = 57374946)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's what I'm expecting.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Specify the StringComparison to avoid a CurrentCulture comparision. Is requestUrlPath expected to be lower case?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ordinal ignore case.

{
hostIndex = 7;
}
else if (requestUrlPath.StartsWith("https://"))
{
hostIndex = 8;
}
else
{
ReportCorruptedHttpRequest(new BadHttpRequestException($"Invalid request path: {requestUrlPath}"));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

officially it's the request target, not path.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also do you want play back user input security wise?

In reply to: 57367957 [](ancestors = 57367957)

return true;
}

int pathIndex = requestUrlPath.IndexOf('/', hostIndex);
if (pathIndex == -1)
{
int queryIndex = requestUrlPath.IndexOf('?', hostIndex);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't the query string already removed from requestUrlPath?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally overlooked that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm renaming those.

if (queryIndex == -1)
{
requestUrlPath = "/";
}
else
{
requestUrlPath = "/" + requestUrlPath.Substring(queryIndex);
}
}
else
{
requestUrlPath = requestUrlPath.Substring(pathIndex);
}
}

bool caseMatches;
if (!string.IsNullOrEmpty(_pathBase) &&
(requestUrlPath.Length == _pathBase.Length || (requestUrlPath.Length > _pathBase.Length && requestUrlPath[_pathBase.Length] == '/')) &&
RequestUrlStartsWithPathBase(requestUrlPath, out caseMatches))
Expand Down
6 changes: 6 additions & 0 deletions src/Microsoft.AspNetCore.Server.Kestrel/Http/FrameOfT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ public override async Task RequestProcessingAsync()

InitializeHeaders();

if (_corruptedRequest)
{
await ProduceEnd();
return;
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems different than what we do for corrupted request headers. I would like to determine the correct behavior and consolidate this logic.

while (!_requestProcessingStopping && !TakeMessageHeaders(SocketInput, FrameRequestHeaders))
{
if (SocketInput.RemoteIntakeFin)
Expand Down
98 changes: 96 additions & 2 deletions test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,6 @@ public async Task DoesNotHangOnConnectionCloseRequest()
{
app.Run(async context =>
{
var connection = context.Connection;
await context.Response.WriteAsync("hello, world");
});
});
Expand Down Expand Up @@ -174,7 +173,6 @@ public void RequestPathIsNormalized()
{
app.Run(async context =>
{
var connection = context.Connection;
Assert.Equal("/\u00C5", context.Request.PathBase.Value);
Assert.Equal("/B/\u00C5", context.Request.Path.Value);
await context.Response.WriteAsync("hello, world");
Expand Down Expand Up @@ -208,6 +206,102 @@ public void RequestPathIsNormalized()
}
}

[Theory]
[InlineData("http://doesnotmatter:8080", "/", "")]
[InlineData("http://doesnotmatter:8080/", "/", "")]
[InlineData("http://doesnotmatter:8080/test", "/test", "")]
[InlineData("http://doesnotmatter:8080/test?arg=value", "/test", "?arg=value")]
[InlineData("http://doesnotmatter:8080?arg=value", "/", "?arg=value")]
[InlineData("https://doesnotmatter:8080", "/", "")]
[InlineData("https://doesnotmatter:8080/", "/", "")]
[InlineData("https://doesnotmatter:8080/test", "/test", "")]
[InlineData("https://doesnotmatter:8080/test?arg=value", "/test", "?arg=value")]
[InlineData("https://doesnotmatter:8080?arg=value", "/", "?arg=value")]
public void RequestWithFullUriHasSchemeHostAndPortIgnored(string requestUri, string expectedPath, string expectedQuery)
{
var port = PortManager.GetPort();
var builder = new WebHostBuilder()
.UseServer("Microsoft.AspNetCore.Server.Kestrel")
.UseUrls($"http://localhost:{port}")
.Configure(app =>
{
app.Run(async context =>
{
Assert.Equal(expectedPath, context.Request.Path.Value);
Assert.Equal(expectedQuery, context.Request.QueryString.Value);
await context.Response.WriteAsync("hello, world");
});
});

using (var host = builder.Build())
{
host.Start();

using (var socket = TestConnection.CreateConnectedLoopbackSocket(port))
{
socket.Send(Encoding.ASCII.GetBytes($"GET {requestUri} HTTP/1.1\r\n\r\n"));
socket.Shutdown(SocketShutdown.Send);

var response = new StringBuilder();
var buffer = new byte[4096];
while (true)
{
var length = socket.Receive(buffer);
if (length == 0)
{
break;
}

response.Append(Encoding.ASCII.GetString(buffer, 0, length));
}

Assert.StartsWith("HTTP/1.1 200 OK", response.ToString());
}
}
}

[Fact]
public void RequestWithInvalidPathReturns400()
{
var port = PortManager.GetPort();
var builder = new WebHostBuilder()
.UseServer("Microsoft.AspNetCore.Server.Kestrel")
.UseUrls($"http://localhost:{port}")
.Configure(app =>
{
app.Run(async context =>
{
await context.Response.WriteAsync("hello, world");
});
});

using (var host = builder.Build())
{
host.Start();

using (var socket = TestConnection.CreateConnectedLoopbackSocket(port))
{
socket.Send(Encoding.ASCII.GetBytes($"GET invalid HTTP/1.1\r\n\r\n"));
socket.Shutdown(SocketShutdown.Send);

var response = new StringBuilder();
var buffer = new byte[4096];
while (true)
{
var length = socket.Receive(buffer);
if (length == 0)
{
break;
}

response.Append(Encoding.ASCII.GetString(buffer, 0, length));
}

Assert.StartsWith("HTTP/1.1 400 Bad Request", response.ToString());
}
}
}

private async Task TestRemoteIPAddress(string registerAddress, string requestAddress, string expectAddress)
{
var port = PortManager.GetPort();
Expand Down