Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make exception handling policy configurable for CORS #147

Closed
itrofimow opened this issue Mar 28, 2018 · 15 comments
Closed

Make exception handling policy configurable for CORS #147

itrofimow opened this issue Mar 28, 2018 · 15 comments
Assignees
Milestone

Comments

@itrofimow
Copy link

In project i am working at custom exception handling OwinMiddleware is used as first in the pipeline, which catches every exception thrown by application and maps it to httpResponse/statusCode based on some logic. We replace IExceptionHanlder with a handler that just rethrows , thus we have a single global exception hanlder(middleware). The problem arised when we tried to add CORS - its System.Web.Http.Cors.CorsMessageHandler.SendAsync swallows exeptions and processes them by its own logic and that just broke plenty of things.

We ended up with basically copy/paste of that Handler with SendAsync not catching everything, then we had to replace CorsMessageHanlder at HttpConfigurations.MessageHanlders after EnsureInitialized has been called and that looks pretty nasty.

So what i am asking for - is it possible to make that exception handling policy configurable?
Or did i miss something and it already is?

@mkArtakMSFT
Copy link
Member

Thanks for contacting us, @itrofimow,
@blowdart, @Tratcher any suggestions?

@Tratcher
Copy link
Member

Tratcher commented Apr 2, 2018

Yeah, this try/catch is capturing way too much:

try
{
if (corsRequestContext.IsPreflight)
{
return await HandleCorsPreflightRequestAsync(request, corsRequestContext, cancellationToken);
}
else
{
return await HandleCorsRequestAsync(request, corsRequestContext, cancellationToken);
}
}
catch (Exception exception)
{
return HandleException(request, exception);
}

It would be one thing if it were only catching exceptions for its own logic, but it's also catching exceptions from any upstream handlers.

Fixing it would have to be opt-in config to avoid breaking people.

@dougbu dougbu self-assigned this Apr 3, 2018
@dougbu
Copy link
Member

dougbu commented Apr 3, 2018

I'll look at this and see what the approach might look like. We can decide based on that…

@itrofimow
Copy link
Author

You guys are the best.

@itrofimow
Copy link
Author

Hey guys. Any recent activity on this?
I might submit a PR if someone shares with me the design solution

@mkArtakMSFT
Copy link
Member

@dougbu, can you please share your findings here?

@itrofimow
Copy link
Author

Hi.
Any recent activity on this?
I mean its been a while, may be i can help somehow?

@dougbu
Copy link
Member

dougbu commented Jul 6, 2018

@itrofimow we have been focused on other priorities. What's needed here is an extensibility point to more easily override or hook into Cors exception handling. I haven't looked closely but other approaches might also solve the issue.

If you would like to give it a shot, feel free. Please open your PR early so that we can provide feedback on your design while it's a work in progress.

@itrofimow
Copy link
Author

Yeah, sure. Thanks

itrofimow pushed a commit to itrofimow/AspNetWebStack that referenced this issue Aug 8, 2018
@itrofimow itrofimow mentioned this issue Aug 8, 2018
@itrofimow
Copy link
Author

@dougbu ping

@itrofimow
Copy link
Author

I've submitted a PR for this, could you please take a look at it? There's just a few non-breaking changes

@MaciekFlis
Copy link

Hi,

Did anyone have a chance to take a look at this? Maybe the labels for this should be updated since we already have a PR? Would be great to know timeline or at least priority for this

@dougbu
Copy link
Member

dougbu commented Aug 16, 2018

I plan to look at #185 tomorrow or over the weekend.

@dougbu dougbu added this to the 3.2.7 milestone Aug 21, 2018
dougbu added a commit that referenced this issue Aug 21, 2018
@dougbu
Copy link
Member

dougbu commented Aug 21, 2018

8044f4e

@dougbu
Copy link
Member

dougbu commented Aug 21, 2018

Many thanks @itrofimow!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants