Skip to content

Commit 2abd369

Browse files
CopilotKrzysztofPajak
Copilot
and
KrzysztofPajak
authored
fix: implement ASP.NET Core 10 error handling (#685)
* fix: implement ASP.NET Core 10 error handling best practices - Add GrandExceptionHandler implementing IExceptionHandler (ASP.NET Core 8+ pattern): logs exceptions, returns RFC 7807 ProblemDetails JSON for API (Bearer) requests, returns false for web requests so the configured error page handles the response. Guards with HasStarted before writing. - Register AddExceptionHandler<GrandExceptionHandler>() and AddProblemDetails() in ErrorHandlerStartup.ConfigureServices. - Fix UseGrandExceptionHandler: remove dual UseExceptionHandler registration bug. Single UseExceptionHandler('/errorpage.htm') now invokes the IExceptionHandler chain first, then falls back to re-executing at /errorpage.htm for non-API requests. - Fix UsePageNotFound: replace 302 Redirect with UseStatusCodePagesWithReExecute ('/page-not-found') which preserves the original 404 status code. Add inline middleware to disable status code pages for API (Bearer) and static-resource requests so those callers receive their original response unchanged. - Fix ApiAuthenticationRegistrar: replace catch(Exception ex) { throw new Exception(ex.Message) } with bare throw to preserve the original stack trace; update OnAuthenticationFailed to return application/problem+json via IProblemDetailsService instead of plain text. - Fix BaseController.LogException: use a constant message template instead of exception.Message to avoid structured-logging format issues. Agent-Logs-Url: https://github.com/grandnode/grandnode2/sessions/ecc051c4-72f1-4d90-8ec9-025f19ab46c3 Co-authored-by: KrzysztofPajak <16772986+KrzysztofPajak@users.noreply.github.com> * fix: use StartsWith for Authorization header scheme check (safer than Split()[0])" Agent-Logs-Url: https://github.com/grandnode/grandnode2/sessions/ecc051c4-72f1-4d90-8ec9-025f19ab46c3 Co-authored-by: KrzysztofPajak <16772986+KrzysztofPajak@users.noreply.github.com> * refactor: extract IsApiRequest helper to eliminate duplicate auth-header check Agent-Logs-Url: https://github.com/grandnode/grandnode2/sessions/1ec7db4a-11fc-49b4-8aa2-85f12990ec16 Co-authored-by: KrzysztofPajak <16772986+KrzysztofPajak@users.noreply.github.com> * fix: restrict UseStatusCodePagesWithReExecute to 404-only; static ContentTypeProvider; remove redundant try/catch; add OnAuthenticationFailed fallback Agent-Logs-Url: https://github.com/grandnode/grandnode2/sessions/24b98e75-69d0-4ef3-a43e-2feb96bb1479 Co-authored-by: KrzysztofPajak <16772986+KrzysztofPajak@users.noreply.github.com> * fix: GrandExceptionHandler now handles web requests with redirect; remove path fallback from UseExceptionHandler Agent-Logs-Url: https://github.com/grandnode/grandnode2/sessions/5190d235-2337-4c71-add0-49709d3379b1 Co-authored-by: KrzysztofPajak <16772986+KrzysztofPajak@users.noreply.github.com> * Disable full error stack display in production Changed "DisplayFullErrorStack" in appsettings.json from true to false to prevent full error stack traces from being shown in production environments. This enhances security and user experience by limiting error details exposed to end users. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: KrzysztofPajak <16772986+KrzysztofPajak@users.noreply.github.com> Co-authored-by: KrzysztofPajak <krzysiek@grandnode.com>
1 parent 43bcfca commit 2abd369

6 files changed

Lines changed: 184 additions & 93 deletions

File tree

‎src/Modules/Grand.Module.Api/Infrastructure/ApiAuthenticationRegistrar.cs‎

Lines changed: 60 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
1-
using Grand.Module.Api.Infrastructure.Extensions;
1+
using Grand.Module.Api.Infrastructure.Extensions;
22
using Grand.Business.Core.Interfaces.Authentication;
33
using Grand.Infrastructure.Configuration;
44
using Microsoft.AspNetCore.Authentication;
55
using Microsoft.AspNetCore.Authentication.JwtBearer;
66
using Microsoft.AspNetCore.Http;
7+
using Microsoft.AspNetCore.Mvc;
78
using Microsoft.Extensions.Configuration;
89
using Microsoft.Extensions.DependencyInjection;
910
using Microsoft.IdentityModel.Tokens;
@@ -32,29 +33,39 @@ public void AddAuthentication(AuthenticationBuilder builder, IConfiguration conf
3233
OnAuthenticationFailed = async context =>
3334
{
3435
context.NoResult();
35-
context.Response.StatusCode = 401;
36-
context.Response.ContentType = "text/plain";
37-
await context.Response.WriteAsync(context.Exception.Message);
38-
},
36+
context.Response.StatusCode = StatusCodes.Status401Unauthorized;
37+
var problemDetailsService = context.HttpContext.RequestServices.GetService<IProblemDetailsService>();
38+
if (problemDetailsService != null)
39+
{
40+
await problemDetailsService.WriteAsync(new ProblemDetailsContext {
41+
HttpContext = context.HttpContext,
42+
ProblemDetails = new ProblemDetails {
43+
Status = StatusCodes.Status401Unauthorized,
44+
Title = "Authentication failed"
45+
}
46+
});
47+
}
48+
else
49+
{
50+
context.Response.ContentType = "application/problem+json";
51+
await context.Response.WriteAsJsonAsync(new ProblemDetails {
52+
Status = StatusCodes.Status401Unauthorized,
53+
Title = "Authentication failed"
54+
});
55+
}
56+
},
3957
OnTokenValidated = async context =>
4058
{
41-
try
59+
if (config.Enabled)
4260
{
43-
if (config.Enabled)
44-
{
45-
var jwtAuthentication = context.HttpContext.RequestServices
46-
.GetRequiredService<IJwtBearerAuthenticationService>();
47-
if (!await jwtAuthentication.Valid(context))
48-
throw new Exception(await jwtAuthentication.ErrorMessage());
49-
}
50-
else
51-
{
52-
throw new Exception("API is disable");
53-
}
61+
var jwtAuthentication = context.HttpContext.RequestServices
62+
.GetRequiredService<IJwtBearerAuthenticationService>();
63+
if (!await jwtAuthentication.Valid(context))
64+
throw new Exception(await jwtAuthentication.ErrorMessage());
5465
}
55-
catch (Exception ex)
66+
else
5667
{
57-
throw new Exception(ex.Message);
68+
throw new Exception("API is disabled");
5869
}
5970
}
6071
};
@@ -80,35 +91,45 @@ public void AddAuthentication(AuthenticationBuilder builder, IConfiguration conf
8091
OnAuthenticationFailed = async context =>
8192
{
8293
context.NoResult();
83-
context.Response.StatusCode = 401;
84-
context.Response.ContentType = "text/plain";
85-
await context.Response.WriteAsync(context.Exception.Message);
86-
},
94+
context.Response.StatusCode = StatusCodes.Status401Unauthorized;
95+
var problemDetailsService = context.HttpContext.RequestServices.GetService<IProblemDetailsService>();
96+
if (problemDetailsService != null)
97+
{
98+
await problemDetailsService.WriteAsync(new ProblemDetailsContext {
99+
HttpContext = context.HttpContext,
100+
ProblemDetails = new ProblemDetails {
101+
Status = StatusCodes.Status401Unauthorized,
102+
Title = "Authentication failed"
103+
}
104+
});
105+
}
106+
else
107+
{
108+
context.Response.ContentType = "application/problem+json";
109+
await context.Response.WriteAsJsonAsync(new ProblemDetails {
110+
Status = StatusCodes.Status401Unauthorized,
111+
Title = "Authentication failed"
112+
});
113+
}
114+
},
87115
OnTokenValidated = async context =>
88116
{
89-
try
117+
if (config.Enabled)
90118
{
91-
if (config.Enabled)
92-
{
93-
var jwtAuthentication = context.HttpContext.RequestServices
94-
.GetRequiredService<IJwtBearerCustomerAuthenticationService>();
95-
var isValid = await jwtAuthentication.Valid(context);
96-
if (!isValid)
97-
throw new Exception(await jwtAuthentication.ErrorMessage());
98-
}
99-
else
100-
{
101-
throw new Exception("API is disable");
102-
}
119+
var jwtAuthentication = context.HttpContext.RequestServices
120+
.GetRequiredService<IJwtBearerCustomerAuthenticationService>();
121+
var isValid = await jwtAuthentication.Valid(context);
122+
if (!isValid)
123+
throw new Exception(await jwtAuthentication.ErrorMessage());
103124
}
104-
catch (Exception ex)
125+
else
105126
{
106-
throw new Exception(ex.Message);
127+
throw new Exception("API is disabled");
107128
}
108129
}
109130
};
110131
});
111132
}
112133

113134
public int Priority => 900;
114-
}
135+
}

‎src/Web/Grand.Web.Common/Controllers/BaseController.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -116,7 +116,7 @@ protected void Error(Exception exception, bool persistNextRequest = true, bool l
116116
private void LogException(Exception exception)
117117
{
118118
var logger = HttpContext.RequestServices.GetRequiredService<ILoggerFactory>().CreateLogger("BaseController");
119-
logger.LogError(exception, exception.Message);
119+
logger.LogError(exception, "An error occurred");
120120
}
121121

122122
/// <summary>

‎src/Web/Grand.Web.Common/Infrastructure/ApplicationBuilderExtensions.cs‎

Lines changed: 45 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,4 @@
1-
using Grand.Data;
2-
using Grand.Infrastructure.Configuration;
1+
using Grand.Infrastructure.Configuration;
32
using Grand.Infrastructure.Endpoints;
43
using Grand.Infrastructure.Plugins;
54
using Grand.Infrastructure.TypeSearch;
@@ -26,6 +25,21 @@ namespace Grand.Web.Common.Infrastructure;
2625
/// </summary>
2726
public static class ApplicationBuilderExtensions
2827
{
28+
// Reused across requests — FileExtensionContentTypeProvider is stateless and thread-safe
29+
private static readonly FileExtensionContentTypeProvider ContentTypeProvider = new();
30+
31+
internal static bool IsApiRequest(HttpRequest request)
32+
{
33+
string authHeader = request.Headers[HeaderNames.Authorization];
34+
return authHeader != null &&
35+
authHeader.StartsWith(JwtBearerDefaults.AuthenticationScheme + " ", StringComparison.OrdinalIgnoreCase);
36+
}
37+
38+
private static bool IsStaticFileRequest(PathString path)
39+
{
40+
return ContentTypeProvider.TryGetContentType(path, out _);
41+
}
42+
2943
/// <summary>
3044
/// Add exception handling
3145
/// </summary>
@@ -39,64 +53,46 @@ public static void UseGrandExceptionHandler(this WebApplication application)
3953
//get detailed exceptions for developing and testing purposes
4054
application.UseDeveloperExceptionPage();
4155
else
42-
//or use special exception handler
43-
application.UseExceptionHandler("/errorpage.htm");
44-
45-
//log errors
46-
application.UseExceptionHandler(handler =>
47-
{
48-
handler.Run(async context =>
49-
{
50-
var exception = context.Features.Get<IExceptionHandlerFeature>()?.Error;
51-
if (exception == null)
52-
return;
53-
54-
string authHeader = context.Request.Headers["Authorization"];
55-
var apiRequest = authHeader != null && authHeader.Split(' ')[0] == "Bearer";
56-
if (apiRequest)
57-
{
58-
await context.Response.WriteAsync(exception.Message);
59-
return;
60-
}
61-
62-
if (DataSettingsManager.DatabaseIsInstalled())
63-
{
64-
var logger = context.RequestServices.GetRequiredService<ILoggerFactory>()
65-
.CreateLogger("UseExceptionHandler");
66-
// Log the error
67-
logger.LogError(exception, exception.Message);
68-
}
69-
70-
});
71-
});
56+
//use registered IExceptionHandler services (GrandExceptionHandler handles both API and web requests)
57+
application.UseExceptionHandler();
7258
}
7359

7460
/// <summary>
75-
/// Adds a special handler that checks for responses with the 404 status code that do not have a body
61+
/// Adds a special handler that checks for responses with the 404 status code that do not have a body.
62+
/// Re-executes the pipeline at /page-not-found (preserving the original 404 status code) while
63+
/// skipping the re-execution for API and static-resource requests.
7664
/// </summary>
7765
/// <param name="application">Builder for configuring an application's request pipeline</param>
7866
public static void UsePageNotFound(this WebApplication application)
7967
{
80-
application.UseStatusCodePages(async context =>
68+
// UseStatusCodePagesWithReExecute sets IStatusCodePagesFeature.Enabled = true and re-executes
69+
// the pipeline at the specified path when a 404 occurs, preserving the original 404 status code.
70+
application.UseStatusCodePagesWithReExecute("/page-not-found");
71+
72+
// Disable status code pages for API (Bearer) requests and static resource requests so that
73+
// those callers receive the original response rather than the HTML not-found page.
74+
// For all other requests, also restrict re-execution to actual 404 responses so that
75+
// 400/401/403/405/500 etc. are not mistakenly routed to /page-not-found.
76+
application.Use(async (context, next) =>
8177
{
82-
//handle 404 Not Found
83-
if (context.HttpContext.Response.StatusCode == 404)
78+
if (IsApiRequest(context.Request) || IsStaticFileRequest(context.Request.Path))
8479
{
85-
string authHeader = context.HttpContext.Request.Headers[HeaderNames.Authorization];
86-
var apiRequest = authHeader != null &&
87-
authHeader.Split(' ')[0] == JwtBearerDefaults.AuthenticationScheme;
80+
var feature = context.Features.Get<IStatusCodePagesFeature>();
81+
if (feature != null)
82+
feature.Enabled = false;
83+
await next(context);
84+
return;
85+
}
8886

89-
var contentTypeProvider = new FileExtensionContentTypeProvider();
90-
var staticResource = contentTypeProvider.TryGetContentType(context.HttpContext.Request.Path, out _);
87+
await next(context);
9188

92-
if (!apiRequest && !staticResource)
93-
{
94-
const string location = "/page-not-found";
95-
context.HttpContext.Response.Redirect(context.HttpContext.Request.PathBase + location);
96-
}
89+
// Only re-execute for 404 Not Found; all other error codes are handled elsewhere.
90+
if (context.Response.StatusCode != StatusCodes.Status404NotFound)
91+
{
92+
var feature = context.Features.Get<IStatusCodePagesFeature>();
93+
if (feature != null)
94+
feature.Enabled = false;
9795
}
98-
99-
await Task.CompletedTask;
10096
});
10197
}
10298

@@ -112,10 +108,7 @@ public static void UseBadRequestResult(this WebApplication application)
112108
if (context.HttpContext.Response.StatusCode != StatusCodes.Status400BadRequest)
113109
return Task.CompletedTask;
114110

115-
string authHeader = context.HttpContext.Request.Headers[HeaderNames.Authorization];
116-
var apiRequest = authHeader != null && authHeader.Split(' ')[0] == JwtBearerDefaults.AuthenticationScheme;
117-
118-
if (apiRequest) return Task.CompletedTask;
111+
if (IsApiRequest(context.HttpContext.Request)) return Task.CompletedTask;
119112
var logger = context.HttpContext.RequestServices.GetRequiredService<ILoggerFactory>()
120113
.CreateLogger("UseBadRequestResult");
121114
logger.LogError("Error 400. Bad request");

‎src/Web/Grand.Web.Common/Infrastructure/GrandExceptionHandler.cs‎

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
using Grand.Data;
2+
using Microsoft.AspNetCore.Diagnostics;
3+
using Microsoft.AspNetCore.Http;
4+
using Microsoft.AspNetCore.Mvc;
5+
using Microsoft.Extensions.DependencyInjection;
6+
using Microsoft.Extensions.Logging;
7+
8+
namespace Grand.Web.Common.Infrastructure;
9+
10+
/// <summary>
11+
/// Handles unhandled exceptions according to ASP.NET Core best practices.
12+
/// For API requests (Bearer token) it writes an RFC 7807 ProblemDetails JSON response.
13+
/// For regular web (Razor/MVC) requests it redirects to the static error page (/errorpage.htm).
14+
/// </summary>
15+
public class GrandExceptionHandler : IExceptionHandler
16+
{
17+
private readonly ILogger<GrandExceptionHandler> _logger;
18+
19+
public GrandExceptionHandler(ILogger<GrandExceptionHandler> logger)
20+
{
21+
_logger = logger;
22+
}
23+
24+
public async ValueTask<bool> TryHandleAsync(
25+
HttpContext httpContext,
26+
Exception exception,
27+
CancellationToken cancellationToken)
28+
{
29+
if (httpContext.Response.HasStarted)
30+
return false;
31+
32+
// Log the exception when the database is available
33+
if (DataSettingsManager.DatabaseIsInstalled())
34+
_logger.LogError(exception, "An unhandled exception has occurred");
35+
36+
if (!ApplicationBuilderExtensions.IsApiRequest(httpContext.Request))
37+
{
38+
// For Razor/MVC web requests, redirect to the static error page so the
39+
// browser always sees a user-friendly page (the path-based re-execution
40+
// fallback in UseExceptionHandler is unreliable in an MVC pipeline).
41+
httpContext.Response.Redirect("/errorpage.htm", permanent: false);
42+
return true;
43+
}
44+
45+
httpContext.Response.StatusCode = StatusCodes.Status500InternalServerError;
46+
47+
var problemDetailsService = httpContext.RequestServices.GetService<IProblemDetailsService>();
48+
if (problemDetailsService != null)
49+
{
50+
await problemDetailsService.WriteAsync(new ProblemDetailsContext {
51+
HttpContext = httpContext,
52+
Exception = exception,
53+
ProblemDetails = new ProblemDetails {
54+
Status = StatusCodes.Status500InternalServerError,
55+
Title = "An error occurred while processing your request",
56+
Instance = httpContext.Request.Path
57+
}
58+
});
59+
}
60+
else
61+
{
62+
httpContext.Response.ContentType = "application/problem+json";
63+
await httpContext.Response.WriteAsJsonAsync(new ProblemDetails {
64+
Status = StatusCodes.Status500InternalServerError,
65+
Title = "An error occurred while processing your request",
66+
Instance = httpContext.Request.Path
67+
}, cancellationToken);
68+
}
69+
70+
return true;
71+
}
72+
}

‎src/Web/Grand.Web.Common/Startup/ErrorHandlerStartup.cs‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,11 @@ public class ErrorHandlerStartup : IStartupApplication
1919
/// <param name="configuration">Configuration root of the application</param>
2020
public void ConfigureServices(IServiceCollection services, IConfiguration configuration)
2121
{
22+
// Register RFC 7807 ProblemDetails support (used by GrandExceptionHandler for API errors)
23+
services.AddProblemDetails();
24+
25+
// Register the IExceptionHandler implementation used by UseExceptionHandler()
26+
services.AddExceptionHandler<GrandExceptionHandler>();
2227
}
2328

2429
/// <summary>

‎src/Web/Grand.Web/App_Data/appsettings.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
{
22
"Application": {
33
//Enable if you want to see the full error in production environment. It's ignored (always enabled) in development environment
4-
"DisplayFullErrorStack": true,
4+
"DisplayFullErrorStack": false,
55
//Value of "Cache-Control" header value for static content
66
"StaticFilesCacheControl": "public,max-age=31536000",
77
//Enable the session-based TempData provider

0 commit comments

Comments
 (0)