v4: Verify the TLS certificate when fetching the legacy notice (#3504)

The legacy notice route fetched the News page from GitHub with verify_ssl=False, so certificate verification was off for every user: whoever sat on the connection could present any certificate for github.com and choose the notice text and links. The sanitizer limits the markup, but it cannot tell whose content it is. - Turn verification off for the notice only when bypass_ssl is set, as manager_util.get_data does - Answer a certificate or connection failure with "Unable to retrieve Notice" where it raised and the notice area showed the server's 500 page - Add tests that run get_notice against a local HTTPS server behind a local CONNECT proxy: an untrusted certificate and a trusted certificate for another host are rejected; bypass_ssl accepts the untrusted certificate; a CA trusted through SSL_CERT_FILE is accepted; a closed proxy, a proxy answering 502 and a proxy that never answers end in the unavailable notice, each with one warning naming the cause - Add a CHANGELOG entry Port of #3424 to v4 (#3302).
2026-11-13 08:19:40 +09:00
parent 857e2ca5a5
commit 72f05493e5
4 changed files with 264 additions and 33 deletions
@@ -32,6 +32,14 @@ and this project adheres to [Semantic Versioning 2.0.0](https://semver.org/spec/
  Entries are case-insensitive, apply to all versions, and take effect without a
  restart. Server startup creates a commented template with no default exceptions;
  other installation security checks still apply.
+- Verify the TLS certificate when the legacy UI fetches its notice from GitHub;
+  verification was off for every user. A response whose certificate is not
+  trusted is discarded and the notice area shows `Unable to retrieve Notice`. It
+  shows the same when the connection fails, where it used to show a server
+  error. Behind a proxy that re-signs TLS traffic, point the `SSL_CERT_FILE`
+  environment variable at a CA bundle that includes the proxy's CA, or set
+  `bypass_ssl = True` in `config.ini`, which turns verification off for this
+  fetch too (#3302).

 ### Fixed

@@ -1851,46 +1851,50 @@ async def get_notice(request):
    url = "github.com"
    path = "/ltdrdata/ltdrdata.github.io/wiki/News"

-    async with aiohttp.ClientSession(trust_env=True, connector=aiohttp.TCPConnector(verify_ssl=False)) as session:
-        async with session.get(f"https://{url}{path}") as response:
-            if response.status == 200:
-                # html_content = response.read().decode('utf-8')
-                html_content = await response.text()
+    try:
+        async with aiohttp.ClientSession(trust_env=True, connector=aiohttp.TCPConnector(verify_ssl=not manager_util.bypass_ssl)) as session:
+            async with session.get(f"https://{url}{path}") as response:
+                if response.status == 200:
+                    # html_content = response.read().decode('utf-8')
+                    html_content = await response.text()

-                pattern = re.compile(r'<div class="markdown-body">([\s\S]*?)</div>')
-                match = pattern.search(html_content)
+                    pattern = re.compile(r'<div class="markdown-body">([\s\S]*?)</div>')
+                    match = pattern.search(html_content)

-                if match:
-                    # Remote page content lands in innerHTML on the client:
-                    # keep formatting markup only.
-                    markdown_content = html_utils.sanitize_html_fragment(match.group(1))
-                    version_tag = os.environ.get('__COMFYUI_DESKTOP_VERSION__')
-                    if version_tag is not None:
-                        markdown_content += f"<HR>ComfyUI: {version_tag} [Desktop]"
-                    else:
-                        version_tag = context.get_comfyui_tag()
-                        if version_tag is None:
-                            markdown_content += f"<HR>ComfyUI: {core.comfy_ui_revision}[{comfy_ui_hash[:6]}]({core.comfy_ui_commit_datetime.date()})"
+                    if match:
+                        # Remote page content lands in innerHTML on the client:
+                        # keep formatting markup only.
+                        markdown_content = html_utils.sanitize_html_fragment(match.group(1))
+                        version_tag = os.environ.get('__COMFYUI_DESKTOP_VERSION__')
+                        if version_tag is not None:
+                            markdown_content += f"<HR>ComfyUI: {version_tag} [Desktop]"
                        else:
-                            markdown_content += (f"<HR>ComfyUI: {version_tag}<BR>"
-                                                 f"&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;({core.comfy_ui_commit_datetime.date()})")
-                    # markdown_content += f"<BR>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;()"
-                    markdown_content += f"<BR>Manager: {core.version_str}"
+                            version_tag = context.get_comfyui_tag()
+                            if version_tag is None:
+                                markdown_content += f"<HR>ComfyUI: {core.comfy_ui_revision}[{comfy_ui_hash[:6]}]({core.comfy_ui_commit_datetime.date()})"
+                            else:
+                                markdown_content += (f"<HR>ComfyUI: {version_tag}<BR>"
+                                                     f"&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;({core.comfy_ui_commit_datetime.date()})")
+                        # markdown_content += f"<BR>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;()"
+                        markdown_content += f"<BR>Manager: {core.version_str}"

-                    try:
-                        if '__COMFYUI_DESKTOP_VERSION__' not in os.environ:
-                            if core.comfy_ui_commit_datetime == datetime(1900, 1, 1, 0, 0, 0):
-                                markdown_content = '<P style="text-align: center; color:red; background-color:white; font-weight:bold">Your ComfyUI isn\'t git repo.</P>' + markdown_content
-                            elif core.comfy_ui_required_commit_datetime.date() > core.comfy_ui_commit_datetime.date():
-                                markdown_content = '<P style="text-align: center; color:red; background-color:white; font-weight:bold">Your ComfyUI is too OUTDATED!!!</P>' + markdown_content
-                    except Exception:
-                        pass
+                        try:
+                            if '__COMFYUI_DESKTOP_VERSION__' not in os.environ:
+                                if core.comfy_ui_commit_datetime == datetime(1900, 1, 1, 0, 0, 0):
+                                    markdown_content = '<P style="text-align: center; color:red; background-color:white; font-weight:bold">Your ComfyUI isn\'t git repo.</P>' + markdown_content
+                                elif core.comfy_ui_required_commit_datetime.date() > core.comfy_ui_commit_datetime.date():
+                                    markdown_content = '<P style="text-align: center; color:red; background-color:white; font-weight:bold">Your ComfyUI is too OUTDATED!!!</P>' + markdown_content
+                        except Exception:
+                            pass

-                    return web.Response(text=markdown_content, status=200)
+                        return web.Response(text=markdown_content, status=200)
+                    else:
+                        return web.Response(text="Unable to retrieve Notice", status=200)
                else:
                    return web.Response(text="Unable to retrieve Notice", status=200)
-            else:
-                return web.Response(text="Unable to retrieve Notice", status=200)
+    except (aiohttp.ClientError, asyncio.TimeoutError) as e:
+        logging.warning(f"[ComfyUI-Manager] Unable to retrieve Notice: {type(e).__name__}: {e}")
+        return web.Response(text="Unable to retrieve Notice", status=200)


 # legacy /manager/notice
@@ -123,6 +123,7 @@ def test_notice_endpoint_sanitizes_remote_content_and_retains_local_version_foot
        'routes': web.RouteTableDef(), 're': re, 'web': web,
        'html_utils': load_renderer()['html_utils'],
        'aiohttp': SimpleNamespace(ClientSession=lambda **kwargs: client, TCPConnector=lambda **kwargs: None),
+        'manager_util': SimpleNamespace(bypass_ssl=False),
        'os': SimpleNamespace(environ={'__COMFYUI_DESKTOP_VERSION__': '1.0'}),
        'core': SimpleNamespace(version_str='4.2.1'),
    })
@@ -0,0 +1,218 @@
+"""The legacy notice route verifies the upstream certificate and survives a failed fetch (#3302).
+
+Each case runs the real `get_notice` in a child process. The child points HTTPS_PROXY at a
+local CONNECT proxy that hands the tunnel for github.com:443 to a local HTTPS server, which
+holds a "github.com" certificate from a throwaway CA: a machine in the middle, unless the
+case trusts that CA through SSL_CERT_FILE. Nothing leaves 127.0.0.1. A child is needed
+because aiohttp builds its verifying SSL context when it is imported.
+"""
+import asyncio
+import json
+import logging
+import os
+import re
+import shutil
+import ssl
+import subprocess
+import sys
+from types import SimpleNamespace
+
+import aiohttp
+import pytest
+from aiohttp import web
+
+from legacy_html_testutil import PACKAGE, load_functions, load_renderer
+
+NOTICE_PATH = "/ltdrdata/ltdrdata.github.io/wiki/News"
+NOTICE_HTML = ('<div class="markdown-body"><p>Served by <b>the test server</b> '
+               '<a href="https://example.com/notes">notes</a><script>x()</script></p></div>')
+UNAVAILABLE = "Unable to retrieve Notice"
+TUNNEL = "CONNECT github.com:443 HTTP/1.1"
+OTHER_HOST = "notice-test.example"
+
+OPENSSL_CONFIG = """
+[req]
+distinguished_name = dn
+prompt = no
+[dn]
+CN = ComfyUI-Manager notice test CA
+[ca]
+basicConstraints = critical, CA:TRUE
+keyUsage = critical, keyCertSign, cRLSign
+subjectKeyIdentifier = hash
+"""
+LEAF_EXTENSIONS = """
+[leaf]
+basicConstraints = critical, CA:FALSE
+keyUsage = critical, digitalSignature, keyEncipherment
+extendedKeyUsage = serverAuth
+subjectAltName = DNS:{host}
+subjectKeyIdentifier = hash
+authorityKeyIdentifier = keyid, issuer
+"""
+
+
+def load_get_notice(bypass_ssl):
+    return load_functions(PACKAGE / "legacy/manager_server.py", {"get_notice"}, {
+        "routes": web.RouteTableDef(), "aiohttp": aiohttp, "asyncio": asyncio, "logging": logging,
+        "re": re, "web": web, "os": os,
+        "html_utils": load_renderer()["html_utils"],
+        "manager_util": SimpleNamespace(bypass_ssl=bypass_ssl),
+        "core": SimpleNamespace(version_str="4.test"),
+    })["get_notice"]
+
+
+async def _pipe(reader, writer):
+    try:
+        while data := await reader.read(65536):
+            writer.write(data)
+            await writer.drain()
+    finally:
+        writer.close()
+
+
+async def fetch_notice(certdir, host, bypass_ssl, proxy_mode):
+    """Child process: what `get_notice` answers and logs, and the tunnels the proxy was asked for."""
+    async def notice_page(request):
+        return web.Response(text=NOTICE_HTML, content_type="text/html")
+
+    upstream = web.Application()
+    upstream.router.add_get(NOTICE_PATH, notice_page)
+    runner = web.AppRunner(upstream)
+    await runner.setup()
+    server_context = ssl.create_default_context(ssl.Purpose.CLIENT_AUTH)
+    server_context.load_cert_chain(os.path.join(certdir, host + ".pem"), os.path.join(certdir, host + ".key"))
+    await web.TCPSite(runner, "127.0.0.1", 0, ssl_context=server_context).start()
+    upstream_port = runner.addresses[0][1]
+
+    tunnels = []
+    answered = asyncio.Event()
+
+    async def connect_proxy(reader, writer):
+        tunnels.append((await reader.readuntil(b"\r\n\r\n")).split(b"\r\n", 1)[0].decode())
+        if proxy_mode == "bad_gateway":
+            writer.write(b"HTTP/1.1 502 Bad Gateway\r\nContent-Length: 0\r\n\r\n")
+            await writer.drain()
+        if proxy_mode == "silent":
+            await answered.wait()
+        if proxy_mode != "tunnel":
+            writer.close()
+            return
+        upstream_reader, upstream_writer = await asyncio.open_connection("127.0.0.1", upstream_port)
+        writer.write(b"HTTP/1.1 200 Connection established\r\n\r\n")
+        await writer.drain()
+        await asyncio.gather(_pipe(reader, upstream_writer), _pipe(upstream_reader, writer), return_exceptions=True)
+
+    proxy = await asyncio.start_server(connect_proxy, "127.0.0.1", 0)
+    proxy_port = proxy.sockets[0].getsockname()[1]
+    if proxy_mode == "closed":
+        proxy.close()
+        await proxy.wait_closed()
+    if proxy_mode == "silent":
+        aiohttp.client.DEFAULT_TIMEOUT = aiohttp.ClientTimeout(total=0.5)
+    os.environ["HTTPS_PROXY"] = f"http://127.0.0.1:{proxy_port}"
+
+    # Only what get_notice logs: aiohttp warns through its own loggers, for example about .netrc.
+    warnings = []
+    handler = logging.Handler(logging.WARNING)
+    handler.emit = lambda record: record.name == "root" and warnings.append(record.getMessage())
+    logging.getLogger().addHandler(handler)
+
+    response = await load_get_notice(bypass_ssl)(None)
+    answered.set()
+    proxy.close()
+    await runner.cleanup()
+    return {"status": response.status, "text": response.text, "tunnels": tunnels, "warnings": warnings}
+
+
+@pytest.fixture(scope="module")
+def certdir(tmp_path_factory):
+    if shutil.which("openssl") is None:
+        pytest.skip("the openssl command is needed to create the test certificates")
+    path = tmp_path_factory.mktemp("notice-tls")
+    config = path / "openssl.cnf"
+    config.write_text(OPENSSL_CONFIG, encoding="utf-8")
+
+    def openssl(*args):
+        subprocess.run(["openssl", *args], cwd=path, check=True, capture_output=True)
+
+    openssl("req", "-x509", "-newkey", "rsa:2048", "-nodes", "-days", "2", "-config", str(config),
+            "-extensions", "ca", "-keyout", "ca.key", "-out", "ca.pem")
+    for serial, host in enumerate(("github.com", OTHER_HOST), start=1):
+        (path / f"{host}.ext").write_text(LEAF_EXTENSIONS.format(host=host), encoding="utf-8")
+        openssl("req", "-newkey", "rsa:2048", "-nodes", "-subj", f"/CN={host}", "-config", str(config),
+                "-keyout", f"{host}.key", "-out", f"{host}.csr")
+        openssl("x509", "-req", "-in", f"{host}.csr", "-CA", "ca.pem", "-CAkey", "ca.key",
+                "-set_serial", str(serial), "-days", "2", "-extfile", f"{host}.ext", "-extensions", "leaf",
+                "-out", f"{host}.pem")
+    return path
+
+
+def fetch(certdir, host="github.com", trust_ca=False, bypass_ssl=False, proxy_mode="tunnel"):
+    env = {key: value for key, value in os.environ.items()
+           if not key.lower().endswith("_proxy") and key not in ("SSL_CERT_FILE", "SSL_CERT_DIR")}
+    env["__COMFYUI_DESKTOP_VERSION__"] = "test"
+    if trust_ca:
+        env["SSL_CERT_FILE"] = str(certdir / "ca.pem")
+    child = subprocess.run(
+        [sys.executable, __file__, str(certdir), host, str(int(bypass_ssl)), proxy_mode],
+        env=env, capture_output=True, text=True, timeout=60)
+    assert child.returncode == 0, "get_notice raised instead of answering:\n" + child.stderr[-2000:]
+    return json.loads(child.stdout.strip().splitlines()[-1])
+
+
+def assert_unavailable(result, *cause):
+    assert (result["status"], result["text"]) == (200, UNAVAILABLE)
+    assert len(result["warnings"]) == 1
+    for text in cause:
+        assert text in result["warnings"][0]
+
+
+def test_untrusted_certificate_is_rejected_before_its_content_is_used(certdir):
+    result = fetch(certdir)
+
+    assert result["tunnels"] == [TUNNEL]
+    assert_unavailable(result, "ClientConnectorCertificateError", "CERTIFICATE_VERIFY_FAILED")
+
+
+def test_bypass_ssl_accepts_the_untrusted_certificate(certdir):
+    result = fetch(certdir, bypass_ssl=True)
+
+    assert result["tunnels"] == [TUNNEL]
+    assert result["status"] == 200
+    assert "<b>the test server</b>" in result["text"]
+    assert result["warnings"] == []
+
+
+def test_trusted_certificate_for_another_host_is_rejected(certdir):
+    result = fetch(certdir, host=OTHER_HOST, trust_ca=True)
+
+    assert result["tunnels"] == [TUNNEL]
+    assert_unavailable(result, "ClientConnectorCertificateError", "not valid for 'github.com'")
+
+
+def test_ca_trusted_through_ssl_cert_file_is_accepted(certdir):
+    result = fetch(certdir, trust_ca=True)
+
+    assert result["tunnels"] == [TUNNEL]
+    assert result["status"] == 200
+    assert "<b>the test server</b>" in result["text"]
+    assert 'href="https://example.com/notes"' in result["text"]
+    assert "<script" not in result["text"]
+    assert result["warnings"] == []
+    assert "Manager: 4.test" in result["text"]
+
+
+@pytest.mark.parametrize("proxy_mode, error", [
+    ("closed", "ClientProxyConnectionError"),
+    ("bad_gateway", "ClientHttpProxyError"),
+    ("silent", "TimeoutError"),
+], ids=["proxy_closed", "proxy_answers_502", "proxy_silent"])
+def test_failed_fetch_answers_with_the_unavailable_notice(certdir, proxy_mode, error):
+    result = fetch(certdir, trust_ca=True, proxy_mode=proxy_mode)
+
+    assert_unavailable(result, error)
+
+
+if __name__ == "__main__":
+    print(json.dumps(asyncio.run(fetch_notice(sys.argv[1], sys.argv[2], sys.argv[3] == "1", sys.argv[4]))))