fix(mail): render HTML email bodies - #1495
Conversation
github-actions
Bot
added
size/XL
Dependency Changes DetectedThis PR modifies dependency files. Please review whether these changes are intentional. Changed files:
Maintainer checklist:
|
There was a problem hiding this comment.
Review at e4ec308a — APPROVE
First-round review of the replacement for #1489 (closed unmerged by the author and deliberately split into this PR for HTML rendering and #1496 for authorization mode copy; see the supersession notes on #1489). Fixes #1492; related backend contract: Mininglamp-OSS/octo-mail#71.
Relationship to #1489 — carried over vs rewritten
Byte-compared this head against #1489's final approved head b0e48f9d on a fresh checkout:
- Byte-identical:
packages/mail/package.json,packages/mail/src/bridge/types.ts,packages/mail/src/features/MessageDetailFeature.tsx,packages/mail/src/features/MessageDetailFeature.test.tsx,packages/mail/src/ui/MailContent/index.css,pnpm-lock.yaml. - i18n only:
en-US.json/zh-CN.jsondiffer fromb0e48f9dsolely because therequestedAutomatic/requestedManualrename was already absorbed intomainseparately; vsmainthis PR adds exactly the two reader keys (mail.reader.htmlBody,mail.reader.bodyTruncated) in zh+en. The authorization mode-copy hunks were removed with the split.
So the entire rendering/isolation surface that went through six rounds on #1489 (ending in three approvals on b0e48f9d) carries over unchanged, and I re-verified every item at this head rather than trusting the history.
Security checklist (byte-verified at e4ec308a)
- Sanitization — DOMPurify resolved to exactly
3.4.2inpnpm-lock.yaml(same verified pin as #1489). Config:WHOLE_DOCUMENT: true+FORBID_TAGScoveringscript, iframe, form, base, meta, link, object, embed, area, map, button, input, select, textarea; no permissive flags. Fail-closed: sanitized output is re-parsed, and an empty/invisible result returnsnull→ plain-textbodyTextfallback (tested: wrapper-only, empty div, remote-image-only,cid:-only, remote-video-only). - Isolation —
sandbox="allow-same-origin allow-popups allow-popups-to-escape-sandbox"with noallow-scripts, asserted by exact-equality test (MessageDetailFeature.test.tsx:234);referrerPolicy="no-referrer"; inbound<base>is forbidden by sanitization and the frame injects its own<base href="about:blank" target="_blank">ahead of any email head content; every anchor is rewritten totarget="_blank" rel="noopener noreferrer",xlink:hrefcollapsed tohref,javascript:hrefs stripped (exact assertions in the isolation test). - CSP — exact string
default-src 'none'; img-src data: blob:; style-src 'unsafe-inline'; font-src data:injected as ameta http-equivinside the srcdoc, asserted by exact-equality test. No script source, nohttp(s)source — remote images cannot load, so the recipient's IP is not leaked; the CSP meta cannot be countered becausemeta/linkare forbidden tags. - Theme/readability — srcdoc defaults set
html{color-scheme:only light;background:Canvas}andbody{color:CanvasText;background:Canvas}, and the wrapper class inMailContent/index.csscarriescolor-scheme: only light; background: Canvas— both halves of the dark-theme fix from #1489 are present. - Render call sites — single render site (
EmailMessageContentatMessageDetailFeature.tsx:1093-1099); zerodangerouslySetInnerHTMLanywhere inpackages/mail;ComposerFeature.tsxandutils.tsuntouched. - Visibility/fallback — two-layer detection: pre-render heuristic (trimmed text /
data:|blob:images / SVG paint elements on a style-stripped clone) plus post-load computed-style TreeWalker walk (visibility,font-size:0, transparent color incl. modernrgb(... / 0)syntax, ancestordisplay:none/opacity:0/clip-to-zero rect), images requirecomplete && naturalWidth/Height > 0; on empty →onEmptylatches thebodyTextfallback. Auto-size height capped to[80, 20000]with a stable-baseline resize for viewport-unit content; zero-width frames do not latch (tested). - bodyTruncated — real backend contract per Mininglamp-OSS/octo-mail#71: oversized (>128 KiB) or multiple eligible HTML bodies →
bodyHtmlomitted +bodyTruncated: true, raw endpoint stays the complete path. The banner copy ("not displayed here; download the raw message") is accurate against that contract, and the banner's download button reuses the pre-existingdownloadRaw/getRawMessagepath (covered by the oversized-body test). Backend #71 is still open, but the field is optional and forward-compatible; #1492 explicitly scopes this PR to the browser side. - i18n — both new keys present in zh-CN and en-US;
pnpm i18n:checkclean.
Local verification at this head
pnpm testinpackages/mail: 25 files, 214/214 passed (includes the exact-equality sandbox/CSP/base/link-hardening tests).pnpm typecheckinpackages/mail: clean.pnpm i18n:check(repo root): clean.- CI on
e4ec308a: all green — Build, install-build, e2e-p0, dependency-review, osv-scan, secret-scan, pr-title-lint.
💬 Non-blocking notes
Carried over from the final round on #1489 (all raised by yujiwei there, all P2, none changed the approvals):
cid:inline images in mixed bodies (text +cid:images, i.e. most Outlook mail) survive sanitization but cannot render under the CSP, painting broken-image glyphs; stripping/neutralizingcid:srcduring sanitization is the cheap fix. Acid:-only body already falls back correctly (tested).- Client-side enforcement of the truncation contract:
html={message.bodyTruncated ? undefined : message.bodyHtml}would make the reader enforce the contract instead of assuming the server never sends both. - The visibility predicates use exact equality (
opacity === 0,fontSize === 0), so near-zero values (opacity:0.001,font-size:0.0001px) still read as visible; an epsilon (plus a viewport-intersection check) was suggested.
From the second-pass review:
4. The visibility heuristic recognizes text, <img>, and SVG paint elements only; bodies whose only content is CSS-generated content or background images may over-fallback to plain text — acceptable degradation, worth covering in a follow-up.
5. The 80px minimum frame height is hard-coded in two places (EMAIL_HTML_MIN_HEIGHT and the CSS min-height); keeping them as a shared documented constant would prevent drift.
✅ Highlights
- Defense-in-depth exactly as verified on #1489: DOMPurify + restrictive CSP + script-less sandbox + referrer suppression + anchor hardening, each pinned by exact-equality tests.
- Careful lifecycle handling: observer/media-listener cleanup, no re-sanitization on thread expansion, zero-width guard.
- The split from #1489 is clean — no mode-copy residue, scope matches #1492 precisely.
yujiawei
left a comment
yujiawei
left a comment
There was a problem hiding this comment.
Code Review — PR #1495 (octo-web)
Reviewed at head e4ec308a6658d3d89909fea8bfa641458751b6bb against main. I read the full diff, traced the data flow from MailService.getMessage through to the reader, probed the sanitizer configuration against a set of bypass payloads locally, and ran the checks below myself.
Verification I ran locally at this head:
| Check | Result |
|---|---|
pnpm --filter @octo/mail test |
✅ 25 files, 214 tests passed |
pnpm --filter @octo/mail typecheck |
✅ clean |
pnpm i18n:check |
✅ 0 candidates, locale keys healthy |
pnpm lint:css |
✅ 0 errors |
The PR description's testing claims match what I observed.
1. Spec compliance
Spec: ✅
Checked against the four Expected Behavior bullets in #1492:
| Requirement | Where | Status |
|---|---|---|
| Sanitize and render backend HTML in a sandboxed iframe | MessageDetailFeature.tsx:98, :382-391 |
✅ |
| Keep content isolated from the Octo Web origin and app theme | :281-292 (<base href="about:blank">, meta CSP), :291 (color-scheme:only light), index.css:558-566 |
✅ |
Fall back to bodyText when sanitized HTML has no visible content |
:129-133 (pre-flight), :246-265 + :332-334 (post-render) |
✅ |
| Show a notice + raw-message download when HTML is omitted | :1082-1096, bodyTruncated in bridge/types.ts:204 |
✅ |
- Missing: none.
- Extra: none out of scope. The auto-height machinery (
:315-370) is not named in the issue, but an iframe with no intrinsic height is unusable without it, so I read it as required implementation detail rather than scope creep. Everything lands inpackages/mail; no shared-layer change, consistent with the PR's module-boundary declaration. - Deviation: none.
MailService.getMessageis a straight typed pass-through (Service/MailService.ts:420-425), so addingbodyTruncatedtoMessageDetailis sufficient plumbing — there is no intermediate mapper that would silently drop it.
Dependency hygiene is also fine: dompurify@^3.4.2 already resolves to the same 3.4.2 that @octo/base pins (pnpm-lock.yaml:471, :843, :4532), so no duplicate copy enters the bundle.
2. Code quality
Quality: Approved — no P0/P1. Everything below is P2 / follow-up.
Security posture — I could not break it
I want to be explicit that I tried, because this is the part that matters most. I ran the exact DOMPurify.sanitize configuration from :99-117 against a battery of payloads. These all survive sanitization:
<style>@import url("https://evil.com/x.css");</style>
<style>body{background:url(https://evil.com/t.png)}</style>
<p style="background:url(https://evil.com/t.png)">
<img srcset="https://evil.com/a.png 1x">
and these are correctly stripped: on* handlers, javascript:/vbscript:/data:text/html hrefs, <form>, <object>, <meta http-equiv=refresh>, <svg><use href="https://…">, <animate attributeName="href">, and the <noscript> mXSS classic.
The surviving payloads are all neutralised by two independent controls, and I confirmed both are correct:
sandboxwithoutallow-scripts(:388) — a hard stop on script execution.allow-same-originis present, so the frame is same-origin with the app, but with no script execution nothing inside can reachparent. The inline comment at:387documents exactly this trade-off, which is the right call.- Meta CSP (
:78-83) —default-src 'none'coversscript-src,connect-src,media-src,frame-src,object-src,worker-srcby fallback;style-src 'unsafe-inline'permits inline CSS but no URL sources, which is what kills@import.form-actionandbase-urido not fall back todefault-src, but<form>and<base>are both inFORBID_TAGS, so the gap is closed.
<base href="about:blank"> correctly inerts relative URLs so an email cannot link into an app route, and rel="noopener noreferrer" is applied to every surviving anchor including SVG ones (:135-145). Nicely done.
P2 — remote-resource blocking rests entirely on the meta CSP. Given the payloads above, if the meta CSP is ever weakened or is not honoured in some embedding context, tracking pixels fire and CSS-based exfiltration via @import becomes live — there is no second line of defence at sanitize time. Consider a DOMPurify afterSanitizeAttributes hook that strips non-data:/blob: values from src/srcset/background and drops url(...) references from inline style, so the CSP is a backstop rather than the only control. Also worth a code comment at :388 warning that adding allow-scripts to that sandbox string would turn this into full XSS.
P2 — a single false-negative measurement permanently downgrades the message to plain text
// MessageDetailFeature.tsx:339-346
const loaded = () => {
...
if (!refresh()) return; // onEmpty() already fired — bail
frameDocument.querySelectorAll("img").forEach((image) => { ... }); // never reached
void frameDocument.fonts?.ready.then(...); // never reachedWhen the first refresh() decides the body is empty, loaded() returns before attaching the img load/error listeners, the fonts.ready hook, or the ResizeObserver. Combined with the latch in EmailMessageContent (useTextFallback = !sanitized || emptyHtml === sanitized), there is no recovery path: one false negative and the message renders as plain text for the rest of the session.
The blast radius is limited (the fallback is the pre-PR behaviour, so nothing is lost that used to work), which is why this is P2 and not blocking. But it makes the heuristic's correctness load-bearing in a one-shot way. Suggestion: attach the media/font listeners before evaluating emptiness, and treat onEmpty as revocable if a later refresh finds content.
P2 — the visibility heuristic has reachable false-positive and false-negative gaps
False negative (legitimate content hidden behind the text fallback). Both the pre-flight at :124-128 and the runtime check at :251-264 only recognise images as img[src] with a data:/blob: URL. These render fine but are classified as empty:
<picture><source srcset="data:image/png;base64,..."><img alt="Chart"></picture>
<div style="width:200px;height:100px;background:url(data:image/png;base64,...)"></div>False positive (blank frame shown, fallback never triggers). isRenderedTextNode (:218-235) and hasVisibleAncestors (:183-201) cover display:none, opacity:0, visibility, font-size:0, transparent colour, and clipped-to-zero. They do not cover off-screen positioning:
<div style="position:absolute;left:-9999px">preheader</div>
<div style="text-indent:-9999px">preheader</div>Those are standard preheader techniques. A marketing email using one of them plus a remote-image-only body (blocked by CSP) will pass hasRenderedText, keep the iframe, and show the user an empty 80px box — the exact case the fallback exists to catch.
Related: when the fallback does fire on an image-only email, getMessageText (utils.ts:237-240) degrades to preview || "", so the reader can end up with a literally empty <p> and no explanation. A "remote images blocked" notice — reusing the existing octo-mail-body-truncated treatment — would close both holes with one affordance and is arguably the more valuable follow-up than tightening the heuristic further.
P2 — the iframe never remeasures on content-height change
// MessageDetailFeature.tsx:361-369
widthObserver = new ResizeObserver((entries) => {
const nextWidth = entries[0]?.contentRect.width ?? iframe.clientWidth;
if (nextWidth === width) return;
width = nextWidth;
refresh();
});
widthObserver.observe(iframe);Only the iframe element is observed, and only width changes act. Height changes originating inside the frame are missed — and these happen without scripts: <details>/<summary> toggling is native and both tags survive DOMPurify, as do CSS :hover/:target expansions and transitions. Result: the content grows, the iframe does not, and the user gets an internal scrollbar in a fixed-height box. Observing the frame document's body/documentElement as well would fix it.
Two things I checked while here, for the record:
- Resize feedback loop.
resize()(:315-325) collapses to 80px before measuring, and the reader is aoverflow-y: autocolumn atwidth: min(100%, 920px)(index.css:92,:96), so a height change can toggle a scrollbar and change the iframe width on narrow viewports. I don't think this loops in practice: thenextWidth === widthguard at:365blocks the same-width case, and a true A↔B oscillation needs narrower-width → shorter-content, which is anti-monotonic for normal reflow. Batching the remeasure into arequestAnimationFramewould make this structurally impossible and would also fix the miss above — worth doing as one change. - Viewport-unit measurement. Measuring from a collapsed 80px baseline means
height:110vhcontent resolves to an 88px frame. The test atMessageDetailFeature.test.tsxasserts exactly"88px", so this is a deliberate stability-over-accuracy choice — fine,vhis rare in email, but it's worth a one-line comment saying so, since the number reads like a bug otherwise.
P2 — inline cid: images have no resolution path
EMAIL_HTML_CSP restricts img-src to data: blob: (:80), and nothing rewrites cid: references into attachment blobs. Emails with inline logos or signature images — very common — will render broken image placeholders even though the parts are already available via MailService.downloadAttachment. Not a regression (nothing rendered before), but the most likely first bug report once this ships.
P2 — heuristic cost on large emails
hasRenderedText (:237-244) walks every text node and calls hasVisibleAncestors, which climbs to the root calling getComputedStyle at each level — O(nodes × depth) synchronous style recalcs, re-run on every image load/error (:346-355) and every width change. Each refresh() also forces two layout passes via resize(). On a deeply nested table-layout marketing email with many images this is a lot of main-thread work. A TreeWalker over SHOW_ELEMENT | SHOW_TEXT returning FILTER_REJECT for hidden subtrees would collapse this to a single pass, and early-exit on the first visible node.
P2 — bodyTruncated copy vs. field name
mail.reader.bodyTruncated reads "This HTML email is not displayed here" (i18n/en-US.json), but the field is named bodyTruncated and the banner at :1082-1096 renders above EmailMessageContent, which still renders whatever body did arrive. If the backend ever sets the flag alongside a partial bodyHtml, the user sees "not displayed here" directly above the displayed body. Either rename the field to match the copy (bodyOmitted) or soften the copy to "may be incomplete". I could not open Mininglamp-OSS/octo-mail#71 from my account to confirm which semantic the backend intends — please double-check that the two sides agree.
Nits
EMAIL_HTML_MIN_HEIGHT = 80(:85) is duplicated asmin-height: 80pxinindex.css:561. If one moves the other silently disagrees.role="status"on the truncated banner (:1083) makes it a live region containing a focusable button; a plain container with the icon markedaria-hiddenwould be more conventional.- The raw-message download now has three entry points on one screen (header action
:876, this banner:1086, footer hint:1221).
Test coverage
The unit tests are thorough about the heuristic and I'm glad they exist. But they do not exercise the pipeline that carries the security properties: several tests replace the loaded srcdoc by hand (frameDocument.body.innerHTML = bodyHtml, then manually setting data-octo-mail-body and synthesising load), which bypasses real sanitization output, CSP enforcement, resource blocking, image decoding, and layout. The sandbox and CSP assertions are string comparisons on attributes, not behaviour.
That's the correct limit for jsdom — but this repo already has a real-browser harness at apps/web/e2e-kit/tests/mail/, and none of the three specs there touches body rendering. One e2e case asserting that a remote <img> genuinely fails to load and that a javascript: link genuinely does not execute would convert the two controls this feature depends on from "asserted as strings" to "verified as behaviour". Strongly recommended as a follow-up.
3. Verdict
APPROVE.
Spec ✅ and Quality Approved. The security model is sound and I could not find a bypass; every payload that survives the sanitizer is stopped by the sandbox or the CSP. Build, tests, typecheck, i18n and CSS lint are all green at this head. Every finding above is P2 — none of them can lose data or regress existing behaviour, because the worst case in each is the plain-text rendering that shipped before this PR.
4. Suggested follow-ups, in priority order
- Attach the media/font listeners before the emptiness check, and allow
onEmptyto be revoked by a later refresh (:339-346). - Add a "remote images blocked" notice — it fixes the blank-frame case, the empty-
<p>case, and the missing user feedback about CSP-blocked content in one stroke. - Batch remeasurement into
requestAnimationFrameand also observe the frame body, fixing both the missed content-height changes and any theoretical scrollbar oscillation (:361-369). - Add a defence-in-depth DOMPurify hook stripping remote
src/srcset/background/url(...), so the meta CSP is a backstop rather than the sole control. - Add an e2e case in
apps/web/e2e-kit/tests/mail/asserting real resource blocking and real script non-execution. - Confirm the
bodyTruncatedcontract with the backend and align the field name with the user-facing copy.
5. Additional observations outside the diff
getMessageTextreturningpreview || ""(utils.ts:239-240) is the reason an empty body can render as an empty paragraph; the new fallback path makes that pre-existing behaviour much more reachable than it used to be.<title>and<style>elements appearing in<body>contribute tovisibilityBody.textContentin the pre-flight at:130even though both aredisplay:noneat runtime. Harmless today — the runtime check corrects it — but it means the pre-flight and the runtime check disagree about what "visible" means, which is worth unifying if this logic grows.
mochashanyao
left a comment
mochashanyao
left a comment
There was a problem hiding this comment.
[Octo-Q · automated review]
Verdict: Approve — no blocking findings; notes below (data-flow traced).
Code Review — PR #1495 (octo-web)
Reviewer: Octo-Q (automated review)
Head: e4ec308a6658d3d89909fea8bfa641458751b6bb · Base: merge-base 30fac65e5 (origin/main) · Range: pr-base...HEAD
Summary
This PR makes the mail reader render HTML email bodies. Untrusted HTML is sanitized with DOMPurify (WHOLE_DOCUMENT, forbidden script/iframe/object/embed/form/base/meta/link/map/area/...), then rendered into a srcdoc iframe that carries a restrictive CSP meta (default-src 'none'; img-src data: blob:; style-src 'unsafe-inline'; font-src data:), a sandbox without allow-scripts, referrerPolicy="no-referrer", and a pinned <base href="about:blank" target="_blank">. An empty-detection pass (text/image/SVG visibility heuristics) falls back to the pre-existing plain-text render, an auto-sizing loop (load/image/font/ResizeObserver) keeps the frame fitted, and a new bodyTruncated banner offers raw download. dompurify 3.4.2, i18n keys, and CSS are added; tests are extensive. The security posture is defense-in-depth and the design is sound; the findings below are non-blocking.
Verification
Static analysis only at head e4ec308a; build and tests not executed in this environment.
- ✅ No script execution path — iframe
sandbox="allow-same-origin allow-popups allow-popups-to-escape-sandbox"has noallow-scripts(packages/mail/src/features/MessageDetailFeature.tsx:388); DOMPurify strips script-bearing tags; CSP falls back todefault-src 'none'for scripts. - ✅ Link hardening — every anchor is forced to
target="_blank" rel="noopener noreferrer"(:135–:145);javascript:hrefs are removed by DOMPurify and locked by tests;xlink:hrefis normalized only after DOMPurify validated it. - ✅ No tracking/external leakage — only
data:/blob:images anddata:fonts can load; no media/connect/script sources;referrerPolicy="no-referrer". - ✅ No regression for text-only mail — when
bodyHtmlis absent or sanitizes to nothing,EmailMessageContentrenders the exact same<p>text path as before (:406–:412);getMessageTextunchanged. - ✅ Async lifecycle — all frame listeners (load/img load+error/fonts/ResizeObserver) are torn down via the
activeflag and effect cleanup; no state is marked before success; the banner reuses the existingbusy/downloadRawtry/finally pattern. - ✅ Authorization unchanged — data still flows through the existing authorized
MailService.getMessagewithX-Octo-Mailbox-ID(packages/mail/src/Service/MailService.ts:420); no new endpoint or exposure surface. - ✅ Previously reported failure classes are fixed at this head — blocked media (remote/cid images, remote video) are no longer counted as visible content (
:107–:111, data/blob gate) with fallback tests;area/mapare forbidden with a test;<base>is pinned toabout:blank.
Findings
No P0/P1 issues; two P2 items below.
P2 — bodyTruncated banner copy can contradict the rendered content (packages/mail/src/features/MessageDetailFeature.tsx:1082)
When message.bodyTruncated is true the banner says the HTML body is "not displayed here", but EmailMessageContent at :1097 renders message.bodyHtml unconditionally right below it. The backend producer of bodyTruncated is not visible in any available repo (the mail API is not in the octo-server/octo-matter checkouts), so the contract is unverified: if the backend sends a partial bodyHtml together with bodyTruncated=true, users see HTML while the banner claims it is not displayed; if the backend omits bodyHtml, the banner is accurate. Either gate the HTML render on !message.bodyTruncated, or confirm and lock the backend semantics. Note the field has no in-repo producer yet, so the banner is dormant until the backend ships it.
P2 — Geometry-based text hiding escapes the empty-detection heuristic (packages/mail/src/features/MessageDetailFeature.tsx:183)
hasVisibleAncestors/isRenderedTextNode (:183, :218) check display/opacity/font-size/color/visibility/overflow-clip, but not offscreen geometry: position:absolute; left:-9999px, text-indent:-9999px, or clip-path:inset(50%) text still passes as rendered because getBoundingClientRect() reports positive dimensions offscreen. A mail whose entire body is geometry-hidden would render a blank frame instead of falling back to text, where pre-PR code showed the extracted text. The shape is bounded (real hidden-preheader mails pair the preheader with visible body content, which renders fine, and raw download remains an escape hatch), but it is the same class as the blocked-media gap fixed earlier in this feature lineage. Fix direction: treat text whose rect lies entirely outside the body box (or under a clipping clip-path) as not rendered, plus a regression test for an offscreen-only body falling back to text.
Human-verify
- Backend semantics of
bodyTruncated(see P2 above) — the mail API backend is not in any OSS checkout available here. Not a merge blocker for this PR: the field is optional and the UI degrades gracefully; settle the wording/gating question together with the backend change. - DOMPurify 3.4.2 upstream behavior against exotic SVG/mXSS vectors — the final gate in this design is the script-less sandbox plus CSP, which holds regardless, but keep dependency updates monitored. Not a merge blocker.
Things I checked that are fine
- Empty-fallback latch:
emptyHtml === sanitizedidentity is stable across re-renders and message switches; zero-width frames do not latch to fallback (both locked by tests). - Frame document construction:
base/charset/csp/defaultsare inserted before the email's own head content, andbase/link/metaare forbidden, so email content cannot override the base URL or CSP. - Attribute propagation:
htmlAttributeslimited todir/lang; body attributes copied only after sanitization. - i18n parity (
en-US/zh-CNboth updated); new banner CSS uses--wk-*tokens (the iframe interior intentionally forcescolor-scheme: only light). pnpm-lock.yamlpins dompurify 3.4.2 matchingpackage.json; thread-summary cards remain text-only by design (HTML renders only for the current message).
Verdict: COMMENT
No P0/P1 issues: script execution, link navigation, resource loading, and fallback behavior were all traced end-to-end and hold. The two P2s are non-blocking — one is dormant until the backend ships bodyTruncated, the other affects only degenerate geometry-hidden bodies. Suggest addressing both in a follow-up.
[Octo-Q] verdict: APPROVE — 无 P0/P1（按 R4 映射：仅 P2/nit → APPROVE）。两条 P2 均非阻塞：bodyTruncated 文案/渲染门控待后端契约确认；几何隐藏文本的空检测缺口仅影响退化形状。建议终审放行，P2 转 follow-up。
|
Correction to my review above, and a resolved open question. I was wrong that I could not read the backend contract. I used
That resolves the
Two smaller notes that follow from the same contract:
My verdict is unchanged: APPROVE at |
5 participants
Summary
Render sanitized HTML email bodies in an isolated reader frame and provide a raw-message fallback when the mail service omits inline HTML.
Related Issue
Fixes #1492
Related backend: Mininglamp-OSS/octo-mail#71
Changes
bodyHtmland render it in a sandboxed iframe with scripts disabled.bodyTextwhen sanitized HTML has no visible content.bodyTruncatedstate and reuse the raw-message download action.Architecture / Module Boundary
packages/mailTesting
pnpm --filter @octo/mail test— 25 files, 214 tests passedpnpm --filter @octo/mail typecheckpnpm i18n:checkpnpm lint:csspnpm buildChecklist
pnpm i18n:check