feat(serve): add /demo debug page for qwen serve daemon#4132
Conversation
Add a self-contained HTML debug page at GET /demo that provides a browser-based UI for exercising all daemon routes: session create/attach, prompt send/cancel, SSE event streaming, model switching, permission voting, and health/capabilities checks. Also add a same-origin exemption middleware (before the CORS deny layer) so browser fetch calls from the demo page pass through while external Origins remain blocked. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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. |
| html += '<div style="font-size:11px;color:var(--text2);margin-bottom:6px">' + escHtml(id) + '</div>'; | ||
| if (req.options && Array.isArray(req.options)) { | ||
| for (const opt of req.options) { | ||
| html += '<button class="btn opt-btn" data-req="' + escHtml(id) + '" data-opt="' + escHtml(opt.optionId) + '">' + escHtml(opt.name || opt.optionId) + '</button>'; |
There was a problem hiding this comment.
[Critical] escHtml() is safe for text nodes, but it is not sufficient for values embedded inside quoted HTML attributes. A permission option id such as " autofocus onfocus=... can close data-opt and inject markup/event handlers when this string is assigned to innerHTML. Since permission requests come from the agent stream, this gives a malicious or compromised tool/agent a DOM XSS path in the daemon origin.
Please build these permission buttons with DOM APIs instead of concatenating HTML, e.g. create each button, assign textContent, and set button.dataset.req / button.dataset.opt directly.
— gpt-5.5 via Qwen Code /review
| const lines = buffer.split('\\n'); | ||
| buffer = lines.pop(); | ||
|
|
||
| let currentEvent = {}; |
There was a problem hiding this comment.
[Critical] currentEvent is re-created for every reader.read() chunk, so a valid SSE frame split across chunks loses any fields parsed before the next read. For example, if event: arrives in one chunk and data: plus the blank terminator arrive in the next, the handler dispatches an event without the original type/id, or drops it entirely. That makes streaming chat and permission events intermittently unreliable under normal network buffering.
| let currentEvent = {}; | |
| let buffer = ''; | |
| let currentEvent = {}; | |
| while (true) { | |
| const { done, value } = await reader.read(); | |
| if (done) break; | |
| buffer += decoder.decode(value, { stream: true }); | |
| const lines = buffer.split('\n'); | |
| buffer = lines.pop(); | |
| for (const line of lines) { |
— gpt-5.5 via Qwen Code /review
| const bridge = | ||
| deps.bridge ?? createHttpAcpBridge({ maxSessions: opts.maxSessions }); | ||
|
|
||
| // --- Demo page: registered BEFORE CORS/auth so the browser can load |
There was a problem hiding this comment.
[Critical] Registering /demo before denyBrowserOriginCors, hostAllowlist, and bearerAuth makes the debug page reachable without the guards that protect the rest of the daemon. On token-protected/non-loopback deployments this exposes a daemon debug surface without authentication, and on loopback it bypasses the Host allowlist that is used to defend against DNS rebinding.
Please keep /demo behind at least the Host allowlist and the non-loopback bearer-auth policy. The same-origin Origin exception can still run before denyBrowserOriginCors, but the page route itself should not bypass all request guards.
— gpt-5.5 via Qwen Code /review
| try { data = JSON.parse(text); } catch { data = text; } | ||
| if (!res.ok) { | ||
| logError(res.status, JSON.stringify(data)); | ||
| return { ok: false, status: res.status, data }; |
There was a problem hiding this comment.
[Suggestion] The demo UI never sends an Authorization header and provides no way to configure the bearer token. The new middleware only strips same-origin Origin; it does not bypass bearerAuth, so after the page loads every API call (/session, prompt, model switch, permission response, SSE) still returns 401 when qwen serve runs with --token / QWEN_SERVER_TOKEN—including non-loopback binds where a token is required.
Please add a token input/config path and include Authorization: Bearer <token> in both api() and the SSE fetch, or explicitly disable/gate /demo when bearer auth is enabled.
— gpt-5.5 via Qwen Code /review
| if (done) break; | ||
| buffer += decoder.decode(value, { stream: true }); | ||
|
|
||
| const lines = buffer.split('\\n'); |
There was a problem hiding this comment.
[Critical] SSE line parsing uses literal '\\n' (backslash + n, two characters) instead of actual newline ' ' (LF, U+000A).
The SSE server (formatSseFrame in server.ts) emits real LF characters as frame delimiters. buffer.split('\\n') looks for the two-byte sequence \ + n which never appears in the stream, so split returns a single-element array containing the entire accumulated buffer. lines.pop() takes the whole buffer, lines is empty, and the event dispatch loop never iterates.
Result: All streaming features are non-functional — chat messages don't appear, Events tab is empty, permission requests never surface, session_died/model_switched notifications are silently dropped.
| const lines = buffer.split('\\n'); | |
| const lines = buffer.split('\n'); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // browser access anyway (per the threat-model docs). | ||
| app.use((req: import('express').Request, _res, next) => { | ||
| const origin = req.headers.origin; | ||
| if (origin) { |
There was a problem hiding this comment.
[Suggestion] The origin-stripping middleware matches only 127.0.0.1, localhost, and [::1], but the hostAllowlist middleware (same request pipeline) additionally allows host.docker.internal for Docker Desktop setups.
When the demo page is accessed via http://host.docker.internal:PORT/demo, the browser sends Origin: http://host.docker.internal:PORT on API fetches. This origin is not recognized, the header is not stripped, and denyBrowserOriginCors rejects with 403. The demo page loads but all API calls silently fail.
| if (origin) { | |
| const selfOrigins = new Set([ | |
| `http://127.0.0.1:${port}`, | |
| `http://localhost:${port}`, | |
| `http://[::1]:${port}`, | |
| `http://host.docker.internal:${port}`, | |
| ]); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| // --- Demo page: registered BEFORE CORS/auth so the browser can load | ||
| // the page and make same-origin API calls. The page is a self-contained | ||
| // HTML debug UI for exercising all daemon routes. | ||
| app.get('/demo', (_req, res) => { |
There was a problem hiding this comment.
[Suggestion] The /demo route handler calls getDemoHtml(getPort()) without try/catch protection. Every other route in this file uses the sendBridgeError pattern that logs to writeStderrLine and returns structured JSON.
If getPort() throws (e.g., server.address() returns null in a race before listen() completes), Express returns a generic 500 with no stderr trace — making production debugging unnecessarily difficult.
| app.get('/demo', (_req, res) => { | |
| app.get('/demo', (_req, res) => { | |
| try { | |
| res.type('html').send(getDemoHtml(getPort())); | |
| } catch (err) { | |
| writeStderrLine( | |
| `qwen serve: /demo render failed: ${err instanceof Error ? err.message : String(err)}`, | |
| ); | |
| res.status(500).json({ error: 'Failed to render demo page' }); | |
| } | |
| }); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| let html = '<h4>' + escHtml(req.tool?.name || 'Permission') + '</h4>'; | ||
| html += '<div style="font-size:11px;color:var(--text2);margin-bottom:6px">' + escHtml(id) + '</div>'; | ||
| if (req.options && Array.isArray(req.options)) { | ||
| for (const opt of req.options) { |
There was a problem hiding this comment.
[Suggestion] Permission card is removed from the UI optimistically before checking whether the server accepted the response.
In the permission button click handler, removePermission(reqId) is called right after await api(...) without checking r.ok. If the server returns 404 (requestId already resolved), 400 (invalid option), or a network error, the permission card disappears but the agent is still blocked waiting for a response. The user has no indication the permission response failed.
| for (const opt of req.options) { | |
| const result = await api('POST', '/permission/' + reqId, { outcome }); | |
| if (result.ok) { | |
| removePermission(reqId); | |
| } else { | |
| logError('PERM-ERR', 'Failed to resolve permission ' + reqId + ': ' + JSON.stringify(result.data)); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| while (true) { | ||
| const { done, value } = await reader.read(); | ||
| if (done) break; |
There was a problem hiding this comment.
[Critical] When the SSE reader loop exits (done === true), two things go wrong:
-
No disconnect feedback: The status dot stays green and "Connected" text persists. Chat controls remain enabled. The user sees a seemingly live page that silently stopped receiving events — exactly the failure mode that makes this debug tool useless when you need it most.
-
Trailing buffer lost: The remaining unparsed content in
buffer(from the lastlines.pop()) is never dispatched. Critical events likesession_diedorstream_errorthat arrive in the final TCP chunk without a trailing newline are silently dropped.
Together these create a debug page that hides the very information it was built to surface.
| if (done) break; | |
| while (true) { | |
| const { done, value } = await reader.read(); | |
| if (done) break; | |
| buffer += decoder.decode(value, { stream: true }); | |
| const lines = buffer.split('\n'); | |
| buffer = lines.pop(); | |
| for (const line of lines) { | |
| if (line.startsWith('data: ')) { | |
| currentEvent.data = line.slice(6); | |
| } else if (line.startsWith('event: ')) { | |
| currentEvent.event = line.slice(7); | |
| } else if (line.startsWith('id: ')) { | |
| currentEvent.id = line.slice(4); | |
| } else if (line === '') { | |
| if (currentEvent.data) { | |
| handleSSEMessage(currentEvent); | |
| } | |
| currentEvent = {}; | |
| } | |
| } | |
| } | |
| // Process any remaining buffered data | |
| if (currentEvent.data) handleSSEMessage(currentEvent); | |
| statusDot.classList.remove('ok'); | |
| statusText.textContent = 'SSE stream ended'; | |
| enablePrompt(false); | |
| logEvent('SSE', 'Stream ended by server'); |
— glm-5.1 via Qwen Code /review
| // the page and make same-origin API calls. The page is a self-contained | ||
| // HTML debug UI for exercising all daemon routes. | ||
| app.get('/demo', (_req, res) => { | ||
| res.type('html').send(getDemoHtml(getPort())); |
There was a problem hiding this comment.
[Suggestion] No test coverage for the new /demo route or the same-origin Origin-stripping middleware below. The existing server.test.ts covers every other route extensively — these additions are the only untested ones:
GET /demoreturns200+text/htmlcontent type- Same-origin loopback
Originheaders are stripped beforedenyBrowserOriginCorsruns - Non-loopback / wrong-port
Originheaders are left untouched
The Origin-stripping middleware is security-sensitive (it weakens denyBrowserOriginCors for same-origin requests). A regression in its logic would silently break the demo page or unintentionally allow non-demo origins through — with no test to catch it.
— glm-5.1 via Qwen Code /review
- Fix XSS: build permission buttons with DOM APIs instead of innerHTML - Fix SSE: move currentEvent outside read loop for cross-chunk frames - Fix SSE: handle stream end (flush trailing buffer, update UI status) - Security: move /demo route behind hostAllowlist and bearerAuth guards - Security: add host.docker.internal to same-origin Origin allowlist - Add Auth Token input and include Authorization header in API/SSE calls - Add try/catch to /demo route handler with writeStderrLine logging - Check API result before removing permission card from UI - Add 7 tests for /demo route and Origin-stripping middleware
|
@wenshao 感谢 review，所有反馈已在 85c9d42 中修复。逐条说明： Critical 修复
Suggestion 修复
全部 78 个测试通过。 |
| @@ -127,6 +152,22 @@ export function createServeApp( | |||
| } | |||
|
|
|||
| app.use(bearerAuth(opts.token)); | |||
There was a problem hiding this comment.
[Critical] Registering /demo behind bearerAuth makes the token-protected browser flow unreachable. A normal address-bar navigation to /demo cannot attach an Authorization: Bearer ... header, so the in-page token field is never loaded when --token is enabled. The Supertest coverage can set this header, but the advertised browser workflow cannot.
A safer split is to serve only the static HTML shell after the CORS/Host guards but before bearerAuth, while keeping all daemon API/SSE routes protected so the token input can authenticate subsequent calls. If the page itself must remain bearer-protected, the docs/UI should not imply the token field is sufficient for opening it.
— gpt-5.5 via Qwen Code /review
| for (const [id, req] of pendingPerms) { | ||
| const card = document.createElement('div'); | ||
| card.className = 'permission-card'; | ||
|
|
There was a problem hiding this comment.
[Critical] The permission card shows only req.tool?.name (which does not match the published toolCall payload) and the request id before rendering approval buttons. It omits the actual command/input, edit diff/content, locations, and other toolCall context being authorized.
This lets a user approve or permanently allow an exec/edit/MCP permission from the demo page without seeing what action is being approved, defeating the safety purpose of the permission flow. Please render the relevant req.toolCall details (for example title/kind/rawInput/locations/content, as applicable) with text-safe DOM APIs before the buttons, and use req.toolCall?.title or a safe fallback for the heading.
— gpt-5.5 via Qwen Code /review
| const origin = req.headers.origin; | ||
| if (origin) { | ||
| const port = getPort(); | ||
| const selfOrigins = new Set([ |
There was a problem hiding this comment.
[Suggestion] The Origin-stripping middleware treats every allowlisted loopback alias as same-origin instead of comparing against the request's actual Host. For example, a request with Host: 127.0.0.1:4170 and Origin: http://host.docker.internal:4170 is stripped even though browsers do not consider those origins equal. It also misses default-port browser serialization such as Origin: http://localhost when getPort() is 80, while hostAllowlist() accepts the corresponding no-port Host value.
Please derive the effective origin from the normalized request Host (including default-port handling) and strip only when Origin exactly matches that effective origin, rather than accepting any alias in the loopback set.
— gpt-5.5 via Qwen Code /review
| function ts() { | ||
| return new Date().toLocaleTimeString('en-US', { hour12: false, hour: '2-digit', minute: '2-digit', second: '2-digit' }); | ||
| } | ||
| function addLog(container, tag, tagClass, msg) { |
There was a problem hiding this comment.
[Suggestion] addLog() appends every request/event entry to the DOM and never prunes old nodes. Since streamed message chunks are also logged, leaving the demo page open during a long or noisy session will grow retained strings and DOM nodes without bound, making scrolling/layout progressively slower.
Please cap each log pane to a bounded number of entries (for example 500–1000) and remove the oldest child when the limit is exceeded; a clear-log action would also help for long debugging sessions.
— gpt-5.5 via Qwen Code /review
| function addLog(container, tag, tagClass, msg) { | ||
| const el = document.createElement('div'); | ||
| el.className = 'log-entry'; | ||
| const text = typeof msg === 'object' ? JSON.stringify(msg, null, 2) : msg; |
There was a problem hiding this comment.
[Suggestion] addLog() builds HTML via innerHTML with unescaped tag and tagClass parameters. The msg argument is correctly passed through escHtml(), but tag is concatenated raw. Two call sites (line ~378 logEvent(kind || type, ...) and line ~394 logEvent(type, ...)) pass server-derived SSE event types as tag.
This is the same class of issue that was fixed in renderPermissions() by switching to DOM APIs (createElement + textContent + dataset), but addLog was not updated to match. Currently all daemon event types are safe strings, but addLog has no internal guard against future event types containing HTML metacharacters.
| const text = typeof msg === 'object' ? JSON.stringify(msg, null, 2) : msg; | |
| function addLog(container, tag, tagClass, msg) { | |
| const el = document.createElement('div'); | |
| el.className = 'log-entry'; | |
| const text = typeof msg === 'object' ? JSON.stringify(msg, null, 2) : msg; | |
| const tsSpan = document.createElement('span'); | |
| tsSpan.className = 'ts'; | |
| tsSpan.textContent = ts(); | |
| const tagSpan = document.createElement('span'); | |
| tagSpan.className = 'tag ' + tagClass; | |
| tagSpan.textContent = tag; | |
| el.appendChild(tsSpan); | |
| el.appendChild(tagSpan); | |
| el.appendChild(document.createTextNode(String(text))); | |
| container.appendChild(el); | |
| container.scrollTop = container.scrollHeight; | |
| } |
— glm-5.1 via Qwen Code /review
| if (!cwd) { alert('Please enter a working directory'); return; } | ||
| const r = await api('POST', '/session', { cwd }); | ||
| if (r.ok) { | ||
| sessionId = r.data.sessionId; |
There was a problem hiding this comment.
[Suggestion] When api('POST', '/session', ...) returns { ok: false }, the function returns without any visible feedback in the Chat tab. The error IS logged to the API Log tab, but a user staring at the Chat tab sees nothing — the button simply appears to do nothing. Consider adding an else branch:
if (r.ok) {
// ... existing success path ...
} else {
addChatMeta('Session creation failed: ' + (r.data?.error || 'HTTP ' + r.status));
}— glm-5.1 via Qwen Code /review
| .set('Authorization', 'Bearer secret'); | ||
| expect(res.status).toBe(200); | ||
| expect(res.headers['content-type']).toMatch(/text\/html/); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] Test coverage gaps in the new Origin-stripping and /demo tests:
-
localhostand[::1]origins untested — only127.0.0.1andhost.docker.internalare tested. These are distinct code paths (e.g., on macOSlocalhostresolves to::1). A typo or refactor dropping one would go undetected. -
Negative assertions —
expect(res.status).not.toBe(403)would pass for any non-403 status including 500. Consider usingexpect(res.status).toBe(200)withexpect(res.body).toEqual({ status: 'ok' })for the/healthtests. -
No
/demoCORS rejection test — no test confirms thatGET /demowithOrigin: https://evil.comreturns 403, documenting the property that the demo page is behind CORS guards.
— glm-5.1 via Qwen Code /review
| sessionInfo.style.display = 'block'; | ||
| sessionIdDisplay.textContent = sessionId; | ||
| enablePrompt(true); | ||
| chatArea.innerHTML = ''; |
There was a problem hiding this comment.
[Suggestion] createSession clears chatArea.innerHTML = '' but never resets the module-level currentAssistantBubble to null. If SSE chunks arrive before the first user message in the new session, appendAssistantChunk writes into a now-detached DOM element — content is updated in memory but invisible to the user.
| chatArea.innerHTML = ''; | |
| chatArea.innerHTML = ''; | |
| currentAssistantBubble = null; | |
| currentThought = ''; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| } | ||
| })(); | ||
| } | ||
|
|
There was a problem hiding this comment.
[Suggestion] The SSE stream's catch error path (non-AbortError) does not call enablePrompt(false), leaving Send/Cancel/Model buttons enabled with a red status dot. The clean-disconnect path (stream end) correctly disables controls — this path should do the same.
| if (err.name !== 'AbortError') { | |
| logEvent('SSE-ERR', err.message); | |
| statusDot.classList.remove('ok'); | |
| statusText.textContent = 'SSE error'; | |
| enablePrompt(false); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| function removePermission(requestId) { | ||
| pendingPerms.delete(requestId); | ||
| renderPermissions(); |
There was a problem hiding this comment.
[Suggestion] sendPrompt adds the user message to the chat area via addUserMessage() BEFORE the API call, but when POST /prompt returns !r.ok, the function returns silently — the user's message stays rendered with no response or error feedback. The error only appears in the API Log tab, which the user may not be viewing.
| renderPermissions(); | |
| if (r.ok) { | |
| addChatMeta('stopReason: ...'); | |
| } else { | |
| addChatMeta('Error: prompt failed (HTTP ' + r.status + ')'); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| if (!res.ok) { | ||
| logEvent('SSE-ERR', 'HTTP ' + res.status); | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] When the SSE fetch returns a non-OK HTTP status (e.g., 401), connectSSE logs the error and returns without calling enablePrompt(false) or updating the status dot. However, createSession already called enablePrompt(true) before invoking connectSSE. Result: controls stay enabled with a green "Connected" dot, but no SSE stream exists.
| } | |
| if (!res.ok) { | |
| logEvent('SSE-ERR', 'HTTP ' + res.status); | |
| statusDot.classList.remove('ok'); | |
| statusText.textContent = 'SSE connect failed (HTTP ' + res.status + ')'; | |
| enablePrompt(false); | |
| return; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
|
||
| function handlePermissionRequest(data) { | ||
| const req = data.data; | ||
| const requestId = req?.requestId; |
There was a problem hiding this comment.
[Suggestion] No concurrent-send guard in sendPrompt: the Send button stays enabled while a prompt is in-flight. Rapidly clicking Send creates interleaved SSE responses because addUserMessage clears currentAssistantBubble and currentThought mid-stream. Consider calling enablePrompt(false) at the start of sendPrompt and re-enabling after the SSE stream's usage meta event or on stream end/error.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
Browsers cannot attach Authorization headers on address-bar navigation, so /demo behind bearerAuth was unreachable when --token was set. Move the /demo route after CORS + Host allowlist but before bearerAuth. The static HTML shell contains no secrets; all API/SSE routes remain bearer-protected and the in-page token input authenticates them.
jifeng
left a comment
jifeng
left a comment
There was a problem hiding this comment.
Addressed the [Critical] /demo behind bearerAuth is unreachable issue in 92a77b9:
Fix: /demo is now registered AFTER CORS + Host allowlist but BEFORE bearerAuth. The static HTML shell contains no secrets — it only serves the page skeleton with the token input field. All daemon API/SSE routes (/session, /prompt, /events, etc.) remain behind bearerAuth, so the in-page token field authenticates subsequent calls.
Tests updated:
- Old "401 without token" test → now verifies
/demoreturns 200 even when--tokenis set (browser address-bar reachability) - New test confirms CORS + Host allowlist still guard
/demo(cross-origin requests get 403) - Removed the duplicate
/demoregistration that was afterbearerAuth
wenshao
left a comment
wenshao
left a comment
There was a problem hiding this comment.
Additional findings (not tied to a diff line):
packages/cli/src/serve/demo.ts— No structural guard preventing future secrets ingetDemoHtml(). The comment at server.ts:101 says "the page itself contains no secrets", but there is nothing preventing someone from embedding sensitive data in the HTML string later. Add aSECURITY:comment abovegetDemoHtml()noting this HTML is served via an unauthenticated endpoint.packages/cli/src/serve/demo.ts(checkHealth) — On non-loopback binds with--token, the demo page's initialcheckHealth()call gets 401 (non-loopback/healthis behindbearerAuth), showing a misleading "Disconnected" status before the user has a chance to enter the token.
| app.use(denyBrowserOriginCors); | ||
| app.use(hostAllowlist(opts.hostname, getPort)); | ||
|
|
||
| // --- Demo page: registered AFTER CORS and Host allowlist but BEFORE |
There was a problem hiding this comment.
[Critical] /demo is unconditionally registered before bearerAuth regardless of loopback vs non-loopback bind. This contradicts the /health security model: /health is gated — loopback → before bearerAuth, non-loopback → after bearerAuth (with an explicit comment explaining that unauthenticated non-loopback probes are an info leak). The same logic applies to /demo, except /demo leaks far more (full API route enumeration + interactive console vs {"status":"ok"}).
On --hostname 0.0.0.0 --token secret, anyone can curl /demo and fingerprint the daemon without credentials. An oncall engineer checking /health (correctly gets 401) would never think to check /demo.
| // --- Demo page: registered AFTER CORS and Host allowlist but BEFORE | |
| const loopback = isLoopbackBind(opts.hostname); | |
| if (loopback) { | |
| app.get('/demo', (_req, res) => { | |
| try { | |
| res.type('html').send(getDemoHtml(getPort())); | |
| } catch (err) { | |
| writeStderrLine( | |
| `qwen serve: /demo render failed: ${err instanceof Error ? err.message : String(err)}`, | |
| ); | |
| res.status(500).json({ error: 'Failed to render demo page' }); | |
| } | |
| }); | |
| } |
Then register after bearerAuth for non-loopback (mirroring the /health pattern at lines 167-179).
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| const origin = req.headers.origin; | ||
| if (origin) { | ||
| const port = getPort(); | ||
| const selfOrigins = new Set([ |
There was a problem hiding this comment.
[Suggestion] The Origin-stripping middleware creates new Set([...4 interpolated strings...]) on every request with an Origin header. The hostAllowlist middleware (auth.ts:46-74) already uses a lazy-caching pattern (rebuild Set only when port changes) with an explicit comment explaining why. Mirror that pattern here:
| const selfOrigins = new Set([ | |
| let cachedStripPort = -1; | |
| let cachedSelfOrigins: Set<string> = new Set(); | |
| app.use((req: import('express').Request, _res, next) => { | |
| const origin = req.headers.origin; | |
| if (origin) { | |
| const port = getPort(); | |
| if (port !== cachedStripPort) { | |
| cachedStripPort = port; | |
| cachedSelfOrigins = new Set([ | |
| `http://127.0.0.1:${port}`, | |
| `http://localhost:${port}`, | |
| `http://[::1]:${port}`, | |
| `http://host.docker.internal:${port}`, | |
| ]); | |
| } | |
| if (cachedSelfOrigins.has(origin)) { | |
| delete req.headers.origin; | |
| } | |
| } | |
| next(); | |
| }); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| `http://localhost:${port}`, | ||
| `http://[::1]:${port}`, | ||
| `http://host.docker.internal:${port}`, | ||
| ]); |
There was a problem hiding this comment.
[Suggestion] Including http://host.docker.internal:${port} in the selfOrigins Set means any Docker container on the same host can reach /demo without auth on loopback binds even when --token is set (both the Host allowlist and Origin-stripping accept it). While Docker networking is often considered "local" in threat models, this widens the blast radius: a compromised container can enumerate the daemon's full API surface without credentials. If the Critical fix above (loopback gating) is applied, this is partially mitigated, but consider documenting the host.docker.internal trust boundary explicitly.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| it('is accessible without bearer token even when --token is set (before bearerAuth)', async () => { | ||
| // /demo is registered BEFORE bearerAuth so browsers can reach the | ||
| // page via address-bar navigation (which cannot attach Authorization | ||
| // headers). The in-page token input authenticates subsequent API calls. |
There was a problem hiding this comment.
[Suggestion] The test 'is accessible without bearer token even when --token is set' binds to hostname: '0.0.0.0' (non-loopback) with token: 'secret' and asserts 200. This validates the non-loopback-no-auth behavior. If the Critical fix above is applied (loopback-gating /demo), this test should be rewritten to bind to 127.0.0.1 (loopback) to verify the intended loopback-with-token path, and a separate test should verify non-loopback with token returns 401.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| expect(res.status).toBe(200); | ||
| expect(res.headers['content-type']).toMatch(/text\/html/); | ||
| }); | ||
|
|
There was a problem hiding this comment.
[Suggestion] The test name 'is still guarded by CORS and Host allowlist' is misleading — the test only exercises CORS (Origin: https://evil.example.com triggers denyBrowserOriginCors) but passes the Host allowlist (Host: 127.0.0.1:<port> is a valid loopback host). Rename to 'is still guarded by CORS' or add a separate test with an invalid Host header.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
| .set('Host', `127.0.0.1:${baseOpts.port}`) | ||
| .set('Origin', 'http://127.0.0.1:4170'); | ||
| // Should NOT be rejected by denyBrowserOriginCors (status != 403) | ||
| expect(res.status).not.toBe(403); |
There was a problem hiding this comment.
[Suggestion] The Origin-stripping tests cover 127.0.0.1 and host.docker.internal origins, but miss localhost and [::1] — both are in the selfOrigins Set (server.ts lines 82-83). Add test cases for Origin: http://localhost:4170 and Origin: http://[::1]:4170 with expect(res.status).not.toBe(403).
— DeepSeek/deepseek-v4-pro via Qwen Code /review
When an API call returns 401 Unauthorized, highlight the Auth Token input field with a yellow border and display a hint message guiding the user to enter their bearer token. Applies to both API calls and SSE connections. The hint auto-dismisses after 6 seconds.
| sessionInfo.style.display = 'block'; | ||
| sessionIdDisplay.textContent = sessionId; | ||
| enablePrompt(true); | ||
| chatArea.innerHTML = ''; |
There was a problem hiding this comment.
[Critical] createSession() switches the active sessionId and clears the chat UI, but it never clears pendingPerms or rerenders/hides the permission panel. If a permission request from the previous session is still pending, its card remains visible after attaching or creating a different session, and clicking it still calls the daemon-global POST /permission/:requestId endpoint. That can approve or cancel an action for the old session while the UI appears to be operating on the new one.
Clear per-session permission state when a new session is selected, and preferably ignore permission events that do not match the currently active session.
| chatArea.innerHTML = ''; | |
| enablePrompt(true); | |
| chatArea.innerHTML = ''; | |
| pendingPerms.clear(); | |
| renderPermissions(); | |
| if (r.data.attached) { |
— gpt-5.5 via Qwen Code /review
| app.get('/demo', (_req, res) => { | ||
| try { | ||
| res.type('html').send(getDemoHtml(getPort())); | ||
| } catch (err) { |
There was a problem hiding this comment.
[Critical] Clickjacking: /demo response has no X-Frame-Options or Content-Security-Policy: frame-ancestors header.
A malicious website can embed the demo page in a cross-origin <iframe src="http://127.0.0.1:<port>/demo">. The browser's GET navigation carries no Origin header, so both denyBrowserOriginCors and hostAllowlist pass. Once loaded, the iframe's JS makes same-origin fetch() calls whose Origin header is stripped by the new Origin-stripping middleware — all API calls succeed.
On the default loopback bind (no --token), an attacker can trick the user into performing daemon actions (create sessions, send prompts, approve permissions) via transparent iframe overlay.
| } catch (err) { | |
| app.get('/demo', (_req, res) => { | |
| try { | |
| res | |
| .type('html') | |
| .set('X-Frame-Options', 'DENY') | |
| .set('Content-Security-Policy', "default-src 'none'; script-src 'unsafe-inline'; style-src 'unsafe-inline'; connect-src 'self'") | |
| .send(getDemoHtml(getPort())); | |
| } catch (err) { |
— glm-5.1 via Qwen Code /review
| $('#btnCancel').disabled = !on; | ||
| $('#btnSetModel').disabled = !on; | ||
| chatInput.disabled = !on; | ||
| $('#btnChatSend').disabled = !on; |
There was a problem hiding this comment.
[Suggestion] promptInput not disabled by enablePrompt.
enablePrompt(on) disables chatInput, btnSendPrompt, btnCancel, btnSetModel, and btnChatSend, but NOT promptInput (the sidebar textarea). Since promptInput has its own keydown listener that calls sendPrompt(promptInput.value) on Enter, the user can still submit prompts from the sidebar after an SSE disconnect. sessionId is never cleared on disconnect, so the prompt reaches the server — but with no SSE subscriber, the response is silently discarded.
| $('#btnChatSend').disabled = !on; | |
| function enablePrompt(on) { | |
| $('#btnSendPrompt').disabled = !on; | |
| $('#btnCancel').disabled = !on; | |
| $('#btnSetModel').disabled = !on; | |
| chatInput.disabled = !on; | |
| promptInput.disabled = !on; | |
| $('#btnChatSend').disabled = !on; | |
| } |
— glm-5.1 via Qwen Code /review
| // through. Only loopback origins are matched — non-loopback deployments | ||
| // require the operator to front the daemon with a reverse proxy for | ||
| // browser access anyway (per the threat-model docs). | ||
| app.use((req: import('express').Request, _res, next) => { |
There was a problem hiding this comment.
[Suggestion] Origin-stripping middleware has a global side effect not reflected in comments.
The middleware applies to ALL routes (registered via app.use), but the comment only mentions the demo page. It permanently weakens denyBrowserOriginCors for all same-origin loopback requests — any future browser-facing feature will inherit this CORS bypass without realizing it.
Consider expanding the comment to explicitly document the global effect, e.g.: "This middleware strips the Origin header for ALL same-origin loopback requests on every route, not just /demo. Future browser-facing features will inherit this CORS bypass."
— glm-5.1 via Qwen Code /review
|
|
||
| function addChatMeta(text) { | ||
| const el = document.createElement('div'); | ||
| el.className = 'chat-msg assistant'; |
There was a problem hiding this comment.
[Suggestion] textContent += causes O(n²) streaming performance.
Each SSE chunk reads the full accumulated string, concatenates the new text, and re-sets the entire textContent. For a long code-generation response with hundreds of chunks, this produces visible jank.
| el.className = 'chat-msg assistant'; | |
| currentAssistantBubble.appendChild(document.createTextNode(text)); |
— glm-5.1 via Qwen Code /review
| if (!cwd) { alert('Please enter a working directory'); return; } | ||
| const r = await api('POST', '/session', { cwd }); | ||
| if (r.ok) { | ||
| sessionId = r.data.sessionId; |
There was a problem hiding this comment.
[Suggestion] createSession doesn't validate sessionId in response.
If the server returns { ok: true, data: { attached: true } } without a sessionId field, the variable becomes undefined. Subsequent URL constructions produce paths like /session/undefined/events, resulting in a confusing "session not found: undefined" error that looks like a server-side bug.
| sessionId = r.data.sessionId; | |
| if (!r.data?.sessionId) { | |
| addChatMeta('Error: server returned no sessionId'); | |
| return; | |
| } | |
| sessionId = r.data.sessionId; |
— glm-5.1 via Qwen Code /review
| * page with no external dependencies so it works without a build step or | ||
| * static-file serving. | ||
| */ | ||
| export function getDemoHtml(_port: number): string { |
There was a problem hiding this comment.
[Suggestion] _port parameter is unused.
The HTML template uses const BASE = '' for relative URLs and never references _port. The call site in server.ts evaluates getPort() on every request for no reason. Either remove the parameter and memoize the HTML, or wire it up to actually embed the port.
— glm-5.1 via Qwen Code /review
3 participants
Summary
Add a self-contained
/demodebug page to theqwen servedaemon. This page provides a browser-based UI for exercising all daemon routes without requiring any external tools or build steps.What's included
packages/cli/src/serve/demo.ts— inline HTML generator for the/demopagepackages/cli/src/serve/server.ts— registers the/demoroute and same-origin CORS workaroundFeatures of the demo page
/health,/health?deep=1, and/capabilities--token-protected deploymentsDesign notes
Originheader when it matches the daemon's own loopback address (includinghost.docker.internalfor Docker Desktop), allowing the demo page's API calls to pass through while maintaining the existingdenyBrowserOriginCorsprotection for non-loopback origins/demoroute is registered after CORS, Host allowlist, and bearer auth middleware, so it is protected by the same security guards as all other routesCR feedback addressed (commit
85c9d42)innerHTMLwith DOM APIs (createElement,textContent,dataset)currentEventscopewhileloop so frames split across chunks are handled correctly/demobypasses authhostAllowlistandbearerAuthmiddlewareAuthorization: Bearerheader inapi()and SSEfetchhost.docker.internal/demomissing error handlinggetDemoHtml(getPort())in try/catch withwriteStderrLineremovePermission()only called after checkingresult.ok; errors logged/demoroute (200/401/token) and Origin-stripping middlewareReviewer Test Plan
qwen serveand openhttp://localhost:<port>/demoin a browser--tokenprotected deployments: enter the token in the Auth Token field and verify API calls succeed202605141033.mp4