-
-
Notifications
You must be signed in to change notification settings - Fork 582
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
update B405 rules #1120
update B405 rules #1120
Conversation
make B405 rules more specific. Because not all in the module is related to parse xml. Some of them is needed for typing, for example Element.
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 don't believe this fixes the things you actually want fixed
"xml.etree.ElementTree.fromstring", | ||
"xml.etree.ElementTree.iterparse", | ||
"xml.etree.ElementTree.parse", |
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 don't believe you can import fromstring
, iterparse
, or parse
as a result this doesn't do what you want.
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.
Ah sorry, looks like B314 rules already cover it. dumb me.
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.
What do you think, because B405 rules are too general and make the entire contents of the ElementTree
module seem dangerous from a security perspective even though there are functions or classes that are not dangerous, for example Element
and that is needed for typing. Do I need to add all the functions or classes that are dangerous and not include those that have nothing to do with security as I have listed above?
Before the changes in this PR, this code will warn B405 rules. But after changes in this PR, this code will not warn B405.
import xml.etree.ElementTree
And after changes in this PR too, this code will warn B405, although bandit only warn the fromstring
and the rest is not. Looks weird.
from xml.etree.ElementTree import fromstring, parse, iterparse, XMLParser
If there was a way for bandit to raise a warning to a module but some of the contents of that module didn't raise a warning things will become simple and and I don't have to give # nosec: B405
everywhere.
I don't believe you can import fromstring, iterparse, or parse as a result this doesn't do what you want.
This code will result in ModuleNotFoundError
.
import xml.etree.ElementTree.fromstring
import xml.etree.ElementTree.parse
import xml.etree.ElementTree.iterparse
import xml.etree.ElementTree.XMLParser
instead someone will use this
from xml.etree.ElementTree import fromstring, parse, iterparse, XMLParser
make B405 rules more specific. Because not all in the module is related to parse xml. Some of them is needed for typing, for example
Element
. the reason why only theElementTree
module is updated is because defusedxml says in its readme thatcElementTree
is deprecated.This PR update the B405 rules, and make it more specific by add these
parse()
,iterparse()
,fromstring()
,XMLParser
.This is necessary because the contents of the
ElementTree
module are not all related to parsing xml. Some of them are needed for typing, such asElement
.From the docs stated that there is more functions or classes that relates with parsing. Here are some, but i may miss some.
The main reason why only these
parse()
,iterparse()
,fromstring()
,XMLParser
that is included in B405 rules, because defusedxml document it and the test example files test it. Maybe some functions or classes listed above could be included. So this PR will change the B405 rules which previously would give warnings about the entire contents of theElementTree
module, to justIssue: #709
Tagging people related on issue: @vanschelven @seanmceligot to find out what is missed in this PR.