[PR #3404] [CLOSED] v3: Verify upstream TLS certificates when retrieving Manager notices #3270

New issue

Closed

opened 2026-10-28 21:16:37 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-28 21:16:37 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/ComfyUI-Manager/pull/3404
Author: @StevenLi-phoenix
Created: 10/22/2026
Status: ❌ Closed

Base: main ← Head: fix/3302-notice-tls-verify

📝 Commits (1)

e1606e3 v3: Verify upstream TLS certificates when retrieving Manager notices

📊 Changes

2 files changed (+229 additions, -46 deletions)

View changed files

📝 glob/manager_server.py (+50 -46)
➕ tests/test_notice_tls.py (+179 -0)

📄 Description

Refs #3302

This is the v3 (main) half. The v4 legacy handler gets the same fix in #3405 against manager-v4, so this PR does not close the issue on its own.

Problem

GET /manager/notice downloads the GitHub wiki News page and shows its text and links in the Manager dialog. The handler built its connector with aiohttp.TCPConnector(verify_ssl=False). Certificate verification was therefore off for every user, whatever the bypass_ssl setting said. Anyone who can intercept the connection (a hostile network, a compromised proxy) can present any certificate for github.com and choose the notice text and links. The sanitizer from #3296 limits the markup, but it cannot authenticate the content.

Also, a connection error made the handler raise, so the notice area showed aiohttp's "500 Internal Server Error" page text.

Fix

Build the connector with TCPConnector(verify_ssl=not manager_util.bypass_ssl). This is the expression manager_util.get_data already uses for the channel and list fetches. Verification is on by default. bypass_ssl = True in config.ini (the README's workaround for CERTIFICATE_VERIFY_FAILED) still turns it off. trust_env=True is unchanged, so HTTP(S)_PROXY and SSL_CERT_FILE keep working.
Catch aiohttp.ClientError and asyncio.TimeoutError around the request. The handler logs a warning with the cause and returns the existing Unable to retrieve Notice response (status 200), the same response it already gives for a non-200 reply.

I used verify_ssl= instead of ssl= on purpose. On aiohttp 3.8.x, TCPConnector(ssl=True) gives an unverified context: _get_ssl_context treats any value that is not None and not an SSLContext as "not verified". I checked this with the new test: with ssl=not manager_util.bypass_ssl, the untrusted-certificate case fails on aiohttp 3.8.6 and passes on 3.14. verify_ssl=True verifies on every 3.x release and gives no deprecation warning. The warning from #2281 appears only when bypass_ssl is enabled, as it already does for get_data.

Most of the diff is re-indentation for the new try:. With git diff -w the change is 5 lines.

Behavior change

Users behind a TLS-intercepting proxy whose CA is not trusted now see Unable to retrieve Notice instead of the notice. They get it back by trusting the proxy's CA (for example through SSL_CERT_FILE) or by setting bypass_ssl = True. Manager's channel and list fetches through get_data already behave this way.

Scope

glob/share_3rdparty.py still builds its connectors with TCPConnector(verify_ssl=False) (4 call sites). I left them alone on purpose, because #3302 covers notice retrieval only. They could be a follow-up.

Tests

New tests/test_notice_tls.py. Each case runs the real get_notice in a child process, extracted from glob/manager_server.py with ast like the other tests do. The child sets HTTPS_PROXY to a local CONNECT proxy. The proxy forwards the tunnel to a local HTTPS server that presents a github.com certificate from a throwaway CA, which the test creates with the openssl CLI. This is a man-in-the-middle with an untrusted certificate, and no traffic leaves 127.0.0.1. The cases:

untrusted certificate, default settings: returns Unable to retrieve Notice, and the proxy saw CONNECT github.com:443
CA trusted through SSL_CERT_FILE: returns the sanitized notice (formatting and link kept, <script> removed, version footer added)
bypass_ssl = True: returns the notice
proxy unreachable: returns Unable to retrieve Notice instead of raising

A child process is needed because recent aiohttp versions build the verified SSL context at import time. The class is skipped if there is no openssl CLI.

Before the fix (base glob/ loaded through MANAGER_GLOB_DIR):

$ MANAGER_GLOB_DIR=<origin/main export>/glob python -m pytest tests/test_notice_tls.py -q
E   aiohttp.client_exceptions.ClientProxyConnectionError: Cannot connect to host 127.0.0.1:59894 ssl:default [Connect call failed ...]
E       AssertionError: Tuples differ: (200, '<p>Served by <b>the test server</b> <a hr[130 chars]est') != (200, 'Unable to retrieve Notice')
FAILED tests/test_notice_tls.py::NoticeTlsVerificationTest::test_unreachable_upstream_returns_the_unavailable_notice
FAILED tests/test_notice_tls.py::NoticeTlsVerificationTest::test_untrusted_certificate_is_rejected_before_its_content_is_used
2 failed, 2 passed

After the fix:

$ python -m pytest tests/test_notice_tls.py -q
4 passed
$ python -m pytest tests -q --ignore=tests/e2e
222 passed, 18 skipped, 223 subtests passed     (skips: Playwright not installed)
$ ruff check .
All checks passed!

Other checks:

The new test also passes on Python 3.10 with aiohttp 3.8.6 (the base fails the same 2 cases), and with the LibreSSL 3.3.6 openssl that ships with macOS as well as OpenSSL.
I ran the fixed handler once against the real github.com with verification on. It returned the current News page with the version footer.
I did not run tests/e2e, which needs a prepared ComfyUI root (E2E_COMFYUI_ROOT).

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/ComfyUI-Manager/pull/3404 **Author:** [@StevenLi-phoenix](https://github.com/StevenLi-phoenix) **Created:** 10/22/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/3302-notice-tls-verify` --- ### 📝 Commits (1) - [`e1606e3`](https://github.com/Comfy-Org/ComfyUI-Manager/commit/e1606e3f1f965f8fdf9dde907049efd0131c28ff) v3: Verify upstream TLS certificates when retrieving Manager notices ### 📊 Changes **2 files changed** (+229 additions, -46 deletions) <details> <summary>View changed files</summary> 📝 `glob/manager_server.py` (+50 -46) ➕ `tests/test_notice_tls.py` (+179 -0) </details> ### 📄 Description Refs #3302 This is the v3 (`main`) half. The v4 legacy handler gets the same fix in #3405 against `manager-v4`, so this PR does not close the issue on its own. ## Problem `GET /manager/notice` downloads the GitHub wiki News page and shows its text and links in the Manager dialog. The handler built its connector with `aiohttp.TCPConnector(verify_ssl=False)`. Certificate verification was therefore off for every user, whatever the `bypass_ssl` setting said. Anyone who can intercept the connection (a hostile network, a compromised proxy) can present any certificate for github.com and choose the notice text and links. The sanitizer from #3296 limits the markup, but it cannot authenticate the content. Also, a connection error made the handler raise, so the notice area showed aiohttp's "500 Internal Server Error" page text. ## Fix - Build the connector with `TCPConnector(verify_ssl=not manager_util.bypass_ssl)`. This is the expression `manager_util.get_data` already uses for the channel and list fetches. Verification is on by default. `bypass_ssl = True` in `config.ini` (the README's workaround for `CERTIFICATE_VERIFY_FAILED`) still turns it off. `trust_env=True` is unchanged, so `HTTP(S)_PROXY` and `SSL_CERT_FILE` keep working. - Catch `aiohttp.ClientError` and `asyncio.TimeoutError` around the request. The handler logs a warning with the cause and returns the existing `Unable to retrieve Notice` response (status 200), the same response it already gives for a non-200 reply. I used `verify_ssl=` instead of `ssl=` on purpose. On aiohttp 3.8.x, `TCPConnector(ssl=True)` gives an unverified context: `_get_ssl_context` treats any value that is not `None` and not an `SSLContext` as "not verified". I checked this with the new test: with `ssl=not manager_util.bypass_ssl`, the untrusted-certificate case fails on aiohttp 3.8.6 and passes on 3.14. `verify_ssl=True` verifies on every 3.x release and gives no deprecation warning. The warning from #2281 appears only when `bypass_ssl` is enabled, as it already does for `get_data`. Most of the diff is re-indentation for the new `try:`. With `git diff -w` the change is 5 lines. ## Behavior change Users behind a TLS-intercepting proxy whose CA is not trusted now see `Unable to retrieve Notice` instead of the notice. They get it back by trusting the proxy's CA (for example through `SSL_CERT_FILE`) or by setting `bypass_ssl = True`. Manager's channel and list fetches through `get_data` already behave this way. ## Scope `glob/share_3rdparty.py` still builds its connectors with `TCPConnector(verify_ssl=False)` (4 call sites). I left them alone on purpose, because #3302 covers notice retrieval only. They could be a follow-up. ## Tests New `tests/test_notice_tls.py`. Each case runs the real `get_notice` in a child process, extracted from `glob/manager_server.py` with `ast` like the other tests do. The child sets `HTTPS_PROXY` to a local CONNECT proxy. The proxy forwards the tunnel to a local HTTPS server that presents a `github.com` certificate from a throwaway CA, which the test creates with the `openssl` CLI. This is a man-in-the-middle with an untrusted certificate, and no traffic leaves 127.0.0.1. The cases: - untrusted certificate, default settings: returns `Unable to retrieve Notice`, and the proxy saw `CONNECT github.com:443` - CA trusted through `SSL_CERT_FILE`: returns the sanitized notice (formatting and link kept, `<script>` removed, version footer added) - `bypass_ssl = True`: returns the notice - proxy unreachable: returns `Unable to retrieve Notice` instead of raising A child process is needed because recent aiohttp versions build the verified SSL context at import time. The class is skipped if there is no `openssl` CLI. Before the fix (base `glob/` loaded through `MANAGER_GLOB_DIR`): ``` $ MANAGER_GLOB_DIR=<origin/main export>/glob python -m pytest tests/test_notice_tls.py -q E aiohttp.client_exceptions.ClientProxyConnectionError: Cannot connect to host 127.0.0.1:59894 ssl:default [Connect call failed ...] E AssertionError: Tuples differ: (200, '<p>Served by <b>the test server</b> <a hr[130 chars]est') != (200, 'Unable to retrieve Notice') FAILED tests/test_notice_tls.py::NoticeTlsVerificationTest::test_unreachable_upstream_returns_the_unavailable_notice FAILED tests/test_notice_tls.py::NoticeTlsVerificationTest::test_untrusted_certificate_is_rejected_before_its_content_is_used 2 failed, 2 passed ``` After the fix: ``` $ python -m pytest tests/test_notice_tls.py -q 4 passed $ python -m pytest tests -q --ignore=tests/e2e 222 passed, 18 skipped, 223 subtests passed (skips: Playwright not installed) $ ruff check . All checks passed! ``` Other checks: - The new test also passes on Python 3.10 with aiohttp 3.8.6 (the base fails the same 2 cases), and with the LibreSSL 3.3.6 `openssl` that ships with macOS as well as OpenSSL. - I ran the fixed handler once against the real github.com with verification on. It returned the current News page with the version footer. - I did not run `tests/e2e`, which needs a prepared ComfyUI root (`E2E_COMFYUI_ROOT`). --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-28 21:16:37 +00:00