PyCQA / bandit

Bandit is a tool designed to find common security issues in Python code.
https://bandit.readthedocs.io
Apache License 2.0
6.5k stars 610 forks source link

"Call to httpx without timeout" when httpx has timeout by default #1175

Closed GuiGav closed 1 month ago

GuiGav commented 1 month ago

Describe the bug

httpx library implements timeout by default : https://www.python-httpx.org/advanced/timeouts/ and release 1.7.10 introduces B113:request_without_timeout on httpx calls. IMO, only calls specifying timeout=None should raise an issue.

Reproduction steps

1.

import httpx
client = httpx.AsyncClient()

2.

bandit main.py

Expected behavior

bandit should not raise an issue in this case.

Bandit version

1.7.10

Python version

3.11

szmoro commented 1 month ago

Having the same issue. Interestingly, explicitly specifying the timeout in the client constructor doesn't have any effect either

>> Issue: [B113:request_without_timeout] Call to httpx 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.10/plugins/b113_request_without_timeout.html
   Location: src/path/to/file.py:132:8
131             output_path.open("w") as file,
132             httpx.Client(timeout=Timeout(timeout=5.0)) as client
133         ):
GuiGav commented 1 month ago

it seems to only accepts int. float is raising an issue as well, while it's valid according to type hints

ericwb commented 1 month ago

This is a current limitation on how Bandit does analysis. It currently doesn't track the flow of data. So if an object is constructed with a timeout in httpx.AsyncClient(), it won't know later that the client instance has a timeout set. Likewise, someone could set the global socket timeout via socket.setdefaulttimeout(), but Bandit would also have no knowledge of that and assume the timeout is unset.

There was so discussion on this when the plugin was added. I would argue that most applications don't set global timeout, but they should set it to a reasonable wait time like 5 seconds (or user configurable for the application). The issue raised by Bandit would bring awareness to do that hopefully and then users could skip this test ID.

ubernostrum commented 1 month ago

@ericwb I read the issue and the PR which added httpx to the B113 check and cannot find any explanation of why this was done.

To be absolutely clear in case it was misunderstood: httpx already applies an automatic default timeout of 5 seconds if the developer forgets to specify a timeout. See the httpx documentation:

The default behavior is to raise a TimeoutException after 5 seconds of network inactivity.

Therefore, there is no need to remind a developer to set timeouts with httpx, because a sensible default timeout is already provided (unlike requests, and the fact that httpx has an automatic default timeout is one reason why httpx is often preferred as an HTTP client over requests nowadays).

Please stop alerting B113 with httpx.

ericwb commented 1 month ago

@ubernostrum I might be confusing myself with the PR introducing checks on timeouts for python-requests.

Thanks for the link to the httpx docs and clarification. I would agree there is no need to check for an undefined timeout value in parameter.