Skip to content

Fix for UseExceptionHandler no longer working since v10.3 RC#13218

Merged
nikolajlauridsen merged 3 commits intoumbraco:v11/contribfrom
justin-nevitech:exception-handler-issue-20221016
Oct 26, 2022
Merged

Fix for UseExceptionHandler no longer working since v10.3 RC#13218
nikolajlauridsen merged 3 commits intoumbraco:v11/contribfrom
justin-nevitech:exception-handler-issue-20221016

Conversation

@justin-nevitech
Copy link
Copy Markdown
Contributor

@justin-nevitech justin-nevitech commented Oct 16, 2022

Summary

This PR fixes an issue with UseExceptionHandler() which no longer works since v10.3 RC.

Description

If you add a custom exception handler to the Configure method in Startup.cs using UseExceptionHandler() pointing to a local static HTML file, this exception handler is no longer called as the exception handler introduced with the new 10.3 Backoffice management API inside ManagementApiComposer.cs takes over and returns the exception as JSON rather than rendering your error page.

The fix for this is to only handle API exceptions and return the error as JSON if the error occurs during a request for an API (i.e. where the path starts with /umbraco/api/). This means any errors during normal page requests will be handled by your error page and will display a nice friendly message rather than an unfriendly JSON response (which returns far too much error information).

This is an example of the JSON error response:

{"type":"Error","title":"Error!","status":500,"detail":"   at AspNetCore.Views_HomePage.ExecuteAsync() in C:\\My Documents\\Projects\\Open Source\\Umbraco-CMS\\src\\Umbraco.Web.UI\\Views\\HomePage.cshtml:line 39\r\n   at Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderPageCoreAsync(IRazorPage page, ViewContext context)\r\n   at Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderPageAsync(IRazorPage page, ViewContext context, Boolean invokeViewStarts)\r\n   at Microsoft.AspNetCore.Mvc.Razor.RazorView.RenderAsync(ViewContext context)\r\n   at Microsoft.AspNetCore.Mvc.ViewFeatures.ViewExecutor.ExecuteAsync(ViewContext viewContext, String contentType, Nullable`1 statusCode)\r\n   at Microsoft.AspNetCore.Mvc.ViewFeatures.ViewExecutor.ExecuteAsync(ViewContext viewContext, String contentType, Nullable`1 statusCode)\r\n   at Microsoft.AspNetCore.Mvc.ViewFeatures.ViewExecutor.ExecuteAsync(ActionContext actionContext, IView view, ViewDataDictionary viewData, ITempDataDictionary tempData, String contentType, Nullable`1 statusCode)\r\n   at Microsoft.AspNetCore.Mvc.ViewFeatures.ViewResultExecutor.ExecuteAsync(ActionContext context, ViewResult result)\r\n   at Microsoft.AspNetCore.Mvc.ViewResult.ExecuteResultAsync(ActionContext context)\r\n   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResultFilterAsync>g__Awaited|30_0[TFilter,TFilterAsync](ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)\r\n   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResultExecutedContextSealed context)\r\n   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.ResultNext[TFilter,TFilterAsync](State& next, Scope& scope, Object& state, Boolean& isCompleted)\r\n   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeResultFilters()\r\n--- End of stack trace from previous location ---\r\n   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeNextResourceFilter>g__Awaited|25_0(ResourceInvoker invoker, Task lastTask, State next, Scope scope, Object state, Boolean isCompleted)\r\n   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Rethrow(ResourceExecutedContextSealed context)\r\n   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.Next(State& next, Scope& scope, Object& state, Boolean& isCompleted)\r\n   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.InvokeFilterPipelineAsync()\r\n--- End of stack trace from previous location ---\r\n   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)\r\n   at Microsoft.AspNetCore.Mvc.Infrastructure.ResourceInvoker.<InvokeAsync>g__Awaited|17_0(ResourceInvoker invoker, Task task, IDisposable scope)\r\n   at Microsoft.AspNetCore.Routing.EndpointMiddleware.<Invoke>g__AwaitRequestTask|6_0(Endpoint endpoint, Task requestTask, ILogger logger)\r\n   at Umbraco.Cms.Web.Common.Middleware.BasicAuthenticationMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in C:\\My Documents\\Projects\\Open Source\\Umbraco-CMS\\src\\Umbraco.Web.Website\\Middleware\\BasicAuthenticationMiddleware.cs:line 62\r\n   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()\r\n--- End of stack trace from previous location ---\r\n   at Umbraco.Cms.Web.BackOffice.Middleware.BackOfficeExternalLoginProviderErrorMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in C:\\My Documents\\Projects\\Open Source\\Umbraco-CMS\\src\\Umbraco.Web.BackOffice\\Middleware\\BackOfficeExternalLoginProviderErrorMiddleware.cs:line 50\r\n   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()\r\n--- End of stack trace from previous location ---\r\n   at NSwag.AspNetCore.Middlewares.SwaggerUiIndexMiddleware.Invoke(HttpContext context)\r\n   at NSwag.AspNetCore.Middlewares.RedirectToIndexMiddleware.Invoke(HttpContext context)\r\n   at NSwag.AspNetCore.Middlewares.OpenApiDocumentMiddleware.Invoke(HttpContext context)\r\n   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)\r\n   at Microsoft.AspNetCore.Session.SessionMiddleware.Invoke(HttpContext context)\r\n   at Microsoft.AspNetCore.Localization.RequestLocalizationMiddleware.Invoke(HttpContext context)\r\n   at Microsoft.AspNetCore.Authorization.AuthorizationMiddleware.Invoke(HttpContext context)\r\n   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)\r\n   at SixLabors.ImageSharp.Web.Middleware.ImageSharpMiddleware.Invoke(HttpContext httpContext, Boolean retry)\r\n   at StackExchange.Profiling.MiniProfilerMiddleware.Invoke(HttpContext context) in C:\\projects\\dotnet\\src\\MiniProfiler.AspNetCore\\MiniProfilerMiddleware.cs:line 119\r\n   at Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in C:\\My Documents\\Projects\\Open Source\\Umbraco-CMS\\src\\Umbraco.Web.Common\\Middleware\\UmbracoRequestMiddleware.cs:line 171\r\n   at Umbraco.Cms.Web.Common.Middleware.UmbracoRequestMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in C:\\My Documents\\Projects\\Open Source\\Umbraco-CMS\\src\\Umbraco.Web.Common\\Middleware\\UmbracoRequestMiddleware.cs:line 177\r\n   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()\r\n--- End of stack trace from previous location ---\r\n   at Umbraco.Cms.Web.Common.Middleware.PreviewAuthenticationMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in C:\\My Documents\\Projects\\Open Source\\Umbraco-CMS\\src\\Umbraco.Web.Common\\Middleware\\PreviewAuthenticationMiddleware.cs:line 76\r\n   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()\r\n--- End of stack trace from previous location ---\r\n   at Umbraco.Cms.Web.Common.Middleware.UmbracoRequestLoggingMiddleware.InvokeAsync(HttpContext context, RequestDelegate next) in C:\\My Documents\\Projects\\Open Source\\Umbraco-CMS\\src\\Umbraco.Web.Common\\Middleware\\UmbracoRequestLoggingMiddleware.cs:line 42\r\n   at Microsoft.AspNetCore.Builder.UseMiddlewareExtensions.<>c__DisplayClass6_1.<<UseMiddlewareInterface>b__1>d.MoveNext()\r\n--- End of stack trace from previous location ---\r\n   at Microsoft.AspNetCore.Diagnostics.ExceptionHandlerMiddleware.<Invoke>g__Awaited|6_0(ExceptionHandlerMiddleware middleware, HttpContext context, Task task)","instance":"Exception"}

Steps to Test

  1. Create a static error.html page in your wwwroot with the following content:
<!DOCTYPE html>
<html>
<head>
    <meta charset="utf-8" />
    <title></title>
</head>
<body>
  <h1>Error!</h1>
</body>
</html>
  1. In your startup.cs, comment out the following lines in the Configure method:
//if (env.IsDevelopment())
//{
    //app.UseDeveloperExceptionPage();
//}
  1. Add the following line directly before or after the lines you have just commented out:
app.UseExceptionHandler("/error.html");
  1. Add a home page document type and template to your site.
  2. Add the following code to your home page template:
@{
    throw new Exception("Error!");
}
  1. Add the home page content and navigate to the root of your site.
  2. The exception should be handled in your exception handler and the static error.html page should be displayed (with 10.3 RC this was showing a JSON response with the error).

Note - there is an existing issue where Umbraco pages cannot be used as error pages, see: #13052. You will only be able to use a static HTML file as an error page to test this.

All tests have been run and pass.

@github-actions
Copy link
Copy Markdown

Hi there @justin-nevitech, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

  • It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
  • The automated tests all pass (see "Checks" tab on this PR)
  • The level of security for this contribution is the same or improved
  • The level of performance for this contribution is the same or improved
  • Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
  • If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
  • 💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

@nul800sebastiaan nul800sebastiaan changed the base branch from v10/contrib to v11/contrib October 25, 2022 12:53
Copy link
Copy Markdown
Contributor

@nikolajlauridsen nikolajlauridsen left a comment

Choose a reason for hiding this comment

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

Hey @justin-nevitech thanks for the PR, the changes looks good and works good as well 👍

It took the liberty of updating the management API path since we've changed it to /umbraco/management/api/ 😄

I'm a bit curious when you say that this broke something in v10.3rc considering that the new backoffice management API is not released anywhere, and thus sites on v10 cannot take a reference to it, so the ManagementApiComposer will never get run. It only runs when working directly with the source code, but maybe that's what you meant. I'm just a bit confused here 😄

But regardless this is (or rather was now) a problem with the management API, so big H5YR for this fix :) I'll go ahead and get this merged.

@justin-nevitech
Copy link
Copy Markdown
Contributor Author

Hi @nikolajlauridsen

The management API is in the v10 contrib branch so I assumed that was also in the V10.3 RC (possibly a wrong assumption). I wasn't sure how to get a copy of the code for V10.3 RC as I didn't know what the branch was called? I only came across this issue as I was trying to fix another issue and ended up seeing a JSON error message.

@nikolajlauridsen
Copy link
Copy Markdown
Contributor

nikolajlauridsen commented Oct 26, 2022

Ahh okay, I see, that makes sense 😄

But no, we've set the projects in the NewBackoffice solution folder as non-packable, so they won't get released. These are the new APIs that we want to use with the new backoffice (targeting v13), so these are currently not used. The reason for them being in the v10 and v11 branches is to avoid having to deal with too many merge conflicts, and of course, because it gets people a chance to take a peek 😉.

In regards to releases, those are prefixed with release/, so for 10.3 the branch was named release/10.3, but these branches are short-lived, so it doesn't exist anymore, so if you want to see the code for 10.3 you have to check out the tag release-10.3.0 😄

@nikolajlauridsen nikolajlauridsen merged commit f5aba34 into umbraco:v11/contrib Oct 26, 2022
nikolajlauridsen added a commit that referenced this pull request Oct 26, 2022
* Fix for UseExceptionHandler no longer working since v10.3 RC

* Update the management api path to match the new one

Co-authored-by: Nikolaj <nikolajlauridsen@protonmail.ch>
@justin-nevitech
Copy link
Copy Markdown
Contributor Author

Hi @nikolajlauridsen

That makes sense, although that means the V10 contrib branch still runs the code as it exists there even if it was not released as part of v10, so running Umbraco via the source code can (and did in this example) result in different behaviour to what would get released. I think that could potentially be a bad thing as that means you are developing against code you are not releasing? Just a thought...

Thanks for the tip re the tags!

@nikolajlauridsen
Copy link
Copy Markdown
Contributor

nikolajlauridsen commented Oct 26, 2022

I see your point, and the runtime will indeed be somewhat different running via the source code (as shown by this PR), but since Umbraco.Web.UI and Umbraco.Tests.Integration is the only project which has a reference to the new management API it's not something I'm that concerned about it since you can't depend on the code itself.

But it's something worth considering, maybe adding a switch of some kind so the composer only runs if you opt-in to it, or something similar, I'll take it up with the team 😄

@justin-nevitech
Copy link
Copy Markdown
Contributor Author

Thanks @nikolajlauridsen

Could you mark this up as a Hacktoberfest contribution please?

@nikolajlauridsen
Copy link
Copy Markdown
Contributor

Oh yes of course, my bad sorry 😅

@justin-nevitech
Copy link
Copy Markdown
Contributor Author

@nikolajlauridsen Thanks!

Zeegaan added a commit that referenced this pull request Nov 8, 2022
* Bump version

* Add IContextCache to deploy connectors (#13287)

* Add IContextCache and implementations

* Update connector interfaces to use IContextCache

* Minor cleanup

* Move DeployContextCache prefix to constant

* Move default implementations to obsolete methods

* Remove DeployContextCache and DictionaryCache

Co-authored-by: Andy Butland <abutland73@gmail.com>

* Add IContextCache to deploy connectors (#13287)

* Add IContextCache and implementations

* Update connector interfaces to use IContextCache

* Minor cleanup

* Move DeployContextCache prefix to constant

* Move default implementations to obsolete methods

* Remove DeployContextCache and DictionaryCache

Co-authored-by: Andy Butland <abutland73@gmail.com>

* Parse lockId as invariant (#13284)

Co-authored-by: Zeegaan <nge@umbraco.dk>

* Fix Sqlite database locking issue (#13246)

* Add locking for creating scope

* Lock the repository instead

* Add scope in action instead of locking in service

* Fix up post-merge

Co-authored-by: Zeegaan <nge@umbraco.dk>

* Bump version to next minor

* Fix for UseExceptionHandler no longer working since v10.3 RC (#13218)

* Fix for UseExceptionHandler no longer working since v10.3 RC

* Update the management api path to match the new one

Co-authored-by: Nikolaj <nikolajlauridsen@protonmail.ch>

* New backoffice: Cleanup management API routes (#13296)

* Rename ModelsBuilderDashboard folder to ModelsBuilder

* Fix modelsbuilder paths and related naming

* Rename analytics route to telemetry

* Fix controller bases - routes and tags

* Fix items route

* Fix more controllerbase routes

* Fix route

* Fix OpenApi file

* Merging DictionaryItem and Dictionary

* Fix TrackedReferences naming

* Update OpenApi file

* Rename Analytics* related types to Telemetry*

* New Backoffice: Return AnalyticsLevelViewModel from Telemetry/ (#13298)

* Return TelemetryLevelViewModel instead of TelemetryLevel

* Fix schema

* Change telemetry/current to telemetry/level

(cherry picked from commit f2b8494)

* Add contants for tree and recycle-bin subpaths

(cherry picked from commit 4449f56)

Co-authored-by: Mole <nikolajlauridsen@protonmail.ch>

* Updated Smidge, Npoco and MailKit (#13310)

* Updated Smidge, Npoco and MailKit

* Added missing command (after breaking interface in npoco)

* OpenId Connect authentication for new management API (#13318)

* First attempt at OpenIddict

* Making headway and more TODOs

* Redo current policies for multiple schemas + clean up auth controller

* Fix bad merge

* Clean up some more test code

* Fix spacing

* Include AddAuthentication() in OpenIddict addition

* A little more clean-up

* Move application creation to its own implementation + prepare for middleware to handle valid callback URL

* Enable refresh token flow

* Fix bad merge from v11/dev

* Support auth for Swagger and Postman in non-production environments + use default login screen for back-office logins

* Add workaround to client side login handling so the OAuth return URL is not corrupted before redirection

* Add temporary configuration handling for new backoffice

* Restructure the code somewhat, move singular responsibility from management API project

* Add recurring task for cleaning up old tokens in the DB

* Fix bad merge + make auth controller align with the new management API structure

* Explicitly handle the new management API path as a backoffice path (NOTE: this is potentially behaviorally breaking!)

* Redo handle the new management API requests as backoffice requests, this time in a non-breaking way

* Add/update TODOs

* Revert duplication of current auth policies for OpenIddict (as it breaks everything for V11 without the new management APIs) and introduce a dedicated PoC policy setup for OpenIddict.

* Fix failing unit tests

* Update src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Update src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Update src/Umbraco.Cms.ManagementApi/Security/BackOfficeApplicationManager.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* Update src/Umbraco.Core/Routing/UmbracoRequestPaths.cs

Co-authored-by: Nikolaj Geisle <70372949+Zeegaan@users.noreply.github.com>

* V11: Using IFileProvider to access assets added from packages (#13141)

* Creating a FileProviderFactory for getting the package.manifest and grid.editors.config.js files through a file provider

* Collecting the package.manifest-s from different sources

* Searching different sources for grid.editors.config.js

* Using an IFileProvider to collect all tours

* Refactoring IconService.cs

* Typo

* Optimizations when looping through the file system

* Moving WebRootFileProviderFactory to Umbraco.Web.Common proj

* Removes double registering

* pluginLangFileSources includes the localPluginFileSources

* Comments

* Remove linq from foreach

* Change workflow for grid.editors.config.js so we check first physical file, then RCL, then Embedded

* Clean up

* Check if config dir exists

* Discover nested package.manifest files

* Fix IFileInfo.PhysicalPath check

* Revert 712810e as that way files in content root are preferred over those in web root

* Adding comments

* Refactoring

* Remove PhysicalPath check

* Fix registration of WebRootFileProviderFactory

* Use Swashbuckle instead of NSwag (#13350)

* First attempt at OpenIddict

* Making headway and more TODOs

* Redo current policies for multiple schemas + clean up auth controller

* Fix bad merge

* Clean up some more test code

* Fix spacing

* Include AddAuthentication() in OpenIddict addition

* A little more clean-up

* Move application creation to its own implementation + prepare for middleware to handle valid callback URL

* Enable refresh token flow

* Fix bad merge from v11/dev

* Support auth for Swagger and Postman in non-production environments + use default login screen for back-office logins

* Add workaround to client side login handling so the OAuth return URL is not corrupted before redirection

* Add temporary configuration handling for new backoffice

* Restructure the code somewhat, move singular responsibility from management API project

* Add recurring task for cleaning up old tokens in the DB

* Fix bad merge + make auth controller align with the new management API structure

* Explicitly handle the new management API path as a backoffice path (NOTE: this is potentially behaviorally breaking!)

* Redo handle the new management API requests as backoffice requests, this time in a non-breaking way

* Add/update TODOs

* Replace NSwag with Swashbuckle and clean up unnecessary client secret workaround

* Revert duplication of current auth policies for OpenIddict (as it breaks everything for V11 without the new management APIs) and introduce a dedicated PoC policy setup for OpenIddict.

* Fix failing unit tests

* A little niceness + export new OpenApi.json and fix path in contract unit test

* Redo after merge with v11/dev + filter out unwanted mime types

* Remove CreatedResult and NotFoundObjectResult where possible

* Custom schema IDs - no more "ViewModel" postfix and make generic lists look less clunky too

* A little more explanation for generic schema ID generation

* Force Swashbuckle to use enum string names

* Update OpenApi.json to match new enum string values

* Add clarifying comment about weird looking construct

* add workflow to schema (#13349)

* add workflow to schema

* add licenses to CMSDefinition - intentionally only adding to schema, not registered as options

Co-authored-by: Nikolaj <nikolajlauridsen@protonmail.ch>
Co-authored-by: Ronald Barendse <ronald@barend.se>
Co-authored-by: Andy Butland <abutland73@gmail.com>
Co-authored-by: Zeegaan <nge@umbraco.dk>
Co-authored-by: Justin Neville <67802060+justin-nevitech@users.noreply.github.com>
Co-authored-by: Elitsa Marinovska <21998037+elit0451@users.noreply.github.com>
Co-authored-by: Bjarke Berg <mail@bergmania.dk>
Co-authored-by: Kenn Jacobsen <kja@umbraco.dk>
Co-authored-by: Nathan Woulfe <nathan@nathanw.com.au>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants