fix(mail): avoid remote avatars in MCP embeds#883
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…-{id} app-state
The compose draft hasn't lived in a file for many releases — it's stored in
the SQL 'application_state' table under the 'compose-{id}' key. The
ComposeEditor comment still claimed a file path that no longer exists, which
shows up in greps for 'compose.json' as a false hit when verifying that the
host bridge no longer leaks file-read wording to ChatGPT. Update the comment
to match reality.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Expand the existing image-block policy to also cover @import and url() in <style> tags and style= attributes. Without this, a tracking pixel embedded as a CSS background image (or referenced via @import) bypassed the policy that already strips <img> tags. Inline data: and cid: URLs are preserved. Also exports processHtmlImages so the new EmailThread.spec.ts can cover the new helpers (4 tests added: CSS url() pass-through when images are shown, remote CSS url() blocking, @import blocking with data: passthrough, and legacy <table background>/<video poster> attribute stripping).
Move processHtmlImages, isInlineImageUrl, stripCssRemoteResources, decodeHtmlEntities, isTrackingUrl, and the TRACKER_DOMAINS list out of the heavy EmailThread.tsx component into a new app/lib/email-image-policy.ts so the policy can be unit-tested without pulling in the whole React component tree (which transitively imports opentelemetry and breaks vitest). Replaces the broken EmailThread.spec.ts (which fails at import time) with app/lib/email-image-policy.spec.ts containing the same 4 regression tests plus the @vitest-environment happy-dom pragma so DOMParser is available.
This comment has been minimized.
This comment has been minimized.
|
Addressed the Builder review finding in the latest head. block-all now runs the image policy over both sanitized email head and body fragments, strips remote CSS url(...) references and @import from inline styles / style tags, and removes legacy remote background / poster resource attributes while preserving inline data:image/* and cid: images. I also moved the policy into app/lib/email-image-policy.ts and added focused regression coverage in app/lib/email-image-policy.spec.ts. Verification: focused Mail vitest suite, pnpm --filter mail typecheck, pnpm --filter mail build, and full pnpm prep are green locally. |
|
Addressed the second Builder review finding in eae0c96. block-all now removes link[href] elements from sanitized email head/body fragments before rendering, which closes the remote stylesheet/preload/font fetch path in MCP embeds. Added a regression test covering remote link href removal. Verification after the change: focused Mail vitest suite, pnpm --filter mail typecheck, pnpm --filter mail build, and full pnpm prep are green locally. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Addressed the latest Builder review findings in b4f9d90. block-all now removes remote SVG fetch attributes (image/feImage/use href and xlink:href) and preserves safe same-document CSS fragment references while still stripping remote CSS resources. Added focused regression tests for both cases. Local verification is green: focused Mail vitest suite, pnpm --filter mail typecheck, pnpm --filter mail build, and full pnpm prep. |
There was a problem hiding this comment.
Builder reviewed your changes and found 1 potential issue 🟡
Review Details
Code Review Summary — Complete PR #883
This PR successfully addresses the four previously-flagged issues and implements comprehensive image blocking for MCP embeds. All four earlier concerns have been resolved through targeted fixes.
Changes:
- Extracted image-blocking logic into dedicated
email-image-policy.tswith comprehensiveblock-allmode - Added MCP embed detection (
isMcpEmbedSurface()) to force stricter policies in embedded contexts - Fixed SVG resource element filtering for
<image>,<feImage>,<use>withhref/xlink:href - Fixed CSS fragment URL preservation to protect inline SVG references like
url(#clipPath) - Fixed
<link>element filtering to block stylesheet fetches in block-all mode - Added 14 comprehensive tests covering all scenarios
Previously-flagged issues (all resolved):
- ✅ CSS background images blocking
- ✅ Missing
<link>filtering - ✅ Remote SVG attributes bypass
- ✅ CSS fragment URLs broken
Risk: Standard
🟡 NEW FINDING (1 MEDIUM issue):
🟡 MEDIUM: Integration gap — SVG cid: fallbacks stripped by earlier sanitization
The new processHtmlImages() function correctly preserves cid: and fragment-only URLs on SVG resource elements. However, the upstream sanitizeEmailHtml() function (not modified in this PR) validates href/xlink:href attributes as link URLs, which reject cid:. Result: <image xlink:href="cid:logo"> gets stripped before processHtmlImages() sees it.
Evidence: The new test at line 86-105 passes because it calls processHtmlImages() directly, bypassing the sanitizer. The real rendering pipeline (EmailThread → sanitizeEmailHtml → processHtmlImages) would fail.
Impact: SVG elements with cid: fallbacks will render without their local resources, even though the fallback should survive per the design.
Fix: Either (a) update sanitizeEmailHtml() to treat SVG resource href/xlink:href as image URLs instead of link URLs, or (b) add an integration test that exercises the full pipeline to catch this issue earlier.
Also noted: 1 LOW severity (double HTML parsing at lines 107-110 in email-image-policy.ts creates a parser that's never used, wasteful but not a correctness bug).
🧪 Browser testing: Skipped — PR modifies backend HTML processing logic only, no UI/visual changes.
| const svgResourceElements = new Set(["feimage", "image", "use"]); | ||
| root.querySelectorAll<Element>("*").forEach((el) => { | ||
| if (!svgResourceElements.has(el.localName.toLowerCase())) return; | ||
|
|
||
| for (const attrName of ["href", "xlink:href"]) { |
There was a problem hiding this comment.
🟡 SVG cid: fallbacks not preserved end-to-end in real rendering pipeline
The function correctly preserves cid: and fragment-only URLs on SVG elements. However, the real rendering pipeline sanitizes these attributes earlier: EmailThread.tsx's sanitizeEmailHtml() validates href/xlink:href as links, not images, so cid: URLs are rejected and removed before processHtmlImages() is called.
Why this matters: Test at line 86-105 passes because it calls processHtmlImages() directly. In production, <image xlink:href="cid:logo"> loses the fallback during sanitization, so the feature doesn't work end-to-end.
Fix: Add an integration test that exercises sanitizeEmailHtml() → processHtmlImages() pipeline, or coordinate with the sanitizer to allow cid: on SVG resource elements.
Or react with 🚀 to auto-fix.
When Calendar is rendered inside an MCP host iframe (ChatGPT / Claude embed *.agent-native.com through their own sandboxed wrapper with strict COEP/CORP headers), Google account / Google Contacts avatar images (`*.googleusercontent.com`) get blocked at the browser level and produce noisy console errors. Mirror the Mail PR #883 pattern: - New `templates/calendar/app/lib/mcp-embed.ts` helper exporting `isMcpEmbedSurface()` that detects the embed flag in the URL (`?embedded=1`). Same shape as the Mail-side helper. - Gate the account-avatar `<img>` in CalendarView.tsx and the contact-avatar `<img>` in AttendeeAutocomplete.tsx on `!isMcpEmbedSurface()`. In MCP embed mode they fall back to the existing same-origin initial chip / IconUserCircle, which is already the no-photo branch — same UX as Mail's COEP fix. Doesn't touch outside the embed surface — normal Calendar UI keeps the existing remote avatars. Tests: 4 new vitest cases in mcp-embed.spec.ts. Typecheck clean.
Summary
Verification