From 166fe3bb34f99bd0baa613726486ce0f13c7a992 Mon Sep 17 00:00:00 2001
From: David Taylor <david@taylorhq.com>
Date: Thu, 19 May 2022 11:18:30 +0100
Subject: [PATCH] FIX: Apply 'allowed_href_schemes' to all src/srcset
 attributes (#16860)

Previously we were only applying the restriction to `a[href]` and `img[src]`. This commit ensures we apply the same logic to all allowlisted media src attributes.
---
 .../pretty-text/addon/allow-lister.js         |  6 +--
 .../pretty-text/addon/sanitizer.js            | 46 +++++++++++++++++--
 spec/lib/pretty_text_spec.rb                  | 31 +++++++++++++
 3 files changed, 76 insertions(+), 7 deletions(-)

diff --git a/app/assets/javascripts/pretty-text/addon/allow-lister.js b/app/assets/javascripts/pretty-text/addon/allow-lister.js
index 764eaa8188c..b044c7cc073 100644
--- a/app/assets/javascripts/pretty-text/addon/allow-lister.js
+++ b/app/assets/javascripts/pretty-text/addon/allow-lister.js
@@ -176,6 +176,7 @@ export const DEFAULT_LIST = [
   "img[title]",
   "img[width]",
   "img[data-thumbnail]",
+  // img[src] handled by sanitizer.js
   "ins",
   "kbd",
   "li",
@@ -203,14 +204,13 @@ export const DEFAULT_LIST = [
   "sub",
   "sup",
   "source[data-orig-src]",
-  "source[src]",
-  "source[srcset]",
+  // source[src] and source[srcset] handled by sanitizer.js
   "source[type]",
   "track",
   "track[default]",
   "track[label]",
   "track[kind]",
-  "track[src]",
+  // track[src] handled by sanitizer.js
   "track[srclang]",
   "ul",
   "video",
diff --git a/app/assets/javascripts/pretty-text/addon/sanitizer.js b/app/assets/javascripts/pretty-text/addon/sanitizer.js
index 492f0c4b162..6592232bef2 100644
--- a/app/assets/javascripts/pretty-text/addon/sanitizer.js
+++ b/app/assets/javascripts/pretty-text/addon/sanitizer.js
@@ -41,6 +41,38 @@ export function hrefAllowed(href, extraHrefMatchers) {
   }
 }
 
+function sanitizeMediaSrc(tag, attrName, value, extraHrefMatchers) {
+  const srcAttrs = {
+    img: ["src"],
+    source: ["src", "srcset"],
+    track: ["src"],
+  };
+
+  if (!srcAttrs[tag]?.includes(attrName)) {
+    return;
+  }
+
+  if (value.startsWith("data:image")) {
+    return attr(attrName, value);
+  }
+
+  if (attrName === "srcset") {
+    const srcset = value.split(",").map((v) => v.split(" ", 2));
+    const sanitizedValue = srcset
+      .map((src) => {
+        const allowedSrc = hrefAllowed(src[0], extraHrefMatchers);
+        if (allowedSrc) {
+          return src[1] ? `${allowedSrc} ${src[1]}` : allowedSrc;
+        }
+      })
+      .join(",");
+    return attr(attrName, sanitizedValue);
+  } else {
+    const returnVal = hrefAllowed(value, extraHrefMatchers);
+    return attr(attrName, returnVal);
+  }
+}
+
 function testDataAttribute(forTag, name, value) {
   return Object.keys(forTag).find((k) => {
     const nameWithMatcher = `^${k.replace(/\*$/, "\\w+?")}`;
@@ -94,10 +126,6 @@ export function sanitize(text, allowLister) {
           (tag === "a" &&
             name === "href" &&
             hrefAllowed(value, extraHrefMatchers)) ||
-          (tag === "img" &&
-            name === "src" &&
-            (/^data:image.*$/i.test(value) ||
-              hrefAllowed(value, extraHrefMatchers))) ||
           (tag === "iframe" &&
             name === "src" &&
             allowedIframes.some((i) => {
@@ -107,6 +135,16 @@ export function sanitize(text, allowLister) {
           return attr(name, value);
         }
 
+        const sanitizedMediaSrc = sanitizeMediaSrc(
+          tag,
+          name,
+          value,
+          extraHrefMatchers
+        );
+        if (sanitizedMediaSrc) {
+          return sanitizedMediaSrc;
+        }
+
         if (tag === "iframe" && name === "src") {
           return "-STRIP-";
         }
diff --git a/spec/lib/pretty_text_spec.rb b/spec/lib/pretty_text_spec.rb
index 51c4df5fd15..9cf780d34c6 100644
--- a/spec/lib/pretty_text_spec.rb
+++ b/spec/lib/pretty_text_spec.rb
@@ -1371,6 +1371,37 @@ describe PrettyText do
     expect(cooked).to eq(n expected)
   end
 
+  it "applies scheme restrictions to img[src] attributes" do
+    SiteSetting.allowed_href_schemes = "steam"
+    cooked = cook "![Steam URL Image](steam://store/452530) ![Other scheme image](itunes://store/452530)"
+    expected = '<p><img src="steam://store/452530" alt="Steam URL Image"> <img src="" alt="Other scheme image"></p>'
+    expect(cooked).to eq(n expected)
+  end
+
+  it "applies scheme restrictions to track[src] and source[src]" do
+    SiteSetting.allowed_href_schemes = "steam"
+    cooked = cook <<~MD
+      <video>
+        <source src="steam://store/452530"><source src="itunes://store/452530"><track src="steam://store/452530"><track src="itunes://store/452530">
+      </video>
+    MD
+    expect(cooked).to include <<~HTML
+      <source src="steam://store/452530"><source src=""><track src="steam://store/452530"><track src="">
+    HTML
+  end
+
+  it "applies scheme restrictions to source[srcset]" do
+    SiteSetting.allowed_href_schemes = "steam"
+    cooked = cook <<~MD
+      <video>
+        <source srcset="steam://store/452530 1x,itunes://store/123 2x"><source srcset="steam://store/452530"><source srcset="itunes://store/452530">
+      </video>
+    MD
+    expect(cooked).to include <<~HTML
+      <source srcset="steam://store/452530 1x,"><source srcset="steam://store/452530"><source srcset="">
+    HTML
+  end
+
   it 'allows only tel URL scheme to start with a plus character' do
     SiteSetting.allowed_href_schemes = "tel|steam"
     cooked = cook("[Tel URL Scheme](tel://+452530579785)")