Restrict notification markdown link schemes and diff pulse recipients by email (#80597)

* Restrict markdown link URI schemes in rendered notifications Only http/https/mailto links and images survive markdown rendering for email/Slack notifications; other schemes and unparseable URIs are dropped instead of being passed through (or, for malformed URIs when a site URL is set, throwing mid-render). * Diff pulse recipients by id or email in the subscription-permission check Key the added-recipients diff the same way pulse-channel keys recipients (user recipients by :id, external ones by :email) so external-address changes are covered by the check too. * Allow inline base64 images in notification markdown The app's markdown renderer accepts data:image/...;base64 URIs (DATA_IMAGE_URI_PATTERN); let notification rendering accept the same pattern so text-card images look the same in email. * Cross-link DATA_IMAGE_URI_PATTERN to its backend counterpart * Share the data-image URI pattern between backend and frontend Lift the pattern into metabase.util.markdown.image (cljc), compile it into the frontend bundle via shadow-cljs, and re-export it as DATA_IMAGE_URI_PATTERN so both renderers accept the same inline images by construction. * Apply resolve-uri to Slack images too * Render a dropped link-ref as its text alone instead of restoring brackets
2026-08-26 13:09:26 +01:00
parent da72c9e970
commit 02a1dc223b
7 changed files with 159 additions and 29 deletions
--- a/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/subscription_test.clj
+++ b/enterprise/backend/test/metabase_enterprise/advanced_permissions/api/subscription_test.clj
@@ -78,6 +78,36 @@
                  (get-form 200)
                  (test-pulse 200))))))))))

+(deftest pulse-raw-email-recipient-diff-test
+  (testing "PUT /api/pulse/:id — adding a raw-email recipient requires subscription permissions, also when the
+            channel already has other raw-email recipients"
+    (with-subscription-disabled-for-all-users!
+      (mt/with-user-in-groups [group {:name "New Group"}
+                               user  [group]]
+        (mt/with-temp [:model/Pulse        pulse {:creator_id (u/the-id user)}
+                       :model/PulseChannel _     {:pulse_id      (:id pulse)
+                                                  :channel_type  :email
+                                                  :schedule_type :daily
+                                                  :schedule_hour 12
+                                                  :enabled       true
+                                                  :details       {:emails ["old@external.com"]}}]
+          ;; :monitoring passes the endpoint's top-level gate; without :subscription the user may update the
+          ;; pulse but must not be able to add recipients
+          (perms/grant-application-permissions! group :monitoring)
+          (mt/with-premium-features #{:advanced-permissions}
+            (let [update-recipients (fn [status recipients]
+                                      (mt/user-http-request user :put status (format "pulse/%d" (:id pulse))
+                                                            {:channels [{:enabled       true
+                                                                         :channel_type  "email"
+                                                                         :schedule_type "daily"
+                                                                         :schedule_hour 12
+                                                                         :recipients    recipients}]}))]
+              (testing "keeping the existing raw-email recipient is allowed"
+                (update-recipients 200 [{:email "old@external.com"}]))
+              (testing "adding a new raw-email recipient is rejected"
+                (update-recipients 403 [{:email "old@external.com"}
+                                        {:email "new@external.com"}])))))))))
+
 (deftest send-time-attachment-perm-drift-test
  (testing "Subscription attachments are gated by the subscription creator's send-time download perms (GH #71696)"
    ;; Regression test for PR #66827. The bug: result-attachment used (:creator_id card) — the card author —
--- a/frontend/src/metabase/visualizations/lib/utils.ts
+++ b/frontend/src/metabase/visualizations/lib/utils.ts
@@ -1,6 +1,7 @@
 import * as d3 from "d3";
 import _ from "underscore";

+import { data_image_uri_pattern } from "cljs/metabase.util.markdown.image";
 import { isNotNull } from "metabase/utils/types";
 import {
  isCoordinate,
@@ -613,5 +614,4 @@ export const segmentIsValid = (
  return allowOpenEnded ? hasMin || hasMax : hasMin && hasMax;
 };

-export const DATA_IMAGE_URI_PATTERN =
-  /^data:image\/(png|jpeg|jpg|gif|svg\+xml|webp);base64,/i;
+export const DATA_IMAGE_URI_PATTERN: RegExp = data_image_uri_pattern;
--- a/shadow-cljs.edn
+++ b/shadow-cljs.edn
@@ -33,6 +33,7 @@
                metabase.util.devtools
                metabase.util.formatting.constants
                metabase.util.formatting.date
+                metabase.util.markdown.image
                metabase.util.time
                metabase.xrays.domain-entities.queries.util]}

--- a/src/metabase/pulse/api/pulse.clj
+++ b/src/metabase/pulse/api/pulse.clj
@@ -295,8 +295,11 @@
    ;; if advanced-permissions is enabled, only superuser or non-admin with subscription permission can
    ;; update pulse's recipients
    (when (premium-features/enable-advanced-permissions?)
-      (let [to-add-recipients (difference (set (map :id (:recipients (email-channel pulse-updates))))
-                                          (set (map :id (:recipients (email-channel pulse-before-update)))))
+      ;; key recipients the same way pulse-channel does: user recipients by :id, external recipients by
+      ;; :email, so changes to external addresses are part of the diff too
+      (let [recipient-key     (some-fn :id :email)
+            to-add-recipients (difference (set (keep recipient-key (:recipients (email-channel pulse-updates))))
+                                          (set (keep recipient-key (:recipients (email-channel pulse-before-update)))))
            current-user-has-application-permissions?
            (and (premium-features/enable-advanced-permissions?)
                 (resolve 'metabase-enterprise.advanced-permissions.common/current-user-has-application-permissions?))
--- a/src/metabase/util/markdown.clj
+++ b/src/metabase/util/markdown.clj
@@ -4,7 +4,8 @@
   [clojure.java.io :as io]
   [clojure.string :as str]
   [clojure.walk :as walk]
-   [metabase.util :as u])
+   [metabase.util :as u]
+   [metabase.util.markdown.image :as markdown.image])
  (:import
   (com.vladsch.flexmark.ast AutoLink BlockQuote BulletList BulletListItem Code Emphasis FencedCodeBlock HardLineBreak
                             Heading HtmlBlock HtmlCommentBlock HtmlEntity HtmlInline HtmlInlineBase HtmlInlineComment
@@ -16,7 +17,7 @@
   (com.vladsch.flexmark.parser Parser)
   (com.vladsch.flexmark.util.ast Document Node)
   (com.vladsch.flexmark.util.data MutableDataSet)
-   (java.net URI)))
+   (java.net URI URISyntaxException)))

 (set! *warn-on-reflection* true)

@@ -202,17 +203,28 @@
  link-resolver callback."
  nil)

+(def ^:private allowed-link-schemes
+  "URI schemes markdown links/images may use in rendered notifications; links with other schemes are dropped."
+  #{"http" "https" "mailto"})
+
 (defn- resolve-uri
  "If the provided URI is a relative path, resolve it relative to the site URL so that links work
-  correctly in Slack/Email."
+  correctly in Slack/Email. Returns nil for URIs that are malformed or whose scheme isn't in
+  [[allowed-link-schemes]] — callers should drop the link."
  [^String uri]
  (letfn [(ensure-slash ^String [s] (when s
                                      (cond-> s
                                        (not (str/ends-with? s "/")) (str "/"))))]
    (when uri
-      (if-let [site-url (ensure-slash *site-url*)]
-        (.. (URI. site-url) (resolve uri) toString)
-        uri))))
+      (if (re-find markdown.image/data-image-uri-pattern uri)
+        uri
+        (try
+          (let [scheme (.getScheme (URI. uri))]
+            (when (or (nil? scheme) (allowed-link-schemes (u/lower-case-en scheme)))
+              (if-let [site-url (ensure-slash *site-url*)]
+                (.. (URI. site-url) (resolve uri) toString)
+                uri)))
+          (catch URISyntaxException _ nil))))))

 (defn- ^:private strip-tag
  "Given the value from the :content field of a Markdown AST node, and a keyword representing a tag type, converts all
@@ -317,20 +329,35 @@
  [{:keys [content attrs]}]
  (let [resolved-uri     (resolve-uri (:href attrs))
        resolved-content (resolved-content content)]
-    (if (contains? #{:image :image-ref} (:tag (first content)))
+    (cond
+      ;; a dropped (malformed or non-allow-listed) link renders as its text alone
+      (nil? resolved-uri)
+      resolved-content
+
      ;; If this is a linked image, add link target on separate line after image placeholder
+      (contains? #{:image :image-ref} (:tag (first content)))
      [resolved-content "\n(" resolved-uri ")"]
+
+      :else
      ["<" resolved-uri "|" resolved-content ">"])))

 (defmethod ast->slack :link-ref
  [{:keys [content attrs]}]
-  (let [resolved-uri     (resolve-uri (-> attrs :reference :attrs :url))
+  (let [url              (-> attrs :reference :attrs :url)
+        resolved-uri     (resolve-uri url)
        resolved-content (resolved-content content)]
-    (if resolved-uri
+    (cond
+      resolved-uri
      ["<" resolved-uri "|" resolved-content ">"]
+
      ;; If this was parsed as a link-ref but has no reference, assume it was just a pair of square brackets and
      ;; restore them. This is a known discrepancy between flexmark-java and Markdown rendering on the frontend.
-      ["[" resolved-content "]"])))
+      (nil? url)
+      ["[" resolved-content "]"]
+
+      ;; a dropped (malformed or non-allow-listed) reference renders as its text alone, like a dropped inline link
+      :else
+      resolved-content)))

 (defmethod ast->slack :auto-link
  [{{href :href} :attrs}]
@@ -363,20 +390,23 @@
  (map-indexed (fn [idx list-item] [(inc idx) ". " list-item])
               (resolved-content content)))

+(defn- slack-image
+  "Replace an image with text that links to its source, including alt text if available. A dropped (malformed
+  or non-allow-listed) source renders the placeholder text alone."
+  [src alt]
+  (let [placeholder (if (str/blank? alt) "[Image]" (str "[Image: " alt "]"))]
+    (if-let [resolved-uri (resolve-uri src)]
+      ["<" resolved-uri "|" placeholder ">"]
+      placeholder)))
+
 (defmethod ast->slack :image
  [{{:keys [src alt]} :attrs}]
-  ;; Replace images with text that links to source, including alt text if available
-  (if (str/blank? alt)
-    ["<" src "|[Image]>"]
-    ["<" src "|[Image: " alt "]>"]))
+  (slack-image src alt))

 (defmethod ast->slack :image-ref
  [{:keys [content attrs]}]
-  (let [src (-> attrs :reference :attrs :url)
-        alt (-> content resolved-content resolved-content-string)]
-    (if (str/blank? alt)
-      ["<" src "|[Image]>"]
-      ["<" src "|[Image: " alt "]>"])))
+  (slack-image (-> attrs :reference :attrs :url)
+               (-> content resolved-content resolved-content-string)))

 (defmethod ast->slack :html-entity
  [{content :content}]
@@ -403,14 +433,21 @@
                     (^LinkResolver apply [_this ^LinkResolverBasicContext _context]
                       (reify LinkResolver
                         (resolveLink [_this node _context link]
-                           (if-let [url (cond
-                                          (instance? MailLink node) (.getUrl link)
-                                          (empty-link-ref? node) nil
-                                          :else (resolve-uri (.getUrl link)))]
+                           (cond
+                             (instance? MailLink node)
                             (.. link
                                 (withStatus LinkStatus/VALID)
-                                 (withUrl url))
-                             link)))))]
+                                 (withUrl (.getUrl link)))
+
+                             (empty-link-ref? node)
+                             link
+
+                             :else
+                             ;; nil from resolve-uri means the URL is malformed or not allow-listed —
+                             ;; blank it out rather than let flexmark render the original one
+                             (.. link
+                                 (withStatus LinkStatus/VALID)
+                                 (withUrl (or (resolve-uri (.getUrl link)) ""))))))))]
    (.build (.linkResolverFactory (HtmlRenderer/builder options) lr-factory))))

 (defmulti process-markdown
--- a/src/metabase/util/markdown/image.cljc
+++ b/src/metabase/util/markdown/image.cljc
@@ -0,0 +1,7 @@
+(ns metabase.util.markdown.image
+  "Markdown image handling shared between the backend notification renderer and the app's markdown
+  renderer (compiled to JS via the shadow-cljs :app build), so both accept the same inline images.")
+
+(def ^:export data-image-uri-pattern
+  "Inline base64 image URIs markdown may embed directly, in addition to regular http(s) images."
+  #"(?i)^data:image/(?:png|jpeg|jpg|gif|svg\+xml|webp);base64,")
--- a/test/metabase/util/markdown_test.clj
+++ b/test/metabase/util/markdown_test.clj
@@ -1,6 +1,8 @@
 (ns metabase.util.markdown-test
  (:require
+   [clojure.string :as str]
   [clojure.test :refer :all]
+   [metabase.util :as u]
   [metabase.util.markdown :as markdown]))

 (defn- slack
@@ -250,3 +252,53 @@
  (testing "HTML in the source markdown is escaped properly, but HTML entities are retained"
    (is (= "<p>&lt;h1&gt;header&lt;/h1&gt;</p>\n" (html "<h1>header</h1>")))
    (is (= "<p>&amp;</p>\n"                       (html "&amp;")))))
+
+(deftest ^:parallel process-markdown-link-scheme-test
+  (testing "Only links/images with allow-listed URI schemes are rendered"
+    (doseq [markdown ["[x](javascript:alert(1))"
+                      "[x](JaVaScRiPt:alert(1))"
+                      "[x](JAVASCRIPT:alert(1))"
+                      "[x](javascript&#58;alert(1))"
+                      "[x](vbscript:msgbox(1))"
+                      "[x](data:text/html;base64,PHNjcmlwdD5hbGVydCgxKTwvc2NyaXB0Pg==)"
+                      "![x](data:text/html;base64,PHNjcmlwdD4=)"]
+            :let     [rendered (u/lower-case-en (html markdown))]
+            scheme   ["javascript" "vbscript" "data:"]]
+      (testing (str markdown " does not render " scheme)
+        (is (not (str/includes? rendered scheme)))))))
+
+(deftest ^:parallel process-markdown-link-scheme-slack-test
+  (testing "Links with non-allow-listed schemes render as their text alone in Slack"
+    (is (= "x" (slack "[x](javascript:alert(1))")))
+    (is (= "x" (slack "[x](DaTa:text/html;base64,PHNjcmlwdD4=)")))
+    (is (= "x" (slack "[x][ref]\n\n[ref]: javascript:alert(1)")))
+    (is (= "[x]" (slack "[x]")) "a bare bracket pair with no reference is still restored"))
+  (testing "Images with non-allow-listed schemes render as their placeholder alone in Slack"
+    (is (= "[Image: x]" (slack "![x](javascript:alert(1))")))
+    (is (= "[Image]"    (slack "![](data:text/html;base64,PHNjcmlwdD4=)")))
+    (is (= "[Image: x]" (slack "![x][ref]\n\n[ref]: javascript:alert(1)"))))
+  (testing "Slack image sources are resolved against the site URL and allow inline base64 images"
+    (is (= "<https://example.com/a.png|[Image: x]>" (slack "![x](/a.png)" "https://example.com")))
+    (is (= "<https://example.com/a.png|[Image: x]>" (slack "![x][ref]\n\n[ref]: a.png" "https://example.com")))
+    (is (= "<data:image/png;base64,iVBORw0KGgo=|[Image: x]>" (slack "![x](data:image/png;base64,iVBORw0KGgo=)")))))
+
+(deftest ^:parallel process-markdown-allowed-scheme-case-test
+  (testing "Scheme allow-listing is case-insensitive for allowed schemes too"
+    (is (= "<p><a href=\"HTTPS://metabase.com\">x</a></p>\n" (html "[x](HTTPS://metabase.com)")))))
+
+(deftest ^:parallel process-markdown-data-image-test
+  (testing "Inline base64 images render like they do in the app's markdown renderer"
+    (is (= "<p><img src=\"data:image/png;base64,iVBORw0KGgo=\" alt=\"x\" /></p>\n"
+           (html "![x](data:image/png;base64,iVBORw0KGgo=)" "https://example.com")))
+    (is (= "<p><img src=\"data:image/svg+xml;base64,PHN2Zy8+\" alt=\"x\" /></p>\n"
+           (html "![x](data:image/svg+xml;base64,PHN2Zy8+)" "https://example.com")))))
+
+(deftest ^:parallel process-markdown-malformed-uri-test
+  (testing "A link destination that isn't a parseable URI drops the link instead of aborting the render"
+    (doseq [markdown ["[x](data:text/html,<b>y</b>)"
+                      "[x](<http://exa mple.com/foo>)"]
+            channel  [:html :slack]
+            :let     [rendered (markdown/process-markdown markdown channel "https://example.com")]]
+      (testing (format "%s via %s" markdown channel)
+        (is (string? rendered))
+        (is (not (str/includes? (u/lower-case-en rendered) "data:text")))))))