CCExtractor / CCExtractor/sample-platform

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

Offen
#1,192 0 Kommentare 0 Reaktionen 0 zugewiesene Personen Auf GitHub ansehen
Vorherrschende Sprache
Python
Sterne
30
Forks
85
Ø Merge
6 T. 21 Std.
Gemergte PRs (30 T.)
12

Beschreibung

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.

Beitragsleitfaden

Beitragsleitfaden öffnen

Bewertung

Dieses Issue wurde noch nicht bewertet.

Neue Issues direkt in Ihr Postfach

Eine kurze Übersicht über anfängerfreundliche GitHub-Issues.