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
DM-40247: Improve webdav interface as suggested by ruff S checks #63
Conversation
The standard library 'xml' package is vulnerable to XML bombs. See: * https://en.wikipedia.org/wiki/Billion_laughs_attack * https://realpython.com/python-xml-parser/#defuse-the-xml-bomb-with-secure-parsers
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #63 +/- ##
==========================================
+ Coverage 85.86% 85.91% +0.05%
==========================================
Files 27 27
Lines 3913 3927 +14
Branches 801 805 +4
==========================================
+ Hits 3360 3374 +14
Misses 433 433
Partials 120 120
☔ View full report in Codecov by Sentry. |
To fix the mypy problem add something like to [mypy-defusedxml.*]
ignore_missing_imports = True This PR works with both xml and defusedxml so we do not have to wait on the rubin-env RFC. Also, there is no reason why we can't add defusedxml as a requirement in the |
Thanks @timj for your feedback. I added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks.
str(path), | ||
stream=False, | ||
timeout=( | ||
_timeout_from_environment("LSST_HTTP_TIMEOUT_CONNECT", 30.0), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we be reading the environment variables here if we are saying that OPTIONS should use a shorter timeout than other activities?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought using the timeout values specified by the user via the environment variables for OPTIONS
as well would be better for consistency. In addition, using the environment allows the user to configure the environment for using a potential slow server, as opposed to hardcoding a magic value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, okay. I was concerned that people might want different values for OPTIONS and other things but if the env var is only used for testing it doesn't matter.
Checklist
doc/changes