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

flake8-bandit import check should not trigger on TYPE_CHECKING imports or classes not in defusedxml #14901

Open
scy opened this issue Dec 10, 2024 · 2 comments
Labels
help wanted Contributions especially welcome

Comments

@scy
Copy link

scy commented Dec 10, 2024

The following code triggers S408 ("xml.dom.minidom is vulnerable to XML attacks"):

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    from xml.dom.minidom import Element

As far as I know, defusedxml, which this rule suggests as an alternative, does not supply alternative implementations for most of the types, only of some functions. In other words, I have to import types like these for the standard library; there is no defusedxml alternative.

So in order to signal to Ruff that "this is fine"™, I've tried moving the import to TYPE_CHECKING, but still received the same error.

This probably applies to other rules in the S4xx range, too.

@scy
Copy link
Author

scy commented Dec 10, 2024

The following code has a similar problem, triggering S409 on the pulldom import, but this time I can't even move the import into a TYPE_CHECKING block:

from xml.dom import pulldom

from defusedxml.pulldom import parseString


doc = parseString("<foo></foo>")

for ev, node in doc:
    if ev == pulldom.START_ELEMENT:
        print(node)

Again, defusedxml does not provide everything required for parsing XML; here it's the event constant to compare against. Nevertheless I'm using defusedxml for the actual parsing, which means I'm not vulnerable.

scy added a commit to AKVorrat/dearmep that referenced this issue Dec 11, 2024
This is a security warning, but I'm only importing types that don't have
a defusedxml alternative anyway. Furthermore, there should be no
warnings in a `TYPE_CHECKING` block. I've opened
<astral-sh/ruff#14901> for this.

Signed-off-by: Tim Weber <[email protected]>
scy added a commit to AKVorrat/dearmep that referenced this issue Dec 11, 2024
Again, this is a false positive; `defusedxml` doesn't provide the
constants we import `pulldom` for. Added to the existing issue report at
<astral-sh/ruff#14901 (comment)>.

Signed-off-by: Tim Weber <[email protected]>
@MichaReiser
Copy link
Member

Thanks. This makes sense to me. So what we need to do here is to allow some imports from xml.dom or at least when under the TYPE_CHECKING

@MichaReiser MichaReiser added the help wanted Contributions especially welcome label Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Contributions especially welcome
Projects
None yet
Development

No branches or pull requests

2 participants