CCExtractor / CCExtractor/sample-platform

[BUG] Missing timeouts on 5 outbound HTTP calls outside mod_ci (follow-up to #943)

Aperta
#1,192 0 commenti 0 reazioni 0 assegnatari Vedi su GitHub
Lingua principale
Python
Stelle
30
Fork
85
Merge medio
6g 21h
PR unite (30g)
12

Descrizione

Sample platform commit: **fe2b4a4**

**In raising this issue, I confirm the following (please check boxes, eg [X]):**

- [X] I have read and understood the [contributors guide](https://github.com/CCExtractor/sample-platform/blob/master/.github/CONTRIBUTING.md).
- [X] I have checked that the bug-fix I am reporting can be replicated, or that the feature I am suggesting isn't already present.
- [X] I have checked that the issue I'm posting isn't already reported.
- [X] I have checked that the issue I'm posting isn't already solved and no duplicates exist in [closed issues](https://github.com/CCExtractor/sample-platform/issues?q=is%3Aissue+is%3Aclosed) and in [opened issues](https://github.com/CCExtractor/sample-platform/issues)
- [X] I have checked the pull requests tab for existing solutions/implementations to my issue/suggestion.

**My familiarity with the project is as follows (check one, eg [X]):**

- [ ] I have never visited/used the platform.
- [X] I have used the platform just a couple of times.
- [ ] I have used the platform extensively, but have not contributed previously.
- [ ] I am an active contributor to the platform.

---

## Summary

#942 identified "GitHub API calls without timeouts" as a critical reliability gap, and #943 fixed it — but only within `mod_ci/controllers.py`. The audit table in #942 only enumerated call sites in that one file.

Five outbound HTTP calls in `utility.py`, `mod_auth/controllers.py`, `mailer.py` and `mod_upload/controllers.py` still have no `timeout=`. Since `requests` has **no default timeout**, each of these blocks its worker indefinitely if the remote end accepts the TCP connection but never responds.

## Affected call sites

| # | File | Call | Reached from |
|---|------|------|--------------|
| 1 | `utility.py:140` | `requests.get('https://api.github.com/meta', ...)` | **unauthenticated** `POST /start-ci` |
| 2 | `mod_auth/controllers.py:212` | `requests.post(...)` OAuth token exchange | `GET /account/github_callback` |
| 3 | `mod_auth/controllers.py:142` | `session.post(...)` token validity check | `github_token_validity()` |
| 4 | `mailer.py:45` | `requests.post(...)` Mailgun send | signup, password reset |
| 5 | `mod_upload/controllers.py:106` | `session.post(...)` create GitHub issue | upload flow |

## Why site 1 is the most serious

The outbound call happens **before any authentication**, because fetching the IP allowlist *is* the first check:

```
POST /start-ci <- no auth required to send this
└─ @request_from_github() utility.py:61
└─ is_github_web_hook_ip() utility.py:110 <- FIRST check
└─ get_cached_web_hook_blocks() utility.py:126
└─ requests.get(...) utility.py:140 <- no timeout
```

The IP allowlist cannot gate this call, because the call is what retrieves the allowlist.

There is also a cache-invalidation flaw that amplifies it. In `get_cached_web_hook_blocks()`, the `KeyError` branch logs the failure but never updates `cached_load_time`, and `cached_web_hook_blocks` stays empty. Since the guard is:

```python
if len(cached_web_hook_blocks) == 0 or cache_has_expired(cached_load_time):
```

...an unexpected payload from the GitHub meta API means the cache never populates and **every subsequent webhook request re-fetches**. Combined with the missing timeout, that turns one hung call into one hung worker per inbound request.

I'd rate the overall severity as moderate rather than critical: it requires GitHub's API to be hanging rather than merely down (a refused connection fails fast). It is primarily a resilience-under-upstream-failure problem, which is exactly the class #942 was opened to address.

## Reproduction

A listener that accepts the connection and never replies reproduces the hang. This is the specific failure mode that a `ConnectionError` handler does *not* catch:

```bash
python -c "import socket; s=socket.socket(); s.setsockopt(socket.SOL_SOCKET, socket.SO_REUSEADDR, 1); s.bind(('127.0.0.1', 9999)); s.listen(1); input()"
```

Point any of the five call sites at `http://127.0.0.1:9999` and the request never returns.

## The convention already exists in-tree

This is an inconsistency rather than an unknown. `mod_ci/controllers.py:55-63` defines named constants (`GITHUB_API_TIMEOUT`, `GCP_API_TIMEOUT`, `ARTIFACT_DOWNLOAD_TIMEOUT`), and `mod_auth/controllers.py:185` already does it correctly:

```python
response = session.get(url, timeout=(3.05, 10)) # line 185 - has a timeout
```

...43 lines below a call in the same file that does not:

```python
response = session.post(url, json={"access_token": token}) # line 142 - no timeout
```

## Proposed fix

Add `timeout=(3.05, 10)` at the five sites, matching the existing `(connect, read)` tuple form at `mod_auth/controllers.py:185`. No signatures, return types, or call sites change.

Worth noting explicitly: sites 1, 2, 3 and 5 have no exception handler, so a timeout converts a hang into a 500. That is still a strict improvement (a fast, logged failure beats a wedged worker), but adding handlers could be a sensible follow-up if you'd prefer that in the same PR.

Tests would assert the kwarg is actually passed, so the regression can't silently return:

```python
mock_get.assert_called_once_with(
'https://api.github.com/meta', auth=mock.ANY, timeout=(3.05, 10))
```

`tests/test_utility.py` already patches `requests.get`, so this extends cleanly.

## Out of scope, happy to file separately

While tracing the above I found that `is_valid_signature()` (`utility.py:152`) raises unhandled exceptions on a malformed `X-Hub-Signature` header, producing a 500 instead of a clean 418:

```
'garbage' -> ValueError: not enough values to unpack (expected 2, got 1)
'bogus=abc' -> TypeError: Missing required parameter 'digestmod'
'__name__=abc' -> ValueError: unsupported hash type hashlib
```

`hashlib.__dict__.get(hash_algorithm)` performs an attribute lookup on a caller-supplied string. Real-world impact is low, since it is only reachable after the GitHub IP check passes and GitHub always sends well-formed signatures. Let me know if you'd like that as its own issue.

---

I'd be glad to take this one if it's not already spoken for.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.