fix(): nested duplicated clipPath causes infinite recursion

.codesandbox/templates/vanilla/package.json

@@ -20,4 +20,4 @@
     "vanilla",
     "fabric"
   ]
-}
+}

.codesandbox/templates/vanilla/src/index.ts

@@ -1,6 +1,6 @@
 import * as fabric from 'fabric';
 import './styles.css';
-import { testCase } from './testcases/simpleTextbox';
+import { testCase } from './testcases/loadingSvgs';
 
 const el = document.getElementById('canvas');
 const canvas = (window.canvas = new fabric.Canvas(el, {

.codesandbox/templates/vanilla/src/testcases/loadingSvgs.ts

@@ -1,21 +1,34 @@
 import * as fabric from 'fabric';
 
-const svgString = `<svg xmlns="http://www.w3.org/2000/svg" xmlns:svg="http://www.w3.org/2000/svg" viewBox="5 3 10 10" version="1.1" width="400" height="400">
-<g>
-    <g>
-    <clipPath id="a">
-        <circle transform="scale(1.2, 1.2) translate(-1, -1)"  r="4"  cx="8" cy="8" />
-    </clipPath>
-    <clipPath id="t" clip-path="url(#a)">
-        <circle r="6" transform="scale(1.3, 0.8) translate(1, 1)"  cx="7" cy="7" />
-    </clipPath>
-    <clipPath id="c" clip-path="url(#t)" >
-        <circle  transform="translate(12, 10) scale(14, 14)"  r="0.5"  cx="0.01" cy="0.01" />
-    </clipPath>
-<path clip-path="url(#c)" d="M15.422,18.129l-5.264-2.768l-5.265,2.768l1.006-5.863L1.64,8.114l5.887-0.855
-	l2.632-5.334l2.633,5.334l5.885,0.855l-4.258,4.152L15.422,18.129z" fill="red"/>
+const svgString = `<?xml version="1.0" encoding="UTF-8"?>
+<svg version="1.1" width="400px" height="400px" viewBox="0 0 100 100"
+    xmlns="http://www.w3.org/2000/svg">
+    <defs>
+        <clipPath id="multiShape">
+            <circle cx="30" cy="30" r="25" />
+            <rect x="50" y="10" width="40" height="40" />
+        </clipPath>
+    </defs>
+
+    <g clip-path="url(#multiShape)">
+        <g clip-path="url(#multiShape)" transform="scale(0.8)">
+            <rect
+                width="100"
+                height="100"
+                fill="purple"
+            />
+            <circle cx="100" cy= "50" r="50" fill="orange" />
+            <g transform="scale(0.8) translate(20, 10)" >
+                <rect clip-path="url(#multiShape)" transform="scale(0.8)"
+                    x="20"
+                    y="20"
+                    width="100"
+                    height="100"
+                    fill="red"
+                />
+            </g>
+        </g>
     </g>
-</g>
 </svg>`;
 
 export async function testCase(canvas: fabric.Canvas) {

CHANGELOG.md

@@ -2,6 +2,7 @@
 
 ## [next]
 
+- fix(): nested duplicated clipPath causes infinite recursion [#10774](https://github.com/fabricjs/fabric.js/pull/10774)
 - chore(): update major version of vitest [#10786](https://github.com/fabricjs/fabric.js/pull/10786)
 - fix(): Prototype pollution risk on text char cache [#10782](https://github.com/fabricjs/fabric.js/pull/10782)
 - chore(): update playwright [#10780](https://github.com/fabricjs/fabric.js/pull/10780)

e2e/tests/visual-output/rendering/testcases/svg-import.ts

@@ -13,6 +13,7 @@ const createTestFromSVG = (svgName: string): renderTestType => {
   return {
     title: `Svg import test ${svgName}`,
     golden: `${svgName}.png`,
+    only: true,
     percentage: 0.055,
     snapshotSuffix: 'svg-import',
     size: [100, 100],
@@ -101,6 +102,11 @@ const svgFiles = [
   'use-svg-style-2',
   'svg_text_underline_thick',
   'coords-viewattr-02-b',
+  // Nested clipPath tests for infinite recursion fix (https://github.com/fabricjs/fabric.js/issues/10659)
+  'nested-clippath-simple',
+  'nested-clippath-triple',
+  'nested-clippath-different',
+  'nested-clippath-complex',
 ];
 
 export const svgImportTests: renderTestType[] = svgFiles.map(createTestFromSVG);

src/parser/elements_parser.spec.ts

@@ -0,0 +1,151 @@
+import { loadSVGFromString } from '../../fabric';
+import { describe, it, expect } from 'vitest';
+
+describe('ElementsParser', () => {
+  describe('clipPath handling', () => {
+    it('should not enter infinite loop with nested duplicated clipPath', async () => {
+      const svg = `<?xml version="1.0" encoding="UTF-8"?>
+<svg version="1.1" width="100px" height="100px" viewBox="0 0 100 100"
+    xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
+    <defs>
+        <clipPath id="i0">
+            <circle cx="0" cy="0" r="50" />
+        </clipPath>
+    </defs>
+
+    <g clip-path="url(#i0)">
+        <g clip-path="url(#i0)">
+            <rect
+                width="50"
+                height="50"
+                fill="red"
+            />
+        </g>
+    </g>
+</svg>`;
+      const { objects } = await loadSVGFromString(svg);
+      expect(objects).toHaveLength(1);
+      expect(objects[0]!.clipPath).toBeDefined();
+    });
+
+    it('should handle triple nested clipPath references', async () => {
+      const svg = `<?xml version="1.0" encoding="UTF-8"?>
+<svg version="1.1" width="100px" height="100px" viewBox="0 0 100 100"
+    xmlns="http://www.w3.org/2000/svg">
+    <defs>
+        <clipPath id="clip1">
+            <circle cx="50" cy="50" r="40" />
+        </clipPath>
+    </defs>
+
+    <g clip-path="url(#clip1)">
+        <g clip-path="url(#clip1)">
+            <g clip-path="url(#clip1)">
+                <rect
+                    width="80"
+                    height="80"
+                    fill="blue"
+                />
+            </g>
+        </g>
+    </g>
+</svg>`;
+      const { objects } = await loadSVGFromString(svg);
+      expect(objects).toHaveLength(1);
+      expect(objects[0]!.clipPath).toBeDefined();
+    });
+
+    it('should handle different clipPaths at different levels', async () => {
+      const svg = `<?xml version="1.0" encoding="UTF-8"?>
+<svg version="1.1" width="100px" height="100px" viewBox="0 0 100 100"
+    xmlns="http://www.w3.org/2000/svg">
+    <defs>
+        <clipPath id="clip1">
+            <circle cx="50" cy="50" r="40" />
+        </clipPath>
+        <clipPath id="clip2">
+            <rect x="10" y="10" width="80" height="80" />
+        </clipPath>
+    </defs>
+
+    <g clip-path="url(#clip1)">
+        <g clip-path="url(#clip2)">
+            <rect
+                width="60"
+                height="60"
+                fill="green"
+            />
+        </g>
+    </g>
+</svg>`;
+      const { objects } = await loadSVGFromString(svg);
+      expect(objects).toHaveLength(1);
+      expect(objects[0]!.clipPath).toBeDefined();
+    });
+
+    it('should handle clipPath with multiple shapes', async () => {
+      const svg = `<?xml version="1.0" encoding="UTF-8"?>
+<svg version="1.1" width="200px" height="200px" viewBox="0 0 200 200"
+    xmlns="http://www.w3.org/2000/svg">
+    <defs>
+        <clipPath id="multiShape">
+            <circle cx="50" cy="50" r="40" />
+            <rect x="100" y="0" width="50" height="50" />
+        </clipPath>
+    </defs>
+
+    <g clip-path="url(#multiShape)">
+        <g clip-path="url(#multiShape)">
+            <rect
+                width="200"
+                height="200"
+                fill="purple"
+            />
+        </g>
+    </g>
+</svg>`;
+      const { objects } = await loadSVGFromString(svg);
+      expect(objects).toHaveLength(1);
+      expect(objects[0]!.clipPath).toBeDefined();
+    });
+
+    it('should handle missing clipPath gracefully', async () => {
+      const svg = `<?xml version="1.0" encoding="UTF-8"?>
+<svg version="1.1" width="100px" height="100px" viewBox="0 0 100 100"
+    xmlns="http://www.w3.org/2000/svg">
+    <g clip-path="url(#nonExistent)">
+        <rect
+            width="50"
+            height="50"
+            fill="orange"
+        />
+    </g>
+</svg>`;
+      const { objects } = await loadSVGFromString(svg);
+      expect(objects).toHaveLength(1);
+      expect(objects[0]!.clipPath).toBeUndefined();
+    });
+
+    it('should handle clipPath on element directly', async () => {
+      const svg = `<?xml version="1.0" encoding="UTF-8"?>
+<svg version="1.1" width="100px" height="100px" viewBox="0 0 100 100"
+    xmlns="http://www.w3.org/2000/svg">
+    <defs>
+        <clipPath id="directClip">
+            <circle cx="50" cy="50" r="45" />
+        </clipPath>
+    </defs>
+
+    <rect
+        clip-path="url(#directClip)"
+        width="100"
+        height="100"
+        fill="yellow"
+    />
+</svg>`;
+      const { objects } = await loadSVGFromString(svg);
+      expect(objects).toHaveLength(1);
+      expect(objects[0]!.clipPath).toBeDefined();
+    });
+  });
+});

src/parser/elements_parser.ts

@@ -30,9 +30,7 @@
 };
 
 type NotParsedFabricObject = FabricObject & {
-  fill: string;
-  stroke: string;
-  clipPath?: string;
+  clipPath?: string | FabricObject['clipPath'];
   clipRule?: CanvasFillRule;
 };
 
@@ -94,13 +92,26 @@
     return null;
   }
 
+  extractPropertyDefinition(
+    obj: NotParsedFabricObject,
+    property: 'fill' | 'stroke',
+    storage: Record<string, SVGGradientElement>,
+  ): { def: SVGGradientElement; id: string } | undefined;
+  extractPropertyDefinition(
+    obj: NotParsedFabricObject,
+    property: 'clipPath',
+    storage: Record<string, Element[]>,
+  ): { def: Element[]; id: string } | undefined;
   extractPropertyDefinition(
     obj: NotParsedFabricObject,
     property: 'fill' | 'stroke' | 'clipPath',
     storage: Record<string, StorageType[typeof property]>,
-  ): StorageType[typeof property] | undefined {
-    const value = obj[property]!,
-      regex = this.regexUrl;
+  ): { def: StorageType[typeof property]; id: string } | undefined {
+    const value = obj[property];
+    if (typeof value !== 'string') {
+      return undefined;
+    }
+    const regex = this.regexUrl;
     if (!regex.test(value)) {
       return undefined;
     }
@@ -110,22 +121,22 @@
     const id = regex.exec(value)![1];
     regex.lastIndex = 0;
     // @todo fix this
-    return storage[id];
+    return storage[id] ? { def: storage[id], id } : undefined;
   }
 
   resolveGradient(
     obj: NotParsedFabricObject,
     el: Element,
     property: 'fill' | 'stroke',
   ) {
-    const gradientDef = this.extractPropertyDefinition(
+    const gradientDefinition = this.extractPropertyDefinition(
       obj,
       property,
       this.gradientDefs,
-    ) as SVGGradientElement;
-    if (gradientDef) {
+    );
+    if (gradientDefinition) {
       const opacityAttr = el.getAttribute(property + '-opacity');
-      const gradient = Gradient.fromElement(gradientDef, obj, {
+      const gradient = Gradient.fromElement(gradientDefinition.def, obj, {
         ...this.options,
         opacity: opacityAttr,
       } as SVGOptions);
@@ -139,15 +150,19 @@
     obj: NotParsedFabricObject,
     usingElement: Element,
     exactOwner?: Element,
+    processedClipPaths: Set<string> = new Set(),
   ) {
-    const clipPathElements = this.extractPropertyDefinition(
+    const clipPathDefinition = this.extractPropertyDefinition(
       obj,
       'clipPath',
       this.clipPaths,
-    ) as Element[];
-    if (clipPathElements) {
+    );
+    if (clipPathDefinition) {
+      const clipPathElements = clipPathDefinition.def;
       const objTransformInv = invertTransform(obj.calcTransformMatrix());
-      const clipPathTag = clipPathElements[0].parentElement!;
+      const clipPathTag = clipPathElements[0].parentElement!.cloneNode(
+        true,
+      ) as HTMLElement;
       let clipPathOwner = usingElement;
       while (
         !exactOwner &&
@@ -163,6 +178,7 @@
       // but i don't have an svg to test it
       // at the first SVG that has a transform on both places and is misplaced
       // try to invert this multiplication order
+      console.log(clipPathOwner.getAttribute('transform'));
       const finalTransform = parseTransformAttribute(
         `${clipPathOwner.getAttribute('transform') || ''} ${
           clipPathTag.getAttribute('originalTransform') || ''
@@ -174,16 +190,28 @@
         `matrix(${finalTransform.join(',')})`,
       );
 
+      const updatedProcessedClipPaths = new Set(processedClipPaths);
+      updatedProcessedClipPaths.add(clipPathDefinition.id);
+
       const container = await Promise.all(
-        clipPathElements.map((clipPathElement) => {
-          return findTag(clipPathElement)
-            .fromElement(clipPathElement, this.options, this.cssRules)
-            .then((enlivedClippath: NotParsedFabricObject) => {
-              removeTransformMatrixForSvgParsing(enlivedClippath);
-              enlivedClippath.fillRule = enlivedClippath.clipRule!;
-              delete enlivedClippath.clipRule;
-              return enlivedClippath;
-            });
+        clipPathElements.map(async (clipPathElement) => {
+          const enlivedClippath: NotParsedFabricObject = await findTag(
+            clipPathElement,
+          ).fromElement(clipPathElement, this.options, this.cssRules);
+          removeTransformMatrixForSvgParsing(enlivedClippath);
+          enlivedClippath.fillRule = enlivedClippath.clipRule!;
+          delete enlivedClippath.clipRule;
+
+          // Resolve clipPath on elements inside the clipPath definition
+          // This prevents nested elements from having unresolved clipPath strings
+          await this.resolveClipPath(
+            enlivedClippath,
+            clipPathElement,
+            undefined,
+            updatedProcessedClipPaths,
+          );
+
+          return enlivedClippath;
         }),
       );
       const clipPath =
@@ -192,6 +220,7 @@
         objTransformInv,
         clipPath.calcTransformMatrix(),
       );
+      // Only resolve if clipPath property is still a string (not already resolved)
       if (clipPath.clipPath) {
         await this.resolveClipPath(
           clipPath,
@@ -200,6 +229,7 @@
           // 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,
+          updatedProcessedClipPaths,
         );
       }
       const { scaleX, scaleY, angle, skewX, translateX, translateY } =