feat: render Image View outputs in chat#4436
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| if (entry.turnId === input.unsettledTurnId) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
🟡 Medium chat/MessagesTimeline.logic.ts:280
appendImageOutputRows drops all unscoped image outputs (entries with turnId === null) whenever there is no unsettled turn. When input.unsettledTurnId is null, the guard entry.turnId === input.unsettledTurnId is true for every unscoped entry, so they are all skipped and never rendered. The filter should only apply when input.unsettledTurnId is non-null.
| if (entry.turnId === input.unsettledTurnId) { | |
| continue; | |
| } | |
| if (input.unsettledTurnId !== null && entry.turnId === input.unsettledTurnId) { | |
| continue; | |
| } |
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/chat/MessagesTimeline.logic.ts around lines 280-282:
`appendImageOutputRows` drops all unscoped image outputs (entries with `turnId === null`) whenever there is no unsettled turn. When `input.unsettledTurnId` is `null`, the guard `entry.turnId === input.unsettledTurnId` is true for every unscoped entry, so they are all skipped and never rendered. The filter should only apply when `input.unsettledTurnId` is non-null.
| }), | ||
| ), | ||
| ); | ||
| if (Option.isNone(canonicalPath)) { |
There was a problem hiding this comment.
🔴 Critical assets/AssetAccess.ts:305
The thread-image case validates input.threadImagePath with isWorkspaceImagePreviewPath before calling realPath, and the resolved canonical path is never re-validated. A symlink like /tmp/image.png -> /etc/passwd passes the image-type check, then realPath resolves to /etc/passwd, which gets stored in the signed claim and served by the asset route. This allows arbitrary file disclosure via a crafted thread image path. After resolving canonicalPath, re-check that it still satisfies isWorkspaceImagePreviewPath before storing it in the claims.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/assets/AssetAccess.ts around line 305:
The `thread-image` case validates `input.threadImagePath` with `isWorkspaceImagePreviewPath` *before* calling `realPath`, and the resolved canonical path is never re-validated. A symlink like `/tmp/image.png -> /etc/passwd` passes the image-type check, then `realPath` resolves to `/etc/passwd`, which gets stored in the signed claim and served by the asset route. This allows arbitrary file disclosure via a crafted thread image path. After resolving `canonicalPath`, re-check that it still satisfies `isWorkspaceImagePreviewPath` before storing it in the claims.
9803691 to
63a8408
Compare
| return yield* fail("savePath must not be an existing symlink"); | ||
| } | ||
|
|
||
| yield* writeScreenshotFile({ |
There was a problem hiding this comment.
🔴 Critical preview/handlers.ts:209
saveSnapshotScreenshot performs path-based containment checks on destination and lexicalParent, then calls writeScreenshotFile using those same path strings. A process with control of the workspace can replace either path with an outward symlink in the gap between the checks and the write, so the write follows the new symlink and creates or overwrites an arbitrary file outside the workspace root. Open the destination by handle (e.g., O_NOFOLLOW + verify the fd's real path) and write to the handle instead of re-resolving the path, so the containment check and the write cannot be separated.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 209:
`saveSnapshotScreenshot` performs path-based containment checks on `destination` and `lexicalParent`, then calls `writeScreenshotFile` using those same path strings. A process with control of the workspace can replace either path with an outward symlink in the gap between the checks and the write, so the write follows the new symlink and creates or overwrites an arbitrary file outside the workspace root. Open the destination by handle (e.g., `O_NOFOLLOW` + verify the fd's real path) and write to the handle instead of re-resolving the path, so the containment check and the write cannot be separated.
| /> | ||
| ); | ||
| }, | ||
| video({ node: _node, src }) { |
There was a problem hiding this comment.
🟡 Medium components/ChatMarkdown.tsx:1536
The custom video renderer forwards only src to MarkdownMedia, discarding all other sanitized attributes. A markdown <video src="clip.webm" muted loop poster="preview.png"> previously rendered with muted, loop, and poster (all allowed by the sanitize schema), but now MarkdownMedia receives none of them and renders a video without those attributes. Pass the remaining props through so sanitized attributes are not lost.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/ChatMarkdown.tsx around line 1536:
The custom `video` renderer forwards only `src` to `MarkdownMedia`, discarding all other sanitized attributes. A markdown `<video src="clip.webm" muted loop poster="preview.png">` previously rendered with `muted`, `loop`, and `poster` (all allowed by the sanitize schema), but now `MarkdownMedia` receives none of them and renders a video without those attributes. Pass the remaining props through so sanitized attributes are not lost.
63a8408 to
3487566
Compare
| type="button" | ||
| className="block size-32 cursor-zoom-in overflow-hidden rounded-xl border border-border/70 bg-muted/20 p-1 transition-colors hover:border-border focus-visible:outline-none focus-visible:ring-2 focus-visible:ring-ring/70" | ||
| aria-label={`Preview ${imageName}`} | ||
| onClick={() => |
There was a problem hiding this comment.
🟡 Medium chat/MessagesTimeline.tsx:2038
ImageOutputThumbnail constructs a single-item images array for onImageExpand, so when a gallery contains multiple thumbnails, expanding any image opens the viewer with only that one image. The expanded viewer cannot navigate to sibling images the way it should for multi-image galleries. Consider passing the gallery's full resolved image list and the clicked index into onImageExpand instead of a one-element array.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 2038:
`ImageOutputThumbnail` constructs a single-item `images` array for `onImageExpand`, so when a gallery contains multiple thumbnails, expanding any image opens the viewer with only that one image. The expanded viewer cannot navigate to sibling images the way it should for multi-image galleries. Consider passing the gallery's full resolved image list and the clicked index into `onImageExpand` instead of a one-element array.
| : null; | ||
| } | ||
|
|
||
| if (claims.kind === "thread-image") { |
There was a problem hiding this comment.
🔴 Critical assets/AssetAccess.ts:516
The thread-image branch in resolveAsset calls stat on the signed canonical path and returns that path without re-resolving it at request time. stat follows symlinks, so if the file is replaced with a symlink after URL issuance, stat still reports File and the response serves the symlink's target — allowing disclosure of arbitrary readable local files. The other workspace branches re-canonicalize via resolveCanonicalWorkspaceFileForRequest at resolution time, but thread-image trusts the stored path. Consider re-resolving and re-validating claims.path against its expected root at request time, or at least using lstat and rejecting symlinks.
🤖 Copy this AI Prompt to have your agent fix this:
In file @apps/server/src/assets/AssetAccess.ts around line 516:
The `thread-image` branch in `resolveAsset` calls `stat` on the signed canonical path and returns that path without re-resolving it at request time. `stat` follows symlinks, so if the file is replaced with a symlink after URL issuance, `stat` still reports `File` and the response serves the symlink's target — allowing disclosure of arbitrary readable local files. The other workspace branches re-canonicalize via `resolveCanonicalWorkspaceFileForRequest` at resolution time, but `thread-image` trusts the stored path. Consider re-resolving and re-validating `claims.path` against its expected root at request time, or at least using `lstat` and rejecting symlinks.
Stacked on #4321
This PR targets and depends on #4321, which provides the shared signed-media asset infrastructure. The diff here is limited to native Codex Image View support.
Summary
imageView.pathandimageGeneration.savedPathoutput eventsTesting