Skip to content

Commit a064431

Browse files
joelverhagenjoelverhagen
committed
Remove redirects from error handling and clean Elmah (#7960)
Progress on #7868
1 parent cfcc1ba commit a064431

10 files changed

Lines changed: 179 additions & 46 deletions

File tree

‎src/NuGetGallery/App_400.aspx‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<% Response.StatusCode = 400 %>
2+
<!DOCTYPE html>
3+
<html lang="en">
4+
<head>
5+
<meta charset="utf-8" />
6+
<meta http-equiv="X-UA-Compatible" content="IE=edge">
7+
<meta name="viewport" content="width=device-width, initial-scale=1">
8+
<title>Bad Request</title>
9+
</head>
10+
<body>
11+
Bad Request
12+
</body>
13+
</html>

‎src/NuGetGallery/App_404.aspx‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<% Response.StatusCode = 404 %>
2+
<!DOCTYPE html>
3+
<html lang="en">
4+
<head>
5+
<meta charset="utf-8" />
6+
<meta http-equiv="X-UA-Compatible" content="IE=edge">
7+
<meta name="viewport" content="width=device-width, initial-scale=1">
8+
<title>Not Found</title>
9+
</head>
10+
<body>
11+
Not Found
12+
</body>
13+
</html>

‎src/NuGetGallery/App_500.aspx‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,13 @@
1+
<% Response.StatusCode = 500 %>
2+
<!DOCTYPE html>
3+
<html lang="en">
4+
<head>
5+
<meta charset="utf-8" />
6+
<meta http-equiv="X-UA-Compatible" content="IE=edge">
7+
<meta name="viewport" content="width=device-width, initial-scale=1">
8+
<title>Internal Server Error</title>
9+
</head>
10+
<body>
11+
Internal Server Error
12+
</body>
13+
</html>

‎src/NuGetGallery/App_Start/AppActivator.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,7 @@ private static void AppPostStart(IAppConfiguration configuration)
237237
GlobalFilters.Filters.Add(new ReadOnlyModeErrorFilter());
238238
GlobalFilters.Filters.Add(new AntiForgeryErrorFilter());
239239
GlobalFilters.Filters.Add(new UserDeletedErrorFilter());
240+
GlobalFilters.Filters.Add(new RequestValidationExceptionFilter());
240241
ValueProviderFactories.Factories.Add(new HttpHeaderValueProviderFactory());
241242
}
242243

‎src/NuGetGallery/Infrastructure/AntiForgeryErrorFilter.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
34
using System.Web.Mvc;
45

56
namespace NuGetGallery.Infrastructure

‎src/NuGetGallery/Infrastructure/CookieTempDataProvider.cs‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,10 @@
11
// Copyright (c) .NET Foundation. All rights reserved.
22
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
34
using System;
45
using System.Collections.Generic;
56
using System.Globalization;
7+
using System.Net;
68
using System.Web;
79
using System.Web.Mvc;
810

@@ -50,7 +52,19 @@ void ITempDataProvider.SaveTempData(ControllerContext controllerContext, IDictio
5052

5153
protected virtual IDictionary<string, object> LoadTempData(ControllerContext controllerContext)
5254
{
53-
var cookie = _httpContext.Request.Cookies[TempDataCookieKey];
55+
HttpCookie cookie;
56+
try
57+
{
58+
cookie = _httpContext.Request.Cookies[TempDataCookieKey];
59+
}
60+
catch (HttpRequestValidationException ex)
61+
{
62+
throw new HttpException(
63+
(int)HttpStatusCode.BadRequest,
64+
$"The cookie {TempDataCookieKey} could not be read.",
65+
ex);
66+
}
67+
5468
var dictionary = new Dictionary<string, object>(StringComparer.OrdinalIgnoreCase);
5569
if ((cookie == null) || String.IsNullOrEmpty(cookie.Value))
5670
{

‎src/NuGetGallery/Infrastructure/RequestValidationExceptionFilter.cs‎

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
// Copyright (c) .NET Foundation. All rights reserved.
2+
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
3+
4+
using System.Web;
5+
using System.Web.Mvc;
6+
7+
namespace NuGetGallery.Infrastructure
8+
{
9+
public sealed class RequestValidationExceptionFilter : FilterAttribute, IExceptionFilter
10+
{
11+
public void OnException(ExceptionContext context)
12+
{
13+
if (context.Exception is HttpRequestValidationException)
14+
{
15+
context.HttpContext.Response.Clear();
16+
context.HttpContext.Response.TrySkipIisCustomErrors = true;
17+
context.HttpContext.Response.StatusCode = 400;
18+
19+
context.Result = new ViewResult
20+
{
21+
ViewName = "~/Views/Errors/BadRequest.cshtml",
22+
};
23+
24+
context.ExceptionHandled = true;
25+
}
26+
}
27+
}
28+
}

‎src/NuGetGallery/NuGetGallery.csproj‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -224,6 +224,7 @@
224224
<Compile Include="Infrastructure\ABTestEnrollmentState.cs" />
225225
<Compile Include="Infrastructure\ABTestEnrollmentFactory.cs" />
226226
<Compile Include="Infrastructure\CookieBasedABTestService.cs" />
227+
<Compile Include="Infrastructure\RequestValidationExceptionFilter.cs" />
227228
<Compile Include="Infrastructure\IABTestEnrollmentFactory.cs" />
228229
<Compile Include="Infrastructure\IABTestService.cs" />
229230
<Compile Include="Infrastructure\ILuceneDocumentFactory.cs" />
@@ -1275,6 +1276,9 @@
12751276
<CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>
12761277
<SubType>Designer</SubType>
12771278
</Content>
1279+
<Content Include="App_404.aspx" />
1280+
<Content Include="App_400.aspx" />
1281+
<Content Include="App_500.aspx" />
12781282
<Content Include="Areas\Admin\DynamicData\FieldTemplates\Url_Edit.ascx" />
12791283
<Content Include="Areas\Admin\DynamicData\PageTemplates\ListDetails.aspx" />
12801284
<Content Include="Content\admin\SupportRequestStyles.css" />

‎src/NuGetGallery/Web.config‎

Lines changed: 17 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -211,7 +211,18 @@
211211
<security allowRemoteAccess="true"/>
212212
<errorFilter>
213213
<test>
214-
<equal binding="HttpStatusCode" value="404" type="Int32"/>
214+
<or>
215+
<equal binding="HttpStatusCode" value="404" type="Int32"/>
216+
<equal binding="HttpStatusCode" value="409" type="Int32"/>
217+
218+
<regex binding="Exception.Message" pattern="(?ix: \b potentially \b.+?\b dangerous \b.+?\b value \b.+?\b detected \b.+?\b client \b )" />
219+
220+
<!-- Ignore errors from the simulated error (fault injection) test path. -->
221+
<equal binding="Context.Request.Path" value="/pages/simulate-error" type="String" />
222+
<equal binding="Context.Request.Path" value="/api/simulate-error" type="String" />
223+
<equal binding="Context.Request.Path" value="/api/v1/SimulateError()" type="String" />
224+
<regex binding="Exception.Message" pattern="^SimulatedErrorType \w+$" />
225+
</or>
215226
</test>
216227
</errorFilter>
217228
<errorLog type="Elmah.SqlErrorLog, Elmah, Version=1.2.14706.0, Culture=neutral, PublicKeyToken=57eac04b2e0f138e, processorArchitecture=MSIL" connectionStringName="NuGetGallery"/>
@@ -339,12 +350,10 @@
339350
<!-- Remove Default HTTP Handler -->
340351
<remove path="*" verb="GET,HEAD,POST"/>
341352
</httpHandlers>
342-
<customErrors mode="RemoteOnly" defaultRedirect="~/Errors/500" redirectMode="ResponseRedirect">
343-
<!-- Adding ? at the end of the redirect URL prevents the illegal request to be passed
344-
as a query parameter to the redirect URL and causing additional failures -->
345-
<error statusCode="400" redirect="~/Errors/400"/>
346-
<error statusCode="404" redirect="~/Errors/404"/>
347-
<error statusCode="500" redirect="~/Errors/500"/>
353+
<customErrors mode="RemoteOnly" defaultRedirect="~/App_500.aspx" redirectMode="ResponseRewrite">
354+
<error statusCode="400" redirect="~/App_400.aspx"/>
355+
<error statusCode="404" redirect="~/App_404.aspx"/>
356+
<error statusCode="500" redirect="~/App_500.aspx"/>
348357
</customErrors>
349358
<sessionState mode="Off"/>
350359
</system.web>
@@ -647,4 +656,4 @@
647656
</dependentAssembly>
648657
</assemblyBinding>
649658
</runtime>
650-
</configuration>
659+
</configuration>

‎tests/NuGetGallery.FunctionalTests/ErrorHandling/ErrorHandlingTests.cs‎

Lines changed: 74 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -37,8 +37,12 @@ public ErrorHandlingTests(ITestOutputHelper testOutputHelper) : base(testOutputH
3737
[Theory]
3838
[Priority(2)]
3939
[Category("P2Tests")]
40-
[MemberData(nameof(RejectedCookies))]
41-
public async Task RejectedCookie(string relativePath, string name, string value, Action<TestResponse> validate)
40+
[InlineData("/", "__Controller::TempData", "Message=You successfully uploaded z̡̜͍̈̍̐̃̊͋́a̜̣͍̬̞̝͉̽ͧ͗l̸̖͕̤̠̹̘͖̃̌ͤg͓̝͓̰̀ͪo͈͌ 1.0.0.", 400, false)]
41+
[InlineData("/", "__Controller::TempData", "Message=<script>alert(1)</script>", 400, false)]
42+
[InlineData("/", "__Controller::TempData", "<script>alert(1)</script>", 400, false)]
43+
[InlineData("/packages", "nugetab", "<script>alert(1)</script>", 400, true)]
44+
[InlineData("/packages", "nugetab", "z̡̜͍̈̍̐̃̊͋́a̜̣͍̬̞̝͉̽ͧ͗l̸̖͕̤̠̹̘͖̃̌ͤg͓̝͓̰̀ͪo͈͌", 400, false)]
45+
public async Task RejectedCookie(string relativePath, string name, string value, int statusCode, bool pretty)
4246
{
4347
// Arrange
4448
_httpClientHandler.UseCookies = false;
@@ -51,23 +55,17 @@ public async Task RejectedCookie(string relativePath, string name, string value,
5155
var response = await GetTestResponseAsync(relativePath, request);
5256

5357
// Assert
54-
validate(response);
58+
if (pretty)
59+
{
60+
Validator.PrettyHtml((HttpStatusCode)statusCode)(response);
61+
}
62+
else
63+
{
64+
Validator.SimpleHtml((HttpStatusCode)statusCode)(response);
65+
}
5566
}
5667
}
5768

58-
public static IEnumerable<object[]> RejectedCookies => new[]
59-
{
60-
new object[] { $"/packages/{Constants.TestPackageId}", "__Controller::TempData", "Message=You successfully uploaded z̡̜͍̈̍̐̃̊͋́a̜̣͍̬̞̝͉̽ͧ͗l̸̖͕̤̠̹̘͖̃̌ͤg͓̝͓̰̀ͪo͈͌ 1.0.0.", Validator.SimpleHtml(HttpStatusCode.BadRequest) },
61-
62-
new object[] { $"/packages/{Constants.TestPackageId}", "__Controller::TempData", "Message=<script>alert(1)</script>", Validator.Redirect("500") },
63-
64-
new object[] { $"/packages/{Constants.TestPackageId}", "__Controller::TempData", "<script>alert(1)</script>", Validator.Redirect("500") },
65-
66-
new object[] { "/packages", "nugetab", "<script>alert(1)</script>", Validator.PrettyInternalServerError() },
67-
68-
new object[] { "/packages", "nugetab", "z̡̜͍̈̍̐̃̊͋́a̜̣͍̬̞̝͉̽ͧ͗l̸̖͕̤̠̹̘͖̃̌ͤg͓̝͓̰̀ͪo͈͌", Validator.SimpleHtml(HttpStatusCode.BadRequest) },
69-
};
70-
7169
/// <summary>
7270
/// Verify the behavior when a URL with restricted characters is used.
7371
/// </summary>
@@ -85,9 +83,7 @@ public async Task RejectedUrl(string relativePath)
8583
var response = await GetTestResponseAsync(relativePath);
8684

8785
// Assert
88-
// Since the HTTP client is configured to not follow redirects, the response we get back is not the
89-
// error page itself but instead a redirect to an error page.
90-
Validator.Redirect("400")(response);
86+
Validator.SimpleHtml(HttpStatusCode.BadRequest)(response);
9187
}
9288

9389
/// <summary>
@@ -96,17 +92,49 @@ public async Task RejectedUrl(string relativePath)
9692
[Theory]
9793
[Priority(2)]
9894
[Category("P2Tests")]
99-
[InlineData("/api/does-not-exist")]
100-
[InlineData("/pages/does-not-exist")]
101-
[InlineData("/api/v2/curated-feed/microsoftdotnet/DoesNotExist()")]
102-
[InlineData("/does-not-exist")]
103-
public async Task PageThatDoesNotExist(string relativePath)
95+
[InlineData("/api/does-not-exist", true)]
96+
[InlineData("/pages/does-not-exist", true)]
97+
[InlineData("/api/v2/curated-feed/microsoftdotnet/DoesNotExist()", true)]
98+
[InlineData("/does-not-exist", true)]
99+
[InlineData("/packages/package--cannot--exist", true)]
100+
[InlineData("/packages/BaseTestPackage/invalid-version/Manage", true)]
101+
// The following behave poorly. See: https://github.com/NuGet/NuGetGallery/issues/7959
102+
[InlineData("/packages/BaseTestPackage/invalid-version", false)]
103+
[InlineData("/packages/BaseTestPackage/1.0.0/Mismanage", false)]
104+
public async Task PageThatDoesNotExist(string relativePath, bool pretty)
104105
{
105106
// Arrange & Act
106107
var response = await GetTestResponseAsync(relativePath);
107108

108109
// Assert
109-
Validator.PrettyHtml(HttpStatusCode.NotFound)(response);
110+
if (pretty)
111+
{
112+
Validator.PrettyHtml(HttpStatusCode.NotFound)(response);
113+
}
114+
else
115+
{
116+
Validator.SimpleHtml(HttpStatusCode.NotFound)(response);
117+
}
118+
}
119+
120+
/// <summary>
121+
/// Verify a matched route but a mismatched HTTP method.
122+
/// </summary>
123+
[Theory]
124+
[Priority(2)]
125+
[Category("P2Tests")]
126+
[InlineData("DELETE", "/api/v2", 405)]
127+
// The following have non-ideal behavior.
128+
[InlineData("DELETE", "/api/status", 500)]
129+
[InlineData("GET", "/packages/manage/reflow", 404)]
130+
[InlineData("POST", "/packages/BaseTestPackage/1.0.0/License", 404)]
131+
public async Task UnsupportedMethod(string httpMethod, string relativePath, int statusCode)
132+
{
133+
// Arrange & Act
134+
var response = await GetTestResponseAsync(new HttpMethod(httpMethod), relativePath);
135+
136+
// Assert
137+
Assert.Equal((HttpStatusCode)statusCode, response.StatusCode);
110138
}
111139

112140
[Fact]
@@ -136,8 +164,7 @@ public async Task ErrorInErrorPageWithoutPath()
136164
var response = await GetTestResponseAsync("/Errors/500", cookies);
137165

138166
// Assert
139-
Validator.Redirect("500")(response);
140-
Assert.Equal("/Errors/500?aspxerrorpath=/Errors/500", response.LocationHeader);
167+
Validator.SimpleHtml(HttpStatusCode.InternalServerError);
141168
}
142169

143170
/// <summary>
@@ -244,14 +271,14 @@ public static IEnumerable<object[]> AllTestData
244271
{ SER(EndpointType.OData, SimulatedErrorType.Result500), Validator.Empty(HttpStatusCode.InternalServerError, SimulatedErrorType.Result500) },
245272
{ SER(EndpointType.OData, SimulatedErrorType.Result503), Validator.Empty(HttpStatusCode.ServiceUnavailable, SimulatedErrorType.Result503) },
246273
{ SER(EndpointType.OData, SimulatedErrorType.UserSafeException), Validator.Xml() },
247-
{ SER(EndpointType.Pages, SimulatedErrorType.HttpException400), Validator.Redirect("400") },
248-
{ SER(EndpointType.Pages, SimulatedErrorType.HttpException404), Validator.Redirect("404") },
249-
{ SER(EndpointType.Pages, SimulatedErrorType.HttpException503), Validator.Redirect("500") },
274+
{ SER(EndpointType.Pages, SimulatedErrorType.HttpException400), Validator.SimpleHtml(HttpStatusCode.BadRequest) },
275+
{ SER(EndpointType.Pages, SimulatedErrorType.HttpException404), Validator.SimpleHtml(HttpStatusCode.NotFound) },
276+
{ SER(EndpointType.Pages, SimulatedErrorType.HttpException503), Validator.SimpleHtml(HttpStatusCode.InternalServerError) },
250277
{ SER(EndpointType.Pages, SimulatedErrorType.ReadOnlyMode), Validator.PrettyHtml(HttpStatusCode.ServiceUnavailable) },
251278
{ SER(EndpointType.Pages, SimulatedErrorType.Result400), Validator.SimpleHtml(HttpStatusCode.BadRequest, SimulatedErrorType.Result400) },
252279
{ SER(EndpointType.Pages, SimulatedErrorType.Result404), Validator.PrettyHtml(HttpStatusCode.NotFound) },
253280
{ SER(EndpointType.Pages, SimulatedErrorType.Result503), Validator.SimpleHtml(HttpStatusCode.ServiceUnavailable, SimulatedErrorType.Result503) },
254-
{ SER(EndpointType.Pages, SimulatedErrorType.ExceptionInInlineErrorPage), Validator.Redirect("500") },
281+
{ SER(EndpointType.Pages, SimulatedErrorType.ExceptionInInlineErrorPage), Validator.SimpleHtml(HttpStatusCode.InternalServerError) },
255282
};
256283

257284
/// <summary>
@@ -284,7 +311,22 @@ private async Task<TestResponse> GetTestResponseAsync(SimulatedErrorRequest erro
284311

285312
private async Task<TestResponse> GetTestResponseAsync(string relativePath, IReadOnlyDictionary<string, string> cookies)
286313
{
287-
using (var request = new HttpRequestMessage(HttpMethod.Get, GetRequestUri(relativePath)))
314+
return await GetTestResponseAsync(HttpMethod.Get, relativePath, cookies);
315+
}
316+
317+
private async Task<TestResponse> GetTestResponseAsync(string relativePath)
318+
{
319+
return await GetTestResponseAsync(HttpMethod.Get, relativePath);
320+
}
321+
322+
private async Task<TestResponse> GetTestResponseAsync(HttpMethod method, string relativePath)
323+
{
324+
return await GetTestResponseAsync(method, relativePath, new Dictionary<string, string>());
325+
}
326+
327+
private async Task<TestResponse> GetTestResponseAsync(HttpMethod method, string relativePath, IReadOnlyDictionary<string, string> cookies)
328+
{
329+
using (var request = new HttpRequestMessage(method, GetRequestUri(relativePath)))
288330
{
289331
foreach (var cookie in cookies)
290332
{
@@ -295,11 +337,6 @@ private async Task<TestResponse> GetTestResponseAsync(string relativePath, IRead
295337
}
296338
}
297339

298-
private async Task<TestResponse> GetTestResponseAsync(string relativePath)
299-
{
300-
return await GetTestResponseAsync(relativePath, new Dictionary<string, string>());
301-
}
302-
303340
private Uri GetRequestUri(string relativePath)
304341
{
305342
return new Uri(new Uri(UrlHelper.BaseUrl), relativePath);

0 commit comments

Comments
 (0)