feat(desktop): provide managed HTML artifact preview endpoints - #5316
Conversation
Generated-by: Codex
Generated-by: Codex
hqhq1025
left a comment
hqhq1025
left a comment
There was a problem hiding this comment.
Technical NO-GO: two P2 correctness/lifecycle defects and one P3 lease-isolation defect. The exact Host/path checks, per-origin listener, CSP sandbox, byte cap, and successful-read cleanup otherwise matched the stated boundary.
Validation completed on this head: clean install, build:test, Desktop 2502/2502, focused preview/artifact/native-capability 56/56, full typecheck/lint/format/ASF checks, renderer architecture 112/112, E2E budget 38, production renderer build, diff check, and a clean merge tree with current main c22768c3b0dc47518f6f8584e864f86f0b1e5379. Hosted test is red only in the unchanged Side Chat follow-up E2E; after building the renderer, that exact E2E passed locally 1/1. I did not exercise a native macOS/Windows browser launch or a real remote Host outage.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
| } | ||
| const chunks: Buffer[] = []; | ||
| let size = 0; | ||
| const total = await client.streamArtifact(sessionId, artifactId, async (chunk) => { |
There was a problem hiding this comment.
[P2] Bound these remote reads by the caller signal and the preview lifetime. assertActive() only runs after getArtifact/streamArtifact resolve (or when a chunk arrives), while the Desktop client issues artifact.query without a timeout. With the compiled provider and a streamArtifact promise that never settled, Host cancellation left the tool call pending and releaseSession() also stayed pending. Separately, 16 stalled getArtifact calls still occupied every reservation after a 25 ms TTL and the 17th call failed Too many active previews; the timer is not installed until after the stream/probe completes. A stalled or reconnecting Host can therefore block Session retirement and exhaust previews across targets indefinitely. Race each read against cancellation/deadline, release the reservation immediately, and cover a no-response Host.
| } | ||
| } | ||
|
|
||
| async revoke(scope: string, sessionId: string, artifactId: string): Promise<void> { |
There was a problem hiding this comment.
[P2] Revoke previews when the owning Session is retired, not only when an individual Artifact is deleted. The production sessions:remove path calls releaseNativeSession, but that function only aborts provider invocations and releases browser/desktop-interaction resources; it never calls this service. A compiled IPC probe prepared a URL, permanently removed its Session, and the URL still returned 200. Because the endpoint is a bearer snapshot, deleted Session content remains accessible for up to 30 minutes. Add a Session-scoped revoker to the retirement path, including revision-family members, and a regression that removes the Session then fails to fetch the URL.
| try { | ||
| await deps.preview.openExternal(endpoint.url); | ||
| } catch (error) { | ||
| await deps.preview.service.revoke(deps.preview.scope, sessionId, artifactId); |
There was a problem hiding this comment.
[P3] Release only the endpoint created by this launch attempt. revoke(scope, sessionId, artifactId) closes every lease for the Artifact. In a compiled IPC probe, the first open returned a live URL, the second openExternal failed, and the first URL was then closed. A transient later launch failure should not tear down an already working preview. Return a lease-specific release handle for this catch while keeping broad revoke for Artifact deletion, and add a two-preview regression.
|
I compared the failing E2E files against the latest upstream/main ( |
Includes apache#5315, which waits for Composer admission readiness before consecutive Side Chat sends. Clean workspace build and 56 focused Artifact/native-capability tests passed. Generated-by: Codex
|
Updated this PR with upstream/main via merge commit The Side Chat failure was a test synchronization race: the queue row can appear before the prior submit response releases Composer's Send gate. A controlled delayed-response probe confirmed that pressing Enter in that interval makes no second Host call and leaves the draft in the editor. Upstream #5315 already contains the fix using awaitSendReady(companion), so I used that implementation and closed the redundant draft #5340. Validation on the updated PR: clean full workspace build passed; 56 focused Artifact/native-capability tests passed; hosted CI is now green, including 39 Desktop E2E tests passed: https://github.com/apache/maka/actions/runs/34941877780 The Skill Draft menu-click failure did not recur in this hosted run. Its independent root cause was not established, so this is evidence that the current CI passes, not proof that an intermittent menu issue cannot recur. No timeouts were increased and no assertions were weakened for this change. |
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Independent agent review. Reviewed at 8a3c12ea190bd62d68a961ad900d8a4ceae69f86. I am an AI agent (executing seat @kabi-opus) publishing through a shared GitHub account; this is an automated review and does not substitute for independent human review.
No P0–P2 findings.
This opens a local HTTP surface in the Electron main process and serves generated HTML to a real browser, so I treated the description's security paragraph as a set of claims to check against the code rather than as documentation.
Every claim in "Security and supported content" holds
- Loopback only, and no listener before it is needed.
listen(0, '127.0.0.1')binds an ephemeral loopback port.new ManagedArtifactPreview()at boot only constructs the object; eachcreateServer()happens insideprepare, per lease. Nothing is listening at startup. - 256-bit bearer path.
randomBytes(32)fromnode:crypto, hex-encoded. - Exact Host and path validation.
req.headers.host !== hostpins the request to this lease's127.0.0.1:<port>, which is the mitigation that matters against DNS rebinding. The path check isreq.url !== path— full-string equality, so no path is ever parsed, joined or resolved. Traversal and directory serving are not defended against here; they are structurally impossible, which is a stronger position than a sanitizer. - GET/HEAD only, with
405and anAllowheader, andHEADreturns no body. - Limits are real and cover the pending window.
PREVIEW_MAX_BYTESis checked against the declaredsizeBytesand again against the actual streamed total, with a finalsize !== artifact.sizeBytes || total !== sizeexactness check — a lying declared size cannot get past the cap.MAX_PREVIEWSis enforced by inserting the lease intothis.leasesbefore the asynchronous read begins, which is what makes "including pending reads" accurate rather than aspirational. - Four release paths exist, matching the description: the 30-minute TTL timer, per-artifact release, scope retirement (which also adds to
retiredScopesso new leases are refused), andclose().assertActive()runs after every await and inside the stream callback, so a lease torn down mid-read aborts rather than completing into a dead lease.
The CSP does what the prose says, directive by directive: sandbox allow-scripts without allow-same-origin gives the document an opaque origin and, with no allow-popups, blocks popups; default-src 'none' covers frame-src/media-src by fallback; scripts and styles are inline-only with no host source; img-src/font-src are data:/blob: so nothing remote loads; connect-src 'none' removes fetch, XHR and WebSocket; base-uri, form-action and frame-ancestors are all 'none'. Cache-Control: no-store, Referrer-Policy: no-referrer and X-Content-Type-Options: nosniff are set on every response, including the 404.
I also want to note what the description does not claim, because that honesty is load-bearing: it states plainly that this is not OS network isolation and that an external browser can navigate away, and it marks the model-driven generation run as not passed rather than quietly omitting it.
Ablation
The description records one mutation check on Host validation. I ran a different one so a second guard is covered: replacing the bearer-path comparison so only Host and lease liveness are checked turns two tests red, including isolates leases by origin and rejects credentials for another preview — the cross-lease isolation property itself. I confirmed the marker reached the emitted dist/ before reading the result.
Evidence
build:main clean (zero TS errors) and 56/56 across managed-artifact-preview, runtime-host-artifacts-ipc-main and runtime-host-native-capabilities, matching the description's figure.
A note for anyone else verifying locally: @maka/desktop's main build consumes the built .d.ts from packages/ui/dist, not that package's sources. My first attempt failed with subscribeToReaderScroll does not exist in two files this PR does not touch, purely because my packages/ui/dist predated the scroll-authority rename that has since landed on main. Rebuilding @maka/ui at this head cleared it. A build's exit code tells you the compile succeeded, not which branch produced the artifacts it read.
Not covered by me
I did not open the endpoint in a real browser, so the manual macOS run described — Chrome Canary loading the preview and the interaction confirming — is the author's evidence, not mine. I did not exercise the model-driven path, which the description already excludes. One deliberate non-finding: the path comparison is an ordinary string compare rather than constant-time, but the attacker model that would need — local code execution to reach the loopback port, plus timing resolution through Node's HTTP stack — already grants direct access to the artifact, so I do not think it is worth changing.
Code review, CI status and merge readiness are separate. At this SHA the reported check is green, mergeable=MERGEABLE, mergeStateStatus=BLOCKED. This approval covers code only and is not a statement that the PR may be merged.
jackwener
left a comment
jackwener
left a comment
There was a problem hiding this comment.
Withdrawing my approval (@kabi-opus, automated agent review, shared account). Reviewed at 8a3c12ea190bd62d68a961ad900d8a4ceae69f86.
Please do not count my earlier APPROVED on this head as a review gate. Another reviewer reported two P2s on the same head; I went back to check them against the code rather than defend my own post, and both are substantiated. My approval was not wrong about what it examined — it was incomplete, in a way I should have caught.
Deletion does not revoke a live preview on every path
What I verified originally was runtime-host-artifacts-ipc-main.ts:85-86: the Desktop delete handler calls deleteArtifact and then preview.service.revoke(...). That is correct, and it is the only place in apps/desktop/src that deletes an artifact. My mistake was generalising from it without asking whether deletion can originate anywhere else. It can:
packages/runtime-host/src/server/deep-research-coordinator.ts:91—delete: (artifactId) => this.#artifacts.deleteOwnedArtifactInSession(sessionId, artifactId, 'deep_research'). An agent deleting its own artifact never reaches the Desktop handler.packages/runtime-host/src/server/session-sidecar-purge.ts:36andsession-retirement-coordinator.ts:125—purgeSessionArtifacts(sessionId)on session teardown.packages/runtime-host/src/server/artifact-coordinator.ts:70— theartifact.deleteoperation itself carries no coupling to the preview service; the revoke lives one layer up, in the Desktop caller.
None of these notify ManagedArtifactPreview, so the lease survives until its 30-minute TTL and the loopback URL keeps serving the deleted artifact's bytes. closeScope does not close this gap either: it is keyed on targetEpoch, not on sessionId, so purging a session's artifacts leaves that session's leases running.
The disclosure is bounded — the bearer path was already handed to the browser that opened it, so no new party gains access. But "I deleted it" not taking effect is exactly the expectation a delete action sets, and it is the reason someone deletes a generated document in the first place.
The preview quota is global, and exhaustion is reachable
My earlier review said the reservation "covers the pending window", and that remains true: the lease is inserted before the async read, so concurrent preparations cannot exceed the bound. But that is a narrower property than the one that matters, and I did not evaluate the wider one.
managed-artifact-preview.ts:71 tests this.leases.size >= MAX_PREVIEWS with no partition by scope or session. With MAX_PREVIEWS = 16, a 30-minute TTL, no eviction and a model-callable tool, one session can hold every lease and deny previews to every other session for half an hour. The thrown message — "Too many active previews; wait for expiry" — describes that outcome accurately; there is no recourse short of waiting.
What still stands from my review
The endpoint's own isolation properties are as described, and I do not want the withdrawal to obscure that: loopback-only binding, no listener before first use, a 256-bit CSPRNG bearer path, exact Host and whole-string URL matching that makes traversal structurally impossible, GET/HEAD only, the CSP directives checked one by one, the double size validation, and the ablation I ran showing the bearer-path check is pinned by the cross-lease isolation test. Those findings do not change; they simply do not cover the two lifecycle properties above.
The two P2s were raised by the Qronos review line on this head. I verified them independently against the source and am reporting what I found; the file and line references above are my own tracing, not a restatement of their analysis.
3 participants
Summary
Fixes #5238. Refs #5213. Based on upstream/main c22768c, including merged #5261.
Security and supported content
Per-lease loopback origin and 256-bit bearer path; exact Host/path validation; GET/HEAD only; no directory serving, CORS, caching or disk staging. Limits: 8 MiB each and 16 concurrent reservations, including pending reads.
Self-contained HTML supports inline interaction and embedded images/fonts. CSP sandbox excludes same-origin authority, remote resources, fetch requests, forms, frames and popups. This is not OS network isolation: an external browser can still navigate away from the document. Workspace assets are not served. Save As is the alternative for documents needing external resources. The URL belongs to the Desktop, even for artifacts streamed from a remote Host.
Verification
Scope boundary
Complete managed-endpoint implementation for #5238; not completion of all harness work. Capability preflight remains #5234 / existing #5254. Structured browser-open/load verification remains #5235. This PR does not close #5213 or #5235.
AI use
Tool(s) and scope: Codex implemented the service, integration, tests and documentation.
Checklist
Does this PR entail a change in behavior?