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

Refs #29714 -- Added support for SafeExceptionReporterFilter to filter sensitive cookies #10886

Closed
wants to merge 1 commit into from

Conversation

nasirhjafri
Copy link
Contributor

@nasirhjafri nasirhjafri changed the title [WIP] Fixed #29714 -- Added support for SafeExceptionReporterFilter to filter sensitive cookies Fixed #29714 -- Added support for SafeExceptionReporterFilter to filter sensitive cookies Jan 23, 2019
@nasirhjafri nasirhjafri changed the title Fixed #29714 -- Added support for SafeExceptionReporterFilter to filter sensitive cookies Refs #29714 -- Added support for SafeExceptionReporterFilter to filter sensitive cookies Jan 23, 2019
Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hi @nasirhjafri.

Thanks for this.

The approach here isn't right for what's on the ticket. There, the idea is to make ExceptionReporter an attribute on AdminHandler so that it can be easily subclassed.

On the adjustments to SafeExceptionReporterFilter :

In general, there are a number of very similar requests to customise the error reporter output. (Not ALL of those, but several...) — If we added API to SafeExceptionReporterFilter for each of them, it'll become out of hand, so it's likely we need to favour a different approach. Whatever solution we take, I think we need to consider all the tickets here. (There's thoughts of deprecating SafeExceptionReporterFilter as the extension point entirely...)

In particular, adding settings wouldn't be the way to go. Rather adding attributes that could be overridden in a subclass would be preferred.

I'm going to close this as-is. If you want to re-open another PR exposing ExceptionReporteronAdminHandler` that would be great.

Thanks again.

@nasirhjafri nasirhjafri deleted the ticket_29714 branch February 24, 2019 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants