[BUG] Missing timeouts on 5 outbound HTTP calls outside mod_ci (follow-up to #943)
Nessuno ha ancora preso questa issue.
Valutazione
- Difficoltà
- 3/5
- Tempo stimato
- 1-2 giorni
- Idoneità per principianti
- 84/100
- Tipo di issue
- Bug
- Chiarezza
- Specificata chiaramente
- Stato di attività
- Attiva
- Stack tecnologico
- python
- Ambito
- backend, networking
Direzione di ricerca
Inizia con i cinque punti di chiamata elencati in utility.py, mod_auth/controllers.py, mailer.py e mod_upload/controllers.py, quindi confrontali con le costanti di timeout in mod_ci/controllers.py e con la chiamata esistente in mod_auth/controllers.py:185. Estendi tests/test_utility.py e aggiungi assertions per le altre chiamate interessate; il lavoro è completato quando tutte e cinque le richieste in uscita usano il timeout concordato e i test pertinenti hanno esito positivo.
Scritto dal modello di indicizzazione a partire dal testo della issue.
Descrizione
Sample platform commit: fe2b4a4
In raising this issue, I confirm the following (please check boxes, eg [X]):
- I have read and understood the contributors guide.
- I have checked that the bug-fix I am reporting can be replicated, or that the feature I am suggesting isn't already present.
- I have checked that the issue I'm posting isn't already reported.
- I have checked that the issue I'm posting isn't already solved and no duplicates exist in closed issues and in opened issues
- 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.
- 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:
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:
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:
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:
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:
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.
- Lingua principale
- Python
- Stelle
- 30
- Fork
- 85
- Merge medio
- 6g 8h
- PR unite (30g)
- 22
Guida per i contributori
Apri la guida per i contributori
Come iniziare
- Leggi tutta la issue e poi la guida ai contributi del progetto.
- Commenta sulla issue per dire che te ne occupi tu — evita che due persone facciano lo stesso lavoro.
- Fai un fork del repository e lavora su un branch.
- Apri una pull request che faccia riferimento al numero della issue.
Altre issue di CCExtractor/sample-platform
-
Difficoltà 2/5 1-3 ore Idoneità per principianti 88/100
CCExtractor/sample-platform#1194 · 1 commento ·
-
Difficoltà 3/5 1-2 giorni Idoneità per principianti 42/100
CCExtractor/sample-platform#1082 ·
-
Difficoltà 4/5 3-5 giorni Idoneità per principianti 48/100
CCExtractor/sample-platform#1079 ·
-
Difficoltà 5/5 Più di una settimana Idoneità per principianti 35/100
CCExtractor/sample-platform#1062 · 2 commenti ·
-
Difficoltà 4/5 3-5 giorni Idoneità per principianti 42/100
CCExtractor/sample-platform#1057 ·
Tutte le issue di CCExtractor/sample-platform
Issue simili
-
bug confirmed issue
Difficoltà 2/5 1-3 ore Idoneità per principianti 75/100
open-webui/open-webui#30750 · 1 commento ·
-
Difficoltà 2/5 1-3 ore Idoneità per principianti 75/100
-
enhancement
Difficoltà 2/5 1-3 ore Idoneità per principianti 75/100
OpenwaterHealth/openmotion-bloodflow-app#604 · 1 commento ·
-
Difficoltà 2/5 1-3 ore Idoneità per principianti 70/100
-
good first issue
Difficoltà 1/5 Meno di un'ora Idoneità per principianti 90/100