(core) Restrict widget/form URL schemes

Summary: Custom widget and form redirect URLs were not restricted to http[s] schemes, leaving the door open to vulnerabilities from injection attacks. URLs are now properly sanitized. Test Plan: Browser and unit tests. Reviewers: dsagal Reviewed By: dsagal Subscribers: dsagal, jordigh, paulfitz, fflorent Differential Revision: https://phab.getgrist.com/D4410
gristlabs · Dec 10, 2024 · 10b0690 · 10b0690
1 parent efefc96
commit 10b0690
Show file tree

Hide file tree

Showing 7 changed files with 144 additions and 27 deletions.
diff --git a/app/client/components/WidgetFrame.ts b/app/client/components/WidgetFrame.ts
@@ -12,7 +12,7 @@ import {AccessLevel, ICustomWidget, isSatisfied, matchWidget} from 'app/common/C
 import {DisposableWithEvents} from 'app/common/DisposableWithEvents';
 import {BulkColValues, fromTableDataAction, RowRecord} from 'app/common/DocActions';
 import {extractInfoFromColType, reencodeAsAny} from 'app/common/gristTypes';
-import {getGristConfig} from 'app/common/urlUtils';
+import {getGristConfig, sanitizeUrl} from 'app/common/urlUtils';
 import {
   AccessTokenOptions, CursorPos, CustomSectionAPI, FetchSelectedOptions, GristDocAPI, GristView,
   InteractionOptionsRequest, WidgetAPI, WidgetColumnMap
@@ -141,7 +141,10 @@ export class WidgetFrame extends DisposableWithEvents {
     const maybeUrl = Computed.create(this, use => use(this._widget)?.url || this._options.url);
 
     // Url to widget or empty page with access level and preferences.
-    this._url = Computed.create(this, use => this._urlWithAccess(use(maybeUrl) || this._getEmptyWidgetPage()));
+    this._url = Computed.create(
+      this,
+      (use) => this._urlWithAccess(use(maybeUrl)) || this._getEmptyWidgetPage()
+    );
 
     // Iframe is empty when url is not set.
     this._isEmpty = Computed.create(this, use => !use(maybeUrl));
@@ -222,22 +225,24 @@ export class WidgetFrame extends DisposableWithEvents {
   }
 
   // Appends access level to query string.
-  private _urlWithAccess(url: string) {
-    if (!url) { return url; }
+  private _urlWithAccess(url: string | null): string | null {
+    if (!url) {
+      return url;
+    }
 
     let urlObj: URL;
     try {
       urlObj = new URL(url);
     } catch (e) {
       console.error(e);
-      return url;
+      return null;
     }
     urlObj.searchParams.append('access', this._options.access);
     urlObj.searchParams.append('readonly', String(this._options.readonly));
     // 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 urlObj.href;
+    return sanitizeUrl(urlObj.href);
   }
 
   private _getEmptyWidgetPage(): string {

diff --git a/app/client/ui/FormPage.ts b/app/client/ui/FormPage.ts
@@ -8,7 +8,7 @@ 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} from 'app/common/urlUtils';
+import {getGristConfig, sanitizeUrl} from 'app/common/urlUtils';
 import {Disposable, dom, makeTestId, Observable, styled, subscribe} from 'grainjs';
 
 const t = makeT('FormPage');
@@ -90,12 +90,9 @@ export class FormPage extends Disposable {
 
     const {successURL} = formLayout;
     if (successURL) {
-      try {
-        const url = new URL(successURL);
-        window.location.href = url.href;
-        return;
-      } catch {
-        // If the URL is invalid, just ignore it.
+      const url = sanitizeUrl(successURL);
+      if (url) {
+        window.location.href = url;
       }
     }
 

diff --git a/app/client/ui/RightPanel.ts b/app/client/ui/RightPanel.ts
@@ -963,11 +963,20 @@ export class RightPanel extends Disposable {
       ),
       cssLabel(t("Redirection")),
       cssRow(
-        labeledSquareCheckbox(redirection, t('Redirect automatically after submission')),
+        labeledSquareCheckbox(
+          redirection,
+          t("Redirect automatically after submission"),
+          testId("form-redirect")
+        )
       ),
       cssRow(
-        cssTextInput(successURL, (val) => successURL.set(val), {placeholder: t('Enter redirect URL')}),
-        dom.show(redirection),
+        cssTextInput(
+          successURL,
+          (val) => successURL.set(val),
+          { placeholder: t("Enter redirect URL") },
+          testId("form-redirect-url")
+        ),
+        dom.show(redirection)
       ),
     ];
   }

diff --git a/app/common/urlUtils.ts b/app/common/urlUtils.ts
@@ -79,3 +79,20 @@ 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;
+  }
+}
diff --git a/test/common/urlUtils.ts b/test/common/urlUtils.ts
@@ -0,0 +1,24 @@
+import { sanitizeUrl } from "app/common/urlUtils";
+import { assert } from "chai";
+
+describe("urlUtils", function () {
+  describe("sanitizeUrl", function () {
+    it("returns the provided URL if the scheme is http[s]", function () {
+      assert.equal(sanitizeUrl("https://example.com"), "https://example.com/");
+      assert.equal(sanitizeUrl("http://example.com"), "http://example.com/");
+      assert.equal(sanitizeUrl("https://example.com"), "https://example.com/");
+    });
+
+    it("returns null if the provided URL is invalid", function () {
+      assert.isNull(sanitizeUrl("www.example.com"));
+      assert.isNull(sanitizeUrl(""));
+      assert.isNull(sanitizeUrl("invalid"));
+    });
+
+    it("returns null if the provided URL's scheme is not http[s]", function () {
+      assert.isNull(sanitizeUrl("mailto:[email protected]"));
+      assert.isNull(sanitizeUrl("ftp://getgrist.com/path"));
+      assert.isNull(sanitizeUrl("javascript:alert()"));
+    });
+  });
+});
diff --git a/test/nbrowser/CustomWidgetsConfig.ts b/test/nbrowser/CustomWidgetsConfig.ts
@@ -17,6 +17,7 @@ const READ_WIDGET = 'Read';
 const FULL_WIDGET = 'Full';
 const COLUMN_WIDGET = 'COLUMN_WIDGET';
 const REQUIRED_WIDGET = 'REQUIRED_WIDGET';
+const INVALID_URL_WIDGET = 'INVALID_URL_WIDGET';
 // Custom URL label.
 const CUSTOM_URL = 'Custom URL';
 // Holds url for sample widget server.
@@ -178,6 +179,11 @@ describe('CustomWidgetsConfig', function () {
             url: createConfigUrl({requiredAccess: AccessLevel.read_table, columns: [{name:'Column', optional: false}]}),
             widgetId: 'tester6',
           },
+          {
+            name: INVALID_URL_WIDGET,
+            url: 'ftp://getgrist.com/path',
+            widgetId: 'tester7'
+          }
         ]);
       });
       addStatic(app);
@@ -1077,6 +1083,12 @@ describe('CustomWidgetsConfig', function () {
     await mainSession.loadDoc(`/doc/${docId}`);
     await refresh();
   });
+
+  it('should display the empty widget page if the URL is invalid', async function () {
+    await gu.setCustomWidget(INVALID_URL_WIDGET);
+    await widget.waitForFrame();
+    assert.match(await widget.url(), /custom-widget\.html$/);
+  });
 });
 
 // Poor man widget rpc. Class that invokes various parts in the tester widget.
@@ -1095,6 +1107,9 @@ const widget = {
       await driver.executeScript('grist.testWaitForPendingRequests();');
     });
   },
+  async url() {
+    return await driver.find('iframe').getAttribute('src');
+  },
   async content() {
     return await this._read('body');
   },

diff --git a/test/nbrowser/FormView.ts b/test/nbrowser/FormView.ts
@@ -20,16 +20,41 @@ describe('FormView', function() {
 
   afterEach(() => gu.checkForErrors());
 
-  async function createFormWith(type: string, more = false) {
+  async function createFormWith(
+    type: string,
+    options: {
+      redirectUrl?: string;
+    } = {}
+  ) {
+    const {redirectUrl} = options;
+
     await gu.addNewSection('Form', 'Table1');
 
+    if (redirectUrl) {
+      await gu.openWidgetPanel();
+      await driver.find(".test-config-submission").click();
+      await driver.find(".test-form-redirect").click();
+      await gu.waitForServer();
+      await driver.find(".test-form-redirect-url").click();
+      await gu.sendKeys(redirectUrl, Key.ENTER);
+      await gu.waitForServer();
+    }
+
     // Make sure column D is not there.
     assert.isUndefined(await api.getTable(docId, 'Table1').then(t => t.D));
 
     // Add a text question
     await plusButton().click();
-    if (more) {
-      await clickMenu('More');
+    if (
+      [
+        "Integer",
+        "Toggle",
+        "Choice List",
+        "Reference",
+        "Reference List",
+      ].includes(type)
+    ) {
+      await clickMenu("More");
     }
     await clickMenu(type);
     await gu.waitForServer();
@@ -366,7 +391,7 @@ describe('FormView', function() {
     });
 
     it('can submit a form with text Integer field', async function() {
-      const formUrl = await createFormWith('Integer', true);
+      const formUrl = await createFormWith('Integer');
       // We are in a new window.
       await gu.onNewTab(async () => {
         await driver.get(formUrl);
@@ -388,7 +413,7 @@ describe('FormView', function() {
     });
 
     it('can submit a form with spinner Integer field', async function() {
-      const formUrl = await createFormWith('Integer', true);
+      const formUrl = await createFormWith('Integer');
       await driver.findContent('.test-numeric-form-field-format .test-select-button', /Spinner/).click();
       await gu.waitForServer();
       // We are in a new window.
@@ -419,7 +444,7 @@ describe('FormView', function() {
     });
 
     it('can submit a form with switch Toggle field', async function() {
-      const formUrl = await createFormWith('Toggle', true);
+      const formUrl = await createFormWith('Toggle');
       // We are in a new window.
       await gu.onNewTab(async () => {
         await driver.get(formUrl);
@@ -449,7 +474,7 @@ describe('FormView', function() {
     });
 
     it('can submit a form with checkbox Toggle field', async function() {
-      const formUrl = await createFormWith('Toggle', true);
+      const formUrl = await createFormWith('Toggle');
       await driver.findContent('.test-toggle-form-field-format .test-select-button', /Checkbox/).click();
       await gu.waitForServer();
       // We are in a new window.
@@ -481,7 +506,7 @@ describe('FormView', function() {
     });
 
     it('can submit a form with ChoiceList field', async function() {
-      const formUrl = await createFormWith('Choice List', true);
+      const formUrl = await createFormWith('Choice List');
       // Add some options.
       await gu.openColumnPanel();
 
@@ -511,7 +536,7 @@ describe('FormView', function() {
     });
 
     it('can submit a form with select Ref field', async function() {
-      const formUrl = await createFormWith('Reference', true);
+      const formUrl = await createFormWith('Reference');
       // Add some options.
       await gu.openColumnPanel();
       await gu.setRefShowColumn('A');
@@ -564,7 +589,7 @@ describe('FormView', function() {
     });
 
     it('can submit a form with radio Ref field', async function() {
-      const formUrl = await createFormWith('Reference', true);
+      const formUrl = await createFormWith('Reference');
       await driver.findContent('.test-form-field-format .test-select-button', /Radio/).click();
       await gu.waitForServer();
       await gu.setRefShowColumn('A');
@@ -603,7 +628,7 @@ describe('FormView', function() {
     });
 
     it('can submit a form with RefList field', async function() {
-      const formUrl = await createFormWith('Reference List', true);
+      const formUrl = await createFormWith('Reference List');
       // Add some options.
       await gu.setRefShowColumn('A');
       // Add 3 records to this table (it is now empty).
@@ -640,6 +665,31 @@ describe('FormView', function() {
       await removeForm();
     });
 
+    it('redirects to valid URLs on submission', async function() {
+      const url = await createFormWith('Text', {
+        redirectUrl: "https://example.com",
+      });
+      await gu.onNewTab(async () => {
+        await driver.get(url);
+        await driver.findWait('input[type="submit"]', 2000).click();
+        await gu.waitForUrl(/example\.com/);
+      });
+      await removeForm();
+    });
+
+    it('does not redirect to invalid URLs on submission', async function() {
+      const url = await createFormWith('Text', {
+        redirectUrl: "javascript:alert()",
+      });
+      await gu.onNewTab(async () => {
+        await driver.get(url);
+        await driver.findWait('input[type="submit"]', 2000).click();
+        await waitForConfirm();
+        assert.isFalse(await gu.isAlertShown());
+      });
+      await removeForm();
+    });
+
     it('excludes formula fields from forms', async function() {
       const formUrl = await createFormWith('Text');