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

Proper handling of Ajax exceptions #999

Closed

Conversation

msimkunas
Copy link
Contributor

@msimkunas msimkunas commented Oct 21, 2023

Unhandled exceptions in Ajax handlers are currently relayed back to the user. Since they can potentially contain sensitive details about the system, such as absolute file paths, these exceptions should be properly masked.

Depends on wintercms/storm#160.

@bennothommo
Copy link
Member

@msimkunas should this perhaps only occur when in production mode?

@msimkunas
Copy link
Contributor Author

@msimkunas should this perhaps only occur when in production mode?

I don't see much benefit in handling these cases differently. The actual error will be logged regardless.

@msimkunas
Copy link
Contributor Author

I think it makes sense to handle debug/prod mode differently when it comes to regular error handling because in those cases the developer is presented with a detailed stack trace and debugging info when in debug mode.

This case, however, is different. We're not showing any kind of detailed debugging info, such as a stack trace, inside the client side error alert so the developer will be forced regardless to go through the logs manually. Showing just the exception message isn't really all that helpful to a developer so I would suggest to not differentiate between dev modes in this particular case.

@bennothommo
Copy link
Member

@msimkunas this may have an effect on client-side error reporting. Immediately, I can think this might affect the Debugbar plugin, because it has some special handling for exceptions thrown in an AJAX call but might not necessarily be able to handle linked/previous exceptions (it also has some special handling for AjaxException throws themselves, but I don't think they're affected by your changes).

If I'm wrong and the Debugbar handles it fine, I'm happy for this PR to proceed.

@msimkunas
Copy link
Contributor Author

msimkunas commented Oct 24, 2023

@bennothommo I've discussed this with @LukeTowers and he suggested throwing an AjaxException and logging the actual exception with trace_log(). This way, the supperfluous AjaxException which is only intended for the end user will not be logged and the debugbar plugin wouldn't have to handle nested exceptions.

I haven't yet tested this but I'll give it a go soon

@bennothommo
Copy link
Member

@msimkunas sounds good to me. :)

@LukeTowers
Copy link
Member

I didn't think of the debugbar plugin, that would be a good point.

@LukeTowers
Copy link
Member

@msimkunas were you able to take a stab at this?

LukeTowers pushed a commit to wintercms/storm that referenced this pull request Dec 21, 2023
@msimkunas
Copy link
Contributor Author

@LukeTowers Not yet but I'll try to after the holidays

Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in this pull request for the last 6 months.
If you intend to continue working on this pull request, please respond within 3 days.
If this pull request is critical for your business, please reach out to us at [email protected].

@github-actions github-actions bot added the stale Issues/PRs that have had no activity and may be archived label Jun 21, 2024
@github-actions github-actions bot closed this Jun 24, 2024
@LukeTowers LukeTowers reopened this Jun 25, 2024
@LukeTowers
Copy link
Member

@msimkunas are you still interested in making the necessary modifications to get this merged?

@LukeTowers
Copy link
Member

Also should we add this handling logic to the backend controller base class as well?

@github-actions github-actions bot closed this Jun 29, 2024
@msimkunas
Copy link
Contributor Author

@LukeTowers I’m sorry but my plans have changed and I’m no longer working on this. Maybe someone else can pick up the mantle…

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Issues/PRs that have had no activity and may be archived
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants