Render saved screenshots and recordings in chat markdown - #4321

Closed
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media
Closed

Render saved screenshots and recordings in chat markdown#4321
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media

Conversation

@juliusmarminge

@juliusmarmingejuliusmarminge commented Jul 22, 2026

Copy link
Copy Markdown
Member

Summary

  • Add secure asset access for saved screenshots, videos, and browser artifacts.
  • Support saving preview screenshots to workspace paths or browser artifact storage.
  • Render saved media directly in chat markdown.
  • Extend preview recording and snapshot contracts and handlers.
  • Tighten pull request head matching and simplify related UI logic.

Testing

  • Added focused coverage for asset URL issuance and browser artifact validation.
  • Added MCP snapshot save-path and artifact persistence tests.
  • Passed focused asset, MCP, timeline, and session tests plus target package typechecks, targeted lint, and a web production build.
  • Verified signed workspace images inline and expanded in the real web client.

Note

High Risk
Changes signed asset authorization, filesystem containment, and MCP-driven writes into the workspace and browser-artifacts store—security-sensitive paths with extensive symlink handling that must stay correct on every request.

Overview
Chat and MCP preview flows can now show saved screenshots and recordings inline instead of leaving paths as plain text.

Signed asset serving adds browser-artifact and thread-image resources, treats workspace videos like images (exact-file tokens), and tightens containment (real .. segments only, not names like ..screenshots). Browser artifacts and thread images are re-canonicalized on every fetch so post-issuance symlink swaps do not keep serving the old target.

preview_snapshot can persist PNGs with save:true into browser-artifacts or with savePath into the thread workspace; workspace writes validate extensions, traversal, and directory/file symlinks before writing. Tool metadata reflects that snapshot is no longer read-only/idempotent when saving.

Thread UI maps Codex image view/generation activities to local paths (pruning huge base64 payloads), derives work-log imagePath/imageActivityId, and shows deduplicated galleries on the terminal assistant message or as image-output rows once the turn settles.

Chat markdown renders img/video via MarkdownMedia, resolving workspace-relative paths and absolute browser-artifacts paths to signed URLs (including Windows drive path sanitization).

Reviewed by Cursor Bugbot for commit 58f9832. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Render saved screenshots and recordings in chat markdown with asset URL resolution

  • Adds save/savePath parameters to the preview_snapshot MCP tool, writing screenshots to either a workspace-relative path or a dedicated browser-artifacts directory; returns savedScreenshotPath in the response.
  • Extends the chat markdown renderer to display <img> and <video> elements via a new MarkdownMedia component that resolves workspace-relative and absolute paths to signed asset URLs.
  • Introduces browser-artifact and thread-image asset resource kinds end-to-end: new claim types in the JWT schema, issuance and re-resolution logic in AssetAccess, and a new assetsCreateUrl WS handler branch for thread-image.
  • Adds image_view support to the messages timeline: appendImageOutputRows groups deduplicated image thumbnails per turn into inline galleries attached to assistant messages or emitted as standalone rows.
  • Adds a Windows drive path rehype plugin (ChatMarkdown.tsx) that prefixes C:\... srcs with / before sanitization so they survive the sanitize pass.
  • Risk: browser-artifact and thread-image claims are re-resolved on every asset request (realpath + stat), adding filesystem I/O to each signed URL fetch.

Macroscope summarized 58f9832.

@coderabbitai

coderabbitaiBot commented Jul 22, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c82b6be7-c79a-4e7b-a19b-79b6c103f8ee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/render-chat-media

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 22, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effect service review: one convention issue found regarding error cause preservation. See inline comment.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
Comment threadapps/web/src/components/ChatMarkdown.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadpackages/contracts/src/previewAutomation.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
@macroscopeapp

macroscopeappBot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

3 blocking correctness issues found. This PR introduces a new feature for rendering screenshots/recordings in chat with significant new code paths. Two unresolved Critical security comments identify TOCTOU race conditions and hard link vulnerabilities in the file-writing logic that require human review before merging.

You can customize Macroscope's approvability policy. Learn more.

@github-actionsgithub-actionsBot added size:XL 500-999 changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Jul 23, 2026
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
@github-actionsgithub-actionsBot added size:XXL 1,000+ changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Jul 27, 2026
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/server/src/assets/AssetAccess.ts Outdated
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/session-logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 0a9ea4b to 3fc373aCompareJuly 29, 2026 15:44

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding on the new PreviewAutomationScreenshotSaveError shape; everything else (namespace imports, environment-based dependency acquisition for ServerConfig/ProjectionSnapshotQuery/WorkspacePaths/FileSystem/Path, Effect.catchTags usage, Schema.TaggedErrorClass errors in packages/contracts/src/assets.ts) matches the conventions.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
// write would follow it, and a dangling link (realPath reports NotFound)
// could silently create a file outside the workspace at its target.
const destination = path.join(canonicalParent, path.basename(resolved.absolutePath));
const destinationIsSymlink = yield* fileSystem.readLink(destination).pipe(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:214

saveSnapshotScreenshot checks whether the destination is a symlink with readLink (line 214), then writes to it in a separate writeScreenshotFile call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing writeFile to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the realPath parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with O_NOFOLLOW | O_CREAT | O_EXCL) rather than verifying the path is not a symlink beforehand.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 214:
`saveSnapshotScreenshot` checks whether the destination is a symlink with `readLink` (line 214), then writes to it in a separate `writeScreenshotFile` call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing `writeFile` to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the `realPath` parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with `O_NOFOLLOW | O_CREAT | O_EXCL`) rather than verifying the path is not a symlink beforehand.

return yield* fail("destination-symlink-validation");
}

yield* writeScreenshotFile({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:219

saveSnapshotScreenshot rejects symlinks at the destination but does not reject hard links. A workspace process can create savePath as a hard link to a file outside the workspace, and writeScreenshotFile then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The readLink check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via stat) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 219:
`saveSnapshotScreenshot` rejects symlinks at the destination but does not reject hard links. A workspace process can create `savePath` as a hard link to a file outside the workspace, and `writeScreenshotFile` then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The `readLink` check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via `stat`) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from faadbf0 to 3c0d0c9CompareJuly 29, 2026 15:56
Comment on lines +37 to +41
function mediaFileName(src: string): string {
const withoutQuery = src.split(/[?#]/, 1)[0] ?? src;
const basename = withoutQuery.slice(withoutQuery.lastIndexOf("/") + 1);
return basename.length > 0 ? safeDecode(basename) : safeDecode(withoutQuery);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Lowchat/MarkdownMedia.tsx:37

mediaFileName only treats / as a path separator, so Windows-style sources like /C:\repo\clip.mp4 (produced by the Windows-drive-path escaping) return the full path C:\repo\clip.mp4 instead of just clip.mp4. When there is no nonempty alt text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both / and \ so Windows backslash separators are handled.

Suggested change
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constbasename=withoutQuery.slice(withoutQuery.lastIndexOf("/")+1);
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constlastSep=Math.max(withoutQuery.lastIndexOf("/"),withoutQuery.lastIndexOf("\\"));
constbasename=lastSep>=0 ? withoutQuery.slice(lastSep+1) : withoutQuery;
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MarkdownMedia.tsx around lines 37-41:
`mediaFileName` only treats `/` as a path separator, so Windows-style sources like `/C:\repo\clip.mp4` (produced by the Windows-drive-path escaping) return the full path `C:\repo\clip.mp4` instead of just `clip.mp4`. When there is no nonempty `alt` text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both `/` and `\` so Windows backslash separators are handled.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 3c0d0c9 to b5bc982CompareJuly 29, 2026 20:58
juliusmarmingeand others added 2 commits July 29, 2026 16:58
- Add secure asset access for workspace media and browser artifacts
- Support saving preview screenshots and embedding playable recordings
- Preserve error causes in screenshot save failures
- Reject save+savePath combinations and describe both schema fields
- Verify canonical (symlink-resolved) containment before workspace writes
- Add collision-resistant suffix to artifact screenshot names
- Fix Windows drive paths, protocol-relative URLs, query strings, and
extensionless video sources in markdown media resolution
- Only treat absolute browser-artifacts paths as server artifacts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
juliusmarmingeand others added 7 commits July 29, 2026 16:58
Verify the deepest existing ancestor canonicalizes inside the workspace
before mkdir, and reject existing destination symlinks that resolve
outside the root.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Re-resolve browser-artifact and thread-image claims on every request so
post-issuance symlink swaps cannot redirect a signed token
- Verify canonical containment and media extension after realPath
- Render unscoped image outputs when no turn is unsettled
- Keep the image activity id paired with its path across collapsing
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Drop readonly/idempotent hints from preview_snapshot now that it writes
- Treat only real .. segments as escapes when resolving workspace assets
- Append image galleries when turns share a target row
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Payload pruning (#4622) stripped every item field outside the command
allowlist, so image view activities reached clients without the path and
the chat gallery rendered nothing. Preserve the item type and path while
still dropping the base64 image bytes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from b5bc982 to 5b2f955CompareJuly 29, 2026 21:00
);
}

function ImageOutputThumbnail({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumchat/MessagesTimeline.tsx:1966

Each thumbnail calls onImageExpand with images containing only its own loadedUrl, so the expanded viewer cannot navigate to other images in the same gallery. The images array should include all loaded thumbnails and index should point to the clicked one, so the preview can show the full gallery as expected.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 1966:
Each thumbnail calls `onImageExpand` with `images` containing only its own `loadedUrl`, so the expanded viewer cannot navigate to other images in the same gallery. The `images` array should include all loaded thumbnails and `index` should point to the clicked one, so the preview can show the full gallery as expected.

Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON, but a cloud agent failed to start.

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.


const projectedData: Record<string, unknown> = {};
const item = projectCommandData(data);
const item = projectCommandData(data) ?? projectImageViewData(data);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command projection drops image paths

Medium Severity

For image_view activities, projectActivityPayload sets data.item from projectCommandData(data) ?? projectImageViewData(data). When the tool item includes any pruned command field (command, input.command, or result.command), command projection wins and image-view projection is skipped, so type, path, and savedPath never reach thread snapshots. Clients then derive no imagePath, so inline galleries and thread-image asset requests for those activities fail even though the full payload in persistence still carries the image path.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.

@juliusmarminge

Copy link
Copy Markdown
MemberAuthor

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL1,000+ changed lines (additions + deletions).vouch:trustedPR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@juliusmarminge@Noojuno@gabrielelpidio
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Render saved screenshots and recordings in chat markdown - #4321

Closed
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media
Closed

Render saved screenshots and recordings in chat markdown#4321
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media

Conversation

@juliusmarminge

@juliusmarmingejuliusmarminge commented Jul 22, 2026

Copy link
Copy Markdown
Member

Summary

  • Add secure asset access for saved screenshots, videos, and browser artifacts.
  • Support saving preview screenshots to workspace paths or browser artifact storage.
  • Render saved media directly in chat markdown.
  • Extend preview recording and snapshot contracts and handlers.
  • Tighten pull request head matching and simplify related UI logic.

Testing

  • Added focused coverage for asset URL issuance and browser artifact validation.
  • Added MCP snapshot save-path and artifact persistence tests.
  • Passed focused asset, MCP, timeline, and session tests plus target package typechecks, targeted lint, and a web production build.
  • Verified signed workspace images inline and expanded in the real web client.

Note

High Risk
Changes signed asset authorization, filesystem containment, and MCP-driven writes into the workspace and browser-artifacts store—security-sensitive paths with extensive symlink handling that must stay correct on every request.

Overview
Chat and MCP preview flows can now show saved screenshots and recordings inline instead of leaving paths as plain text.

Signed asset serving adds browser-artifact and thread-image resources, treats workspace videos like images (exact-file tokens), and tightens containment (real .. segments only, not names like ..screenshots). Browser artifacts and thread images are re-canonicalized on every fetch so post-issuance symlink swaps do not keep serving the old target.

preview_snapshot can persist PNGs with save:true into browser-artifacts or with savePath into the thread workspace; workspace writes validate extensions, traversal, and directory/file symlinks before writing. Tool metadata reflects that snapshot is no longer read-only/idempotent when saving.

Thread UI maps Codex image view/generation activities to local paths (pruning huge base64 payloads), derives work-log imagePath/imageActivityId, and shows deduplicated galleries on the terminal assistant message or as image-output rows once the turn settles.

Chat markdown renders img/video via MarkdownMedia, resolving workspace-relative paths and absolute browser-artifacts paths to signed URLs (including Windows drive path sanitization).

Reviewed by Cursor Bugbot for commit 58f9832. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Render saved screenshots and recordings in chat markdown with asset URL resolution

  • Adds save/savePath parameters to the preview_snapshot MCP tool, writing screenshots to either a workspace-relative path or a dedicated browser-artifacts directory; returns savedScreenshotPath in the response.
  • Extends the chat markdown renderer to display <img> and <video> elements via a new MarkdownMedia component that resolves workspace-relative and absolute paths to signed asset URLs.
  • Introduces browser-artifact and thread-image asset resource kinds end-to-end: new claim types in the JWT schema, issuance and re-resolution logic in AssetAccess, and a new assetsCreateUrl WS handler branch for thread-image.
  • Adds image_view support to the messages timeline: appendImageOutputRows groups deduplicated image thumbnails per turn into inline galleries attached to assistant messages or emitted as standalone rows.
  • Adds a Windows drive path rehype plugin (ChatMarkdown.tsx) that prefixes C:\... srcs with / before sanitization so they survive the sanitize pass.
  • Risk: browser-artifact and thread-image claims are re-resolved on every asset request (realpath + stat), adding filesystem I/O to each signed URL fetch.

Macroscope summarized 58f9832.

@coderabbitai

coderabbitaiBot commented Jul 22, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c82b6be7-c79a-4e7b-a19b-79b6c103f8ee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/render-chat-media

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 22, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effect service review: one convention issue found regarding error cause preservation. See inline comment.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
Comment threadapps/web/src/components/ChatMarkdown.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadpackages/contracts/src/previewAutomation.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
@macroscopeapp

macroscopeappBot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

3 blocking correctness issues found. This PR introduces a new feature for rendering screenshots/recordings in chat with significant new code paths. Two unresolved Critical security comments identify TOCTOU race conditions and hard link vulnerabilities in the file-writing logic that require human review before merging.

You can customize Macroscope's approvability policy. Learn more.

@github-actionsgithub-actionsBot added size:XL 500-999 changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Jul 23, 2026
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
@github-actionsgithub-actionsBot added size:XXL 1,000+ changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Jul 27, 2026
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/server/src/assets/AssetAccess.ts Outdated
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/session-logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 0a9ea4b to 3fc373aCompareJuly 29, 2026 15:44

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding on the new PreviewAutomationScreenshotSaveError shape; everything else (namespace imports, environment-based dependency acquisition for ServerConfig/ProjectionSnapshotQuery/WorkspacePaths/FileSystem/Path, Effect.catchTags usage, Schema.TaggedErrorClass errors in packages/contracts/src/assets.ts) matches the conventions.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
// write would follow it, and a dangling link (realPath reports NotFound)
// could silently create a file outside the workspace at its target.
const destination = path.join(canonicalParent, path.basename(resolved.absolutePath));
const destinationIsSymlink = yield* fileSystem.readLink(destination).pipe(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:214

saveSnapshotScreenshot checks whether the destination is a symlink with readLink (line 214), then writes to it in a separate writeScreenshotFile call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing writeFile to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the realPath parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with O_NOFOLLOW | O_CREAT | O_EXCL) rather than verifying the path is not a symlink beforehand.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 214:
`saveSnapshotScreenshot` checks whether the destination is a symlink with `readLink` (line 214), then writes to it in a separate `writeScreenshotFile` call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing `writeFile` to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the `realPath` parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with `O_NOFOLLOW | O_CREAT | O_EXCL`) rather than verifying the path is not a symlink beforehand.

return yield* fail("destination-symlink-validation");
}

yield* writeScreenshotFile({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:219

saveSnapshotScreenshot rejects symlinks at the destination but does not reject hard links. A workspace process can create savePath as a hard link to a file outside the workspace, and writeScreenshotFile then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The readLink check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via stat) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 219:
`saveSnapshotScreenshot` rejects symlinks at the destination but does not reject hard links. A workspace process can create `savePath` as a hard link to a file outside the workspace, and `writeScreenshotFile` then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The `readLink` check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via `stat`) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from faadbf0 to 3c0d0c9CompareJuly 29, 2026 15:56
Comment on lines +37 to +41
function mediaFileName(src: string): string {
const withoutQuery = src.split(/[?#]/, 1)[0] ?? src;
const basename = withoutQuery.slice(withoutQuery.lastIndexOf("/") + 1);
return basename.length > 0 ? safeDecode(basename) : safeDecode(withoutQuery);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Lowchat/MarkdownMedia.tsx:37

mediaFileName only treats / as a path separator, so Windows-style sources like /C:\repo\clip.mp4 (produced by the Windows-drive-path escaping) return the full path C:\repo\clip.mp4 instead of just clip.mp4. When there is no nonempty alt text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both / and \ so Windows backslash separators are handled.

Suggested change
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constbasename=withoutQuery.slice(withoutQuery.lastIndexOf("/")+1);
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constlastSep=Math.max(withoutQuery.lastIndexOf("/"),withoutQuery.lastIndexOf("\\"));
constbasename=lastSep>=0 ? withoutQuery.slice(lastSep+1) : withoutQuery;
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MarkdownMedia.tsx around lines 37-41:
`mediaFileName` only treats `/` as a path separator, so Windows-style sources like `/C:\repo\clip.mp4` (produced by the Windows-drive-path escaping) return the full path `C:\repo\clip.mp4` instead of just `clip.mp4`. When there is no nonempty `alt` text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both `/` and `\` so Windows backslash separators are handled.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 3c0d0c9 to b5bc982CompareJuly 29, 2026 20:58
juliusmarmingeand others added 2 commits July 29, 2026 16:58
- Add secure asset access for workspace media and browser artifacts
- Support saving preview screenshots and embedding playable recordings
- Preserve error causes in screenshot save failures
- Reject save+savePath combinations and describe both schema fields
- Verify canonical (symlink-resolved) containment before workspace writes
- Add collision-resistant suffix to artifact screenshot names
- Fix Windows drive paths, protocol-relative URLs, query strings, and
extensionless video sources in markdown media resolution
- Only treat absolute browser-artifacts paths as server artifacts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
juliusmarmingeand others added 7 commits July 29, 2026 16:58
Verify the deepest existing ancestor canonicalizes inside the workspace
before mkdir, and reject existing destination symlinks that resolve
outside the root.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Re-resolve browser-artifact and thread-image claims on every request so
post-issuance symlink swaps cannot redirect a signed token
- Verify canonical containment and media extension after realPath
- Render unscoped image outputs when no turn is unsettled
- Keep the image activity id paired with its path across collapsing
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Drop readonly/idempotent hints from preview_snapshot now that it writes
- Treat only real .. segments as escapes when resolving workspace assets
- Append image galleries when turns share a target row
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Payload pruning (#4622) stripped every item field outside the command
allowlist, so image view activities reached clients without the path and
the chat gallery rendered nothing. Preserve the item type and path while
still dropping the base64 image bytes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from b5bc982 to 5b2f955CompareJuly 29, 2026 21:00
);
}

function ImageOutputThumbnail({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumchat/MessagesTimeline.tsx:1966

Each thumbnail calls onImageExpand with images containing only its own loadedUrl, so the expanded viewer cannot navigate to other images in the same gallery. The images array should include all loaded thumbnails and index should point to the clicked one, so the preview can show the full gallery as expected.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 1966:
Each thumbnail calls `onImageExpand` with `images` containing only its own `loadedUrl`, so the expanded viewer cannot navigate to other images in the same gallery. The `images` array should include all loaded thumbnails and `index` should point to the clicked one, so the preview can show the full gallery as expected.

Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON, but a cloud agent failed to start.

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.


const projectedData: Record<string, unknown> = {};
const item = projectCommandData(data);
const item = projectCommandData(data) ?? projectImageViewData(data);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command projection drops image paths

Medium Severity

For image_view activities, projectActivityPayload sets data.item from projectCommandData(data) ?? projectImageViewData(data). When the tool item includes any pruned command field (command, input.command, or result.command), command projection wins and image-view projection is skipped, so type, path, and savedPath never reach thread snapshots. Clients then derive no imagePath, so inline galleries and thread-image asset requests for those activities fail even though the full payload in persistence still carries the image path.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.

@juliusmarminge

Copy link
Copy Markdown
MemberAuthor

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL1,000+ changed lines (additions + deletions).vouch:trustedPR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@juliusmarminge@Noojuno@gabrielelpidio
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Render saved screenshots and recordings in chat markdown - #4321

Closed
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media
Closed

Render saved screenshots and recordings in chat markdown#4321
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media

Conversation

@juliusmarminge

@juliusmarmingejuliusmarminge commented Jul 22, 2026

Copy link
Copy Markdown
Member

Summary

  • Add secure asset access for saved screenshots, videos, and browser artifacts.
  • Support saving preview screenshots to workspace paths or browser artifact storage.
  • Render saved media directly in chat markdown.
  • Extend preview recording and snapshot contracts and handlers.
  • Tighten pull request head matching and simplify related UI logic.

Testing

  • Added focused coverage for asset URL issuance and browser artifact validation.
  • Added MCP snapshot save-path and artifact persistence tests.
  • Passed focused asset, MCP, timeline, and session tests plus target package typechecks, targeted lint, and a web production build.
  • Verified signed workspace images inline and expanded in the real web client.

Note

High Risk
Changes signed asset authorization, filesystem containment, and MCP-driven writes into the workspace and browser-artifacts store—security-sensitive paths with extensive symlink handling that must stay correct on every request.

Overview
Chat and MCP preview flows can now show saved screenshots and recordings inline instead of leaving paths as plain text.

Signed asset serving adds browser-artifact and thread-image resources, treats workspace videos like images (exact-file tokens), and tightens containment (real .. segments only, not names like ..screenshots). Browser artifacts and thread images are re-canonicalized on every fetch so post-issuance symlink swaps do not keep serving the old target.

preview_snapshot can persist PNGs with save:true into browser-artifacts or with savePath into the thread workspace; workspace writes validate extensions, traversal, and directory/file symlinks before writing. Tool metadata reflects that snapshot is no longer read-only/idempotent when saving.

Thread UI maps Codex image view/generation activities to local paths (pruning huge base64 payloads), derives work-log imagePath/imageActivityId, and shows deduplicated galleries on the terminal assistant message or as image-output rows once the turn settles.

Chat markdown renders img/video via MarkdownMedia, resolving workspace-relative paths and absolute browser-artifacts paths to signed URLs (including Windows drive path sanitization).

Reviewed by Cursor Bugbot for commit 58f9832. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Render saved screenshots and recordings in chat markdown with asset URL resolution

  • Adds save/savePath parameters to the preview_snapshot MCP tool, writing screenshots to either a workspace-relative path or a dedicated browser-artifacts directory; returns savedScreenshotPath in the response.
  • Extends the chat markdown renderer to display <img> and <video> elements via a new MarkdownMedia component that resolves workspace-relative and absolute paths to signed asset URLs.
  • Introduces browser-artifact and thread-image asset resource kinds end-to-end: new claim types in the JWT schema, issuance and re-resolution logic in AssetAccess, and a new assetsCreateUrl WS handler branch for thread-image.
  • Adds image_view support to the messages timeline: appendImageOutputRows groups deduplicated image thumbnails per turn into inline galleries attached to assistant messages or emitted as standalone rows.
  • Adds a Windows drive path rehype plugin (ChatMarkdown.tsx) that prefixes C:\... srcs with / before sanitization so they survive the sanitize pass.
  • Risk: browser-artifact and thread-image claims are re-resolved on every asset request (realpath + stat), adding filesystem I/O to each signed URL fetch.

Macroscope summarized 58f9832.

@coderabbitai

coderabbitaiBot commented Jul 22, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c82b6be7-c79a-4e7b-a19b-79b6c103f8ee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/render-chat-media

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 22, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effect service review: one convention issue found regarding error cause preservation. See inline comment.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
Comment threadapps/web/src/components/ChatMarkdown.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadpackages/contracts/src/previewAutomation.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
@macroscopeapp

macroscopeappBot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

3 blocking correctness issues found. This PR introduces a new feature for rendering screenshots/recordings in chat with significant new code paths. Two unresolved Critical security comments identify TOCTOU race conditions and hard link vulnerabilities in the file-writing logic that require human review before merging.

You can customize Macroscope's approvability policy. Learn more.

@github-actionsgithub-actionsBot added size:XL 500-999 changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Jul 23, 2026
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
@github-actionsgithub-actionsBot added size:XXL 1,000+ changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Jul 27, 2026
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/server/src/assets/AssetAccess.ts Outdated
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/session-logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 0a9ea4b to 3fc373aCompareJuly 29, 2026 15:44

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding on the new PreviewAutomationScreenshotSaveError shape; everything else (namespace imports, environment-based dependency acquisition for ServerConfig/ProjectionSnapshotQuery/WorkspacePaths/FileSystem/Path, Effect.catchTags usage, Schema.TaggedErrorClass errors in packages/contracts/src/assets.ts) matches the conventions.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
// write would follow it, and a dangling link (realPath reports NotFound)
// could silently create a file outside the workspace at its target.
const destination = path.join(canonicalParent, path.basename(resolved.absolutePath));
const destinationIsSymlink = yield* fileSystem.readLink(destination).pipe(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:214

saveSnapshotScreenshot checks whether the destination is a symlink with readLink (line 214), then writes to it in a separate writeScreenshotFile call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing writeFile to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the realPath parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with O_NOFOLLOW | O_CREAT | O_EXCL) rather than verifying the path is not a symlink beforehand.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 214:
`saveSnapshotScreenshot` checks whether the destination is a symlink with `readLink` (line 214), then writes to it in a separate `writeScreenshotFile` call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing `writeFile` to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the `realPath` parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with `O_NOFOLLOW | O_CREAT | O_EXCL`) rather than verifying the path is not a symlink beforehand.

return yield* fail("destination-symlink-validation");
}

yield* writeScreenshotFile({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:219

saveSnapshotScreenshot rejects symlinks at the destination but does not reject hard links. A workspace process can create savePath as a hard link to a file outside the workspace, and writeScreenshotFile then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The readLink check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via stat) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 219:
`saveSnapshotScreenshot` rejects symlinks at the destination but does not reject hard links. A workspace process can create `savePath` as a hard link to a file outside the workspace, and `writeScreenshotFile` then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The `readLink` check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via `stat`) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from faadbf0 to 3c0d0c9CompareJuly 29, 2026 15:56
Comment on lines +37 to +41
function mediaFileName(src: string): string {
const withoutQuery = src.split(/[?#]/, 1)[0] ?? src;
const basename = withoutQuery.slice(withoutQuery.lastIndexOf("/") + 1);
return basename.length > 0 ? safeDecode(basename) : safeDecode(withoutQuery);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Lowchat/MarkdownMedia.tsx:37

mediaFileName only treats / as a path separator, so Windows-style sources like /C:\repo\clip.mp4 (produced by the Windows-drive-path escaping) return the full path C:\repo\clip.mp4 instead of just clip.mp4. When there is no nonempty alt text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both / and \ so Windows backslash separators are handled.

Suggested change
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constbasename=withoutQuery.slice(withoutQuery.lastIndexOf("/")+1);
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constlastSep=Math.max(withoutQuery.lastIndexOf("/"),withoutQuery.lastIndexOf("\\"));
constbasename=lastSep>=0 ? withoutQuery.slice(lastSep+1) : withoutQuery;
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MarkdownMedia.tsx around lines 37-41:
`mediaFileName` only treats `/` as a path separator, so Windows-style sources like `/C:\repo\clip.mp4` (produced by the Windows-drive-path escaping) return the full path `C:\repo\clip.mp4` instead of just `clip.mp4`. When there is no nonempty `alt` text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both `/` and `\` so Windows backslash separators are handled.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 3c0d0c9 to b5bc982CompareJuly 29, 2026 20:58
juliusmarmingeand others added 2 commits July 29, 2026 16:58
- Add secure asset access for workspace media and browser artifacts
- Support saving preview screenshots and embedding playable recordings
- Preserve error causes in screenshot save failures
- Reject save+savePath combinations and describe both schema fields
- Verify canonical (symlink-resolved) containment before workspace writes
- Add collision-resistant suffix to artifact screenshot names
- Fix Windows drive paths, protocol-relative URLs, query strings, and
extensionless video sources in markdown media resolution
- Only treat absolute browser-artifacts paths as server artifacts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
juliusmarmingeand others added 7 commits July 29, 2026 16:58
Verify the deepest existing ancestor canonicalizes inside the workspace
before mkdir, and reject existing destination symlinks that resolve
outside the root.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Re-resolve browser-artifact and thread-image claims on every request so
post-issuance symlink swaps cannot redirect a signed token
- Verify canonical containment and media extension after realPath
- Render unscoped image outputs when no turn is unsettled
- Keep the image activity id paired with its path across collapsing
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Drop readonly/idempotent hints from preview_snapshot now that it writes
- Treat only real .. segments as escapes when resolving workspace assets
- Append image galleries when turns share a target row
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Payload pruning (#4622) stripped every item field outside the command
allowlist, so image view activities reached clients without the path and
the chat gallery rendered nothing. Preserve the item type and path while
still dropping the base64 image bytes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from b5bc982 to 5b2f955CompareJuly 29, 2026 21:00
);
}

function ImageOutputThumbnail({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumchat/MessagesTimeline.tsx:1966

Each thumbnail calls onImageExpand with images containing only its own loadedUrl, so the expanded viewer cannot navigate to other images in the same gallery. The images array should include all loaded thumbnails and index should point to the clicked one, so the preview can show the full gallery as expected.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 1966:
Each thumbnail calls `onImageExpand` with `images` containing only its own `loadedUrl`, so the expanded viewer cannot navigate to other images in the same gallery. The `images` array should include all loaded thumbnails and `index` should point to the clicked one, so the preview can show the full gallery as expected.

Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON, but a cloud agent failed to start.

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.


const projectedData: Record<string, unknown> = {};
const item = projectCommandData(data);
const item = projectCommandData(data) ?? projectImageViewData(data);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command projection drops image paths

Medium Severity

For image_view activities, projectActivityPayload sets data.item from projectCommandData(data) ?? projectImageViewData(data). When the tool item includes any pruned command field (command, input.command, or result.command), command projection wins and image-view projection is skipped, so type, path, and savedPath never reach thread snapshots. Clients then derive no imagePath, so inline galleries and thread-image asset requests for those activities fail even though the full payload in persistence still carries the image path.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.

@juliusmarminge

Copy link
Copy Markdown
MemberAuthor

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL1,000+ changed lines (additions + deletions).vouch:trustedPR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@juliusmarminge@Noojuno@gabrielelpidio
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Render saved screenshots and recordings in chat markdown - #4321

Closed
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media
Closed

Render saved screenshots and recordings in chat markdown#4321
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media

Conversation

@juliusmarminge

@juliusmarmingejuliusmarminge commented Jul 22, 2026

Copy link
Copy Markdown
Member

Summary

  • Add secure asset access for saved screenshots, videos, and browser artifacts.
  • Support saving preview screenshots to workspace paths or browser artifact storage.
  • Render saved media directly in chat markdown.
  • Extend preview recording and snapshot contracts and handlers.
  • Tighten pull request head matching and simplify related UI logic.

Testing

  • Added focused coverage for asset URL issuance and browser artifact validation.
  • Added MCP snapshot save-path and artifact persistence tests.
  • Passed focused asset, MCP, timeline, and session tests plus target package typechecks, targeted lint, and a web production build.
  • Verified signed workspace images inline and expanded in the real web client.

Note

High Risk
Changes signed asset authorization, filesystem containment, and MCP-driven writes into the workspace and browser-artifacts store—security-sensitive paths with extensive symlink handling that must stay correct on every request.

Overview
Chat and MCP preview flows can now show saved screenshots and recordings inline instead of leaving paths as plain text.

Signed asset serving adds browser-artifact and thread-image resources, treats workspace videos like images (exact-file tokens), and tightens containment (real .. segments only, not names like ..screenshots). Browser artifacts and thread images are re-canonicalized on every fetch so post-issuance symlink swaps do not keep serving the old target.

preview_snapshot can persist PNGs with save:true into browser-artifacts or with savePath into the thread workspace; workspace writes validate extensions, traversal, and directory/file symlinks before writing. Tool metadata reflects that snapshot is no longer read-only/idempotent when saving.

Thread UI maps Codex image view/generation activities to local paths (pruning huge base64 payloads), derives work-log imagePath/imageActivityId, and shows deduplicated galleries on the terminal assistant message or as image-output rows once the turn settles.

Chat markdown renders img/video via MarkdownMedia, resolving workspace-relative paths and absolute browser-artifacts paths to signed URLs (including Windows drive path sanitization).

Reviewed by Cursor Bugbot for commit 58f9832. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Render saved screenshots and recordings in chat markdown with asset URL resolution

  • Adds save/savePath parameters to the preview_snapshot MCP tool, writing screenshots to either a workspace-relative path or a dedicated browser-artifacts directory; returns savedScreenshotPath in the response.
  • Extends the chat markdown renderer to display <img> and <video> elements via a new MarkdownMedia component that resolves workspace-relative and absolute paths to signed asset URLs.
  • Introduces browser-artifact and thread-image asset resource kinds end-to-end: new claim types in the JWT schema, issuance and re-resolution logic in AssetAccess, and a new assetsCreateUrl WS handler branch for thread-image.
  • Adds image_view support to the messages timeline: appendImageOutputRows groups deduplicated image thumbnails per turn into inline galleries attached to assistant messages or emitted as standalone rows.
  • Adds a Windows drive path rehype plugin (ChatMarkdown.tsx) that prefixes C:\... srcs with / before sanitization so they survive the sanitize pass.
  • Risk: browser-artifact and thread-image claims are re-resolved on every asset request (realpath + stat), adding filesystem I/O to each signed URL fetch.

Macroscope summarized 58f9832.

@coderabbitai

coderabbitaiBot commented Jul 22, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c82b6be7-c79a-4e7b-a19b-79b6c103f8ee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/render-chat-media

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 22, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effect service review: one convention issue found regarding error cause preservation. See inline comment.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
Comment threadapps/web/src/components/ChatMarkdown.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadpackages/contracts/src/previewAutomation.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
@macroscopeapp

macroscopeappBot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

3 blocking correctness issues found. This PR introduces a new feature for rendering screenshots/recordings in chat with significant new code paths. Two unresolved Critical security comments identify TOCTOU race conditions and hard link vulnerabilities in the file-writing logic that require human review before merging.

You can customize Macroscope's approvability policy. Learn more.

@github-actionsgithub-actionsBot added size:XL 500-999 changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Jul 23, 2026
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
@github-actionsgithub-actionsBot added size:XXL 1,000+ changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Jul 27, 2026
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/server/src/assets/AssetAccess.ts Outdated
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/session-logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 0a9ea4b to 3fc373aCompareJuly 29, 2026 15:44

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding on the new PreviewAutomationScreenshotSaveError shape; everything else (namespace imports, environment-based dependency acquisition for ServerConfig/ProjectionSnapshotQuery/WorkspacePaths/FileSystem/Path, Effect.catchTags usage, Schema.TaggedErrorClass errors in packages/contracts/src/assets.ts) matches the conventions.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
// write would follow it, and a dangling link (realPath reports NotFound)
// could silently create a file outside the workspace at its target.
const destination = path.join(canonicalParent, path.basename(resolved.absolutePath));
const destinationIsSymlink = yield* fileSystem.readLink(destination).pipe(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:214

saveSnapshotScreenshot checks whether the destination is a symlink with readLink (line 214), then writes to it in a separate writeScreenshotFile call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing writeFile to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the realPath parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with O_NOFOLLOW | O_CREAT | O_EXCL) rather than verifying the path is not a symlink beforehand.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 214:
`saveSnapshotScreenshot` checks whether the destination is a symlink with `readLink` (line 214), then writes to it in a separate `writeScreenshotFile` call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing `writeFile` to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the `realPath` parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with `O_NOFOLLOW | O_CREAT | O_EXCL`) rather than verifying the path is not a symlink beforehand.

return yield* fail("destination-symlink-validation");
}

yield* writeScreenshotFile({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:219

saveSnapshotScreenshot rejects symlinks at the destination but does not reject hard links. A workspace process can create savePath as a hard link to a file outside the workspace, and writeScreenshotFile then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The readLink check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via stat) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 219:
`saveSnapshotScreenshot` rejects symlinks at the destination but does not reject hard links. A workspace process can create `savePath` as a hard link to a file outside the workspace, and `writeScreenshotFile` then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The `readLink` check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via `stat`) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from faadbf0 to 3c0d0c9CompareJuly 29, 2026 15:56
Comment on lines +37 to +41
function mediaFileName(src: string): string {
const withoutQuery = src.split(/[?#]/, 1)[0] ?? src;
const basename = withoutQuery.slice(withoutQuery.lastIndexOf("/") + 1);
return basename.length > 0 ? safeDecode(basename) : safeDecode(withoutQuery);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Lowchat/MarkdownMedia.tsx:37

mediaFileName only treats / as a path separator, so Windows-style sources like /C:\repo\clip.mp4 (produced by the Windows-drive-path escaping) return the full path C:\repo\clip.mp4 instead of just clip.mp4. When there is no nonempty alt text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both / and \ so Windows backslash separators are handled.

Suggested change
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constbasename=withoutQuery.slice(withoutQuery.lastIndexOf("/")+1);
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constlastSep=Math.max(withoutQuery.lastIndexOf("/"),withoutQuery.lastIndexOf("\\"));
constbasename=lastSep>=0 ? withoutQuery.slice(lastSep+1) : withoutQuery;
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MarkdownMedia.tsx around lines 37-41:
`mediaFileName` only treats `/` as a path separator, so Windows-style sources like `/C:\repo\clip.mp4` (produced by the Windows-drive-path escaping) return the full path `C:\repo\clip.mp4` instead of just `clip.mp4`. When there is no nonempty `alt` text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both `/` and `\` so Windows backslash separators are handled.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 3c0d0c9 to b5bc982CompareJuly 29, 2026 20:58
juliusmarmingeand others added 2 commits July 29, 2026 16:58
- Add secure asset access for workspace media and browser artifacts
- Support saving preview screenshots and embedding playable recordings
- Preserve error causes in screenshot save failures
- Reject save+savePath combinations and describe both schema fields
- Verify canonical (symlink-resolved) containment before workspace writes
- Add collision-resistant suffix to artifact screenshot names
- Fix Windows drive paths, protocol-relative URLs, query strings, and
extensionless video sources in markdown media resolution
- Only treat absolute browser-artifacts paths as server artifacts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
juliusmarmingeand others added 7 commits July 29, 2026 16:58
Verify the deepest existing ancestor canonicalizes inside the workspace
before mkdir, and reject existing destination symlinks that resolve
outside the root.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Re-resolve browser-artifact and thread-image claims on every request so
post-issuance symlink swaps cannot redirect a signed token
- Verify canonical containment and media extension after realPath
- Render unscoped image outputs when no turn is unsettled
- Keep the image activity id paired with its path across collapsing
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Drop readonly/idempotent hints from preview_snapshot now that it writes
- Treat only real .. segments as escapes when resolving workspace assets
- Append image galleries when turns share a target row
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Payload pruning (#4622) stripped every item field outside the command
allowlist, so image view activities reached clients without the path and
the chat gallery rendered nothing. Preserve the item type and path while
still dropping the base64 image bytes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from b5bc982 to 5b2f955CompareJuly 29, 2026 21:00
);
}

function ImageOutputThumbnail({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumchat/MessagesTimeline.tsx:1966

Each thumbnail calls onImageExpand with images containing only its own loadedUrl, so the expanded viewer cannot navigate to other images in the same gallery. The images array should include all loaded thumbnails and index should point to the clicked one, so the preview can show the full gallery as expected.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 1966:
Each thumbnail calls `onImageExpand` with `images` containing only its own `loadedUrl`, so the expanded viewer cannot navigate to other images in the same gallery. The `images` array should include all loaded thumbnails and `index` should point to the clicked one, so the preview can show the full gallery as expected.

Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON, but a cloud agent failed to start.

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.


const projectedData: Record<string, unknown> = {};
const item = projectCommandData(data);
const item = projectCommandData(data) ?? projectImageViewData(data);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command projection drops image paths

Medium Severity

For image_view activities, projectActivityPayload sets data.item from projectCommandData(data) ?? projectImageViewData(data). When the tool item includes any pruned command field (command, input.command, or result.command), command projection wins and image-view projection is skipped, so type, path, and savedPath never reach thread snapshots. Clients then derive no imagePath, so inline galleries and thread-image asset requests for those activities fail even though the full payload in persistence still carries the image path.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.

@juliusmarminge

Copy link
Copy Markdown
MemberAuthor

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL1,000+ changed lines (additions + deletions).vouch:trustedPR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@juliusmarminge@Noojuno@gabrielelpidio
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Render saved screenshots and recordings in chat markdown - #4321

Closed
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media
Closed

Render saved screenshots and recordings in chat markdown#4321
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media

Conversation

@juliusmarminge

@juliusmarmingejuliusmarminge commented Jul 22, 2026

Copy link
Copy Markdown
Member

Summary

  • Add secure asset access for saved screenshots, videos, and browser artifacts.
  • Support saving preview screenshots to workspace paths or browser artifact storage.
  • Render saved media directly in chat markdown.
  • Extend preview recording and snapshot contracts and handlers.
  • Tighten pull request head matching and simplify related UI logic.

Testing

  • Added focused coverage for asset URL issuance and browser artifact validation.
  • Added MCP snapshot save-path and artifact persistence tests.
  • Passed focused asset, MCP, timeline, and session tests plus target package typechecks, targeted lint, and a web production build.
  • Verified signed workspace images inline and expanded in the real web client.

Note

High Risk
Changes signed asset authorization, filesystem containment, and MCP-driven writes into the workspace and browser-artifacts store—security-sensitive paths with extensive symlink handling that must stay correct on every request.

Overview
Chat and MCP preview flows can now show saved screenshots and recordings inline instead of leaving paths as plain text.

Signed asset serving adds browser-artifact and thread-image resources, treats workspace videos like images (exact-file tokens), and tightens containment (real .. segments only, not names like ..screenshots). Browser artifacts and thread images are re-canonicalized on every fetch so post-issuance symlink swaps do not keep serving the old target.

preview_snapshot can persist PNGs with save:true into browser-artifacts or with savePath into the thread workspace; workspace writes validate extensions, traversal, and directory/file symlinks before writing. Tool metadata reflects that snapshot is no longer read-only/idempotent when saving.

Thread UI maps Codex image view/generation activities to local paths (pruning huge base64 payloads), derives work-log imagePath/imageActivityId, and shows deduplicated galleries on the terminal assistant message or as image-output rows once the turn settles.

Chat markdown renders img/video via MarkdownMedia, resolving workspace-relative paths and absolute browser-artifacts paths to signed URLs (including Windows drive path sanitization).

Reviewed by Cursor Bugbot for commit 58f9832. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Render saved screenshots and recordings in chat markdown with asset URL resolution

  • Adds save/savePath parameters to the preview_snapshot MCP tool, writing screenshots to either a workspace-relative path or a dedicated browser-artifacts directory; returns savedScreenshotPath in the response.
  • Extends the chat markdown renderer to display <img> and <video> elements via a new MarkdownMedia component that resolves workspace-relative and absolute paths to signed asset URLs.
  • Introduces browser-artifact and thread-image asset resource kinds end-to-end: new claim types in the JWT schema, issuance and re-resolution logic in AssetAccess, and a new assetsCreateUrl WS handler branch for thread-image.
  • Adds image_view support to the messages timeline: appendImageOutputRows groups deduplicated image thumbnails per turn into inline galleries attached to assistant messages or emitted as standalone rows.
  • Adds a Windows drive path rehype plugin (ChatMarkdown.tsx) that prefixes C:\... srcs with / before sanitization so they survive the sanitize pass.
  • Risk: browser-artifact and thread-image claims are re-resolved on every asset request (realpath + stat), adding filesystem I/O to each signed URL fetch.

Macroscope summarized 58f9832.

@coderabbitai

coderabbitaiBot commented Jul 22, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c82b6be7-c79a-4e7b-a19b-79b6c103f8ee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/render-chat-media

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 22, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effect service review: one convention issue found regarding error cause preservation. See inline comment.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
Comment threadapps/web/src/components/ChatMarkdown.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadpackages/contracts/src/previewAutomation.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
@macroscopeapp

macroscopeappBot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

3 blocking correctness issues found. This PR introduces a new feature for rendering screenshots/recordings in chat with significant new code paths. Two unresolved Critical security comments identify TOCTOU race conditions and hard link vulnerabilities in the file-writing logic that require human review before merging.

You can customize Macroscope's approvability policy. Learn more.

@github-actionsgithub-actionsBot added size:XL 500-999 changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Jul 23, 2026
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
@github-actionsgithub-actionsBot added size:XXL 1,000+ changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Jul 27, 2026
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/server/src/assets/AssetAccess.ts Outdated
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/session-logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 0a9ea4b to 3fc373aCompareJuly 29, 2026 15:44

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding on the new PreviewAutomationScreenshotSaveError shape; everything else (namespace imports, environment-based dependency acquisition for ServerConfig/ProjectionSnapshotQuery/WorkspacePaths/FileSystem/Path, Effect.catchTags usage, Schema.TaggedErrorClass errors in packages/contracts/src/assets.ts) matches the conventions.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
// write would follow it, and a dangling link (realPath reports NotFound)
// could silently create a file outside the workspace at its target.
const destination = path.join(canonicalParent, path.basename(resolved.absolutePath));
const destinationIsSymlink = yield* fileSystem.readLink(destination).pipe(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:214

saveSnapshotScreenshot checks whether the destination is a symlink with readLink (line 214), then writes to it in a separate writeScreenshotFile call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing writeFile to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the realPath parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with O_NOFOLLOW | O_CREAT | O_EXCL) rather than verifying the path is not a symlink beforehand.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 214:
`saveSnapshotScreenshot` checks whether the destination is a symlink with `readLink` (line 214), then writes to it in a separate `writeScreenshotFile` call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing `writeFile` to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the `realPath` parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with `O_NOFOLLOW | O_CREAT | O_EXCL`) rather than verifying the path is not a symlink beforehand.

return yield* fail("destination-symlink-validation");
}

yield* writeScreenshotFile({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:219

saveSnapshotScreenshot rejects symlinks at the destination but does not reject hard links. A workspace process can create savePath as a hard link to a file outside the workspace, and writeScreenshotFile then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The readLink check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via stat) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 219:
`saveSnapshotScreenshot` rejects symlinks at the destination but does not reject hard links. A workspace process can create `savePath` as a hard link to a file outside the workspace, and `writeScreenshotFile` then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The `readLink` check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via `stat`) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from faadbf0 to 3c0d0c9CompareJuly 29, 2026 15:56
Comment on lines +37 to +41
function mediaFileName(src: string): string {
const withoutQuery = src.split(/[?#]/, 1)[0] ?? src;
const basename = withoutQuery.slice(withoutQuery.lastIndexOf("/") + 1);
return basename.length > 0 ? safeDecode(basename) : safeDecode(withoutQuery);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Lowchat/MarkdownMedia.tsx:37

mediaFileName only treats / as a path separator, so Windows-style sources like /C:\repo\clip.mp4 (produced by the Windows-drive-path escaping) return the full path C:\repo\clip.mp4 instead of just clip.mp4. When there is no nonempty alt text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both / and \ so Windows backslash separators are handled.

Suggested change
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constbasename=withoutQuery.slice(withoutQuery.lastIndexOf("/")+1);
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constlastSep=Math.max(withoutQuery.lastIndexOf("/"),withoutQuery.lastIndexOf("\\"));
constbasename=lastSep>=0 ? withoutQuery.slice(lastSep+1) : withoutQuery;
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MarkdownMedia.tsx around lines 37-41:
`mediaFileName` only treats `/` as a path separator, so Windows-style sources like `/C:\repo\clip.mp4` (produced by the Windows-drive-path escaping) return the full path `C:\repo\clip.mp4` instead of just `clip.mp4`. When there is no nonempty `alt` text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both `/` and `\` so Windows backslash separators are handled.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 3c0d0c9 to b5bc982CompareJuly 29, 2026 20:58
juliusmarmingeand others added 2 commits July 29, 2026 16:58
- Add secure asset access for workspace media and browser artifacts
- Support saving preview screenshots and embedding playable recordings
- Preserve error causes in screenshot save failures
- Reject save+savePath combinations and describe both schema fields
- Verify canonical (symlink-resolved) containment before workspace writes
- Add collision-resistant suffix to artifact screenshot names
- Fix Windows drive paths, protocol-relative URLs, query strings, and
extensionless video sources in markdown media resolution
- Only treat absolute browser-artifacts paths as server artifacts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
juliusmarmingeand others added 7 commits July 29, 2026 16:58
Verify the deepest existing ancestor canonicalizes inside the workspace
before mkdir, and reject existing destination symlinks that resolve
outside the root.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Re-resolve browser-artifact and thread-image claims on every request so
post-issuance symlink swaps cannot redirect a signed token
- Verify canonical containment and media extension after realPath
- Render unscoped image outputs when no turn is unsettled
- Keep the image activity id paired with its path across collapsing
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Drop readonly/idempotent hints from preview_snapshot now that it writes
- Treat only real .. segments as escapes when resolving workspace assets
- Append image galleries when turns share a target row
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Payload pruning (#4622) stripped every item field outside the command
allowlist, so image view activities reached clients without the path and
the chat gallery rendered nothing. Preserve the item type and path while
still dropping the base64 image bytes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from b5bc982 to 5b2f955CompareJuly 29, 2026 21:00
);
}

function ImageOutputThumbnail({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumchat/MessagesTimeline.tsx:1966

Each thumbnail calls onImageExpand with images containing only its own loadedUrl, so the expanded viewer cannot navigate to other images in the same gallery. The images array should include all loaded thumbnails and index should point to the clicked one, so the preview can show the full gallery as expected.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 1966:
Each thumbnail calls `onImageExpand` with `images` containing only its own `loadedUrl`, so the expanded viewer cannot navigate to other images in the same gallery. The `images` array should include all loaded thumbnails and `index` should point to the clicked one, so the preview can show the full gallery as expected.

Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON, but a cloud agent failed to start.

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.


const projectedData: Record<string, unknown> = {};
const item = projectCommandData(data);
const item = projectCommandData(data) ?? projectImageViewData(data);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command projection drops image paths

Medium Severity

For image_view activities, projectActivityPayload sets data.item from projectCommandData(data) ?? projectImageViewData(data). When the tool item includes any pruned command field (command, input.command, or result.command), command projection wins and image-view projection is skipped, so type, path, and savedPath never reach thread snapshots. Clients then derive no imagePath, so inline galleries and thread-image asset requests for those activities fail even though the full payload in persistence still carries the image path.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.

@juliusmarminge

Copy link
Copy Markdown
MemberAuthor

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL1,000+ changed lines (additions + deletions).vouch:trustedPR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@juliusmarminge@Noojuno@gabrielelpidio
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Render saved screenshots and recordings in chat markdown - #4321

Closed
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media
Closed

Render saved screenshots and recordings in chat markdown#4321
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media

Conversation

@juliusmarminge

@juliusmarmingejuliusmarminge commented Jul 22, 2026

Copy link
Copy Markdown
Member

Summary

  • Add secure asset access for saved screenshots, videos, and browser artifacts.
  • Support saving preview screenshots to workspace paths or browser artifact storage.
  • Render saved media directly in chat markdown.
  • Extend preview recording and snapshot contracts and handlers.
  • Tighten pull request head matching and simplify related UI logic.

Testing

  • Added focused coverage for asset URL issuance and browser artifact validation.
  • Added MCP snapshot save-path and artifact persistence tests.
  • Passed focused asset, MCP, timeline, and session tests plus target package typechecks, targeted lint, and a web production build.
  • Verified signed workspace images inline and expanded in the real web client.

Note

High Risk
Changes signed asset authorization, filesystem containment, and MCP-driven writes into the workspace and browser-artifacts store—security-sensitive paths with extensive symlink handling that must stay correct on every request.

Overview
Chat and MCP preview flows can now show saved screenshots and recordings inline instead of leaving paths as plain text.

Signed asset serving adds browser-artifact and thread-image resources, treats workspace videos like images (exact-file tokens), and tightens containment (real .. segments only, not names like ..screenshots). Browser artifacts and thread images are re-canonicalized on every fetch so post-issuance symlink swaps do not keep serving the old target.

preview_snapshot can persist PNGs with save:true into browser-artifacts or with savePath into the thread workspace; workspace writes validate extensions, traversal, and directory/file symlinks before writing. Tool metadata reflects that snapshot is no longer read-only/idempotent when saving.

Thread UI maps Codex image view/generation activities to local paths (pruning huge base64 payloads), derives work-log imagePath/imageActivityId, and shows deduplicated galleries on the terminal assistant message or as image-output rows once the turn settles.

Chat markdown renders img/video via MarkdownMedia, resolving workspace-relative paths and absolute browser-artifacts paths to signed URLs (including Windows drive path sanitization).

Reviewed by Cursor Bugbot for commit 58f9832. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Render saved screenshots and recordings in chat markdown with asset URL resolution

  • Adds save/savePath parameters to the preview_snapshot MCP tool, writing screenshots to either a workspace-relative path or a dedicated browser-artifacts directory; returns savedScreenshotPath in the response.
  • Extends the chat markdown renderer to display <img> and <video> elements via a new MarkdownMedia component that resolves workspace-relative and absolute paths to signed asset URLs.
  • Introduces browser-artifact and thread-image asset resource kinds end-to-end: new claim types in the JWT schema, issuance and re-resolution logic in AssetAccess, and a new assetsCreateUrl WS handler branch for thread-image.
  • Adds image_view support to the messages timeline: appendImageOutputRows groups deduplicated image thumbnails per turn into inline galleries attached to assistant messages or emitted as standalone rows.
  • Adds a Windows drive path rehype plugin (ChatMarkdown.tsx) that prefixes C:\... srcs with / before sanitization so they survive the sanitize pass.
  • Risk: browser-artifact and thread-image claims are re-resolved on every asset request (realpath + stat), adding filesystem I/O to each signed URL fetch.

Macroscope summarized 58f9832.

@coderabbitai

coderabbitaiBot commented Jul 22, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c82b6be7-c79a-4e7b-a19b-79b6c103f8ee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/render-chat-media

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 22, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effect service review: one convention issue found regarding error cause preservation. See inline comment.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
Comment threadapps/web/src/components/ChatMarkdown.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadpackages/contracts/src/previewAutomation.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
@macroscopeapp

macroscopeappBot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

3 blocking correctness issues found. This PR introduces a new feature for rendering screenshots/recordings in chat with significant new code paths. Two unresolved Critical security comments identify TOCTOU race conditions and hard link vulnerabilities in the file-writing logic that require human review before merging.

You can customize Macroscope's approvability policy. Learn more.

@github-actionsgithub-actionsBot added size:XL 500-999 changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Jul 23, 2026
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
@github-actionsgithub-actionsBot added size:XXL 1,000+ changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Jul 27, 2026
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/server/src/assets/AssetAccess.ts Outdated
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/session-logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 0a9ea4b to 3fc373aCompareJuly 29, 2026 15:44

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding on the new PreviewAutomationScreenshotSaveError shape; everything else (namespace imports, environment-based dependency acquisition for ServerConfig/ProjectionSnapshotQuery/WorkspacePaths/FileSystem/Path, Effect.catchTags usage, Schema.TaggedErrorClass errors in packages/contracts/src/assets.ts) matches the conventions.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
// write would follow it, and a dangling link (realPath reports NotFound)
// could silently create a file outside the workspace at its target.
const destination = path.join(canonicalParent, path.basename(resolved.absolutePath));
const destinationIsSymlink = yield* fileSystem.readLink(destination).pipe(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:214

saveSnapshotScreenshot checks whether the destination is a symlink with readLink (line 214), then writes to it in a separate writeScreenshotFile call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing writeFile to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the realPath parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with O_NOFOLLOW | O_CREAT | O_EXCL) rather than verifying the path is not a symlink beforehand.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 214:
`saveSnapshotScreenshot` checks whether the destination is a symlink with `readLink` (line 214), then writes to it in a separate `writeScreenshotFile` call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing `writeFile` to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the `realPath` parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with `O_NOFOLLOW | O_CREAT | O_EXCL`) rather than verifying the path is not a symlink beforehand.

return yield* fail("destination-symlink-validation");
}

yield* writeScreenshotFile({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:219

saveSnapshotScreenshot rejects symlinks at the destination but does not reject hard links. A workspace process can create savePath as a hard link to a file outside the workspace, and writeScreenshotFile then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The readLink check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via stat) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 219:
`saveSnapshotScreenshot` rejects symlinks at the destination but does not reject hard links. A workspace process can create `savePath` as a hard link to a file outside the workspace, and `writeScreenshotFile` then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The `readLink` check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via `stat`) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from faadbf0 to 3c0d0c9CompareJuly 29, 2026 15:56
Comment on lines +37 to +41
function mediaFileName(src: string): string {
const withoutQuery = src.split(/[?#]/, 1)[0] ?? src;
const basename = withoutQuery.slice(withoutQuery.lastIndexOf("/") + 1);
return basename.length > 0 ? safeDecode(basename) : safeDecode(withoutQuery);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Lowchat/MarkdownMedia.tsx:37

mediaFileName only treats / as a path separator, so Windows-style sources like /C:\repo\clip.mp4 (produced by the Windows-drive-path escaping) return the full path C:\repo\clip.mp4 instead of just clip.mp4. When there is no nonempty alt text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both / and \ so Windows backslash separators are handled.

Suggested change
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constbasename=withoutQuery.slice(withoutQuery.lastIndexOf("/")+1);
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constlastSep=Math.max(withoutQuery.lastIndexOf("/"),withoutQuery.lastIndexOf("\\"));
constbasename=lastSep>=0 ? withoutQuery.slice(lastSep+1) : withoutQuery;
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MarkdownMedia.tsx around lines 37-41:
`mediaFileName` only treats `/` as a path separator, so Windows-style sources like `/C:\repo\clip.mp4` (produced by the Windows-drive-path escaping) return the full path `C:\repo\clip.mp4` instead of just `clip.mp4`. When there is no nonempty `alt` text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both `/` and `\` so Windows backslash separators are handled.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 3c0d0c9 to b5bc982CompareJuly 29, 2026 20:58
juliusmarmingeand others added 2 commits July 29, 2026 16:58
- Add secure asset access for workspace media and browser artifacts
- Support saving preview screenshots and embedding playable recordings
- Preserve error causes in screenshot save failures
- Reject save+savePath combinations and describe both schema fields
- Verify canonical (symlink-resolved) containment before workspace writes
- Add collision-resistant suffix to artifact screenshot names
- Fix Windows drive paths, protocol-relative URLs, query strings, and
extensionless video sources in markdown media resolution
- Only treat absolute browser-artifacts paths as server artifacts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
juliusmarmingeand others added 7 commits July 29, 2026 16:58
Verify the deepest existing ancestor canonicalizes inside the workspace
before mkdir, and reject existing destination symlinks that resolve
outside the root.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Re-resolve browser-artifact and thread-image claims on every request so
post-issuance symlink swaps cannot redirect a signed token
- Verify canonical containment and media extension after realPath
- Render unscoped image outputs when no turn is unsettled
- Keep the image activity id paired with its path across collapsing
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Drop readonly/idempotent hints from preview_snapshot now that it writes
- Treat only real .. segments as escapes when resolving workspace assets
- Append image galleries when turns share a target row
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Payload pruning (#4622) stripped every item field outside the command
allowlist, so image view activities reached clients without the path and
the chat gallery rendered nothing. Preserve the item type and path while
still dropping the base64 image bytes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from b5bc982 to 5b2f955CompareJuly 29, 2026 21:00
);
}

function ImageOutputThumbnail({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumchat/MessagesTimeline.tsx:1966

Each thumbnail calls onImageExpand with images containing only its own loadedUrl, so the expanded viewer cannot navigate to other images in the same gallery. The images array should include all loaded thumbnails and index should point to the clicked one, so the preview can show the full gallery as expected.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 1966:
Each thumbnail calls `onImageExpand` with `images` containing only its own `loadedUrl`, so the expanded viewer cannot navigate to other images in the same gallery. The `images` array should include all loaded thumbnails and `index` should point to the clicked one, so the preview can show the full gallery as expected.

Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON, but a cloud agent failed to start.

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.


const projectedData: Record<string, unknown> = {};
const item = projectCommandData(data);
const item = projectCommandData(data) ?? projectImageViewData(data);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command projection drops image paths

Medium Severity

For image_view activities, projectActivityPayload sets data.item from projectCommandData(data) ?? projectImageViewData(data). When the tool item includes any pruned command field (command, input.command, or result.command), command projection wins and image-view projection is skipped, so type, path, and savedPath never reach thread snapshots. Clients then derive no imagePath, so inline galleries and thread-image asset requests for those activities fail even though the full payload in persistence still carries the image path.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.

@juliusmarminge

Copy link
Copy Markdown
MemberAuthor

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL1,000+ changed lines (additions + deletions).vouch:trustedPR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@juliusmarminge@Noojuno@gabrielelpidio
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Render saved screenshots and recordings in chat markdown - #4321

Closed
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media
Closed

Render saved screenshots and recordings in chat markdown#4321
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media

Conversation

@juliusmarminge

@juliusmarmingejuliusmarminge commented Jul 22, 2026

Copy link
Copy Markdown
Member

Summary

  • Add secure asset access for saved screenshots, videos, and browser artifacts.
  • Support saving preview screenshots to workspace paths or browser artifact storage.
  • Render saved media directly in chat markdown.
  • Extend preview recording and snapshot contracts and handlers.
  • Tighten pull request head matching and simplify related UI logic.

Testing

  • Added focused coverage for asset URL issuance and browser artifact validation.
  • Added MCP snapshot save-path and artifact persistence tests.
  • Passed focused asset, MCP, timeline, and session tests plus target package typechecks, targeted lint, and a web production build.
  • Verified signed workspace images inline and expanded in the real web client.

Note

High Risk
Changes signed asset authorization, filesystem containment, and MCP-driven writes into the workspace and browser-artifacts store—security-sensitive paths with extensive symlink handling that must stay correct on every request.

Overview
Chat and MCP preview flows can now show saved screenshots and recordings inline instead of leaving paths as plain text.

Signed asset serving adds browser-artifact and thread-image resources, treats workspace videos like images (exact-file tokens), and tightens containment (real .. segments only, not names like ..screenshots). Browser artifacts and thread images are re-canonicalized on every fetch so post-issuance symlink swaps do not keep serving the old target.

preview_snapshot can persist PNGs with save:true into browser-artifacts or with savePath into the thread workspace; workspace writes validate extensions, traversal, and directory/file symlinks before writing. Tool metadata reflects that snapshot is no longer read-only/idempotent when saving.

Thread UI maps Codex image view/generation activities to local paths (pruning huge base64 payloads), derives work-log imagePath/imageActivityId, and shows deduplicated galleries on the terminal assistant message or as image-output rows once the turn settles.

Chat markdown renders img/video via MarkdownMedia, resolving workspace-relative paths and absolute browser-artifacts paths to signed URLs (including Windows drive path sanitization).

Reviewed by Cursor Bugbot for commit 58f9832. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Render saved screenshots and recordings in chat markdown with asset URL resolution

  • Adds save/savePath parameters to the preview_snapshot MCP tool, writing screenshots to either a workspace-relative path or a dedicated browser-artifacts directory; returns savedScreenshotPath in the response.
  • Extends the chat markdown renderer to display <img> and <video> elements via a new MarkdownMedia component that resolves workspace-relative and absolute paths to signed asset URLs.
  • Introduces browser-artifact and thread-image asset resource kinds end-to-end: new claim types in the JWT schema, issuance and re-resolution logic in AssetAccess, and a new assetsCreateUrl WS handler branch for thread-image.
  • Adds image_view support to the messages timeline: appendImageOutputRows groups deduplicated image thumbnails per turn into inline galleries attached to assistant messages or emitted as standalone rows.
  • Adds a Windows drive path rehype plugin (ChatMarkdown.tsx) that prefixes C:\... srcs with / before sanitization so they survive the sanitize pass.
  • Risk: browser-artifact and thread-image claims are re-resolved on every asset request (realpath + stat), adding filesystem I/O to each signed URL fetch.

Macroscope summarized 58f9832.

@coderabbitai

coderabbitaiBot commented Jul 22, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c82b6be7-c79a-4e7b-a19b-79b6c103f8ee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/render-chat-media

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 22, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effect service review: one convention issue found regarding error cause preservation. See inline comment.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
Comment threadapps/web/src/components/ChatMarkdown.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadpackages/contracts/src/previewAutomation.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
@macroscopeapp

macroscopeappBot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

3 blocking correctness issues found. This PR introduces a new feature for rendering screenshots/recordings in chat with significant new code paths. Two unresolved Critical security comments identify TOCTOU race conditions and hard link vulnerabilities in the file-writing logic that require human review before merging.

You can customize Macroscope's approvability policy. Learn more.

@github-actionsgithub-actionsBot added size:XL 500-999 changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Jul 23, 2026
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
@github-actionsgithub-actionsBot added size:XXL 1,000+ changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Jul 27, 2026
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/server/src/assets/AssetAccess.ts Outdated
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/session-logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 0a9ea4b to 3fc373aCompareJuly 29, 2026 15:44

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding on the new PreviewAutomationScreenshotSaveError shape; everything else (namespace imports, environment-based dependency acquisition for ServerConfig/ProjectionSnapshotQuery/WorkspacePaths/FileSystem/Path, Effect.catchTags usage, Schema.TaggedErrorClass errors in packages/contracts/src/assets.ts) matches the conventions.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
// write would follow it, and a dangling link (realPath reports NotFound)
// could silently create a file outside the workspace at its target.
const destination = path.join(canonicalParent, path.basename(resolved.absolutePath));
const destinationIsSymlink = yield* fileSystem.readLink(destination).pipe(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:214

saveSnapshotScreenshot checks whether the destination is a symlink with readLink (line 214), then writes to it in a separate writeScreenshotFile call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing writeFile to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the realPath parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with O_NOFOLLOW | O_CREAT | O_EXCL) rather than verifying the path is not a symlink beforehand.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 214:
`saveSnapshotScreenshot` checks whether the destination is a symlink with `readLink` (line 214), then writes to it in a separate `writeScreenshotFile` call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing `writeFile` to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the `realPath` parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with `O_NOFOLLOW | O_CREAT | O_EXCL`) rather than verifying the path is not a symlink beforehand.

return yield* fail("destination-symlink-validation");
}

yield* writeScreenshotFile({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:219

saveSnapshotScreenshot rejects symlinks at the destination but does not reject hard links. A workspace process can create savePath as a hard link to a file outside the workspace, and writeScreenshotFile then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The readLink check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via stat) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 219:
`saveSnapshotScreenshot` rejects symlinks at the destination but does not reject hard links. A workspace process can create `savePath` as a hard link to a file outside the workspace, and `writeScreenshotFile` then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The `readLink` check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via `stat`) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from faadbf0 to 3c0d0c9CompareJuly 29, 2026 15:56
Comment on lines +37 to +41
function mediaFileName(src: string): string {
const withoutQuery = src.split(/[?#]/, 1)[0] ?? src;
const basename = withoutQuery.slice(withoutQuery.lastIndexOf("/") + 1);
return basename.length > 0 ? safeDecode(basename) : safeDecode(withoutQuery);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Lowchat/MarkdownMedia.tsx:37

mediaFileName only treats / as a path separator, so Windows-style sources like /C:\repo\clip.mp4 (produced by the Windows-drive-path escaping) return the full path C:\repo\clip.mp4 instead of just clip.mp4. When there is no nonempty alt text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both / and \ so Windows backslash separators are handled.

Suggested change
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constbasename=withoutQuery.slice(withoutQuery.lastIndexOf("/")+1);
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constlastSep=Math.max(withoutQuery.lastIndexOf("/"),withoutQuery.lastIndexOf("\\"));
constbasename=lastSep>=0 ? withoutQuery.slice(lastSep+1) : withoutQuery;
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MarkdownMedia.tsx around lines 37-41:
`mediaFileName` only treats `/` as a path separator, so Windows-style sources like `/C:\repo\clip.mp4` (produced by the Windows-drive-path escaping) return the full path `C:\repo\clip.mp4` instead of just `clip.mp4`. When there is no nonempty `alt` text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both `/` and `\` so Windows backslash separators are handled.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 3c0d0c9 to b5bc982CompareJuly 29, 2026 20:58
juliusmarmingeand others added 2 commits July 29, 2026 16:58
- Add secure asset access for workspace media and browser artifacts
- Support saving preview screenshots and embedding playable recordings
- Preserve error causes in screenshot save failures
- Reject save+savePath combinations and describe both schema fields
- Verify canonical (symlink-resolved) containment before workspace writes
- Add collision-resistant suffix to artifact screenshot names
- Fix Windows drive paths, protocol-relative URLs, query strings, and
extensionless video sources in markdown media resolution
- Only treat absolute browser-artifacts paths as server artifacts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
juliusmarmingeand others added 7 commits July 29, 2026 16:58
Verify the deepest existing ancestor canonicalizes inside the workspace
before mkdir, and reject existing destination symlinks that resolve
outside the root.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Re-resolve browser-artifact and thread-image claims on every request so
post-issuance symlink swaps cannot redirect a signed token
- Verify canonical containment and media extension after realPath
- Render unscoped image outputs when no turn is unsettled
- Keep the image activity id paired with its path across collapsing
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Drop readonly/idempotent hints from preview_snapshot now that it writes
- Treat only real .. segments as escapes when resolving workspace assets
- Append image galleries when turns share a target row
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Payload pruning (#4622) stripped every item field outside the command
allowlist, so image view activities reached clients without the path and
the chat gallery rendered nothing. Preserve the item type and path while
still dropping the base64 image bytes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from b5bc982 to 5b2f955CompareJuly 29, 2026 21:00
);
}

function ImageOutputThumbnail({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumchat/MessagesTimeline.tsx:1966

Each thumbnail calls onImageExpand with images containing only its own loadedUrl, so the expanded viewer cannot navigate to other images in the same gallery. The images array should include all loaded thumbnails and index should point to the clicked one, so the preview can show the full gallery as expected.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 1966:
Each thumbnail calls `onImageExpand` with `images` containing only its own `loadedUrl`, so the expanded viewer cannot navigate to other images in the same gallery. The `images` array should include all loaded thumbnails and `index` should point to the clicked one, so the preview can show the full gallery as expected.

Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON, but a cloud agent failed to start.

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.


const projectedData: Record<string, unknown> = {};
const item = projectCommandData(data);
const item = projectCommandData(data) ?? projectImageViewData(data);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command projection drops image paths

Medium Severity

For image_view activities, projectActivityPayload sets data.item from projectCommandData(data) ?? projectImageViewData(data). When the tool item includes any pruned command field (command, input.command, or result.command), command projection wins and image-view projection is skipped, so type, path, and savedPath never reach thread snapshots. Clients then derive no imagePath, so inline galleries and thread-image asset requests for those activities fail even though the full payload in persistence still carries the image path.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.

@juliusmarminge

Copy link
Copy Markdown
MemberAuthor

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL1,000+ changed lines (additions + deletions).vouch:trustedPR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@juliusmarminge@Noojuno@gabrielelpidio
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Render saved screenshots and recordings in chat markdown - #4321

Closed
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media
Closed

Render saved screenshots and recordings in chat markdown#4321
juliusmarminge wants to merge 9 commits into
mainfrom
t3code/render-chat-media

Conversation

@juliusmarminge

@juliusmarmingejuliusmarminge commented Jul 22, 2026

Copy link
Copy Markdown
Member

Summary

  • Add secure asset access for saved screenshots, videos, and browser artifacts.
  • Support saving preview screenshots to workspace paths or browser artifact storage.
  • Render saved media directly in chat markdown.
  • Extend preview recording and snapshot contracts and handlers.
  • Tighten pull request head matching and simplify related UI logic.

Testing

  • Added focused coverage for asset URL issuance and browser artifact validation.
  • Added MCP snapshot save-path and artifact persistence tests.
  • Passed focused asset, MCP, timeline, and session tests plus target package typechecks, targeted lint, and a web production build.
  • Verified signed workspace images inline and expanded in the real web client.

Note

High Risk
Changes signed asset authorization, filesystem containment, and MCP-driven writes into the workspace and browser-artifacts store—security-sensitive paths with extensive symlink handling that must stay correct on every request.

Overview
Chat and MCP preview flows can now show saved screenshots and recordings inline instead of leaving paths as plain text.

Signed asset serving adds browser-artifact and thread-image resources, treats workspace videos like images (exact-file tokens), and tightens containment (real .. segments only, not names like ..screenshots). Browser artifacts and thread images are re-canonicalized on every fetch so post-issuance symlink swaps do not keep serving the old target.

preview_snapshot can persist PNGs with save:true into browser-artifacts or with savePath into the thread workspace; workspace writes validate extensions, traversal, and directory/file symlinks before writing. Tool metadata reflects that snapshot is no longer read-only/idempotent when saving.

Thread UI maps Codex image view/generation activities to local paths (pruning huge base64 payloads), derives work-log imagePath/imageActivityId, and shows deduplicated galleries on the terminal assistant message or as image-output rows once the turn settles.

Chat markdown renders img/video via MarkdownMedia, resolving workspace-relative paths and absolute browser-artifacts paths to signed URLs (including Windows drive path sanitization).

Reviewed by Cursor Bugbot for commit 58f9832. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

Render saved screenshots and recordings in chat markdown with asset URL resolution

  • Adds save/savePath parameters to the preview_snapshot MCP tool, writing screenshots to either a workspace-relative path or a dedicated browser-artifacts directory; returns savedScreenshotPath in the response.
  • Extends the chat markdown renderer to display <img> and <video> elements via a new MarkdownMedia component that resolves workspace-relative and absolute paths to signed asset URLs.
  • Introduces browser-artifact and thread-image asset resource kinds end-to-end: new claim types in the JWT schema, issuance and re-resolution logic in AssetAccess, and a new assetsCreateUrl WS handler branch for thread-image.
  • Adds image_view support to the messages timeline: appendImageOutputRows groups deduplicated image thumbnails per turn into inline galleries attached to assistant messages or emitted as standalone rows.
  • Adds a Windows drive path rehype plugin (ChatMarkdown.tsx) that prefixes C:\... srcs with / before sanitization so they survive the sanitize pass.
  • Risk: browser-artifact and thread-image claims are re-resolved on every asset request (realpath + stat), adding filesystem I/O to each signed URL fetch.

Macroscope summarized 58f9832.

@coderabbitai

coderabbitaiBot commented Jul 22, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: c82b6be7-c79a-4e7b-a19b-79b6c103f8ee

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch t3code/render-chat-media

Comment @coderabbitai help to get the list of available commands.

@github-actionsgithub-actionsBot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:L 100-499 changed lines (additions + deletions). labels Jul 22, 2026

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Effect service review: one convention issue found regarding error cause preservation. See inline comment.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
Comment threadapps/web/src/components/ChatMarkdown.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadpackages/contracts/src/previewAutomation.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/web/src/components/chat/MarkdownMedia.tsx Outdated
@macroscopeapp

macroscopeappBot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs human review

3 blocking correctness issues found. This PR introduces a new feature for rendering screenshots/recordings in chat with significant new code paths. Two unresolved Critical security comments identify TOCTOU race conditions and hard link vulnerabilities in the file-writing logic that require human review before merging.

You can customize Macroscope's approvability policy. Learn more.

@github-actionsgithub-actionsBot added size:XL 500-999 changed lines (additions + deletions). and removed size:L 100-499 changed lines (additions + deletions). labels Jul 23, 2026
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts
@github-actionsgithub-actionsBot added size:XXL 1,000+ changed lines (additions + deletions). and removed size:XL 500-999 changed lines (additions + deletions). labels Jul 27, 2026
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/server/src/assets/AssetAccess.ts Outdated
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/session-logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
Comment threadapps/server/src/assets/AssetAccess.ts
Comment threadapps/web/src/components/chat/MessagesTimeline.logic.ts Outdated
Comment threadapps/server/src/mcp/toolkits/preview/tools.ts
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 0a9ea4b to 3fc373aCompareJuly 29, 2026 15:44

@macroscopeappmacroscopeappBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One finding on the new PreviewAutomationScreenshotSaveError shape; everything else (namespace imports, environment-based dependency acquisition for ServerConfig/ProjectionSnapshotQuery/WorkspacePaths/FileSystem/Path, Effect.catchTags usage, Schema.TaggedErrorClass errors in packages/contracts/src/assets.ts) matches the conventions.

Posted via Macroscope — Effect Service Conventions

Comment threadpackages/contracts/src/previewAutomation.ts Outdated
// write would follow it, and a dangling link (realPath reports NotFound)
// could silently create a file outside the workspace at its target.
const destination = path.join(canonicalParent, path.basename(resolved.absolutePath));
const destinationIsSymlink = yield* fileSystem.readLink(destination).pipe(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:214

saveSnapshotScreenshot checks whether the destination is a symlink with readLink (line 214), then writes to it in a separate writeScreenshotFile call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing writeFile to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the realPath parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with O_NOFOLLOW | O_CREAT | O_EXCL) rather than verifying the path is not a symlink beforehand.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 214:
`saveSnapshotScreenshot` checks whether the destination is a symlink with `readLink` (line 214), then writes to it in a separate `writeScreenshotFile` call (line 222). A concurrent process can replace the destination with a symlink after the check and before the write, causing `writeFile` to follow that symlink and overwrite an arbitrary file outside the workspace. The same check/use race exists for the `realPath` parent-directory checks. The preflight symlink checks do not close this TOCTOU gap — the final write needs no-follow/exclusive file-opening semantics (e.g. opening with `O_NOFOLLOW | O_CREAT | O_EXCL`) rather than verifying the path is not a symlink beforehand.

return yield* fail("destination-symlink-validation");
}

yield* writeScreenshotFile({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Criticalpreview/handlers.ts:219

saveSnapshotScreenshot rejects symlinks at the destination but does not reject hard links. A workspace process can create savePath as a hard link to a file outside the workspace, and writeScreenshotFile then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The readLink check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via stat) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/server/src/mcp/toolkits/preview/handlers.ts around line 219:
`saveSnapshotScreenshot` rejects symlinks at the destination but does not reject hard links. A workspace process can create `savePath` as a hard link to a file outside the workspace, and `writeScreenshotFile` then truncates and overwrites the shared inode, corrupting the external file despite the containment checks. The `readLink` check only catches symbolic links, not hard links. Consider checking the destination's link count (e.g. via `stat`) and refusing to write if it is greater than 1, or document that hard-link attacks are out of scope.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from faadbf0 to 3c0d0c9CompareJuly 29, 2026 15:56
Comment on lines +37 to +41
function mediaFileName(src: string): string {
const withoutQuery = src.split(/[?#]/, 1)[0] ?? src;
const basename = withoutQuery.slice(withoutQuery.lastIndexOf("/") + 1);
return basename.length > 0 ? safeDecode(basename) : safeDecode(withoutQuery);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Lowchat/MarkdownMedia.tsx:37

mediaFileName only treats / as a path separator, so Windows-style sources like /C:\repo\clip.mp4 (produced by the Windows-drive-path escaping) return the full path C:\repo\clip.mp4 instead of just clip.mp4. When there is no nonempty alt text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both / and \ so Windows backslash separators are handled.

Suggested change
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constbasename=withoutQuery.slice(withoutQuery.lastIndexOf("/")+1);
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
functionmediaFileName(src: string): string{
constwithoutQuery=src.split(/[?#]/,1)[0]??src;
constlastSep=Math.max(withoutQuery.lastIndexOf("/"),withoutQuery.lastIndexOf("\\"));
constbasename=lastSep>=0 ? withoutQuery.slice(lastSep+1) : withoutQuery;
returnbasename.length>0 ? safeDecode(basename) : safeDecode(withoutQuery);
}
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MarkdownMedia.tsx around lines 37-41:
`mediaFileName` only treats `/` as a path separator, so Windows-style sources like `/C:\repo\clip.mp4` (produced by the Windows-drive-path escaping) return the full path `C:\repo\clip.mp4` instead of just `clip.mp4`. When there is no nonempty `alt` text, this full path becomes the media name shown in the UI and used for accessibility labels. Consider splitting on both `/` and `\` so Windows backslash separators are handled.

@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from 3c0d0c9 to b5bc982CompareJuly 29, 2026 20:58
juliusmarmingeand others added 2 commits July 29, 2026 16:58
- Add secure asset access for workspace media and browser artifacts
- Support saving preview screenshots and embedding playable recordings
- Preserve error causes in screenshot save failures
- Reject save+savePath combinations and describe both schema fields
- Verify canonical (symlink-resolved) containment before workspace writes
- Add collision-resistant suffix to artifact screenshot names
- Fix Windows drive paths, protocol-relative URLs, query strings, and
extensionless video sources in markdown media resolution
- Only treat absolute browser-artifacts paths as server artifacts
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
juliusmarmingeand others added 7 commits July 29, 2026 16:58
Verify the deepest existing ancestor canonicalizes inside the workspace
before mkdir, and reject existing destination symlinks that resolve
outside the root.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Re-resolve browser-artifact and thread-image claims on every request so
post-issuance symlink swaps cannot redirect a signed token
- Verify canonical containment and media extension after realPath
- Render unscoped image outputs when no turn is unsettled
- Keep the image activity id paired with its path across collapsing
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Drop readonly/idempotent hints from preview_snapshot now that it writes
- Treat only real .. segments as escapes when resolving workspace assets
- Append image galleries when turns share a target row
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Payload pruning (#4622) stripped every item field outside the command
allowlist, so image view activities reached clients without the path and
the chat gallery rendered nothing. Preserve the item type and path while
still dropping the base64 image bytes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@gabrielelpidio
gabrielelpidioforce-pushed the t3code/render-chat-media branch from b5bc982 to 5b2f955CompareJuly 29, 2026 21:00
);
}

function ImageOutputThumbnail({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Mediumchat/MessagesTimeline.tsx:1966

Each thumbnail calls onImageExpand with images containing only its own loadedUrl, so the expanded viewer cannot navigate to other images in the same gallery. The images array should include all loaded thumbnails and index should point to the clicked one, so the preview can show the full gallery as expected.

🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @apps/web/src/components/chat/MessagesTimeline.tsx around line 1966:
Each thumbnail calls `onImageExpand` with `images` containing only its own `loadedUrl`, so the expanded viewer cannot navigate to other images in the same gallery. The `images` array should include all loaded thumbnails and `index` should point to the clicked one, so the preview can show the full gallery as expected.

Comment threadapps/server/src/mcp/toolkits/preview/handlers.ts

@cursorcursorBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

Bugbot Autofix is ON, but a cloud agent failed to start.

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.


const projectedData: Record<string, unknown> = {};
const item = projectCommandData(data);
const item = projectCommandData(data) ?? projectImageViewData(data);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Command projection drops image paths

Medium Severity

For image_view activities, projectActivityPayload sets data.item from projectCommandData(data) ?? projectImageViewData(data). When the tool item includes any pruned command field (command, input.command, or result.command), command projection wins and image-view projection is skipped, so type, path, and savedPath never reach thread snapshots. Clients then derive no imagePath, so inline galleries and thread-image asset requests for those activities fail even though the full payload in persistence still carries the image path.

Fix in CursorFix in Web

Reviewed by Cursor Bugbot for commit 58f9832. Configure here.

@juliusmarminge

Copy link
Copy Markdown
MemberAuthor

Closing in favor of #2829 (orchestration V2).

#2829 deletes the V1 orchestration layer this PR builds on — apps/server/src/orchestration/**, provider/Layers/*Adapter.ts and provider/Services/** are removed and replaced by apps/server/src/orchestration-v2/**, with the IPC surface renamed to ORCHESTRATION_V2_WS_METHODS. The files this PR touches either no longer exist or are rewritten, so it can't be rebased — it would need reimplementing against the V2 adapters.

This is not a judgement on the change itself. Several of these are real gaps we still want fixed; the base just moved out from under them.

Once #2829 merges, please rebase onto main, port the change to the V2 equivalent, and reopen (or open a fresh PR). Ping me and I'll prioritise the review.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XXL1,000+ changed lines (additions + deletions).vouch:trustedPR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@juliusmarminge@Noojuno@gabrielelpidio