From b33fa6d6f64d27adc3a47b25c0aa55711740d0af Mon Sep 17 00:00:00 2001 From: Christopher Chedeau Date: Thu, 29 Jun 2023 03:14:42 -0700 Subject: [PATCH] fix: stronger enforcement of normalizeLink (#6728) Co-authored-by: dwelle --- package.json | 1 + src/components/App.tsx | 16 ++++++++++----- src/data/restore.ts | 3 ++- src/data/url.test.tsx | 30 ++++++++++++++++++++++++++++ src/data/url.ts | 9 +++++++++ src/element/Hyperlink.tsx | 26 +++++++++--------------- src/packages/excalidraw/CHANGELOG.md | 1 + src/packages/excalidraw/index.tsx | 2 ++ src/renderer/renderElement.ts | 3 ++- yarn.lock | 5 +++++ 10 files changed, 72 insertions(+), 24 deletions(-) create mode 100644 src/data/url.test.tsx create mode 100644 src/data/url.ts diff --git a/package.json b/package.json index 584bdcbc..d1198fa2 100644 --- a/package.json +++ b/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", diff --git a/src/components/App.tsx b/src/components/App.tsx index 99be4b0d..2479731e 100644 --- a/src/components/App.tsx +++ b/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,12 +3351,19 @@ class App extends React.Component { 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"; @@ -3365,7 +3371,7 @@ class App extends React.Component { // https://mathiasbynens.github.io/rel-noopener/ if (newWindow) { newWindow.opener = null; - newWindow.location = normalizeLink(url); + newWindow.location = url; } } } diff --git a/src/data/restore.ts b/src/data/restore.ts index 57a0265f..5f2adc00 100644 --- a/src/data/restore.ts +++ b/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, }; diff --git a/src/data/url.test.tsx b/src/data/url.test.tsx new file mode 100644 index 00000000..36bed0a3 --- /dev/null +++ b/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("")).toBe(""); + }); +}); diff --git a/src/data/url.ts b/src/data/url.ts new file mode 100644 index 00000000..d34320a5 --- /dev/null +++ b/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("/")); +}; diff --git a/src/element/Hyperlink.tsx b/src/element/Hyperlink.tsx index 6bf3b17e..cf741ce0 100644 --- a/src/element/Hyperlink.tsx +++ b/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 = ({ /> ) : ( { - 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) => { diff --git a/src/packages/excalidraw/CHANGELOG.md b/src/packages/excalidraw/CHANGELOG.md index 30292159..1cd27d95 100644 --- a/src/packages/excalidraw/CHANGELOG.md +++ b/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) diff --git a/src/packages/excalidraw/index.tsx b/src/packages/excalidraw/index.tsx index 22f79dd3..8ee2956b 100644 --- a/src/packages/excalidraw/index.tsx +++ b/src/packages/excalidraw/index.tsx @@ -247,3 +247,5 @@ export { WelcomeScreen }; export { LiveCollaborationTrigger }; export { DefaultSidebar } from "../../components/DefaultSidebar"; + +export { normalizeLink } from "../../data/url"; diff --git a/src/renderer/renderElement.ts b/src/renderer/renderElement.ts index ed6ccb69..0efe5df9 100644 --- a/src/renderer/renderElement.ts +++ b/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; } diff --git a/yarn.lock b/yarn.lock index 2e3c1ea8..2e8bdda5 100644 --- a/yarn.lock +++ b/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/sanitize-url@6.0.2": + 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"