fix: use certifi ssl context on Windows#7778
Conversation
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- Since
_SYSTEM_SSL_CTXis now created viabuild_ssl_context_with_certifi()at import time, consider whether this context should be created lazily to avoid unnecessary work or environment-dependent failures during module import. - If
build_ssl_context_with_certifi()behavior differs across platforms or Python versions, it might be safer to keep a small wrapper in this module (e.g.,build_system_ssl_context()) to centralize any future platform-specific adjustments without touching callers again.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Since `_SYSTEM_SSL_CTX` is now created via `build_ssl_context_with_certifi()` at import time, consider whether this context should be created lazily to avoid unnecessary work or environment-dependent failures during module import.
- If `build_ssl_context_with_certifi()` behavior differs across platforms or Python versions, it might be safer to keep a small wrapper in this module (e.g., `build_system_ssl_context()`) to centralize any future platform-specific adjustments without touching callers again.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request updates the initialization of the global _SYSTEM_SSL_CTX in network_utils.py to use a custom build_ssl_context_with_certifi function instead of the standard library's default context. Feedback suggests updating related docstrings that now contain outdated information regarding certifi usage, adding unit tests for this core change, and passing the local logger to the initialization function for better traceability.
|
|
||
| _SYSTEM_SSL_CTX = ssl.create_default_context() | ||
| from astrbot.utils.http_ssl_common import build_ssl_context_with_certifi | ||
| _SYSTEM_SSL_CTX = build_ssl_context_with_certifi() |
There was a problem hiding this comment.
The change to _SYSTEM_SSL_CTX makes the docstring for create_proxy_client (specifically lines 96-98 and 111) incorrect, as it explicitly states that it "avoids certifi" and uses only the "system SSL context". Please update the docstring to reflect that a hybrid context (system + certifi) is now used.
Additionally, per the general rules, this change to the core SSL context should be accompanied by unit tests to ensure connectivity remains stable across different environments (e.g., with custom CAs or proxies).
You might also consider passing the local logger to build_ssl_context_with_certifi for better log attribution.
| _SYSTEM_SSL_CTX = build_ssl_context_with_certifi() | |
| _SYSTEM_SSL_CTX = build_ssl_context_with_certifi(logger) |
References
- New functionality, such as handling attachments, should be accompanied by corresponding unit tests.
* fix: use certifi ssl context on Windows * docs: update docstring to reflect hybrid SSL context * chore: ruff --------- Co-authored-by: Soulter <905617992@qq.com>
Fixes #7775
On Windows + Python 3.11, ssl.create_default_context() cannot reliably load system certificates, causing SSL verification failures when connecting to APIs like DeepSeek (which use Amazon Root CA 1). This fix replaces the bare ssl.create_default_context() call with the existing build_ssl_context_with_certifi() utility, which loads both system certs and certifi as a fallback.
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Checklist / 检查清单
😊 If there are new features added in the PR, I have discussed it with the authors through issues/emails, etc.
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
👀 My changes have been well-tested, and "Verification Steps" and "Screenshots" have been provided above.
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
🤓 I have ensured that no new dependencies are introduced, OR if new dependencies are introduced, they have been added to the appropriate locations in
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。😮 My changes do not introduce malicious code.
/ 我的更改没有引入恶意代码。
Summary by Sourcery
Bug Fixes: