[BUG] Missing timeouts on 5 outbound HTTP calls outside mod_ci (follow-up to #943)
まだ誰も着手していません。
評価
- 難易度
- 3/5
- 見積もり時間
- 1〜2日
- 初心者へのやさしさ
- 84/100
- issue の種類
- バグ
- 明瞭さ
- 明確に書かれている
- 活発さ
- 活発
- 技術スタック
- python
- 領域
- backend, networking
調査の方向性
utility.py、mod_auth/controllers.py、mailer.py、mod_upload/controllers.py に記載されている5つの呼び出し箇所から始め、mod_ci/controllers.py の timeout 定数および mod_auth/controllers.py:185 にある既存の呼び出しと比較します。tests/test_utility.py を拡張し、その他の影響を受ける呼び出しに対する assertions を追加します。5つすべての outbound requests が合意した timeout を使用し、関連するテストが成功すれば完了です。
索引モデルが issue の本文から書いたものです。
説明
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.
- 主要言語
- Python
- スター
- 30
- フォーク
- 85
- 平均マージ
- 6日 8時間
- マージ済み PR(30日)
- 22
コントリビューションガイド
はじめの一歩
- issue を最後まで読み、次にプロジェクトのコントリビューションガイドを読みます。
- 着手することを issue にコメントします — 二人が同じ作業をするのを防げます。
- リポジトリをフォークし、ブランチを切って変更します。
- issue 番号を参照したプルリクエストを送ります。
CCExtractor/sample-platform のほかの issue
-
難易度 2/5 1〜3時間 初心者へのやさしさ 88/100
CCExtractor/sample-platform#1194 · コメント 1 件 ·
-
難易度 3/5 1〜2日 初心者へのやさしさ 42/100
CCExtractor/sample-platform#1082 ·
-
難易度 4/5 3〜5日 初心者へのやさしさ 48/100
CCExtractor/sample-platform#1079 ·
-
難易度 5/5 1週間以上 初心者へのやさしさ 35/100
CCExtractor/sample-platform#1062 · コメント 2 件 ·
-
難易度 4/5 3〜5日 初心者へのやさしさ 42/100
CCExtractor/sample-platform#1057 ·
CCExtractor/sample-platform の issue をすべて見る
似ている issue
-
area: harness bug status: needs-triage
難易度 2/5 1〜3時間 初心者へのやさしさ 75/100
Human-Agent-Society/reef#625 ·
-
難易度 2/5 1〜3時間 初心者へのやさしさ 70/100
-
難易度 1/5 1時間未満 初心者へのやさしさ 80/100
learningequality/kolibri#15351 · コメント 2 件 ·
-
難易度 2/5 1〜3時間 初心者へのやさしさ 75/100
-
Name consistency オープン
難易度 2/5 1〜3時間 初心者へのやさしさ 75/100
eellak/triplestore#65 · コメント 1 件 ·