CCExtractor / CCExtractor/sample-platform

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

Abierto
#1,192 0 comentarios 0 reacciones 0 asignados Ver en GitHub
Lenguaje dominante
Python
Estrellas
30
Forks
85
Merge medio
6 d 21 h
PR fusionados (30 d)
12

Descripción

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.

Guía de contribución

Abrir la guía de contribución

Línea de trabajo

Comienza con los cinco puntos de llamada indicados en utility.py, mod_auth/controllers.py, mailer.py y mod_upload/controllers.py; después, compáralos con las constantes de timeout de mod_ci/controllers.py y con la llamada existente en mod_auth/controllers.py:185. Amplía tests/test_utility.py y añade assertions para las demás llamadas afectadas; se considera terminado cuando las cinco solicitudes salientes usan el timeout acordado y pasan las pruebas relevantes.

Escrito por el modelo de indexación a partir del texto del issue.

Evaluación

Stack tecnológico
python
Área
backend, networking
Tipo de issue
Error
Dificultad
3/5
Tiempo estimado
1-2 días
Estado de actividad
Activo
Claridad
Bien especificado
Aptitud para principiantes
84/100

Recibe los nuevos issues en tu correo

Un resumen breve de issues de GitHub para principiantes.