rtf-codec: build a real OLE compound file for embedded objects - #1008
Merged
Conversation
Mearman
force-pushed
the
feat/rtf-codec-ole-embedded-objects
branch
3 times, most recently
from
September 5, 2026 17:56
37bbdf3 to
d542dd3
Compare
Mearman
marked this pull request as ready for review
September 5, 2026 18:00
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Mearman
force-pushed
the
feat/rtf-codec-ole-embedded-objects
branch
4 times, most recently
from
September 6, 2026 07:22
410a2ef to
d51f1f3
Compare
Member
Author
|
@codex security review Requesting a fresh review: the previous Codex security review ran against |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Mearman
force-pushed
the
feat/rtf-codec-ole-embedded-objects
branch
2 times, most recently
from
September 6, 2026 11:46
dc183dc to
a4177fc
Compare
Mearman
force-pushed
the
feat/rtf-codec-ole-embedded-objects
branch
2 times, most recently
from
September 6, 2026 15:40
638c7f3 to
cfb1bde
Compare
Builds a Package stream's bytes from the same {label, sourcePath,
tempPath, fileBytes} shape readOlePackage returns, so a caller wrapping
a file for embedding can round-trip through both directions rather than
only reading what a real Word/PowerPoint embed already wrote.
The three descriptive strings are restricted to ASCII: readOlePackage
decodes them as windows-1252 on the way in, but encoding an arbitrary
string back to that codepage would need a full codepage table this
package deliberately does not carry (the identical trade-off
oleps/write.ts already makes for VT_LPSTR, resolved there by writing
VT_LPWSTR unconditionally instead). Writing a non-ASCII label or path
throws OlePackageWriteError rather than silently mangling it.
…t's \objdata An RTF \object's \objdata is, per the specification, "the structure produced by the OLESaveToStream function" -- a real OLE compound file. This package used to drop every embeddedObject block on write and every \object destination on read because it had no way to build or parse that container; archive-codec's writeCompoundFile/readCompoundFile and writeOlePackage/readOlePackage now do, so a dependency on it turns the stated impossibility into ordinary work. src/embedded-object.ts packages a ContentEmbeddedObject's own JSON -- objectKind, document, frame, and the anchor fields -- as the Package stream's "file", wrapped in a real [MS-CFB] compound file: rtf-codec cannot depend on ooxml.js/odf.js to re-serialise the nested document into a real docx/xlsx/etc, so its own JSON is the one payload this codec can both write and read back losslessly. The write side (writeEmbeddedObjectBlock) emits \object\objemb with \objw/\objh sizing hints, an \*\objclass label, the real compound-file bytes as \*\objdata hex, and a minimal \result fallback paragraph. The read side adds an "object"/"objectData" destination pair to the state machine (mirroring "picture"'s own hex/binary capture) and attempts to decode \objdata as this package's own payload; a real, foreign OLE object's native data degrades with EMBEDDED_OBJECT_UNREADABLE instead of throwing, and \result's own fallback content is discarded rather than folded into the surrounding paragraph, exactly as a reader that can use the real object is supposed to ignore its cached appearance. EMBEDDED_OBJECT_DROPPED is removed: every embeddedObject block now writes successfully, so the code describing why one couldn't is no longer reachable. hexToBytes' return type is narrowed to Uint8Array<ArrayBuffer> (it already only ever constructs one via Uint8Array.from), which is what lets its result flow into archive-codec's own Uint8Array<ArrayBuffer> parameters without a cast. BREAKING CHANGE: RtfDiagnosticCodes.EMBEDDED_OBJECT_DROPPED, a public export since rtf-codec@1.0.0, is removed with no replacement and no deprecated alias, since every embeddedObject block now writes successfully and the code it named is unreachable.
…-payload degrade path Adds real fixtures rather than relying on the diagnostics-coverage suite's own placeholder: writing an embeddedObject block produces a genuine [MS-CFB] container (checked against the format's own magic bytes) inside \object\objemb's \objdata, a full write-then-read cycle preserves objectKind/frame/document exactly, and reading a hand-built \object whose \objdata is not this package's own JSON envelope degrades with EMBEDDED_OBJECT_UNREADABLE instead of producing a block. A third read-side test confirms \result's own fallback text never leaks into the surrounding paragraph flow. The workers suite gets the identical round trip run inside a real workerd isolate, proving embedded-object.ts's archive-codec dependency is genuinely Worker-isomorphic rather than merely bundleable.
…rchive-codec dependency Adds an Embedded objects section explaining what rides inside the [MS-CFB] container (this codec's own JSON envelope, not a foreign format's bytes, since rtf-codec cannot depend on ooxml.js/odf.js to produce one) and why reading a real Word-authored \object still degrades rather than decodes. Updates the architecture diagram, the scope tables, and the dependency-choices note to name archive-codec as a real sibling dependency, and splits the old combined "embedded/drawing objects, dropped" row now that only drawing objects (\do/\shp, a schema gap rather than a container one) remain unhandled. The root workspace README's rtf-codec and archive-codec rows are updated to match.
…dObject An RTF \object's \objdata is "the structure produced by the OLESaveToStream function" (RTF 1.9.1), and OLESaveToStream's own output is MS-OLEDS 2.2.5 EmbeddedObject: an ObjectHeader (2.2.4 -- OLEVersion, a FormatID of 0x00000002, then ClassName/TopicName/ItemName as LengthPrefixedAnsiStrings), a NativeDataSize, and the NativeData that size names. writeEmbeddedObjectData was emitting the raw [MS-CFB] compound file directly as that payload, with no ObjectHeader/NativeDataSize envelope in front of it, and readEmbeddedObjectData called readCompoundFile straight on the raw bytes. Confirmed non-conformant both by inspection (the emitted payload begins with the CFB magic bytes, so a real ObjectHeader.FormatID field would parse as garbage there) and decisively by wrapping a spec-shaped ObjectHeader around the old writer's own output and finding its own reader could not read its own conformant payload back. writeObjectHeader/readObjectHeader now build and parse the real envelope. ClassName is written as "Package", the class real Word/PowerPoint use for the identical shape (an OLE Package stream sitting inside the object's native data), so a real OLE-aware consumer that cannot decode this package's own JSON payload still sees an accurate class label. readObjectHeader accepts either FormatID (0x00000001 LinkedObject or 0x00000002 EmbeddedObject) as structurally valid framing, per the ObjectHeader's own invariant, even though this module only ever writes an EmbeddedObject.
… decode
RTF 1.9.1's own advice for \object's \result fallback is direct: it "allows
RTF readers that do not understand objects ... to use the current result,
in place of the object, to maintain appearance." This reader always tries
\objdata first and prefers it when it decodes, exactly as Word itself does,
but on the degrade path -- a real, foreign OLE object this reader cannot
decode -- \result was discarded unconditionally rather than used for
exactly the case the spec wrote it for.
\result now reads as ordinary body content, but only when its sibling
\objdata has already failed to decode: a new ObjectState, shared by
reference across the whole {\object ...} group the same way FieldState
already is across \fldinst/\fldrslt, tracks whether \objdata succeeded, and
the group-start handler overrides \result's destination from "skip" to
"body" precisely when it has not. \objw/\objh (the object's own size hint,
previously not read by this package at all) are now captured on that same
state and folded into the EMBEDDED_OBJECT_UNREADABLE diagnostic's message,
so the size is reported rather than silently discarded on the path where
nothing in the content model has a position left to carry it.
…ery path embedded-object.test.ts builds spec-conformant and deliberately malformed ObjectHeader/NativeDataSize payloads by hand, independently of this package's own writer, and checks readEmbeddedObjectData against them: a real ObjectHeader-framed payload decodes, a LinkedObject-shaped FormatID (0x00000001) is accepted as structurally valid framing, an unrecognised FormatID is rejected, an overrunning NativeDataSize is rejected, and -- the regression this whole fix exists for -- a bare [MS-CFB] compound file with no ObjectHeader envelope at all (the pre-fix writer's own shape) no longer round-trips. read.test.ts adds the \result recovery path at the full read pipeline level: a real Word-shaped, undecodable \object recovers its \result paragraphs in the object's own place, with the surrounding paragraph text intact on both sides and no UNBALANCED_GROUP diagnostic, proving the new conditional destination kind does not corrupt group/brace handling; a separate test pins the \objw/\objh size hint now appearing in the degrade diagnostic's own message; and a last test pins that \result is still discarded, with no unbalanced group either, when \objdata decodes successfully and the real object is used in its place.
…ta framing The Embedded objects section previously described \objdata's payload as "a real OLE compound file" outright -- true of the old, non-conformant writer, but not of what MS-OLEDS 2.2.5 EmbeddedObject actually specifies: the compound file is the NativeData, one field inside an ObjectHeader + NativeDataSize + NativeData structure, not the whole of \objdata. Restated the section around that structure, and around \result now being read as the object's own fallback content when \objdata cannot decode rather than always discarded. Also notes archive-codec's writeCompoundFile still cannot set a root-storage CLSID, so even correctly-framed output will not self-identify its object class to a real OLE consumer.
\objdata's own grammar is (\binN #BDATA) | #SDATA, but every existing test delivered the payload as plain #SDATA hex-digit text. The reader already handles the \binN raw-byte-run and repeated \'hh-escape forms identically (both route through the same objectData.binary accumulation as picture payloads do), but nothing exercised that path directly.
…pping it objectSizeHintClause required both widthTwips and heightTwips before folding \object's own size hint into the EMBEDDED_OBJECT_UNREADABLE degrade message, so a producer stating only \objw or only \objh (both independently optional per RTF 1.9.1's <objsize>) had its size silently discarded rather than reported. Report whichever of the two is actually present.
…ing is read isResultFallback decided \result's fate from state.object.decoded at the exact moment \result's own group opened -- correct when \objdata precedes \result in the source, but RTF 1.9.1's own <obj> grammar does not forbid the other order, and \result genuinely preceding \objdata is legal RTF. In that order \objdata's own success was not yet known when \result's fate was decided, so a producer writing \result before \objdata got both the cached preview and the real decoded object rendered together. \object's own group-start now looks ahead at its \objdata child (if any) before either sibling is actually read, decoding its payload the same way buildEmbeddedObject's live read does, so objectState.decoded is a pre-resolved verdict rather than "has \objdata been read yet". \result's own fallback check is then correct regardless of which sibling the source lists first.
…ata at all
An \object with only a \result fallback and no {\*\objdata ...} child
at all previously rendered \result's content with zero diagnostics --
indistinguishable from a reader that never saw the \object at all,
exactly the silent substitution SILENT_SKIP_DESTINATIONS's own policy
comment warns against. buildEmbeddedObject already reports the
"exists but fails to decode" case on its own terms, so the new
diagnostic fires only for the genuinely-no-\objdata-at-all case,
resolved via the same lookahead that resolves \objdata's own decode
outcome.
… ObjectHeader's own spec text
The test claimed the spec requires accepting FormatID 0x00000001 in
this context, citing [MS-OLEDS] 2.2.4's generic ObjectHeader
definition ("MUST be set to 0x00000001 or 0x00000002"). That range
belongs to ObjectHeader in isolation; 2.2.5's EmbeddedObject, the
structure readEmbeddedObjectData actually decodes, narrows FormatID
to 0x00000002 specifically -- a genuine 0x00000001 marks a LinkedObject
(2.2.6), whose Header is followed by NetworkName/Reserved1/
LinkUpdateOption, not NativeDataSize/NativeData. Accepting both values
here is readObjectHeader's own deliberate leniency, not something the
spec mandates for this context.
…n \objdata
[MS-OLEDS] 2.2.5's EmbeddedObject has four fields, not two: an
ObjectHeader, NativeDataSize and NativeData, then a mandatory fourth
field, Presentation ("This MUST be a MetaFilePresentationObject, a
BitmapPresentationObject, a DIBPresentationObject, a
StandardClipboardFormatPresentationObject, or a
RegisteredClipboardFormatPresentationObject"). Emitting only the first
three produces a payload a real OLE1.0 consumer reads as truncated --
it keeps looking for Presentation once NativeData ends and hits EOF.
writeEmbeddedObjectData now appends a StandardClipboardFormatPresentationObject
(the smallest of the five legitimate shapes) wrapping a minimal 1x1
monochrome CF_DIB image -- a real, decodable placeholder preview, not
size-matching filler. readEmbeddedObjectData confirms the field is
genuinely present and well-formed after NativeData decodes
successfully, rejecting a payload whose NativeData is this package's
own JSON but whose Presentation is missing or malformed, exactly as it
already rejects any other foreign shape.
Also brings the embedded-objects README section up to date with the
\result-fallback lookahead, partial-size-hint reporting, and
no-\objdata-at-all diagnostic already landed.
… via a lookahead
The \object group-start lookahead resolved \result's fate by flatly scanning
\objdata's own token range ahead of time (scanObjectDataTokens), but RTF
1.9.1's own <objdata> production is '{\*' \objdata (<objalias>? & <objsect>?)
<data> '}' -- a spec-legal \objdata can nest {\*\objalias ...}/{\*\objsect
...} sub-groups before its real payload, and the flat scan folded their
bytes into the payload it predicted from while the live read (which walks
the group tree properly) correctly skips them. The two disagreed about
whether \objdata would decode, and \result's render/discard decision was
made by whichever of them ran, producing a double render (or a wrongly
discarded object) on such input.
Removes the lookahead (scanObjectDataTokens, findChildDestinationGroup)
entirely: \result's content is now always rendered provisionally the moment
its group is seen, tracked as a block range on the enclosing \object's
shared state, and retracted at \object's own group end once \objdata's
real, live decode outcome is finally known -- correct regardless of which
of <objdata>/<result> the source lists first, without a second
implementation of the same decode to keep in sync.
Also recognises \objalias/\objsect/\objtime as \object's own legal
sub-productions (alongside \objclass/\objname) so ordinary, spec-legal
\object content no longer trips UNKNOWN_DESTINATION_SKIPPED; discards a
duplicate \objdata sibling (RTF's own grammar allows only one) into a
single block with a diagnostic instead of two identical ones; and reports
a diagnostic when an \object's group closes having found neither \objdata
nor \result, closing the one case among the three (objdata-fails /
no-objdata-with-result / no-objdata-and-no-result) that previously
produced no diagnostic at all.
… buffers \'hh is a generic RTF character escape valid anywhere in a destination's text, not only inside a destination shaped for it, so a single \objdata payload can legitimately deliver part of its data as plain #SDATA hex-digit text and the rest as scattered \'hh escapes. objectDataBytes kept the two sources in separate buffers (hex text, \'hh/\binN bytes) and picked whichever one was non-empty, silently discarding the other source whenever a payload actually used both. Collects both into one ordered byte array as the token stream is read, so a payload split between the two forms decodes in full regardless of which comes first or how many times they interleave.
…mulator
The previous design rendered \result's content directly into whatever
paragraph or block list \object itself sat in, then retracted a
[start, end) block-index range once \objdata's fate was known. That
range never matched what \result actually contributed once its
content shared live paragraph state with text around it:
- Text already accumulating before \object opened (the "before" in
`before {\object...}`) shared the same live run buffer as \result's
content, so retracting \result's own paragraph deleted that
pre-existing text along with it.
- A bare-inline \result with no trailing \par (legal per RTF 1.9.1's
`<result> = '{' \result <para>+ '}'`, since the group's closing
brace can stand in for the final paragraph's \par) closed no block
at all, so the retraction range was empty and the fallback text
stayed in the shared run buffer, bleeding into whatever paragraph
closed next.
- The same block-index confusion applied one list deeper, inside a
table cell's own cellBlocks.
- Two \result siblings (malformed, but RTF's grammar doesn't forbid
writing them) both wrote the same resultRange field, so the second
silently overwrote the first's range with no diagnostic.
\result's content now renders into a completely isolated accumulator
(ContentBuilder's beginResultScratch/endResultScratch, swapping every
field that represents "what is currently being built" -- runs, block
lists, table-building state, bookmarks -- for a fresh empty set and
back), so it shares no state with the surrounding document and is
identified by construction rather than an index range into a list
something else was concurrently writing to. \object's group-end
handling splices the finished scratch blocks in only if \objdata
never decoded, discarding them otherwise. A second \result sibling is
recognised as a duplicate and discarded with its own diagnostic,
mirroring how a second \objdata sibling is already handled.
addBlock (for a decoded \pict/\object) is folded into the same
addBlocks(blocks, inTable) the scratch splice uses, which also fixes
a related placement bug: a decoded object or picture inside a table
cell previously always landed in the section's own blocks rather than
the cell's.
The archive-codec row's own text grew when it picked up rtf-codec's Package-stream-wrapped-object mention, which shifted the column width every other row in the same table pads to. Prettier's markdown table formatter enforces consistent padding across a table, so the document-compute.js row needed the same trailing-space count restored.
… own group-end handler
cloneGroupState carries `destination`, `objectData` and `object` forward BY REFERENCE to
every descendant GroupState, including a plain, unrecognised nested group RTF's own
<obj>/<objdata> grammar does not allow but a malformed producer can still write (e.g. a
stray {\b x} sibling). The group-end handlers keyed on `state.destination ===
"objectData"`/`"object"` had no way to tell the group that actually opened the
destination apart from a descendant that merely inherited it by reference, so they fired
once per such descendant instead of once per construct.
This duplicated buildEmbeddedObject's own decoded block and its
EMBEDDED_OBJECT_UNREADABLE diagnostic, spliced \result's recovered fallback content into
the document twice, and could report a false "no \objdata and no \result" diagnostic for
an \object whose \result sibling had simply not been read yet when an earlier plain
nested group closed.
Mirrors resultOf's own existing "set on the direct child, cleared by cloneGroupState"
shape: two new boolean fields, `objectDataOwner`/`objectOwner`, are set true only where
`child.objectData`/`child.object` are freshly created, reset to false by
cloneGroupState, and gate both group-end handlers so each fires exactly once, when the
group that actually opened the destination closes.
RTF 1.9.1's own <obj> production lists <objclsid> ('{\*' \oleclsid #PCDATA
'}') as a direct, optional child of \object, right alongside <objalias>,
<objsect> and <objtime> -- all four already covered here except this one.
An ordinary, spec-legal \object carrying \oleclsid tripped
UNKNOWN_DESTINATION_SKIPPED as though it were an unrecognised destination.
Added to DESTINATION_KINDS (as a "skip", matching its siblings) and to
SILENT_SKIP_DESTINATIONS, since it is an informational \object sub-part
with no position in ContentEmbeddedObjectBlock and \object already
reports once for the whole construct.
…jh pair RTF 1.9.1's own "Objects" section defines <objsize> as a larger production (\objsetsize?, \objalignN?, \objtransyN?, <objhw>?, \objcroptN?, \objcropbN?, \objcroplN?, \objcroprN?, \objscalexN?, \objscaleyN?), not the \objw/\objh pair alone. The pair itself is a separate, narrower production: <objhw> = \objhN & \objwN.
The comment above OBJECT_HEADER_FORMAT_ID_LINKED claimed a LinkedObject FormatID "must be recognised as structurally valid" by citing 2.2.4 ObjectHeader's own generic invariant (0x00000001 or 0x00000002). That invariant is real, but it is not what governs this reader: [MS-OLEDS] 2.2.5 EmbeddedObject, the specific structure readEmbeddedObjectData decodes, narrows FormatID to "MUST be set to 0x00000002" outright. Accepting 0x00000001 here is a deliberate leniency beyond what this context's own spec section requires, not a requirement of it -- already stated correctly in embedded-object.test.ts's own "a deliberate leniency, not a spec requirement of this context" test title. The source comment now matches.
…ddedObject envelope The comment above writeEmbeddedObjectBlock and its local variable name (cfbBytes) both described writeEmbeddedObjectData's return value as "the real [MS-CFB] container", true before commit 9faee06 but not since: the function now returns a full [MS-OLEDS] EmbeddedObject structure (ObjectHeader + NativeDataSize + the compound file as NativeData + a mandatory Presentation field), with the compound file as only one field inside it. Renamed the variable to objdataBytes and reworded the comment to match what the function actually builds.
beginResultScratch() swaps the whole ContentBuilder accumulator onto resultScratchStack while \result's own fallback content renders in isolation, restored only when \result's own group closes via endResultScratch(). A truncated or unbalanced input can leave that group -- and every group around it -- open at end of input with no closing brace, so endResultScratch never runs and the swap is never undone: finish() built the document straight from the abandoned scratch state, discarding the real body accumulated before \object opened and keeping \result's own truncated text in its place instead, exactly backwards from \object's own real-content-over-fallback preference. Adds ContentBuilder.discardUnclosedResultScratches(), called from finish() before the document is built, restoring every still- suspended accumulator state (most-recently-opened first). Well-formed input always closes its own \result groups normally, so resultScratchStack is already empty by the time finish() runs and this is a no-op.
\result's own destination group inherits inTable=true by cloning \object's own para when \object sits in a table cell. A \pard inside \result's own content (RTF 1.9.1's \pard resets every paragraph property, \intbl included) resets a DESCENDANT group's own copy of that field to false, and the paragraph it closes is filed under whichever of blocks/cellBlocks that descendant's own inTable says -- not whatever \result's own outer group still (stale) says. endResultScratch then reads back from the OTHER list, finding it empty, and the whole fallback is silently lost. \object's real placement is already captured correctly and permanently in the enclosing group's own para.inTable, read again once \object's group-end decides where to splice the fallback in. \result's own scratch rendering has no reason to inherit that value: it starts a totally isolated accumulator with no real open cell of its own, so starting its own inTable at false makes the scratch's write side and endResultScratch's read side agree by construction, regardless of what \result's own body does with \pard/\intbl.
…'s kind
buildEmbeddedObject built its return value as `{ kind: "embeddedObject", ...embedded }`,
spreading the decoded payload AFTER the literal, so any "kind" key present in that payload
silently overrides the block's own discriminant. \objdata comes from an arbitrary,
potentially hostile input file: a spec-conformant ObjectHeader/NativeDataSize/NativeData/
Presentation envelope wrapping a doctored JSON payload (otherwise a valid
ContentEmbeddedObject, but also carrying its own "kind": "paragraph" plus arbitrary extra
fields) made readRtfContent emit a block whose kind and content a downstream consumer
switching on block.kind would trust, with zero diagnostics.
Swaps the spread order so the literal always wins. Separately, isContentEmbeddedObject
is a predicate over untrusted input, not a reconstructive parse -- it confirms the fields
ContentEmbeddedObject needs are present and well-shaped, but does not strip any other key
the same parsed object carries, so readEmbeddedObjectData now rebuilds its return value
from only the fields ContentEmbeddedObject actually declares rather than returning the
raw safeParse result verbatim, closing this off at the source rather than leaving every
caller to remember to strip it themselves.
… file The worked example's own trailing comment said "\objdata ... is a real [MS-CFB] compound file", directly contradicting the Embedded objects section two paragraphs above it: "the real OLE compound file is NativeData, one field inside the structure, not the whole of \objdata". \objdata's own bytes are the ObjectHeader/NativeDataSize/ NativeData/Presentation envelope as a whole; only NativeData, one field inside it, is the compound file itself.
…lback
The comment above the splice call, and its test's own comment, both
claimed \result's recovered fallback lands "in its place" with the
surrounding text surviving "untouched around it". Actual behaviour:
addBlocks flushes the pending run but never ends the paragraph
\object was sitting inside, so the fallback is appended BEFORE that
paragraph closes rather than splitting it in two -- for
"before {\object...} after", the fallback lands as its own block
ahead of one paragraph holding "before "/" after" as two merged runs,
not three blocks in "before, fallback, after" order.
The test passed regardless because it mapped paragraphs to a
"|"-joined string and asserted toContain on each fragment
independently, never asserting order. Replaced with an exact
block-by-block assertion that pins the real order and content, and
corrected both comments to describe what the code actually does.
…me picture
picture is carried forward by reference across every descendant group inside
{\pict ...} (a stray hex byte or \picwN/\pichN in a nested group must still
reach the same PictureState the real \pict destination started), but the
group-end handler that calls buildPicture had no matching ownership check --
unlike objectDataOwner/objectOwner, which guard the identical pattern for
{\*\objdata ...} and {\object ...}. A plain nested group with no destination
of its own (a malformed producer's stray "{}", not RTF's own <pict> grammar,
which has no legitimate use for one) inherits destination "picture" too, so
its own closing brace re-fired buildPicture on the identical PictureState the
outer \pict group fires on again when IT closes, doubling the image with no
diagnostic.
Adds a pictureOwner marker mirroring objectDataOwner/objectOwner exactly:
set only on the one GroupState created directly for a {\pict ...} destination,
reset to false by cloneGroupState so no descendant inherits it, and checked
alongside destination/picture in the group-end handler so buildPicture fires
once per real \pict construct regardless of what closes underneath it.
…a>/<result> ordering
RTF 1.9.1's own Formal Syntax legend gives plain juxtaposition ("AB") as
"Item A followed by item B" and reserves '&' for either order; the <obj>
production writes <objclsid>? <objdata> <result> as plain juxtaposition,
so the grammar does require <objdata> before <result>. The comments here
claimed the opposite. This reader's order-independence is still correct
robustness (the spec's own "should be robust enough to handle some minor
variations" clause covers a producer that swaps two required siblings),
so the comments now cite that instead of a permissiveness the grammar
does not actually grant.
… \result's own group
endResultScratch picked one of the scratch accumulator's two block lists
(cellBlocks or blocks) by reading \result's own group-level para.inTable,
but that flag can diverge from where a nested paragraph's content actually
landed: \intbl restated directly on \result's own group (legal per RTF
1.9.1's <result> grammar, which admits \intbl among <parfmt>* there) sets
para.inTable on that group, while a nested {\pard\plain ...} child clones
it and then \pard-resets its OWN copy back to false before closing the
paragraph -- filing it into the list endResultScratch does not read from.
The whole \result fallback was silently dropped in that shape, and the
"no \objdata" diagnostic wrongly claimed the fallback was used in its
place. Concatenate both scratch lists instead of picking one by a flag
that does not reliably say where the content ended up: beginResultScratch
starts both empty and nothing outside \result's own content can write to
either, so everything either list holds by now is \result's regardless of
which one it is. closeTable() now runs unconditionally too, matching
endSection's own unconditional call rather than gating on the same
unreliable flag.
writeCellBlocks filtered a cell's own content down to paragraphs with a bare .filter(), silently dropping any embeddedObject, image, table, or pageBreak block placed directly in a cell -- unlike every other unrepresentable construct this writer degrades, which reports through CONSTRUCT_UNREPRESENTED rather than vanishing with no trace. Sink that diagnostic for each dropped block instead, matching the pattern already used for a list membership with no minted numId and every other write-side gap.
…cells "there is no ContentDocument shape this writer refuses to embed" was true at the section level but not inside a table cell: writeCellBlocks writes paragraphs only, so an embeddedObject/image/table/pageBreak block placed directly in a cell degrades instead of embedding. Scope the sentence to outside a table cell and add the cell case to the deliberately-not-handled table, alongside the diagnostic that now reports it.
… it through knownContentEmbeddedObjectFields rebuilds readEmbeddedObjectData's result field-by-field to strip any extra key a hostile \objdata JSON payload smuggled in, but copied `source` straight through unconditionally -- isContentEmbeddedObject's own guard validates objectKind/frame/document/the four anchor fields but never inspects `source` at all, so a forged payload's `source` value reached the returned ContentEmbeddedObject exactly as written, unvalidated against SourceResidueSchema. Validate `source` with SourceResidueSchema.safeParse before including it, dropping it silently otherwise -- the same treatment every other unrecognised key already gets here.
…e cells writeCellBlocks reported every non-paragraph block placed in a table cell, constructStart/constructEnd included, as CONSTRUCT_UNREPRESENTED with a message claiming RTF's cell grammar "has no room for a nested destination that isn't one" -- true for embeddedObject/image/table/pageBreak, but constructStart/constructEnd are not a nested destination at all. They are the same zero-width bookmark brackets writeBlock already splices inline into the top-level flow, and read.ts's own cellBlockExtents/ insertConstructMarkers already reconstructs the identical pair back out of a cell's own block list, so a bookmark bracketing whole paragraphs inside a cell was already a real, round-trippable document-schema.js shape with no reader-side gap to justify dropping it on write. Route constructStart/constructEnd through the same openConstruct/ closeConstruct machinery writeBlock uses at the top level, reserving the CONSTRUCT_UNREPRESENTED diagnostic for block kinds RTF's cell grammar genuinely cannot hold. Updates the README's own description of what writeCellBlocks writes to match.
…onstruct marker writeCellBlocks defers each paragraph's \par until it knows another paragraph follows, so the last paragraph in a cell gets none. A constructStart/constructEnd marker sitting between two paragraphs skipped that flush entirely, so the deferred \par for the paragraph before it was written only once the next paragraph arrived - landing after the marker instead of before it. That puts the marker inside the RTF group for the preceding paragraph rather than between the two paragraphs, so a bookmark meant to start partway through a cell reads back widened to cover the paragraph before it too. Flush the pending \par before a marker whenever a later paragraph in the same cell still needs that separator; a trailing marker with nothing left after it flushes nothing, so it still adds no empty paragraph of its own.
…tring field asciiZeroTerminated already rejected any character above 0x7f, but a NUL byte (U+0000) passes that check since it's well within the ASCII range. label/sourcePath/tempPath are encoded as null-terminated strings, so an embedded NUL silently truncates the field at that byte and shifts every field written after it - contradicting the function's own doc comment two lines above, which states this writer round-trips whatever it is given. Reject code === 0 alongside code > 0x7f, the same class of check the validator already performs.
…y in a table cell writeCellBlocks dropped every non-paragraph block in a table cell outright, including image and embeddedObject -- a regression against the merge-base, where such an image at least escaped to the top level and survived a round trip. Since read.ts now attributes a \pict/\object read out of a cell to the cell's own block list (rather than letting it escape to the top level), the write side's drop turned into an outright loss: a \pict or \object nested inside an ordinary \intbl paragraph within a cell is a real, already-round-trippable shape this package's own reader already proves legal, so writeImagePict and writeEmbeddedObjectBlock now take the same \intbl variant writeParagraph(paragraph, inTable) already does, and writeCellBlocks routes image/embeddedObject blocks through them instead of dropping them. An image or embeddedObject block never leaves a pending run behind the way a paragraph does -- read.ts's own addBlocks (fired when the \pict/\object destination's group closes) pushes the block directly rather than going through endParagraph -- so the separator-tracking flag in writeCellBlocks only gets set after a written paragraph now, not after an image/embeddedObject; setting it unconditionally emitted a stray trailing \par that read back as a spurious empty paragraph spliced in after the image. A table or pageBreak block placed directly in a cell still has no \intbl shape to borrow (a nested table needs its own \itapN row grammar this writer does not build, and a mid-row \page would \pard-reset the row's own \intbl state), so those two kinds are still dropped -- but the diagnostic and the comment above writeCellBlocks no longer claim RTF's cell grammar has no room for a nested destination at all, since it demonstrably does for a \pict/\object group.
… only The README stated that an embeddedObject/image/table/pageBreak block placed directly in a table cell is dropped, and framed the gap as the read side's own pre-existing inverse. That was already inaccurate for the round-trip effect: this package's own \pict/\object read path already proves a picture or object group inside an ordinary \intbl paragraph is a legal, recoverable shape, so the write side's drop was a real regression once read.ts started attributing such a block to the cell rather than letting it escape to the top level, not a pre-existing symmetry with reading. Now that writeCellBlocks writes image/embeddedObject blocks through the same \intbl shell a cell paragraph gets, only a table or pageBreak block placed directly in a cell still has no such shell to borrow and is dropped.
Mearman
force-pushed
the
feat/rtf-codec-ole-embedded-objects
branch
from
September 6, 2026 16:27
cfb1bde to
061ff9e
Compare
Contributor
|
🎉 This PR is included in version 3.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #974
rtf-codec dropped every embedded object on write and every
\objectdestination on read, citing an OLE container it said this package did not build. archive-codec now ships that container (writeCompoundFile/readCompoundFile) plus thePackagestream wrapper real Word/PowerPoint embeds use inside it (readOlePackage, joined here by a newwriteOlePackagemirror) — the same pairdoc-codec/xls-codec/ppt-codecalready depend on for their own OLE container.archive-codec: addswriteOlePackage, the write-side mirror of the existingreadOlePackage.rtf-codec: adds thearchive-codecdependency and a newsrc/embedded-object.tsmodule that packages aContentEmbeddedObjectBlock's own JSON (objectKind/document/frame/anchors) as thePackagestream's file, wrapped in a real[MS-CFB]compound file, which is itself wrapped as a full [MS-OLEDS]EmbeddedObjectenvelope (ObjectHeader+NativeDataSize+ the compound file asNativeData+ a mandatoryPresentationfield). The write side (writeEmbeddedObjectBlock) emits\object\objembwith\objw/\objhsizing hints, an\*\objclasslabel, that envelope hex-encoded as\*\objdata, and a minimal\resultfallback. The read side addsobject/objectDatadestinations to the state machine (mirroringpicture's own hex/binary capture) and decodes\objdataback into aContentEmbeddedObjectBlockwhen it's this package's own payload; a real, foreign OLE object's native data degrades with a newEMBEDDED_OBJECT_UNREADABLEdiagnostic instead of throwing.EMBEDDED_OBJECT_DROPPEDis removed since it's no longer reachable.[MS-CFB]magic bytes) inwrite.test.ts/read.test.ts, an updated diagnostics-coverage fixture for the new degrade path, and a workers-runtime round trip proving thearchive-codecdependency is genuinely Worker-isomorphic, not just bundleable.\do/\shpremain a genuine schema gap, unrelated to the container fix). Root workspace README updated to match.\do/\shp(Word's native in-document drawing layer) remain unhandled — that's aContentBlockschema gap (no raw drawing-shape block for wordprocessing flow), not a container problem, so it's out of scope here.