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

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

Closed
GuiGav opened this issue Sep 24, 2024 · 5 comments · Fixed by #1177
Closed

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

GuiGav opened this issue Sep 24, 2024 · 5 comments · Fixed by #1177
Labels
bug Something isn't working
Milestone

Comments

@GuiGav
Copy link

GuiGav commented Sep 24, 2024

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

import httpx
client = httpx.AsyncClient()
bandit main.py

Expected behavior

bandit should not raise an issue in this case.

Bandit version

1.7.10

Python version

3.11

@GuiGav GuiGav added the bug Something isn't working label Sep 24, 2024
@szmoro
Copy link

szmoro commented Sep 24, 2024

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
Copy link
Author

GuiGav commented Sep 24, 2024

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

@ericwb
Copy link
Member

ericwb commented Sep 25, 2024

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
Copy link

ubernostrum commented Sep 26, 2024

@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
Copy link
Member

ericwb commented Sep 27, 2024

@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.

ericwb added a commit to ericwb/bandit that referenced this issue Sep 27, 2024
Unlike python-requests, the httpx client has a default
timeout of 5 seconds on its class and functions. As such,
there is no need for Bandit to check for an undefined
timeout. However, explicitly setting the timeout to None
is still a potential problem as that would create a
situtation where the client would block forever.

Fixes: PyCQA#1175

Signed-off-by: Eric Brown <eric_wade_brown@yahoo.com>
ericwb added a commit to ericwb/bandit that referenced this issue Sep 27, 2024
Unlike python-requests, the httpx client has a default
timeout of 5 seconds on its class and functions. As such,
there is no need for Bandit to check for an undefined
timeout. However, explicitly setting the timeout to None
is still a potential problem as that would create a
situtation where the client would block forever.

Fixes: PyCQA#1175

Signed-off-by: Eric Brown <eric_wade_brown@yahoo.com>
ericwb added a commit to ericwb/bandit that referenced this issue Sep 27, 2024
Unlike python-requests, the httpx client has a default
timeout of 5 seconds on its class and functions. As such,
there is no need for Bandit to check for an undefined
timeout. However, explicitly setting the timeout to None
is still a potential problem as that would create a
situtation where the client would block forever.

Fixes: PyCQA#1175

Signed-off-by: Eric Brown <eric_wade_brown@yahoo.com>
ericwb added a commit to ericwb/bandit that referenced this issue Oct 1, 2024
Unlike python-requests, the httpx client has a default
timeout of 5 seconds on its class and functions. As such,
there is no need for Bandit to check for an undefined
timeout. However, explicitly setting the timeout to None
is still a potential problem as that would create a
situtation where the client would block forever.

Fixes: PyCQA#1175

Signed-off-by: Eric Brown <eric_wade_brown@yahoo.com>
@ericwb ericwb added this to the Release 1.8.0 milestone Oct 8, 2024
@ericwb ericwb closed this as completed in 071386b Oct 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants