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

fix for #7516 #7518

Closed
wants to merge 1 commit into from
Closed

fix for #7516 #7518

wants to merge 1 commit into from

Conversation

a-recknagel
Copy link
Contributor

@a-recknagel a-recknagel commented Apr 20, 2020

Bugfix

Subject: Enable secure object inspection in autodoc'd modules.

Purpose

Inspecting objects in user modules can be risky if they raise errors in their __getattr__ implementations. One such case is the flask.request proxy which will complain about not being in a valid context if is accessed in any way and throws a RuntimeError.

Detail

I just wrapped the access in a catchall, since users can raise whatever Exception they want. Maybe BaseException/a bare except would be even better, but it seemed risky to include SystemExit.

Relates

#7516

@a-recknagel
Copy link
Contributor Author

@tk0miya since you are familiar with this section of the code, can you review this fix to check if it is fine?

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

Thank you for your work. But we have similar implementation named safe_getattr(). So it would be better to use it instead of getattr(). Sorry, I forgot to use it...
Could you update this, please?

@tk0miya
Copy link
Member

tk0miya commented Apr 20, 2020

In addition, could you rebase this into 3.0.x branch? Then I'll merge this and release as 3.0.3.

@a-recknagel a-recknagel mentioned this pull request Apr 20, 2020
@a-recknagel
Copy link
Contributor Author

Closed in favor of #7520

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants