allow using urls from other columns in link templates (#42438)

frontend/src/metabase/lib/formatting/link.ts

-import { formatValue } from "metabase/lib/formatting";
+import {
+  formatValue,
+  getUrlProtocol,
+  isDefaultLinkProtocol,
+} from "metabase/lib/formatting";
 import { isDate } from "metabase-lib/v1/types/utils/isa";
 import type { ParameterValueOrArray } from "metabase-types/api";
 import type { DatasetColumn, RowValue } from "metabase-types/api/dataset";
@@ -41,6 +45,11 @@ export function renderLinkTextForClick(
   );
 }
 
+function isSafeUrl(urlString: string): boolean {
+  const protocol = getUrlProtocol(urlString);
+  return protocol != null && isDefaultLinkProtocol(protocol);
+}
+
 export function renderLinkURLForClick(
   template: string,
   data: ValueAndColumnForColumnNameDate,
@@ -48,14 +57,36 @@ export function renderLinkURLForClick(
   return renderTemplateForClick(
     template,
     data,
-    ({ value, column }: TemplateForClickFormatFunctionParamsType) => {
+    (
+      { value, column }: TemplateForClickFormatFunctionParamsType,
+      offset: number,
+    ) => {
       const valueForLinkTemplate = formatValueForLinkTemplate(value, column);
 
       if ([null, NULL_DISPLAY_VALUE].includes(valueForLinkTemplate)) {
         return "";
       }
 
-      return encodeURIComponent(valueForLinkTemplate);
+      // We intentionally want to allow users making column link templates like "{{url_from_another_column}}"
+      // where url_from_another_column value can be "http://metabase.com/". In such cases, we do not need to
+      // apply encodeURIComponent function.
+      // To keep it secure we should allow skipping encodeURIComponent only when the template value is coming from
+      // a dataset result which means it has a column. Allowing filter parameters is not secure because it enables
+      // composing urls like the following:
+      // https://myinstance.metabase.com/dashboard/1?my_parameter=https%3A%2F%2Fphishing.com
+      // which would make link with "{{my_parameter}}" template open https://phishing.com.
+      // Although, having target="_blank" attribute on the links prevents urls like "javascript:alert(document.cookies)" from being
+      // executed in the context of the current page and rel="noopener noreferrer" ensures that the linked page does not
+      // have access to the window.opener property, additionally checking for safe protocols will not hurt.
+      // Also, this only makes sense when such parameters are at the beginning of the link template.
+      const isColumnValue = column != null;
+      const isStart = offset === 0;
+      const shouldSkipEncoding =
+        isColumnValue && isStart && isSafeUrl(valueForLinkTemplate);
+
+      return shouldSkipEncoding
+        ? valueForLinkTemplate
+        : encodeURIComponent(valueForLinkTemplate);
     },
   );
 }
@@ -68,10 +99,10 @@ function renderTemplateForClick(
 ) {
   return template.replace(
     /{{([^}]+)}}/g,
-    (whole: string, columnName: string) => {
+    (_whole: string, columnName: string, offset: number) => {
       const valueAndColumn = getValueAndColumnForColumnName(data, columnName);
       if (valueAndColumn) {
-        return formatFunction(valueAndColumn);
+        return formatFunction(valueAndColumn, offset);
       }
       return "";
     },

frontend/src/metabase/lib/formatting/link.unit.spec.tsx

+/* eslint-disable jest/expect-expect */
+import { createMockColumn } from "metabase-types/api/mocks";
+
+import type { ValueAndColumnForColumnNameDate } from "./link";
+import { renderLinkURLForClick } from "./link";
+
+const createMockLinkData = (
+  params: Partial<ValueAndColumnForColumnNameDate> = {},
+): ValueAndColumnForColumnNameDate => {
+  return {
+    column: {},
+    parameter: {},
+    parameterBySlug: {},
+    parameterByName: {},
+    userAttribute: {},
+    ...params,
+  };
+};
+
+describe("formatting/link", () => {
+  describe("renderLinkURLForClick", () => {
+    const testLinkTemplate = (
+      template: string,
+      data: Partial<ValueAndColumnForColumnNameDate>,
+      expectedUrl: string,
+    ) => {
+      // eslint-disable-next-line testing-library/render-result-naming-convention
+      const actualUrl = renderLinkURLForClick(
+        template,
+        createMockLinkData(data),
+      );
+      expect(actualUrl).toBe(expectedUrl);
+    };
+
+    it.each([
+      "https://metabase.com",
+      "http://metabase.com",
+      "mailto:example@example.com",
+    ])(
+      "should not encode safe urls from dataset columns when a link template starts with it",
+      url => {
+        testLinkTemplate(
+          "{{col}}",
+          {
+            column: {
+              col: { value: url, column: createMockColumn() },
+            },
+          },
+          url,
+        );
+      },
+    );
+
+    it.each([
+      ["https://metabase.com", "_https%3A%2F%2Fmetabase.com"],
+      ["http://metabase.com", "_http%3A%2F%2Fmetabase.com"],
+      ["mailto:example@example.com", "_mailto%3Aexample%40example.com"],
+    ])(
+      "should encode safe urls from dataset columns when a link template does not start with it",
+      (url, expectedUrl) => {
+        testLinkTemplate(
+          "_{{col}}",
+          {
+            column: {
+              col: { value: url, column: createMockColumn() },
+            },
+          },
+          expectedUrl,
+        );
+      },
+    );
+
+    it.each([
+      [
+        "javascript:alert(document.cookies)",
+        "javascript%3Aalert(document.cookies)",
+      ],
+      [
+        "tg://resolve?domain=my_support_bot",
+        "tg%3A%2F%2Fresolve%3Fdomain%3Dmy_support_bot",
+      ],
+    ])(
+      "should encode unsafe urls from dataset columns when a link template starts with it",
+      (url, expectedUrl) => {
+        testLinkTemplate(
+          "{{col}}",
+          {
+            column: {
+              col: { value: url, column: createMockColumn() },
+            },
+          },
+          expectedUrl,
+        );
+      },
+    );
+
+    it.each([
+      ["https://metabase.com", "https%3A%2F%2Fmetabase.com"],
+      ["http://metabase.com", "http%3A%2F%2Fmetabase.com"],
+      ["mailto:example@example.com", "mailto%3Aexample%40example.com"],
+    ])(
+      "should encode safe urls not from url parameters when a link template starts with it",
+      (url, expectedUrl) => {
+        testLinkTemplate(
+          "{{param}}",
+          {
+            parameterBySlug: {
+              param: { value: url },
+            },
+          },
+          expectedUrl,
+        );
+      },
+    );
+
+    it.each([
+      ["https://metabase.com", "https%3A%2F%2Fmetabase.com"],
+      ["http://metabase.com", "http%3A%2F%2Fmetabase.com"],
+      ["mailto:example@example.com", "mailto%3Aexample%40example.com"],
+    ])(
+      "should encode safe urls not from parameters when a link template starts with it",
+      (url, expectedUrl) => {
+        testLinkTemplate(
+          "{{param}}",
+          {
+            parameterByName: {
+              param: { value: url },
+            },
+          },
+          expectedUrl,
+        );
+      },
+    );
+
+    it.each([
+      ["https://metabase.com", "https%3A%2F%2Fmetabase.com"],
+      ["http://metabase.com", "http%3A%2F%2Fmetabase.com"],
+      ["mailto:example@example.com", "mailto%3Aexample%40example.com"],
+    ])(
+      "should encode safe urls not from user attributes when a link template starts with it",
+      (url, expectedUrl) => {
+        testLinkTemplate(
+          "{{param}}",
+          {
+            userAttribute: {
+              param: { value: url },
+            },
+          },
+          expectedUrl,
+        );
+      },
+    );
+  });
+});

frontend/src/metabase/lib/formatting/url.tsx

@@ -15,7 +15,7 @@ function isSafeProtocol(protocol: string) {
   );
 }
 
-function isDefaultLinkProtocol(protocol: string) {
+export function isDefaultLinkProtocol(protocol: string) {
   return (
     protocol === "http:" || protocol === "https:" || protocol === "mailto:"
   );