add check for "requests" calls without timeout

[mschfh]

as mentioned in the requests documentation, not specifying a timeout argument can lead to a process hanging indefinitely, which may lead to a denial of service situation.

[sigmavirus24]

Confidence here should be low. Not specifying a timeout will rely on the operating system which tends to have same defaults in properly configured environments (read any production environment).

Further many people use libraries that allow for a session level default on top of requests to bypass this exactly. Others wrap requests to specify it for each request by default.

[sigmavirus24]

This also won't catch a scenario where the user builds a dictionary dynamically with a timeout value in it and lead to false reports.

[mschfh]

Not specifying a timeout will rely on the operating system which tends to have same defaults in properly configured environments (read any production environment).

that is a very optimistic assumption for many production environments, default TCP timeouts can be quite high and there may be certain types of environments where those can't be adjusted on a system level (e.g. FaaS)

This also won't catch a scenario where the user builds a dictionary dynamically with a timeout value in it and lead to false reports.

if that dict is built at runtime it would probably result in a false negative?
the check should evaluate if timeout is present and if it's set to None

Confidence here should be low

Is confidence just related to findings (higher confidence = false positives unlikely) or also the absence of findings (high confidence = false negatives unlikely)?

[sigmavirus24]

the check should evaluate if timeout is present and if it's set to None

The very code you wrote warns with unreasonable confidence if there is no static call argument named timeout. If someone has code along these lines:

class Client:
    def __init__(self, default_read_timeout=10.0, default_connect_timeout=5.0) -> :
        self.default_read_timeout = default_read_timeout
        self.default_connect_timeout = default_connect_timeout
        self.session = requests.Session()

    @property
    def timeout(self) -> tuple[float, float]:
        return (self.default_read_timeout, self.default_connect_timeout)

    def request(self, method, url, **kwargs) -> requests.Response:
        kwargs.setdefault('timeout', self.timeout)
        # ... additional logic
        return self.session.request(method, url, **kwargs)

You're going to tell people, claiming you have high confidence you're correct, that this is insecure code.

that is a very optimistic assumption for many production environments,

It's based on experience at companies with any semblance of real production experience.

Is confidence just related to findings

Confidence is related to the confidence in the accuracy of the findings. There can be no confidence here given the wide use of requests and the many ways in which it can be used (you're also only looking at the functional API, not session usage)

[mschfh]

Thanks for the elaboration, I fully agree and have changed the check to low confidence

[ericwb]

Not necessary unless there are unicode characters in this file.

[mschfh]

should I remove it?
most plugins seem to include the line, even if there are no unicode characters in the file

[ericwb]

Yes please. Recently Black was integrated as a format check. It'll flag this.