fix: stronger enforcement of normalizeLink (excalidraw#6728)

Co-authored-by: dwelle <[email protected]>

package.json

@@ -19,6 +19,7 @@
     ]
   },
   "dependencies": {
+    "@braintree/sanitize-url": "6.0.2",
     "@excalidraw/random-username": "1.0.0",
     "@radix-ui/react-popover": "1.0.3",
     "@radix-ui/react-tabs": "1.0.2",

src/components/App.tsx

@@ -291,13 +291,12 @@ import {
 } from "../element/textElement";
 import { isHittingElementNotConsideringBoundingBox } from "../element/collision";
 import {
-  normalizeLink,
   showHyperlinkTooltip,
   hideHyperlinkToolip,
   Hyperlink,
   isPointHittingLinkIcon,
-  isLocalLink,
 } from "../element/Hyperlink";
+import { isLocalLink, normalizeLink } from "../data/url";
 import { shouldShowBoundingBox } from "../element/transformHandles";
 import { actionUnlockAllElements } from "../actions/actionElementLock";
 import { Fonts } from "../scene/Fonts";
@@ -3352,20 +3351,27 @@ class App extends React.Component<AppProps, AppState> {
       this.device.isMobile,
     );
     if (lastPointerDownHittingLinkIcon && lastPointerUpHittingLinkIcon) {
-      const url = this.hitLinkElement.link;
+      let url = this.hitLinkElement.link;
       if (url) {
+        url = normalizeLink(url);
         let customEvent;
         if (this.props.onLinkOpen) {
           customEvent = wrapEvent(EVENT.EXCALIDRAW_LINK, event.nativeEvent);
-          this.props.onLinkOpen(this.hitLinkElement, customEvent);
+          this.props.onLinkOpen(
+            {
+              ...this.hitLinkElement,
+              link: url,
+            },
+            customEvent,
+          );
         }
         if (!customEvent?.defaultPrevented) {
           const target = isLocalLink(url) ? "_self" : "_blank";
           const newWindow = window.open(undefined, target);
           // https://mathiasbynens.github.io/rel-noopener/
           if (newWindow) {
             newWindow.opener = null;
-            newWindow.location = normalizeLink(url);
+            newWindow.location = url;
           }
         }
       }

src/data/restore.ts

@@ -41,6 +41,7 @@ import {
   measureBaseline,
 } from "../element/textElement";
 import { COLOR_PALETTE } from "../colors";
+import { normalizeLink } from "./url";
 
 type RestoredAppState = Omit<
   AppState,
@@ -142,7 +143,7 @@ const restoreElementWithProperties = <
       ? element.boundElementIds.map((id) => ({ type: "arrow", id }))
       : element.boundElements ?? [],
     updated: element.updated ?? getUpdatedTimestamp(),
-    link: element.link ?? null,
+    link: element.link ? normalizeLink(element.link) : null,
     locked: element.locked ?? false,
   };
 

src/data/url.test.tsx

@@ -0,0 +1,30 @@
+import { normalizeLink } from "./url";
+
+describe("normalizeLink", () => {
+  // NOTE not an extensive XSS test suite, just to check if we're not
+  // regressing in sanitization
+  it("should sanitize links", () => {
+    expect(
+      // eslint-disable-next-line no-script-url
+      normalizeLink(`javascript://%0aalert(document.domain)`).startsWith(
+        // eslint-disable-next-line no-script-url
+        `javascript:`,
+      ),
+    ).toBe(false);
+    expect(normalizeLink("ola")).toBe("ola");
+    expect(normalizeLink(" ola")).toBe("ola");
+
+    expect(normalizeLink("https://www.excalidraw.com")).toBe(
+      "https://www.excalidraw.com",
+    );
+    expect(normalizeLink("www.excalidraw.com")).toBe("www.excalidraw.com");
+    expect(normalizeLink("/ola")).toBe("/ola");
+    expect(normalizeLink("http://test")).toBe("http://test");
+    expect(normalizeLink("ftp://test")).toBe("ftp://test");
+    expect(normalizeLink("file://")).toBe("file://");
+    expect(normalizeLink("file://")).toBe("file://");
+    expect(normalizeLink("[test](https://test)")).toBe("[test](https://test)");
+    expect(normalizeLink("[[test]]")).toBe("[[test]]");
+    expect(normalizeLink("<test>")).toBe("<test>");
+  });
+});

src/data/url.ts

@@ -0,0 +1,9 @@
+import { sanitizeUrl } from "@braintree/sanitize-url";
+
+export const normalizeLink = (link: string) => {
+  return sanitizeUrl(link);
+};
+
+export const isLocalLink = (link: string | null) => {
+  return !!(link?.includes(location.origin) || link?.startsWith("/"));
+};

src/element/Hyperlink.tsx

@@ -29,6 +29,7 @@ import { getTooltipDiv, updateTooltipPosition } from "../components/Tooltip";
 import { getSelectedElements } from "../scene";
 import { isPointHittingElementBoundingBox } from "./collision";
 import { getElementAbsoluteCoords } from "./";
+import { isLocalLink, normalizeLink } from "../data/url";
 
 import "./Hyperlink.scss";
 import { trackEvent } from "../analytics";
@@ -166,7 +167,7 @@ export const Hyperlink = ({
         />
       ) : (
         <a
-          href={element.link || ""}
+          href={normalizeLink(element.link || "")}
           className={clsx("excalidraw-hyperlinkContainer-link", {
             "d-none": isEditing,
           })}
@@ -177,7 +178,13 @@ export const Hyperlink = ({
                 EVENT.EXCALIDRAW_LINK,
                 event.nativeEvent,
               );
-              onLinkOpen(element, customEvent);
+              onLinkOpen(
+                {
+                  ...element,
+                  link: normalizeLink(element.link),
+                },
+                customEvent,
+              );
               if (customEvent.defaultPrevented) {
                 event.preventDefault();
               }
@@ -231,21 +238,6 @@ const getCoordsForPopover = (
   return { x, y };
 };
 
-export const normalizeLink = (link: string) => {
-  link = link.trim();
-  if (link) {
-    // prefix with protocol if not fully-qualified
-    if (!link.includes("://") && !/^[[\\/]/.test(link)) {
-      link = `https://${link}`;
-    }
-  }
-  return link;
-};
-
-export const isLocalLink = (link: string | null) => {
-  return !!(link?.includes(location.origin) || link?.startsWith("/"));
-};
-
 export const actionLink = register({
   name: "hyperlink",
   perform: (elements, appState) => {

src/packages/excalidraw/CHANGELOG.md

@@ -15,6 +15,7 @@ Please add the latest change on the top under the correct section.
 
 ### Features
 
+- Properly sanitize element `link` urls. [#6728](https://github.com/excalidraw/excalidraw/pull/6728).
 - Sidebar component now supports tabs — for more detailed description of new behavior and breaking changes, see the linked PR. [#6213](https://github.com/excalidraw/excalidraw/pull/6213)
 - Exposed `DefaultSidebar` component to allow modifying the default sidebar, such as adding custom tabs to it. [#6213](https://github.com/excalidraw/excalidraw/pull/6213)
 

src/packages/excalidraw/index.tsx

@@ -247,3 +247,5 @@ export { WelcomeScreen };
 export { LiveCollaborationTrigger };
 
 export { DefaultSidebar } from "../../components/DefaultSidebar";
+
+export { normalizeLink } from "../../data/url";

src/renderer/renderElement.ts

@@ -50,6 +50,7 @@ import {
 } from "../element/textElement";
 import { LinearElementEditor } from "../element/linearElementEditor";
 import { getContainingFrame } from "../frame";
+import { normalizeLink } from "../data/url";
 
 // using a stronger invert (100% vs our regular 93%) and saturate
 // as a temp hack to make images in dark theme look closer to original
@@ -1203,7 +1204,7 @@ export const renderElementToSvg = (
   // if the element has a link, create an anchor tag and make that the new root
   if (element.link) {
     const anchorTag = svgRoot.ownerDocument!.createElementNS(SVG_NS, "a");
-    anchorTag.setAttribute("href", element.link);
+    anchorTag.setAttribute("href", normalizeLink(element.link));
     root.appendChild(anchorTag);
     root = anchorTag;
   }

yarn.lock

@@ -1086,6 +1086,11 @@
   resolved "https://registry.yarnpkg.com/@bcoe/v8-coverage/-/v8-coverage-0.2.3.tgz#75a2e8b51cb758a7553d6804a5932d7aace75c39"
   integrity sha512-0hYQ8SB4Db5zvZB4axdMHGwEaQjkZzFjQiN9LVYvIFB2nSUHW9tYpxWriPrWDASIxiaXax83REcLxuSdnGPZtw==
 
+"@braintree/[email protected]":
+  version "6.0.2"
+  resolved "https://registry.yarnpkg.com/@braintree/sanitize-url/-/sanitize-url-6.0.2.tgz#6110f918d273fe2af8ea1c4398a88774bb9fc12f"
+  integrity sha512-Tbsj02wXCbqGmzdnXNk0SOF19ChhRU70BsroIi4Pm6Ehp56in6vch94mfbdQ17DozxkL3BAVjbZ4Qc1a0HFRAg==
+
 "@csstools/normalize.css@*":
   version "12.0.0"
   resolved "https://registry.yarnpkg.com/@csstools/normalize.css/-/normalize.css-12.0.0.tgz#a9583a75c3f150667771f30b60d9f059473e62c4"