diff --git a/CHANGELOG.md b/CHANGELOG.md index 917d3e06a6d..9142fcfea91 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## [next] +- fix(): nested duplicated clipPath causes infinite recursion [#10667](https://github.com/fabricjs/fabric.js/pull/10667) - chore(): update playwright [#10657](https://github.com/fabricjs/fabric.js/pull/10657) - refactor(): swap lodash with es-toolkit [#10651](https://github.com/fabricjs/fabric.js/pull/10651) - chore(): update vitest [#10648](https://github.com/fabricjs/fabric.js/pull/10648) diff --git a/src/parser/elements_parser.spec.ts b/src/parser/elements_parser.spec.ts new file mode 100644 index 00000000000..04fbfddf1ae --- /dev/null +++ b/src/parser/elements_parser.spec.ts @@ -0,0 +1,70 @@ +import { Gradient, loadSVGFromString } from '../../fabric'; +import { describe, it, expect } from 'vitest'; + +describe('Parser', () => { + it('should not enter an infinite loop with nested duplicated clipPath', async () => { + const svg = ` + + + + + + + + + + + + +`; + const { objects } = await loadSVGFromString(svg); + expect(objects).toHaveLength(1); + }); + + it('should not throw when gradient is missing', async () => { + const svg = ` + + + + +`; + const { objects } = await loadSVGFromString(svg); + expect(objects).toHaveLength(1); + expect(typeof objects[0]!.fill).toBe('string'); + expect(objects[0]!.fill).toBe('red'); + }); + + it('should correctly parse a valid gradient', async () => { + const svg = ` + + + + + + + + +`; + const { objects } = await loadSVGFromString(svg); + expect(objects).toHaveLength(1); + const rect = objects[0]; + expect(rect!.fill).toBeInstanceOf(Gradient); + const gradient = rect!.fill as Gradient<'linear'>; + expect(gradient.type).toBe('linear'); + expect(gradient.colorStops).toHaveLength(2); + const colors = gradient.colorStops.map((cs) => cs.color); + expect(colors).toContain('rgba(255,0,0,1)'); + expect(colors).toContain('rgba(0,0,255,1)'); + }); +}); diff --git a/src/parser/elements_parser.ts b/src/parser/elements_parser.ts index 4b801cc3bc1..1597d5d6917 100644 --- a/src/parser/elements_parser.ts +++ b/src/parser/elements_parser.ts @@ -94,11 +94,21 @@ export class ElementsParser { return null; } + extractPropertyDefinition( + obj: NotParsedFabricObject, + property: 'fill' | 'stroke', + storage: Record, + ): { def: SVGGradientElement; id: string } | undefined; + extractPropertyDefinition( + obj: NotParsedFabricObject, + property: 'clipPath', + storage: Record, + ): { def: Element[]; id: string } | undefined; extractPropertyDefinition( obj: NotParsedFabricObject, property: 'fill' | 'stroke' | 'clipPath', storage: Record, - ): StorageType[typeof property] | undefined { + ): { def: StorageType[typeof property]; id: string } | undefined { const value = obj[property]!, regex = this.regexUrl; if (!regex.test(value)) { @@ -110,7 +120,7 @@ export class ElementsParser { const id = regex.exec(value)![1]; regex.lastIndex = 0; // @todo fix this - return storage[id]; + return { def: storage[id], id }; } resolveGradient( @@ -122,10 +132,10 @@ export class ElementsParser { obj, property, this.gradientDefs, - ) as SVGGradientElement; - if (gradientDef) { + ); + if (gradientDef && gradientDef.def) { const opacityAttr = el.getAttribute(property + '-opacity'); - const gradient = Gradient.fromElement(gradientDef, obj, { + const gradient = Gradient.fromElement(gradientDef.def, obj, { ...this.options, opacity: opacityAttr, } as SVGOptions); @@ -139,13 +149,19 @@ export class ElementsParser { obj: NotParsedFabricObject, usingElement: Element, exactOwner?: Element, + processedClipPaths: string[] = [], ) { - const clipPathElements = this.extractPropertyDefinition( + const clipPathDefinition = this.extractPropertyDefinition( obj, 'clipPath', this.clipPaths, - ) as Element[]; - if (clipPathElements) { + ); + if ( + clipPathDefinition && + clipPathDefinition.def && + !processedClipPaths.includes(clipPathDefinition.id) + ) { + const clipPathElements = clipPathDefinition.def; const objTransformInv = invertTransform(obj.calcTransformMatrix()); const clipPathTag = clipPathElements[0].parentElement!; let clipPathOwner = usingElement; @@ -199,7 +215,8 @@ export class ElementsParser { // this is tricky. // it tries to differentiate from when clipPaths are inherited by outside groups // or when are really clipPaths referencing other clipPaths - clipPathTag.getAttribute('clip-path') ? clipPathOwner : undefined, + exactOwner || usingElement, + [...processedClipPaths, clipPathDefinition.id], ); } const { scaleX, scaleY, angle, skewX, translateX, translateY } =