Skip to content

Commit 9c7fc2e

Browse files
caio-pizzolcaio-pizzol
authored
fix: guard drawing export against invalid structures and zero IDs (SD-824) (#2363)
* fix: guard drawing export against invalid structures and zero IDs (SD-824) - Add guard in drawing-translator decode to skip w:drawing wrap when child is not wp:inline or wp:anchor (prevents corrupt OOXML) - Add guard in translateInlineNode to bail when translateImageNode returns a non-drawing result (e.g. text fallback for invalid signatures) - Add same guard in translateAnchorNode for parity - Generate positive random IDs for wp:docPr and pic:cNvPr instead of defaulting to 0 (violates OOXML uniqueness requirement per §20.4.2.5) - Ensure both elements share the same generated ID per image * fix(export): patch zero drawing IDs in preserved original children (SD-824) mergeDrawingChildren prefers original elements over generated ones for round-trip fidelity, but originals may carry invalid id="0" on wp:docPr and pic:cNvPr that Word rejects. Post-process the merged output to patch zero IDs using the valid ID from the generated wp:docPr.
1 parent 0132d0e commit 9c7fc2e

9 files changed

Lines changed: 341 additions & 17 deletions

File tree

‎packages/super-editor/src/core/super-converter/v3/handlers/w/drawing/drawing-translator.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,11 @@ function decode(params) {
7070
const childTranslator = node.attrs.isAnchor ? wpAnchorTranslator : wpInlineTranslator;
7171
const resultNode = childTranslator.decode(params);
7272

73+
// Guard: only wrap when we have valid drawing content.
74+
if (!resultNode || (resultNode.name !== 'wp:inline' && resultNode.name !== 'wp:anchor')) {
75+
return resultNode;
76+
}
77+
7378
return wrapTextInRun(
7479
{
7580
name: 'w:drawing',

‎packages/super-editor/src/core/super-converter/v3/handlers/w/drawing/drawing-translator.test.js‎

Lines changed: 52 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -95,38 +95,82 @@ describe('w:drawing translator', () => {
9595
});
9696

9797
describe('decode', () => {
98-
it('delegates to wp:anchor when node.attrs.isAnchor is true', () => {
98+
it('wraps valid wp:anchor child in w:drawing', () => {
9999
const params = { node: { type: 'element', attrs: { isAnchor: true } } };
100-
anchorTranslatorMock.decode.mockReturnValue({ decoded: 'anchor' });
100+
anchorTranslatorMock.decode.mockReturnValue({ name: 'wp:anchor', decoded: 'anchor' });
101101

102102
const result = translator.decode(params);
103103

104104
expect(anchorTranslatorMock.decode).toHaveBeenCalledWith(params);
105105
expect(wrapTextInRun).toHaveBeenCalledWith(
106106
expect.objectContaining({
107107
name: 'w:drawing',
108-
elements: [{ decoded: 'anchor' }],
108+
elements: [expect.objectContaining({ name: 'wp:anchor' })],
109109
}),
110110
[],
111111
);
112-
expect(result).toEqual({ wrapped: { name: 'w:drawing', elements: [{ decoded: 'anchor' }] } });
112+
expect(result).toEqual({
113+
wrapped: {
114+
name: 'w:drawing',
115+
elements: [expect.objectContaining({ name: 'wp:anchor' })],
116+
},
117+
});
113118
});
114119

115-
it('delegates to wp:inline when node.attrs.isAnchor is false', () => {
120+
it('wraps valid wp:inline child in w:drawing', () => {
116121
const params = { node: { type: 'element', attrs: { isAnchor: false } } };
117-
inlineTranslatorMock.decode.mockReturnValue({ decoded: 'inline' });
122+
inlineTranslatorMock.decode.mockReturnValue({ name: 'wp:inline', decoded: 'inline' });
118123

119124
const result = translator.decode(params);
120125

121126
expect(inlineTranslatorMock.decode).toHaveBeenCalledWith(params);
122127
expect(wrapTextInRun).toHaveBeenCalledWith(
123128
expect.objectContaining({
124129
name: 'w:drawing',
125-
elements: [{ decoded: 'inline' }],
130+
elements: [expect.objectContaining({ name: 'wp:inline' })],
126131
}),
127132
[],
128133
);
129-
expect(result).toEqual({ wrapped: { name: 'w:drawing', elements: [{ decoded: 'inline' }] } });
134+
expect(result).toEqual({
135+
wrapped: {
136+
name: 'w:drawing',
137+
elements: [expect.objectContaining({ name: 'wp:inline' })],
138+
},
139+
});
140+
});
141+
142+
it('returns child as-is when anchor child is not a drawing node', () => {
143+
const params = { node: { type: 'element', attrs: { isAnchor: true } } };
144+
const textRun = { name: 'w:r', elements: [{ name: 'w:t', elements: [{ text: 'x' }] }] };
145+
anchorTranslatorMock.decode.mockReturnValue(textRun);
146+
147+
const result = translator.decode(params);
148+
149+
expect(anchorTranslatorMock.decode).toHaveBeenCalledWith(params);
150+
expect(wrapTextInRun).not.toHaveBeenCalled();
151+
expect(result).toEqual(textRun);
152+
});
153+
154+
it('returns child as-is when inline child is not a drawing node', () => {
155+
const params = { node: { type: 'element', attrs: { isAnchor: false } } };
156+
const textRun = { name: 'w:r', elements: [{ name: 'w:t', elements: [{ text: 'y' }] }] };
157+
inlineTranslatorMock.decode.mockReturnValue(textRun);
158+
159+
const result = translator.decode(params);
160+
161+
expect(inlineTranslatorMock.decode).toHaveBeenCalledWith(params);
162+
expect(wrapTextInRun).not.toHaveBeenCalled();
163+
expect(result).toEqual(textRun);
164+
});
165+
166+
it('returns null when child translator returns null', () => {
167+
const params = { node: { type: 'element', attrs: { isAnchor: false } } };
168+
inlineTranslatorMock.decode.mockReturnValue(null);
169+
170+
const result = translator.decode(params);
171+
172+
expect(wrapTextInRun).not.toHaveBeenCalled();
173+
expect(result).toBeNull();
130174
});
131175

132176
it('returns null if node is missing', () => {

‎packages/super-editor/src/core/super-converter/v3/handlers/wp/anchor/helpers/translate-anchor-node.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,11 @@ export function translateAnchorNode(params) {
6868

6969
const nodeElements = translateImageNode(params);
7070

71+
// Guard: bail out if translateImageNode produced a non-drawing result (e.g. text fallback).
72+
if (!nodeElements?.elements?.some((el) => el?.name === 'wp:extent')) {
73+
return nodeElements;
74+
}
75+
7176
const inlineAttrs = {
7277
...(attrs.originalAttributes || {}),
7378
...(nodeElements.attributes || {}),

‎packages/super-editor/src/core/super-converter/v3/handlers/wp/anchor/helpers/translate-anchor-node.test.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,10 @@ describe('translateAnchorNode', () => {
1515
beforeEach(() => {
1616
vi.clearAllMocks();
1717

18-
// default mock for translateImageNode
18+
// default mock for translateImageNode — must include wp:extent so the guard passes
1919
translateImageNode.mockReturnValue({
2020
attributes: { fakeAttr: 'val' },
21-
elements: [{ name: 'pic:fake' }],
21+
elements: [{ name: 'wp:extent' }, { name: 'wp:effectExtent' }, { name: 'pic:fake' }],
2222
});
2323

2424
// default mock for pixelsToEmu

‎packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/decode-image-node-helpers.js‎

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -37,9 +37,9 @@ function buildHlinkClickElement(attrs, hlinkRId) {
3737
* - `a:hlinkClick` child when hyperlink is set (Word's canonical placement per §20.4.2.5)
3838
* - Decorative extension child when attrs.decorative is true
3939
*/
40-
function buildDocPrElement(attrs, imageName, hlinkRId) {
40+
function buildDocPrElement(attrs, imageName, hlinkRId, drawingId) {
4141
const docPrAttrs = {
42-
id: attrs.id || 0,
42+
id: drawingId,
4343
name: attrs.alt || `Picture ${imageName}`,
4444
};
4545
// Emit descr (accessibility description) unless decorative
@@ -84,7 +84,7 @@ function buildDocPrElement(attrs, imageName, hlinkRId) {
8484
* - `a:hlinkClick` child when hyperlink is set (mirrors wp:docPr for compatibility)
8585
* - `a:picLocks/@noChangeAspect` ← dynamic from attrs.lockAspectRatio
8686
*/
87-
function buildNvPicPrElement(attrs, imageName, hlinkRId) {
87+
function buildNvPicPrElement(attrs, imageName, hlinkRId, drawingId) {
8888
// --- pic:cNvPr children (hyperlink) ---
8989
const cNvPrChildren = [];
9090
const hlinkEl = buildHlinkClickElement(attrs, hlinkRId);
@@ -96,7 +96,7 @@ function buildNvPicPrElement(attrs, imageName, hlinkRId) {
9696
{
9797
name: 'pic:cNvPr',
9898
attributes: {
99-
id: attrs.id || 0,
99+
id: drawingId,
100100
name: attrs.alt || `Picture ${imageName}`,
101101
},
102102
...(cNvPrChildren.length ? { elements: cNvPrChildren } : {}),
@@ -295,6 +295,9 @@ export const translateImageNode = (params) => {
295295
// Resolve hyperlink relationship once; shared by wp:docPr and pic:cNvPr.
296296
const hlinkRId = resolveHyperlinkRId(attrs, params);
297297

298+
// Ensure valid positive docPr/cNvPr IDs (OOXML requires id > 0).
299+
const drawingId = attrs.id && Number(attrs.id) > 0 ? attrs.id : Math.max(1, parseInt(generateDocxRandomId(), 16));
300+
298301
return {
299302
attributes: inlineAttrs,
300303
elements: [
@@ -309,7 +312,7 @@ export const translateImageNode = (params) => {
309312
name: 'wp:effectExtent',
310313
attributes: effectExtentAttrs,
311314
},
312-
buildDocPrElement(attrs, imageName, hlinkRId),
315+
buildDocPrElement(attrs, imageName, hlinkRId, drawingId),
313316
{
314317
name: 'wp:cNvGraphicFramePr',
315318
elements: [
@@ -334,7 +337,7 @@ export const translateImageNode = (params) => {
334337
name: 'pic:pic',
335338
attributes: { 'xmlns:pic': pictureXmlns },
336339
elements: [
337-
buildNvPicPrElement(attrs, imageName, hlinkRId),
340+
buildNvPicPrElement(attrs, imageName, hlinkRId, drawingId),
338341
{
339342
name: 'pic:blipFill',
340343
elements: [

‎packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/merge-drawing-children.js‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,8 +15,13 @@ import { carbonCopy } from '@core/utilities/carbonCopy.js';
1515
export function mergeDrawingChildren({ order, generated, original }) {
1616
const genQueues = groupByName(generated);
1717
const originalsByIndex = groupByIndex(original);
18+
const merged = mergeWithOrder(order, genQueues, originalsByIndex);
1819

19-
return mergeWithOrder(order, genQueues, originalsByIndex);
20+
// Originals may carry invalid IDs (e.g. id="0") that Word rejects.
21+
// Patch them using the valid ID from the generated wp:docPr.
22+
fixZeroDrawingIds(merged, generated);
23+
24+
return merged;
2025
}
2126

2227
function groupByIndex(entries = []) {
@@ -70,6 +75,7 @@ function mergeWithOrder(order = [], genQueues, originalsByIndex) {
7075

7176
return out;
7277
}
78+
7379
function groupByName(nodes = []) {
7480
const map = new Map();
7581
nodes.forEach((el) => {
@@ -80,3 +86,28 @@ function groupByName(nodes = []) {
8086
});
8187
return map;
8288
}
89+
90+
/**
91+
* Patch zero/missing IDs on wp:docPr and pic:cNvPr in merged output.
92+
* When the merge prefers an original element, it may carry an invalid id="0"
93+
* that Word rejects. We fix it using the valid ID from the generated wp:docPr.
94+
*/
95+
function fixZeroDrawingIds(merged, generated) {
96+
const genDocPr = generated?.find((el) => el?.name === 'wp:docPr');
97+
const validId = genDocPr?.attributes?.id;
98+
if (!validId || !(Number(validId) > 0)) return;
99+
100+
const docPr = merged.find((el) => el?.name === 'wp:docPr');
101+
if (docPr?.attributes && !(Number(docPr.attributes.id) > 0)) {
102+
docPr.attributes.id = validId;
103+
}
104+
105+
const graphic = merged.find((el) => el?.name === 'a:graphic');
106+
const graphicData = graphic?.elements?.find((el) => el?.name === 'a:graphicData');
107+
const pic = graphicData?.elements?.find((el) => el?.name === 'pic:pic');
108+
const nvPicPr = pic?.elements?.find((el) => el?.name === 'pic:nvPicPr');
109+
const cNvPr = nvPicPr?.elements?.find((el) => el?.name === 'pic:cNvPr');
110+
if (cNvPr?.attributes && !(Number(cNvPr.attributes.id) > 0)) {
111+
cNvPr.attributes.id = validId;
112+
}
113+
}

‎packages/super-editor/src/core/super-converter/v3/handlers/wp/helpers/merge-drawing-children.test.js‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,111 @@ describe('mergeDrawingChildren', () => {
204204
});
205205
});
206206

207+
describe('zero drawing ID fix', () => {
208+
it('patches wp:docPr id=0 using the generated ID', () => {
209+
const result = mergeDrawingChildren({
210+
order: ['wp:extent', 'wp:docPr', 'a:graphic'],
211+
generated: [
212+
{ name: 'wp:extent', attributes: { cx: 100 } },
213+
{ name: 'wp:docPr', attributes: { id: 42, name: 'Picture 1' } },
214+
{ name: 'a:graphic', attributes: {} },
215+
],
216+
original: [
217+
{ index: 1, xml: { name: 'wp:docPr', attributes: { id: 0, name: 'Picture 1', descr: 'alt text' } } },
218+
{ index: 2, xml: { name: 'a:graphic', attributes: {} } },
219+
],
220+
});
221+
222+
const docPr = result.find((el) => el.name === 'wp:docPr');
223+
expect(docPr.attributes.id).toBe(42);
224+
expect(docPr.attributes.descr).toBe('alt text');
225+
});
226+
227+
it('patches pic:cNvPr id=0 inside original a:graphic subtree', () => {
228+
const result = mergeDrawingChildren({
229+
order: ['wp:extent', 'wp:docPr', 'a:graphic'],
230+
generated: [
231+
{ name: 'wp:extent', attributes: { cx: 100 } },
232+
{ name: 'wp:docPr', attributes: { id: 7 } },
233+
{
234+
name: 'a:graphic',
235+
elements: [
236+
{
237+
name: 'a:graphicData',
238+
elements: [
239+
{
240+
name: 'pic:pic',
241+
elements: [
242+
{
243+
name: 'pic:nvPicPr',
244+
elements: [{ name: 'pic:cNvPr', attributes: { id: 7 } }],
245+
},
246+
],
247+
},
248+
],
249+
},
250+
],
251+
},
252+
],
253+
original: [
254+
{ index: 1, xml: { name: 'wp:docPr', attributes: { id: 0 } } },
255+
{
256+
index: 2,
257+
xml: {
258+
name: 'a:graphic',
259+
elements: [
260+
{
261+
name: 'a:graphicData',
262+
elements: [
263+
{
264+
name: 'pic:pic',
265+
elements: [
266+
{
267+
name: 'pic:nvPicPr',
268+
elements: [{ name: 'pic:cNvPr', attributes: { id: 0, name: 'Original' } }],
269+
},
270+
],
271+
},
272+
],
273+
},
274+
],
275+
},
276+
},
277+
],
278+
});
279+
280+
const graphic = result.find((el) => el.name === 'a:graphic');
281+
const cNvPr = graphic.elements[0].elements[0].elements[0].elements[0];
282+
expect(cNvPr.attributes.id).toBe(7);
283+
expect(cNvPr.attributes.name).toBe('Original');
284+
});
285+
286+
it('does not overwrite valid positive IDs on originals', () => {
287+
const result = mergeDrawingChildren({
288+
order: ['wp:extent', 'wp:docPr'],
289+
generated: [
290+
{ name: 'wp:extent', attributes: {} },
291+
{ name: 'wp:docPr', attributes: { id: 99 } },
292+
],
293+
original: [{ index: 1, xml: { name: 'wp:docPr', attributes: { id: 5 } } }],
294+
});
295+
296+
const docPr = result.find((el) => el.name === 'wp:docPr');
297+
expect(docPr.attributes.id).toBe(5);
298+
});
299+
300+
it('handles missing generated wp:docPr gracefully', () => {
301+
const result = mergeDrawingChildren({
302+
order: ['wp:extent', 'wp:docPr'],
303+
generated: [{ name: 'wp:extent', attributes: {} }],
304+
original: [{ index: 1, xml: { name: 'wp:docPr', attributes: { id: 0 } } }],
305+
});
306+
307+
const docPr = result.find((el) => el.name === 'wp:docPr');
308+
expect(docPr.attributes.id).toBe(0);
309+
});
310+
});
311+
207312
describe('deep copy behavior', () => {
208313
it('returns deep copies, not references to original objects', () => {
209314
const originalXml = { name: 'wp:docPr', attributes: { id: 1 } };

‎packages/super-editor/src/core/super-converter/v3/handlers/wp/inline/helpers/translate-inline-node.js‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,11 @@ export function translateInlineNode(params) {
1010
const { attrs } = params.node;
1111
const nodeElements = translateImageNode(params);
1212

13+
// Guard: bail out if translateImageNode produced a non-drawing result (e.g. text fallback).
14+
if (!nodeElements?.elements?.some((el) => el?.name === 'wp:extent')) {
15+
return nodeElements;
16+
}
17+
1318
const inlineAttrs = {
1419
...(attrs.originalAttributes || {}),
1520
...(nodeElements.attributes || {}),

0 commit comments

Comments
 (0)