Bandit 1.7.5 false positive for request_without_timeout (B113)

[volans-]

Describe the bug

Bandit is incorrectly marking calls to requests library without a timeout while the code it's actually not calling directly the requests library and the timeout is already set elsewhere.

Reproduction steps

1. Define this code:

from mylibrary import my_session

class Repro:

    def __init__(self):
        self.requests = my_session(timeout=5)

    def get(self, uri):
        return self.requests.get(uri)

2. Run bandit: bandit -l -i -r --skip B404,B603 somepath/

3. Get an error:

>> Issue: [B113:request_without_timeout] Requests call without timeout
   Severity: Medium   Confidence: Low
   CWE: CWE-400 (https://cwe.mitre.org/data/definitions/400.html)
   More Info: https://bandit.readthedocs.io/en/1.7.5/plugins/b113_request_without_timeout.html
   Location: repro.py:10:15
9	    def get(self, uri):
10	        return self.requests.get(uri)
11

Expected behavior

Bandit should not report any error because the session has a default timeout set via an HTTPAdapter in another library.

The code calls self.requests that could be any kind of object, does bandit do code inspection of the object to detect that is actually a requests session? I don't think so as it triggered the issue also with my pseudo code that doesn't import the real library. If it was indeed doing introspection, it should probably also check if there is a default timeout set in the session.

Bandit version

1.7.5 (Default)

Python version

3.9,3.10

Additional context

[note] The dropdown menu of the issues template here on Github has a Python 3.1 version and is missing 3.10, possibly a typo in the template.

[ericwb]

Yes, if you look back at PR #743 that created this new check, there was concern over introducing such a low confidence plugin. The best course of action for now would be to skip the test if you're getting too many false positives.

[mathieu-lemay]

I've found a few more examples of false positives.

1. Getting merge requests with the GitLab client: merge_request = project.mergerequests.get(pull_request_id, lazy=True)
2. Setting up mocks with requests_mock: requests_mock.post("https://example.org", json={})

[tucked]

This repros...

import requests
requests.post(
  "https://httpbin.org/post", timeout=60 * 3
)

edit: It's because of the 60 * 3 instead of 180 😩

[ericwb]

Should be fixed with #1011

[tucked]

@ericwb, this still hits on 3260f137873798b4b0c0c289373cc5e8fa2d93ed:

>> Issue: [B113:request_without_timeout] Requests call without timeout
   Severity: Medium   Confidence: Low
   CWE: CWE-400 (https://cwe.mitre.org/data/definitions/400.html)
   More Info: https://bandit.readthedocs.io/en/1.7.6/plugins/b113_request_without_timeout.html
   Location: sscce.py:2:0
1       import requests
2       requests.post(
3         "https://httpbin.org/post", timeout=60 * 3
4       )

Should I open a new issue?

[ericwb]

@tucked Good catch. Seems timeout arg gets missed as the node is an ast.BinOp. The context._get_literal_value() doesn't handle BinOp and defaults to None.

[ericwb]

There are some limitations in what Bandit can identify. So in the case of BinOp node, Bandit would be required to evaluate the value (possibly via eval()). But BinOp nodes can also include variables and Bandit doesn't keep track of things like a stack and heap. It moves Bandit into the territory of a compiler or runtime analysis.

This example is simple as it is only "60 * 3", but what about "60 * x" or "x ** 2" or "(5 - 1) ** 2". It can get complicated fast as you possibly have to handle many types of operations and precedence.

One possible way to fix in scenarios with all constants is to:

    code = ast.unparse(literal)
    literal_value = eval(code)

However, even Bandit itself warns about using eval(). And ast.literal_eval() doesn't work on BinOp nodes.