(core) Fix attachment and hyperlink vulnerabilities

Summary:
Attachments were prone to XSS-based attacks if attachments injected with scripts
were previewed or opened. This is now addressed by CSP.

Hyperlink cells were prone to similar attacks if `javascript:...` URLs were inserted into
cells. This has also been addressed by sanitizing URLs.

Thank you to Florent <[email protected]> and Grégoire Cutzach <[email protected]>
for reporting and co-authoring these changes.

Co-authored-by: Florent <[email protected]>
Co-authored-by: Grégoire Cutzach <[email protected]>

Test Plan: Browser and unit tests.

Reviewers: dsagal, paulfitz

Reviewed By: dsagal, paulfitz

Subscribers: dsagal, paulfitz, fflorent

Differential Revision: https://phab.getgrist.com/D4413

app/client/components/WidgetFrame.ts

@@ -5,14 +5,15 @@ import {GristDoc} from 'app/client/components/GristDoc';
 import {hooks} from 'app/client/Hooks';
 import {get as getBrowserGlobals} from 'app/client/lib/browserGlobals';
 import {makeTestId} from 'app/client/lib/domUtils';
+import {sanitizeHttpUrl} from 'app/client/lib/sanitizeUrl';
 import {ColumnRec, ViewSectionRec} from 'app/client/models/DocModel';
 import {reportError} from 'app/client/models/errors';
 import {gristThemeObs} from 'app/client/ui2018/theme';
 import {AccessLevel, ICustomWidget, isSatisfied, matchWidget} from 'app/common/CustomWidget';
 import {DisposableWithEvents} from 'app/common/DisposableWithEvents';
 import {BulkColValues, fromTableDataAction, RowRecord} from 'app/common/DocActions';
 import {extractInfoFromColType, reencodeAsAny} from 'app/common/gristTypes';
-import {getGristConfig, sanitizeUrl} from 'app/common/urlUtils';
+import {getGristConfig} from 'app/common/urlUtils';
 import {
   AccessTokenOptions, CursorPos, CustomSectionAPI, FetchSelectedOptions, GristDocAPI, GristView,
   InteractionOptionsRequest, WidgetAPI, WidgetColumnMap
@@ -242,7 +243,7 @@ export class WidgetFrame extends DisposableWithEvents {
     // Append user and document preferences to query string.
     const settingsParams = new URLSearchParams(this._options.preferences);
     settingsParams.forEach((value, key) => urlObj.searchParams.append(key, value));
-    return sanitizeUrl(urlObj.href);
+    return sanitizeHttpUrl(urlObj.href);
   }
 
   private _getEmptyWidgetPage(): string {

app/client/lib/sanitizeUrl.ts

@@ -0,0 +1,42 @@
+import DOMPurify from "dompurify";
+
+// Export dependencies for stubbing in tests.
+export const Deps = { DOMPurify };
+
+/**
+ * Returns the provided URL if it is valid and safe to use in
+ * HTTP-only contexts, such as form redirects and custom widget
+ * URLs.
+ *
+ * Returns `null` if the URL is invalid or unsafe.
+ *
+ * For sanitizing hyperlink URLs, such as those used by `a`
+ * elements, see `sanitizeLinkUrl`.
+ */
+export function sanitizeHttpUrl(url: string): string | null {
+  try {
+    const parsedUrl = new URL(url);
+    if (!["http:", "https:"].includes(parsedUrl.protocol)) {
+      return null;
+    }
+
+    return parsedUrl.href;
+  } catch (e) {
+    return null;
+  }
+}
+
+/**
+ * Returns the provided URL if it is valid and safe to use for hyperlinks,
+ * such as those used by `a` elements. This includes URLs prefixed with
+ * `http[s]:`, `mailto:`, and `tel:`, and excludes URLs prefixed with
+ * `javascript:`.
+ *
+ * Returns `null` if the URL is invalid or unsafe.
+ *
+ * For sanitizing HTTP-only URLs, such as those used for redirects, see
+ * `sanitizeHttpUrl`.
+ */
+export function sanitizeLinkUrl(url: string): string | null {
+  return Deps.DOMPurify.isValidAttribute("a", "href", url) ? url : null;
+}

app/client/ui/FormPage.ts

@@ -1,14 +1,15 @@
 import {FormRenderer} from 'app/client/components/FormRenderer';
 import {handleSubmit, TypedFormData} from 'app/client/lib/formUtils';
 import {makeT} from 'app/client/lib/localization';
+import {sanitizeHttpUrl} from 'app/client/lib/sanitizeUrl';
 import {FormModel, FormModelImpl} from 'app/client/models/FormModel';
 import {buildFormFooter} from 'app/client/ui/FormContainer';
 import {FormErrorPage} from 'app/client/ui/FormErrorPage';
 import {FormSuccessPage} from 'app/client/ui/FormSuccessPage';
 import {colors} from 'app/client/ui2018/cssVars';
 import {ApiError} from 'app/common/ApiError';
 import {getPageTitleSuffix} from 'app/common/gristUrls';
-import {getGristConfig, sanitizeUrl} from 'app/common/urlUtils';
+import {getGristConfig} from 'app/common/urlUtils';
 import {Disposable, dom, makeTestId, Observable, styled, subscribe} from 'grainjs';
 
 const t = makeT('FormPage');
@@ -90,7 +91,7 @@ export class FormPage extends Disposable {
 
     const {successURL} = formLayout;
     if (successURL) {
-      const url = sanitizeUrl(successURL);
+      const url = sanitizeHttpUrl(successURL);
       if (url) {
         window.location.href = url;
       }

app/client/ui/sanitizeHTML.ts

@@ -24,10 +24,10 @@ function handleSanitizeAttribute(node: Element) {
   node.setAttribute('target', '_blank');
 }
 
-function handleSanitizeTutorialElement(node: Element, data: createDOMPurifier.SanitizeElementHookEvent) {
+function handleSanitizeTutorialElement(node: Node, data: createDOMPurifier.UponSanitizeElementHookEvent) {
   if (data.tagName !== 'iframe') { return; }
 
-  const src = node.getAttribute('src');
+  const src = (node as Element).getAttribute('src');
   if (src?.startsWith('https://www.youtube.com/embed/')) {
     return;
   }

app/client/widgets/HyperLinkTextBox.ts

@@ -1,3 +1,4 @@
+import { sanitizeLinkUrl } from 'app/client/lib/sanitizeUrl';
 import { DataRowModel } from 'app/client/models/DataRowModel';
 import { ViewFieldRec } from 'app/client/models/entities/ViewFieldRec';
 import { constructUrl } from 'app/client/models/gristUrlState';
@@ -20,7 +21,10 @@ export class HyperLinkTextBox extends NTextBox {
 
   public buildDom(row: DataRowModel) {
     const value = row.cells[this.field.colId()];
-    const url = Computed.create(null, (use) => constructUrl(use(value)));
+    const url = Computed.create(
+      null,
+      (use) => sanitizeLinkUrl(constructUrl(use(value))) ?? "about:blank"
+    );
     return cssFieldClip(
       dom.autoDispose(url),
       dom.style('text-align', this.alignment),

app/common/urlUtils.ts

@@ -79,20 +79,3 @@ export function fetchFromHome(path: string, opts: RequestInit): Promise<Response
   const baseUrl = addCurrentOrgToPath(getGristConfig().homeUrl!);
   return window.fetch(`${baseUrl}${path}`, opts);
 }
-
-/**
- * Returns the provided URL if it has a valid protocol (`http:` or `https:`), or
- * `null` otherwise.
- */
-export function sanitizeUrl(url: string): string | null {
-  try {
-    const parsedUrl = new URL(url);
-    if (!["http:", "https:"].includes(parsedUrl.protocol)) {
-      return null;
-    }
-
-    return parsedUrl.href;
-  } catch (e) {
-    return null;
-  }
-}

app/server/lib/DocWorker.ts

@@ -61,6 +61,7 @@ export class DocWorker {
         .type(ext)
         .set('Content-Disposition', contentDispHeader)
         .set('Cache-Control', 'private, max-age=3600')
+        .set("Content-Security-Policy", "sandbox; default-src: 'none'")
         .send(data);
     } catch (err) {
       res.status(404).send({error: err.toString()});

package.json

@@ -49,7 +49,6 @@
     "@types/chai-as-promised": "7.1.0",
     "@types/content-disposition": "0.5.2",
     "@types/diff-match-patch": "1.0.32",
-    "@types/dompurify": "3.0.5",
     "@types/double-ended-queue": "2.1.0",
     "@types/express": "4.17.17",
     "@types/form-data": "2.2.1",
@@ -139,7 +138,7 @@
     "csv": "6.3.8",
     "currency-symbol-map": "5.1.0",
     "diff-match-patch": "1.0.5",
-    "dompurify": "3.0.6",
+    "dompurify": "3.2.3",
     "double-ended-queue": "2.1.0-0",
     "engine.io": "^6.5.4",
     "engine.io-client": "^6.5.3",

test/client/lib/sanitizeUrl.ts

@@ -0,0 +1,71 @@
+import {
+  Deps,
+  sanitizeHttpUrl,
+  sanitizeLinkUrl,
+} from "app/client/lib/sanitizeUrl";
+import { assert } from "chai";
+import DOMPurify from "dompurify";
+import { JSDOM } from "jsdom";
+import * as sinon from "sinon";
+
+describe("sanitizeUrl", function () {
+  let sandbox: sinon.SinonSandbox;
+
+  beforeEach(function () {
+    // These grainjs browserGlobals are needed for using dom() in tests.
+    const jsdomDoc = new JSDOM("<!doctype html><html><body></body></html>");
+    sandbox = sinon.createSandbox();
+    sandbox.stub(Deps, "DOMPurify").value(DOMPurify(jsdomDoc.window));
+  });
+
+  afterEach(function () {
+    sandbox.restore();
+  });
+
+  describe("sanitizeHttpUrl", function () {
+    it("returns the provided URL if valid", function () {
+      assert.equal(
+        sanitizeHttpUrl("https://example.com"),
+        "https://example.com/"
+      );
+      assert.equal(
+        sanitizeHttpUrl("http://example.com"),
+        "http://example.com/"
+      );
+    });
+
+    it("returns null if the provided URL is invalid", function () {
+      assert.isNull(sanitizeHttpUrl("www.example.com"));
+      assert.isNull(sanitizeHttpUrl(""));
+      assert.isNull(sanitizeHttpUrl("invalid"));
+      assert.isNull(sanitizeHttpUrl("mailto:[email protected]"));
+      assert.isNull(sanitizeHttpUrl("ftp://getgrist.com/path"));
+      assert.isNull(sanitizeHttpUrl("javascript:alert()"));
+    });
+  });
+
+  describe("sanitizeLinkUrl", function () {
+    it("returns the provided URL if valid", function () {
+      assert.equal(
+        sanitizeLinkUrl("https://example.com"),
+        "https://example.com"
+      );
+      assert.equal(sanitizeLinkUrl("http://example.com"), "http://example.com");
+      assert.equal(sanitizeLinkUrl("www.example.com"), "www.example.com");
+      assert.equal(sanitizeLinkUrl(""), "");
+      assert.equal(
+        sanitizeLinkUrl("mailto:[email protected]"),
+        "mailto:[email protected]"
+      );
+      assert.equal(sanitizeLinkUrl("tel:0123456789"), "tel:0123456789");
+      assert.equal(
+        sanitizeLinkUrl("ftp://getgrist.com/path"),
+        "ftp://getgrist.com/path"
+      );
+    });
+
+    it("returns null if the provided URL is unsafe", function () {
+      assert.isNull(sanitizeLinkUrl("javascript:alert()"));
+    });
+  });
+});