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

Commit aaf80e7

Browse files
author
Nate McMaster
committed
PR feedback. Use System.Uri to validate absolute-URI. Add more tests for malformed requests
1 parent 59b169f commit aaf80e7

File tree

5 files changed

+98
-47
lines changed

5 files changed

+98
-47
lines changed

‎src/Microsoft.AspNetCore.Server.Kestrel/Internal/Http/Frame.cs

Lines changed: 42 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -1030,42 +1030,27 @@ public RequestLineStatus TakeStartLine(SocketInput input)
10301030
// begin consuming request-target
10311031
begin = scan;
10321032

1033-
var targetIsAbsolute = false;
1034-
string requestUriScheme;
1035-
string requestAuthority = null;
1036-
if (scan.GetKnownHttpSchema(out requestUriScheme))
1037-
{
1038-
// Start-line can contain uri in absolute form. e.g. 'GET http://contoso.com/favicon.ico HTTP/1.1'
1039-
// Clients should only send this to proxies, but the spec requires we handle it anyways.
1040-
// cref https://tools.ietf.org/html/rfc7230#section-5.3
1041-
1042-
// This will skip over scheme and authority so they do not end up in .Path,
1043-
// but preserving the values for rawTarget. We rely on the Host header and
1044-
// server configuration to determine the effective host, port, and
1045-
// for this request.
1046-
1047-
targetIsAbsolute = true;
1048-
scan.Skip(requestUriScheme.Length);
1049-
begin = scan;
1033+
var targetBegin = scan;
10501034

1051-
// an absolute URI is not required to end in a slash but host must not be empty
1052-
// see https://tools.ietf.org/html/rfc3986#section-4.3
1053-
var chNext = scan.Peek();
1054-
if (chNext == ByteForwardSlash || chNext == ByteSpace)
1035+
if (targetBegin.Peek() != ByteForwardSlash)
1036+
{
1037+
string requestUriScheme;
1038+
if (scan.GetKnownHttpSchema(out requestUriScheme))
10551039
{
1056-
RejectRequest(RequestRejectionReason.InvalidRequestLine,
1057-
Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty);
1058-
}
1040+
// Start-line can contain uri in absolute form. e.g. 'GET http://contoso.com/favicon.ico HTTP/1.1'
1041+
// Clients should only send this to proxies, but the spec requires we handle it anyways.
1042+
// cref https://tools.ietf.org/html/rfc7230#section-5.3
1043+
1044+
scan.Skip(requestUriScheme.Length);
10591045

1060-
if (scan.Seek(ByteForwardSlash, ByteSpace, ref end) == -1)
1061-
{
1062-
RejectRequest(RequestRejectionReason.InvalidRequestLine,
1063-
Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty);
1046+
if (scan.Seek(ByteForwardSlash, ByteSpace, ref end) == -1)
1047+
{
1048+
RejectRequest(RequestRejectionReason.InvalidRequestLine,
1049+
Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty);
1050+
}
1051+
1052+
begin = scan;
10641053
}
1065-
1066-
// TODO consider handling UTF-8 host names
1067-
requestAuthority = begin.GetAsciiString(ref scan);
1068-
begin = scan;
10691054
}
10701055

10711056
var needDecode = false;
@@ -1089,7 +1074,7 @@ public RequestLineStatus TakeStartLine(SocketInput input)
10891074
var pathBegin = begin;
10901075
var pathEnd = scan;
10911076

1092-
var queryString = "";
1077+
var queryString = string.Empty;
10931078
if (chFound == ByteQuestionMark)
10941079
{
10951080
begin = scan;
@@ -1103,7 +1088,7 @@ public RequestLineStatus TakeStartLine(SocketInput input)
11031088

11041089
var queryEnd = scan;
11051090

1106-
if (pathBegin.Peek() == ByteSpace && !targetIsAbsolute)
1091+
if (pathBegin.Peek() == ByteSpace && targetBegin.Index == pathBegin.Index)
11071092
{
11081093
RejectRequest(RequestRejectionReason.InvalidRequestLine,
11091094
Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty);
@@ -1146,11 +1131,11 @@ public RequestLineStatus TakeStartLine(SocketInput input)
11461131
// Multibyte Internationalized Resource Identifiers (IRIs) are first converted to utf8;
11471132
// then encoded/escaped to ASCII https://www.ietf.org/rfc/rfc3987.txt "Mapping of IRIs to URIs"
11481133
string requestUrlPath;
1149-
string rawUrlPath;
1134+
string rawTarget;
11501135
if (needDecode)
11511136
{
11521137
// Read raw target before mutating memory.
1153-
rawUrlPath = pathBegin.GetAsciiString(ref queryEnd);
1138+
rawTarget = targetBegin.GetAsciiString(ref queryEnd);
11541139

11551140
// URI was encoded, unescape and then parse as utf8
11561141
pathEnd = UrlPathDecoder.Unescape(pathBegin, pathEnd);
@@ -1161,15 +1146,30 @@ public RequestLineStatus TakeStartLine(SocketInput input)
11611146
// URI wasn't encoded, parse as ASCII
11621147
requestUrlPath = pathBegin.GetAsciiString(ref pathEnd);
11631148

1164-
if (queryString.Length == 0)
1149+
if (queryString.Length == 0 && targetBegin.Index == pathBegin.Index)
11651150
{
11661151
// No need to allocate an extra string if the path didn't need
1167-
// decoding and there's no query string following it.
1168-
rawUrlPath = requestUrlPath;
1152+
// decoding and there's no query string following it, and the
1153+
// request-target isn't absolute-form
1154+
rawTarget = requestUrlPath;
11691155
}
11701156
else
11711157
{
1172-
rawUrlPath = pathBegin.GetAsciiString(ref queryEnd);
1158+
rawTarget = targetBegin.GetAsciiString(ref queryEnd);
1159+
}
1160+
}
1161+
1162+
if (targetBegin.Index < pathBegin.Index)
1163+
{
1164+
// validation of absolute-form URI may be slow, but clients
1165+
// should not be sending this form anyways, so perf optimization
1166+
// not high priority
1167+
1168+
Uri _;
1169+
if (!Uri.TryCreate(rawTarget, UriKind.Absolute, out _))
1170+
{
1171+
RejectRequest(RequestRejectionReason.InvalidRequestLine,
1172+
Log.IsEnabled(LogLevel.Information) ? start.GetAsciiStringEscaped(end, MaxInvalidRequestLineChars) : string.Empty);
11731173
}
11741174
}
11751175

@@ -1180,9 +1180,7 @@ public RequestLineStatus TakeStartLine(SocketInput input)
11801180
consumed = scan;
11811181
Method = method;
11821182
QueryString = queryString;
1183-
RawTarget = targetIsAbsolute
1184-
? requestUriScheme + requestAuthority + rawUrlPath
1185-
: rawUrlPath;
1183+
RawTarget = rawTarget;
11861184
HttpVersion = httpVersion;
11871185

11881186
bool caseMatches;
@@ -1191,7 +1189,7 @@ public RequestLineStatus TakeStartLine(SocketInput input)
11911189
PathBase = caseMatches ? _pathBase : normalizedUrlPath.Substring(0, _pathBase.Length);
11921190
Path = normalizedUrlPath.Substring(_pathBase.Length);
11931191
}
1194-
else if (rawUrlPath?.Length > 0 && rawUrlPath[0] == '/') // check rawUrlPath since normalizedUrlPath can be "" or "/" after dot segment removal
1192+
else if (requestUrlPath?.Length > 0 && requestUrlPath[0] == '/') // check requestUrlPath since normalizedUrlPath can be "" or "/" after dot segment removal
11951193
{
11961194
Path = normalizedUrlPath;
11971195
}

‎test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/RequestTests.cs

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -427,8 +427,13 @@ public async Task ThrowsOnReadAfterConnectionError()
427427
[InlineData("https://localhost:22/abs/path", "/abs/path")] // handles mismatched ports
428428
[InlineData("https://differenthost/abs/path", "/abs/path")] // handles mismatched hostname
429429
[InlineData("http://localhost/", "/")]
430+
[InlineData("http://[email protected]/path", "/path")]
431+
[InlineData("http://root:[email protected]/path", "/path")]
430432
[InlineData("https://localhost/", "/")]
431433
[InlineData("http://localhost", "")]
434+
[InlineData("http://127.0.0.1/", "/")]
435+
[InlineData("http://[::1]/", "/")]
436+
[InlineData("http://[::1]:8080/", "/")]
432437
public async Task CanHandleRequestsWithUrlInAbsoluteForm(string requestUrl, string expectedPath)
433438
{
434439
var pathTcs = new TaskCompletionSource<PathString>();

‎test/Microsoft.AspNetCore.Server.KestrelTests/BadHttpRequestTests.cs

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,12 +52,26 @@ public class BadHttpRequestTests
5252
[InlineData("} / HTTP/1.0\r\n")]
5353
[InlineData("get@ / HTTP/1.0\r\n")]
5454
[InlineData("post= / HTTP/1.0\r\n")]
55-
// malformed request target
55+
[InlineData("GET otherscheme://host/ HTTP/1.1\r\n")]
56+
[InlineData("GET ws://host/ HTTP/1.1\r\n")]
57+
[InlineData("GET wss://host/ HTTP/1.1\r\n")]
58+
[InlineData("GET http HTTP/1.1\r\n")]
59+
[InlineData("GET http: HTTP/1.1\r\n")]
60+
[InlineData("GET http:/ HTTP/1.1\r\n")]
5661
[InlineData("GET http:// HTTP/1.1\r\n")]
62+
[InlineData("GET https HTTP/1.1\r\n")]
63+
[InlineData("GET https: HTTP/1.1\r\n")]
64+
[InlineData("GET https:/ HTTP/1.1\r\n")]
5765
[InlineData("GET https:// HTTP/1.1\r\n")]
5866
[InlineData("GET http:/// HTTP/1.1\r\n")]
5967
[InlineData("GET http://// HTTP/1.1\r\n")]
60-
//[InlineData("GET http://:80/abc HTTP/1.1")]
68+
[InlineData("GET http://:80 HTTP/1.1")]
69+
[InlineData("GET http://:80/abc HTTP/1.1")]
70+
[InlineData("GET http://user@ HTTP/1.1")]
71+
[InlineData("GET http://user@/abc HTTP/1.1")]
72+
[InlineData("GET http://abc%20xyz/abc HTTP/1.1")]
73+
[InlineData("GET http://%20/abc?query=%0A HTTP/1.1")]
74+
[InlineData("GET www.contoso.com HTTP/1.1")]
6175
public async Task TestInvalidRequestLines(string request)
6276
{
6377
using (var server = new TestServer(context => TaskCache.CompletedTask))
@@ -88,6 +102,11 @@ public async Task TestInvalidRequestLines(string request)
88102
[InlineData("GET / HTTP/1.\r\n")]
89103
[InlineData("GET / hello\r\n")]
90104
[InlineData("GET / 8charact\r\n")]
105+
[InlineData("GET / HTTP/1.1\r\n")]
106+
[InlineData("GET / / HTTP/1.1\r\n")]
107+
[InlineData("GET http:// HTTP/1.1\r\n")]
108+
[InlineData("GET https:// HTTP/1.1\r\n")]
109+
[InlineData("GET https:// / HTTP/1.1\r\n")]
91110
public async Task TestInvalidRequestLinesWithUnsupportedVersion(string request)
92111
{
93112
using (var server = new TestServer(context => TaskCache.CompletedTask))

‎test/Microsoft.AspNetCore.Server.KestrelTests/FrameTests.cs

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -460,6 +460,35 @@ public void TakeStartLineCallsConsumingCompleteWithFurthestExamined()
460460
Assert.False(_socketInput.IsCompleted);
461461
}
462462

463+
[Theory]
464+
[InlineData("http://host/abs/path", "/abs/path")]
465+
[InlineData("https://host/abs/path", "/abs/path")]
466+
[InlineData("https://host:22/abs/path", "/abs/path")]
467+
[InlineData("https://user@host:9080/abs/path", "/abs/path")]
468+
[InlineData("http://host/", "/")]
469+
[InlineData("https://host/", "/")]
470+
[InlineData("http://host", "")]
471+
[InlineData("http://user@host/", "/")]
472+
[InlineData("http://127.0.0.1/", "/")]
473+
[InlineData("http://[email protected]/", "/")]
474+
[InlineData("http://[email protected]:8080/", "/")]
475+
[InlineData("http://127.0.0.1:8080/", "/")]
476+
[InlineData("http://[::1]", "")]
477+
[InlineData("http://[::1]/path", "/path")]
478+
[InlineData("http://[::1]:8080/", "/")]
479+
[InlineData("http://user@[::1]:8080/", "/")]
480+
public void TakeStartLineHandlesRequestTarget(string rawTarget, string expectedPath)
481+
{
482+
var firstLine = Encoding.ASCII.GetBytes($"GET {rawTarget} HTTP/1.1\r\n");
483+
_socketInput.IncomingData(firstLine, 0, firstLine.Length);
484+
485+
var status = _frame.TakeStartLine(_socketInput);
486+
487+
Assert.Equal(Frame.RequestLineStatus.Done, status);
488+
Assert.Equal(expectedPath, _frame.Path);
489+
Assert.Equal(rawTarget, _frame.RawTarget);
490+
}
491+
463492
[Theory]
464493
[InlineData("", Frame.RequestLineStatus.Empty)]
465494
[InlineData("G", Frame.RequestLineStatus.Incomplete)]

‎test/Microsoft.AspNetCore.Server.KestrelTests/MemoryPoolIteratorTests.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -612,7 +612,7 @@ public void GetsKnownMethod(string input, bool expectedResult, string expectedKn
612612
[InlineData("HtTp:// ", false, null)]
613613
[InlineData("https://", true, MemoryPoolIteratorExtensions.HttpsScheme)]
614614
[InlineData("https://abc", true, MemoryPoolIteratorExtensions.HttpsScheme)]
615-
public void GetKnownHttpSchema(string input, bool expectedResult, string expectedString)
615+
public void GetsKnownHttpSchema(string input, bool expectedResult, string expectedString)
616616
{
617617
// Test within one block
618618
var block = _pool.Lease();

0 commit comments

Comments
 (0)