Improve SVG validation

.changeset/wise-eyes-warn.md

@@ -0,0 +1,21 @@
+---
+"@comet/cms-api": patch
+---
+
+Improve SVG validation
+
+Following tags are banned in SVGs:
+
+-   script
+-   \[new\] foreignObject
+-   \[new\] use
+-   \[new\] image
+-   \[new\] animate
+-   \[new\] animateMotion
+-   \[new\] animateTransform
+-   \[new\] set
+
+Following attributes are banned:
+
+-   Event handlers (`onload`, `onclick`, ...)
+-   \[new\] `href` and `xlink:href` (if the value starts with `http://`, `https://` or `javascript:`)

packages/admin/cms-admin/src/dam/DataGrid/fileUpload/fileUploadErrorMessages.tsx

@@ -77,8 +77,8 @@ export const FileExtensionTypeMismatchError = ({ extension, mimetype }: FileExte
 
 export const SvgContainsJavaScriptError = () => (
     <FormattedMessage
-        id="comet.file.errors.svgContainsJavaScript"
-        defaultMessage="<strong>The SVG contains JavaScript.</strong> JavaScript is not allowed inside SVG files. Please remove all JavaScript code from the file."
+        id="comet.file.errors.svgContainsForbiddenContent"
+        defaultMessage="<strong>The SVG contains forbidden content.</strong> The SVG must not contain JavaScript or security-relevant tags or attributes."
         values={{
             strong: formatStrong,
         }}

packages/admin/cms-admin/src/dam/DataGrid/fileUpload/useDamFileUpload.tsx

@@ -442,7 +442,7 @@ export const useDamFileUpload = (options: UploadDamFileOptions): FileUploadApi =
                         } else {
                             addValidationError(file, <UnknownError />);
                         }
-                    } else if (typedErr.response?.data.message === "Rejected File Upload: SVG must not contain JavaScript") {
+                    } else if (typedErr.response?.data.message.includes("SVG contains forbidden content")) {
                         addValidationError(file, <SvgContainsJavaScriptError />);
                     } else if (typedErr.response === undefined && typedErr.request) {
                         addValidationError(file, <NetworkError />);

packages/api/cms-api/src/dam/files/file-validation.service.ts

@@ -1,7 +1,7 @@
 import { readFile } from "fs/promises";
 
 import { FileUploadInput } from "./dto/file-upload.input";
-import { getValidExtensionsForMimetype, svgContainsJavaScript } from "./files.utils";
+import { getValidExtensionsForMimetype, isValidSvg } from "./files.utils";
 
 export class FileValidationService {
     constructor(public config: { maxFileSize: number; acceptedMimeTypes: string[] }) {}
@@ -45,8 +45,8 @@ export class FileValidationService {
         if (file.mimetype === "image/svg+xml") {
             const fileContent = await readFile(file.path, { encoding: "utf-8" });
 
-            if (svgContainsJavaScript(fileContent)) {
-                return "SVG must not contain JavaScript";
+            if (!isValidSvg(fileContent)) {
+                return "SVG contains forbidden content (e.g., JavaScript, security-relevant tags or attributes)";
             }
         }
 

packages/api/cms-api/src/dam/files/files.utils.spec.ts

@@ -1,32 +1,42 @@
-import { svgContainsJavaScript } from "./files.utils";
+import { isValidSvg } from "./files.utils";
 
 describe("Files Utils", () => {
-    describe("svgContainsJavaScript", () => {
-        it("should return false if the svg doesn't contain a script tag or event handler", async () => {
+    describe("isValidSvg", () => {
+        it("should return true if the svg doesn't contain any forbidden content", async () => {
             const cleanSvg = `<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">
                 <path fill="#242424" fill-rule="evenodd" d=""/>
             </svg>`;
 
-            expect(svgContainsJavaScript(cleanSvg)).toBe(false);
+            expect(isValidSvg(cleanSvg)).toBe(true);
         });
 
-        it("should return true if the svg contains a script tag", async () => {
+        it("should return false if the svg contains a script tag", async () => {
             const svgWithScriptTag = `<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">
                 <path fill="#242424" fill-rule="evenodd" d=""/>
                 <script type="text/javascript">
                     alert("XSS");
                 </script>
             </svg>`;
 
-            expect(svgContainsJavaScript(svgWithScriptTag)).toBe(true);
+            expect(isValidSvg(svgWithScriptTag)).toBe(false);
         });
 
-        it("should return true if the svg contains an event handler", async () => {
+        it("should return false if the svg contains an event handler", async () => {
             const svgWithOnloadHandler = `<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">
                 <path onload="alert('XSS');" fill="#242424" fill-rule="evenodd" d=""/>
             </svg>`;
 
-            expect(svgContainsJavaScript(svgWithOnloadHandler)).toBe(true);
+            expect(isValidSvg(svgWithOnloadHandler)).toBe(false);
+        });
+
+        it("should return false if the svg contains a href attribute containing javascript", async () => {
+            const svgWithOnloadHandler = `<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 16 16">
+                <a href="javascript:alert(1)">
+                <path onload="alert('XSS');" fill="#242424" fill-rule="evenodd" d=""/>
+                </a>
+            </svg>`;
+
+            expect(isValidSvg(svgWithOnloadHandler)).toBe(false);
         });
     });
 });

packages/api/cms-api/src/dam/files/files.utils.ts

@@ -49,34 +49,54 @@ type SvgNode =
           [key: string]: SvgNode;
       };
 
-const recursivelyFindJSInSvg = (node: SvgNode): boolean => {
+const disallowedSvgTags = [
+    "script", // can lead to XSS
+    "foreignObject", // can embed non-SVG content
+    "use", // can load external resources
+    "image", // can load external resources
+    "animate", // can modify attributes; resource exhaustion
+    "animateMotion", // can modify attributes; resource exhaustion
+    "animateTransform", // can modify attributes; resource exhaustion
+    "set", // can modify attributes
+];
+
+const recursiveIsValidSvgNode = (node: SvgNode): boolean => {
     if (typeof node === "string") {
         // is plain text -> can't contain JS
-        return false;
+        return true;
     }
 
-    for (const tagOrAttr of Object.keys(node)) {
-        if (tagOrAttr.toLowerCase() === "script" || tagOrAttr.toLowerCase().startsWith("on")) {
-            // is script tag or event handler
-            return true;
+    for (const [tagOrAttributeName, value] of Object.entries(node)) {
+        const containsDisallowedTags = disallowedSvgTags.map((tag) => tag.toLowerCase()).includes(tagOrAttributeName.toLowerCase());
+
+        const containsEventHandler = tagOrAttributeName.toLowerCase().startsWith("on"); // can execute JavaScript
+
+        const containsHref = // can execute JavaScript or link to malicious targets
+            ["href", "xlink:href"].includes(tagOrAttributeName) &&
+            typeof value === "string" &&
+            (value.startsWith("http://") || value.startsWith("https://") || value.startsWith("javascript:"));
+
+        if (containsDisallowedTags || containsEventHandler || containsHref) {
+            return false;
         }
 
         // is node -> children can contain JS
-        const children = node[tagOrAttr];
-        const childrenContainJS = recursivelyFindJSInSvg(children);
-        if (childrenContainJS) {
-            return true;
+        const children = node[tagOrAttributeName];
+        const childrenAreValid = recursiveIsValidSvgNode(children);
+
+        if (!childrenAreValid) {
+            return false;
         }
     }
 
-    return false;
+    return true;
 };
 
-export const svgContainsJavaScript = (svg: string) => {
+export const isValidSvg = (svg: string) => {
     const parser = new XMLParser({ ignoreAttributes: false, attributeNamePrefix: "" });
     const jsonObj = parser.parse(svg) as SvgNode;
 
-    return recursivelyFindJSInSvg(jsonObj);
+    return recursiveIsValidSvgNode(jsonObj);
 };
 
 export const removeMulterTempFile = async (file: FileUploadInput) => {