feat: render generated images in conversation threads - #367
Conversation
wingleeio
left a comment
There was a problem hiding this comment.
Security review of c3cf4c3: requesting changes for two desktop generated-media validation/resource-management defects below. No evidence of malicious intent, a backdoor, credential harvesting, or a new exfiltration endpoint was found. Intent cannot be proven by source review, and this is not a whole-application security certification.
Reviewed all changed files and six commits' aggregate diff, plus the prospective merge 77708ea against main 6d39f8c. The merge retains the intervening working-word/appshot-fade changes. No dependencies, lockfiles, CI workflows, binary/font assets, executable file modes, or symlinks are added or changed. The existing executable fake-codex fixture only gains test responses. Added-line control/format-character scanning and git diff --check passed.
Scope examined: provider event normalization and JSON-RPC value ownership; file-import path confinement, file-descriptor walking, metadata checks, byte bounds, temporary files and destination identity; parent/subagent event sanitization before journal/document publication; Rust/TypeScript/iOS metadata contracts and malformed references; attachment RPC routing and existing path jail; desktop/iOS byte and raster limits; caches, retries, loading cancellation, row identity and lightbox behavior; associated fixtures, tests and validation documentation.
Positive security properties: savedPath is the sole normalizer source; inline result/revisedPrompt are not copied into normalized output. Engine intake accepts an absolute source canonicalized beneath the configured generated_images root, checks a raster signature and 24 MiB limit, uses component-wise O_NOFOLLOW descriptor traversal on Unix, checks source identity/change before publication, and atomically persists under a SHA-256-derived uploads filename. Nested generated events pass through intake before publication, and import errors publish generic messages. Attachment transport uses the existing authenticated/targeted RPC and uploads/workspace-root restrictions rather than introducing arbitrary URL fetching. iOS validates declared versus actual raster type, limits dimensions to 4096 per axis, downsamples one static frame to 2048, and separates generated-policy cache keys. Desktop fresh generated loads cap bytes, dimensions and decoder allocation and normalize to a static PNG, but the cache path does not consistently enforce those protections (finding 1).
Validation evidence: CI logs show 995 desktop UI tests and 114 iOS tests passing, including the generated-image tests and native iOS image-arrival layout test. Other reported CI jobs are green. Thus the PR/documentation statement that iOS compilation/XCTest remain pending is stale. I independently ran the three generated-image import tests against the exact production uploads.rs in an isolated Rust harness; all passed (format/idempotence, invalid/outside/symlink/oversized sources, source modification/replacement and cleanup). This harness supplies only an EngineError wrapper and locally available dependencies; it is not a full workspace build under the repository lockfile.
I also compiled the exact production desktop cache logic in a small harness with lightweight Image/SharedString substitutes and valid PNG fixtures. It reproduced both findings without allocating large decoded images: a cached 4097×1 image is returned without a new load/validation, and ten protected 4096×4096 image entries retain 1,342,897,400 accounted bytes against the 67,108,864-byte budget with no eviction. The latter is an accounting/eviction reproduction, not a measured 1.25 GiB process-memory spike or an end-to-end exploit. Production call-site inspection establishes that generated rows use these cache paths and that protection covers all transcript rows.
Limits: no local full workspace rebuild, real-provider quota-consuming smoke test, manual multi-device/iPhone test, or exploit against another device was performed. Existing attachment RPC authorization/path-jail code was traced but not exhaustively penetration-tested. The issues below concern the new generated-image path; generic-attachment cache behavior already existed, and I am not claiming it was newly introduced in isolation.
| self.spawn_attachment_load( | ||
| dev.clone(), | ||
| path.to_string(), | ||
| expected_raster_mime.map(str::to_owned), |
There was a problem hiding this comment.
[P2] Keep validated generated images separate from generic cache entries
expected_raster_mime is only passed when starting a fresh load. attachment_state first returns any attachment_snapshot(dev, path) that is already Loaded, and begin_load/store_loaded use only (device, path). Therefore a generic user attachment or seeded upload at that path bypasses the generated-image MIME checks, 24 MiB/4096-pixel bounds and static-frame normalization entirely. A metadata MIME correction also reuses the old entry without revalidation.
Reproduced using the production cache logic and a valid 4097×1 PNG seeded through seed_attachment: begin_load returns false and attachment_snapshot returns the original image. A generated reference to it therefore never reaches decode_raster_image. The iOS test testGeneratedCacheCannotReuseAnUnvalidatedAttachment protects against exactly this case, but desktop currently permits it.
Key generated entries and in-flight claims by validation policy (including expected MIME), or require every cached image to meet the generated policy before returning it. Ensure upload aliases cannot bypass that distinction. Add desktop tests for generic-cache reuse, MIME correction and oversized/animated cached inputs.
| let devices = self.attachment_device_ids(cx); | ||
| let mut keys = std::collections::HashSet::new(); | ||
| for row in &self.rows { | ||
| if let RowKind::GeneratedImage { owner, path, .. } = &row.kind { |
There was a problem hiding this comment.
[P2] Limit eviction protection to visible generated images
protected_attachment_keys iterates every row in the conversation and now shields every generated image, including off-screen history. ImageCache::insert_loaded excludes all shielded keys from eviction and simply breaks when no candidate remains. Consequently the 64 MiB budget is not a bound for an image-heavy active conversation: previously loaded images remain protected as the user scrolls through more history.
A 4096×4096 image alone is accounted as roughly 128 MiB of CPU/GPU pixels. With ten such entries, the production eviction logic retained 1,342,897,400 accounted bytes with zero eviction in a targeted harness. Individual input/decoder caps do not constrain this aggregate retention. Long or adversarial image-heavy output can cause memory pressure or exhaust client memory without any oversized individual file.
Protect only images actually visible/in use, with bounded overscan if needed, and allow old generated rows to release encoded, decoded and GPU resources. Consider display-sized rasterization plus a bounded full-size lightbox load. Add a scrolling/history or cache test that verifies resident retention stays bounded as previously visible generated rows leave the viewport.
wingleeio
left a comment
There was a problem hiding this comment.
Security re-review of 7d454f4, superseding my changes-requested review.
Both reported blockers are fixed:
- Generated cache entries, snapshots, errors, cancellation guards and in-flight tasks now include the declared MIME/validation policy. Generic attachments and upload aliases cannot satisfy a generated-image lookup, and a MIME correction requires a separate validated load.
- Generated history is no longer protected from eviction. Generated previews have their own 64 MiB LRU accounting budget, including encoded bytes and estimated CPU/GPU pixels. Each preview is normalized to one static RGBA8 frame at most 2048 pixels per axis, so one entry fits the budget. Original files remain on the host; the lightbox uses the bounded preview.
Additional hardening verifies chunk MIME metadata and actual raster format against the declared MIME. Existing 24 MiB intake, 4096-per-axis decode limits, 64 MiB decoder allocation limit, authenticated attachment routing, source confinement, mutation checks and atomic publication remain in place.
Validation: the full locked desktop library suite passed locally (999/999), including new cache/alias/load-claim isolation, MIME mismatch, downsampling, two-frame GIF-to-static-PNG, and 100-image eviction regressions. The locked engine generated-image suite passed (4/4), including source rejection, copy-time mutation/cleanup, replay/format handling and nested-subagent sanitization. git diff --check passed. Rechecked the prospective merge b0850a1 against current main; unrelated upstream changes are preserved.
The earlier full-diff audit and this fix review found no evidence of malicious code, backdoors, credential harvesting or new exfiltration endpoints, and no remaining security blocker in the reviewed changes. This is a scoped source/test assessment, not proof of intent or a guarantee of zero vulnerabilities. Real-provider and manual multi-device/iPhone smoke tests were not performed. Fresh Linux/macOS/iOS CI must finish successfully before merge.
Closes #365.
Changes
savedPathsources undergenerated_imagesand atomically import PNG/JPEG/WebP/GIF files up to 24 MiB into profile uploads before journaling or publishing. Replayed completions reuse their image identity and destination.resultpayload. Generation remains an ordinary tool until the image is available.Media bytes remain on the generating host. Remote clients fetch them after generation completes; this does not provide progressive previews during generation. An uncached image requires its host to be online.
Validation
Previously completed desktop/backend validation:
online_runtime_shutdown_stops_edge_workers_and_retires_the_graphexcluded. Its failure was also reproduced on the original1ab04195checkout.zeron-themelints; the full audit completes with--cap-lints warn.New iOS validation:
git diff --checkpassed.Real-provider smoke tests remain ignored because they consume image quota. Manual light/dark, multi-client, and iPhone checks remain pending before marking this ready for review. See
docs/generated-images-validation.mdfor commands and the validation checklist.Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.