Improve DOCX render fidelity#993
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
All contributors have signed the CLA ✍️ ✅ Posted by the CLA bot. |
Greptile SummaryThis PR improves DOCX render fidelity across three areas: multi-column layout, image parsing, and browser-safe image painting. It also adds regression test coverage for the fixed scenarios.
Confidence Score: 4/5Safe to merge after adding a changeset file; the layout and painter changes are well-tested, and the refactoring is mechanically consistent across all six image render sites. The three independent fix areas each have matching synthetic test coverage and the changes are narrowly scoped. The missing changeset file is the only blocker — without it the Release workflow will produce an incorrect CHANGELOG entry or fail packages/core/src/layout-painter/renderImage.ts (new exported API surface) and packages/core/src/layout-engine/index.ts (column-layout geometry change) are the most impactful files and warrant a careful read. All other changed files are straightforward call-site updates or tests.
|
| Filename | Overview |
|---|---|
| packages/core/src/layout-painter/renderImage.ts | New applyImageSourceForBrowser centralises the browser-render guard; TIFF/EMF/WMF data-URLs are hidden instead of producing broken-image icons. Minor semantic issue: undefined src incorrectly sets data-unsupported-image. |
| packages/core/src/layout-engine/index.ts | Multi-column fixes: paragraph/table fragments now use paginator.columnWidth instead of the captured content width, and table justification is computed from result.x (post-addFragment) for correctness. Minor: fragment.width is initialised then immediately overwritten. |
| packages/core/src/docx/imageParser.ts | parseInline/parseAnchor now return null when no blip rId is found, cleanly skipping non-picture DrawingML shapes; parseDrawing already returned `null |
| packages/core/src/docx/textBoxParser.ts | Previously unused _media parameter is now named media and forwarded to the paragraph parser, fixing image resolution for pictures nested inside text-box paragraphs. |
| packages/core/src/layout-painter/renderWatermark.ts | Replaces direct img.src = wm.dataUrl with applyImageSourceForBrowser, adding the browser-format guard to the watermark paint path that previously had no validation. |
| packages/core/src/layout-painter/renderPage/headerFooter.ts | Floating header/footer images now go through applyImageSourceForBrowser instead of direct src assignment; straightforward defensive fix. |
| packages/core/src/layout-painter/floatingImageLayer.ts | Same browser-format guard applied to the floating image layer renderer; one-line change. |
| packages/core/src/layout-painter/renderParagraph/runs.ts | applyImageSourceForBrowser replaces direct img.src assignment in both renderInlineImageRun and renderBlockImage; logic is otherwise unchanged. |
| packages/core/src/layout-engine/integration/section-breaks.test.ts | New integration tests assert that paragraph fragments in two-column sections use columnWidth and that table fragments receive the correct post-addFragment column x-coordinate. |
| docs/render-fidelity-qa.md | New QA checklist for render-fidelity investigations; documentation-only, no code risk. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant P as DOCX Parser
participant IP as imageParser
participant TB as textBoxParser
participant LE as layoutEngine
participant RI as renderImage
participant DOM as DOM
P->>IP: parseImage(drawingEl, rels, media)
IP->>IP: parseAnchor / parseInline
alt no blip rId (non-picture shape)
IP-->>P: null (skip)
else picture found
IP->>IP: resolveImageData(rId, rels, media)
IP-->>P: "Image { rId, src, ... }"
end
P->>TB: parseTextBoxContent(..., rels, media)
Note over TB: media now forwarded to parseParagraph
TB->>IP: parseImage (nested picture)
IP-->>TB: "Image { src: resolved dataUrl }"
P->>LE: layoutDocument(blocks, measures, options)
LE->>LE: layoutParagraph / layoutTable
Note over LE: width = paginator.columnWidth
Note over LE: table x = result.x + justification
LE-->>P: "Layout { pages, fragments }"
LE->>RI: renderImageFragment / renderFloatingImagesLayer
RI->>RI: applyImageSourceForBrowser(img, src)
alt src is data:image/tiff or EMF/WMF
RI->>DOM: removeAttribute(src), visibility:hidden
else src is data:image/png, blob:, https:
RI->>DOM: "img.src = src"
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant P as DOCX Parser
participant IP as imageParser
participant TB as textBoxParser
participant LE as layoutEngine
participant RI as renderImage
participant DOM as DOM
P->>IP: parseImage(drawingEl, rels, media)
IP->>IP: parseAnchor / parseInline
alt no blip rId (non-picture shape)
IP-->>P: null (skip)
else picture found
IP->>IP: resolveImageData(rId, rels, media)
IP-->>P: "Image { rId, src, ... }"
end
P->>TB: parseTextBoxContent(..., rels, media)
Note over TB: media now forwarded to parseParagraph
TB->>IP: parseImage (nested picture)
IP-->>TB: "Image { src: resolved dataUrl }"
P->>LE: layoutDocument(blocks, measures, options)
LE->>LE: layoutParagraph / layoutTable
Note over LE: width = paginator.columnWidth
Note over LE: table x = result.x + justification
LE-->>P: "Layout { pages, fragments }"
LE->>RI: renderImageFragment / renderFloatingImagesLayer
RI->>RI: applyImageSourceForBrowser(img, src)
alt src is data:image/tiff or EMF/WMF
RI->>DOM: removeAttribute(src), visibility:hidden
else src is data:image/png, blob:, https:
RI->>DOM: "img.src = src"
end
Comments Outside Diff (1)
-
packages/core/src/layout-engine/index.ts, line 381-397 (link)fragment.widthinitialised then unconditionally overwrittenIn the empty-paragraph path the fragment is created with
width: paginator.columnWidthand thenfragment.width = paginator.columnWidthis written again immediately afteraddFragment. The post-addFragmentwrite is the authoritative one (in caseensureFitsadvanced to a page with different geometry), so the constructor value is always dead. Settingwidth: 0(or any placeholder) at construction and relying solely on the post-addFragmentassignment would remove the ambiguity and match howxandyare handled. The same pattern recurs in the non-empty paragraph loop at line 458/475.
Reviews (1): Last reviewed commit: "Improve DOCX render fidelity" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
LGTM overall — render-fidelity logic is sound, typecheck clean, tests green, React/Vue parity holds. One security finding to fix before merge: HIGH — zero-click DoS in the new TIFF decoder ( Fix: clamp |
| stripByteCounts: number[] | ||
| ): Uint8Array { | ||
| const total = stripByteCounts.reduce((sum, count) => sum + Math.max(0, count), 0); | ||
| const out = new Uint8Array(total); |
There was a problem hiding this comment.
Security (HIGH): unbounded allocation from attacker-controlled bytes.
total is the raw sum of the file's StripByteCounts (each a u32, no upper clamp), and the per-strip bounds check at line 142 runs after this allocation. A ~138-byte crafted TIFF with StripByteCounts = 0x40000000 forces new Uint8Array(1 GiB); values near 2^32 throw RangeError that propagates uncaught through buildMediaMap (parser.ts:403/452, no try/catch) and aborts document open. Zero-click DoS on untrusted input. Reproduced locally.
Suggest clamping before allocating, e.g.:
const total = stripByteCounts.reduce((sum, count) => sum + Math.max(0, count), 0);
if (total > bytes.length) return new Uint8Array(0); // strip data can't exceed the source file
const out = new Uint8Array(total);Also add a width*height sanity cap before the BMP allocation (line 164) and wrap the decode in try/catch → return undefined. Per CLAUDE.md: never feed a file-supplied number straight into allocation.
There was a problem hiding this comment.
Thanks, fixed in 6e324d5a.
The TIFF preview path now validates/clamps strip byte counts before allocation, validates strip offsets/counts before copying, caps preview dimensions before BMP allocation, rejects invalid samples-per-pixel values, and wraps preview decoding so malformed TIFF input returns undefined instead of aborting document open. I also added regression tests for oversized strip byte counts, oversized dimensions, and malformed buffers.
Latest PR head is green for lint, test, build, CLA, and Vercel.
…elity # Conflicts: # packages/core/src/docx/parser.ts # packages/core/src/prosemirror/conversion/toProseDoc/textbox.ts
|
Addressed the remaining review feedback on this PR:
Latest PR head is green for lint, test, build, CLA, and Vercel. |
2 participants
Summary
Validation