From 733a9cf8702ef8e6dc404a15d8217ff08ea9c59f Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Thu, 6 Feb 2025 17:33:27 +0100
Subject: [PATCH 1/5] Set content-type to text/plain if dataset not safe

We only care about XSS in the content of the webapp, and for that it is
sufficient to set the content-type to text/plain.
---
 lib/galaxy/datatypes/data.py | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/lib/galaxy/datatypes/data.py b/lib/galaxy/datatypes/data.py
index 66462f51740..61a17f551ed 100644
--- a/lib/galaxy/datatypes/data.py
+++ b/lib/galaxy/datatypes/data.py
@@ -60,7 +60,6 @@ from galaxy.util.markdown import (
     indicate_data_truncated,
     literal_via_fence,
 )
-from galaxy.util.sanitize_html import sanitize_html
 from galaxy.util.zipstream import ZipstreamWrapper
 from . import (
     dataproviders as p_dataproviders,
@@ -635,21 +634,16 @@ class Data(metaclass=DataMeta):
         return result
 
     def _yield_user_file_content(self, trans, from_dataset: HasCreatingJob, filename: str, headers: Headers) -> IO:
-        """This method is responsible for sanitizing the HTML if needed."""
+        """This method sets the content type header to text/plain if we don't trust html content."""
         if trans.app.config.sanitize_all_html and headers.get("content-type", None) == "text/html":
-            # Sanitize anytime we respond with plain text/html content.
             # Check to see if this dataset's parent job is allowlisted
             # We cannot currently trust imported datasets for rendering.
-            if not from_dataset.creating_job.imported and from_dataset.creating_job.tool_id.startswith(
-                tuple(trans.app.config.sanitize_allowlist)
-            ):
-                return open(filename, mode="rb")
-
-            # This is returning to the browser, it needs to be encoded.
-            # TODO Ideally this happens a layer higher, but this is a bad
-            # issue affecting many tools
-            with open(filename) as f:
-                return sanitize_html(f.read()).encode("utf-8")
+            content_type = "text/html"
+            if from_dataset.creating_job.imported:
+                content_type = "text/plain"
+            if not from_dataset.creating_job.tool_id.startswith(tuple(trans.app.config.sanitize_allowlist)):
+                content_type = "text/plain"
+            headers["content-type"] = content_type
 
         return open(filename, mode="rb")
 

From 9233d19caa61b95f4f27516841f4ae7cbc614d68 Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Fri, 7 Feb 2025 12:42:40 +0100
Subject: [PATCH 2/5] Display warning if ouput sanitized

---
 .../entry/analysis/modules/CenterFrame.vue    | 131 ++++++++++++------
 client/src/entry/analysis/router.js           |   1 +
 lib/galaxy/datatypes/data.py                  |   2 +
 3 files changed, 94 insertions(+), 40 deletions(-)

diff --git a/client/src/entry/analysis/modules/CenterFrame.vue b/client/src/entry/analysis/modules/CenterFrame.vue
index 20e6a0cb080..01edade50b8 100644
--- a/client/src/entry/analysis/modules/CenterFrame.vue
+++ b/client/src/entry/analysis/modules/CenterFrame.vue
@@ -1,46 +1,97 @@
-<template>
-    <iframe
-        :id="id"
-        :name="id"
-        :src="srcWithRoot"
-        class="center-frame"
-        frameborder="0"
-        title="galaxy frame"
-        width="100%"
-        height="100%"
-        @load="onLoad" />
-</template>
-<script>
-import { withPrefix } from "utils/redirect";
+<script setup lang="ts">
+import { computed, ref, watch } from "vue";
 
-export default {
-    props: {
-        id: {
-            type: String,
-            default: "frame",
-        },
-        src: {
-            type: String,
-            default: "",
-        },
-    },
-    computed: {
-        srcWithRoot() {
-            return withPrefix(this.src);
-        },
-    },
-    methods: {
-        onLoad(ev) {
-            const iframe = ev.currentTarget;
-            const location = iframe.contentWindow && iframe.contentWindow.location;
+import { useUserStore } from "@/stores/userStore";
+import { withPrefix } from "@/utils/redirect";
+
+import Alert from "@/components/Alert.vue";
+import LoadingSpan from "@/components/LoadingSpan.vue";
+
+const emit = defineEmits(["load"]);
+const props = withDefaults(
+    defineProps<{
+        id?: string;
+        src?: string;
+        isPreview?: boolean;
+    }>(),
+    {
+        id: "frame",
+        src: "",
+        isPreview: false,
+    }
+);
+
+const { isAdmin } = useUserStore();
+const srcWithRoot = computed(() => withPrefix(props.src));
+const sanitizedImport = ref(false);
+const sanitizedToolId = ref<String | false>(false);
+const isLoading = ref(true);
+watch(
+    () => srcWithRoot.value,
+    async () => {
+        sanitizedImport.value = false;
+        sanitizedToolId.value = false;
+        if (props.isPreview) {
             try {
-                if (location && location.host && location.pathname != "/") {
-                    this.$emit("load");
+                const response = await fetch(srcWithRoot.value, { method: "HEAD" });
+                const isImported = response.headers.get("x-sanitized-job-imported");
+                const toolId = response.headers.get("x-sanitized-tool-id");
+                if (isImported !== null) {
+                    sanitizedImport.value = true;
+                } else if (toolId !== null) {
+                    sanitizedToolId.value = toolId;
                 }
-            } catch (err) {
-                console.warn("CenterFrame - onLoad location access forbidden.", ev, location);
+            } catch (e) {
+                // I guess that's fine and the center panel will show something
+                console.error(e);
             }
-        },
+        }
     },
-};
+    { immediate: true }
+);
+
+const plainText = "Contents are shown as plain text.";
+const sanitizedMessage = computed(() => {
+    if (sanitizedImport.value) {
+        return `Dataset has been imported. ${plainText}`;
+    } else if (sanitizedToolId.value) {
+        return `Dataset created by a tool that is not known to create safe HTML. ${plainText}`;
+    }
+    return undefined;
+});
+
+function onLoad(ev: Event) {
+    isLoading.value = false;
+    const iframe = ev.currentTarget as HTMLIFrameElement;
+    const location = iframe?.contentWindow && iframe.contentWindow.location;
+    try {
+        if (location && location.host && location.pathname != "/") {
+            emit("load");
+        }
+    } catch (err) {
+        console.warn("CenterFrame - onLoad location access forbidden.", ev, location);
+    }
+}
 </script>
+<template>
+    <div>
+        <Alert v-if="sanitizedMessage" :dismissible="true" variant="warning">
+            {{ sanitizedMessage }}
+            <p v-if="isAdmin && sanitizedToolId">
+                <router-link to="/admin/sanitize_allow">Review Allowlist</router-link> if outputs of
+                {{ sanitizedToolId }} are trusted and should be shown as HTML.
+            </p>
+        </Alert>
+        <LoadingSpan v-if="isLoading">Loading ...</LoadingSpan>
+        <iframe
+            :id="id"
+            :name="id"
+            :src="srcWithRoot"
+            class="center-frame"
+            frameborder="0"
+            title="galaxy frame"
+            width="100%"
+            height="100%"
+            @load="onLoad" />
+    </div>
+</template>
diff --git a/client/src/entry/analysis/router.js b/client/src/entry/analysis/router.js
index 033b70136cb..ba7da642ada 100644
--- a/client/src/entry/analysis/router.js
+++ b/client/src/entry/analysis/router.js
@@ -238,6 +238,7 @@ export function getRouter(Galaxy) {
                         component: CenterFrame,
                         props: (route) => ({
                             src: `/datasets/${route.params.datasetId}/display/?preview=True`,
+                            isPreview: true,
                         }),
                     },
                     {
diff --git a/lib/galaxy/datatypes/data.py b/lib/galaxy/datatypes/data.py
index 61a17f551ed..5ff2da26b13 100644
--- a/lib/galaxy/datatypes/data.py
+++ b/lib/galaxy/datatypes/data.py
@@ -641,8 +641,10 @@ class Data(metaclass=DataMeta):
             content_type = "text/html"
             if from_dataset.creating_job.imported:
                 content_type = "text/plain"
+                headers["x-sanitized-job-imported"] = True
             if not from_dataset.creating_job.tool_id.startswith(tuple(trans.app.config.sanitize_allowlist)):
                 content_type = "text/plain"
+                headers["x-sanitized-tool-id"] = from_dataset.creating_job.tool_id
             headers["content-type"] = content_type
 
         return open(filename, mode="rb")

From b5a784cbe2b7b575093e08768a3cccefc584cdd3 Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Fri, 7 Feb 2025 15:57:32 +0100
Subject: [PATCH 3/5] Expand center-frame to full-height

Seems required for center page.
---
 client/src/entry/analysis/modules/CenterFrame.vue | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/client/src/entry/analysis/modules/CenterFrame.vue b/client/src/entry/analysis/modules/CenterFrame.vue
index 01edade50b8..4d59a53eb26 100644
--- a/client/src/entry/analysis/modules/CenterFrame.vue
+++ b/client/src/entry/analysis/modules/CenterFrame.vue
@@ -1,4 +1,5 @@
 <script setup lang="ts">
+import { storeToRefs } from "pinia";
 import { computed, ref, watch } from "vue";
 
 import { useUserStore } from "@/stores/userStore";
@@ -21,7 +22,7 @@ const props = withDefaults(
     }
 );
 
-const { isAdmin } = useUserStore();
+const { isAdmin } = storeToRefs(useUserStore());
 const srcWithRoot = computed(() => withPrefix(props.src));
 const sanitizedImport = ref(false);
 const sanitizedToolId = ref<String | false>(false);
@@ -74,12 +75,12 @@ function onLoad(ev: Event) {
 }
 </script>
 <template>
-    <div>
-        <Alert v-if="sanitizedMessage" :dismissible="true" variant="warning">
+    <div class="h-100">
+        <Alert v-if="sanitizedMessage" :dismissible="true" variant="warning" data-description="sanitization warning">
             {{ sanitizedMessage }}
             <p v-if="isAdmin && sanitizedToolId">
-                <router-link to="/admin/sanitize_allow">Review Allowlist</router-link> if outputs of
-                {{ sanitizedToolId }} are trusted and should be shown as HTML.
+                <router-link data-description="allowlist link" to="/admin/sanitize_allow">Review Allowlist</router-link>
+                if outputs of {{ sanitizedToolId }} are trusted and should be shown as HTML.
             </p>
         </Alert>
         <LoadingSpan v-if="isLoading">Loading ...</LoadingSpan>

From 871a6901be109b6633cab4fbfc71188b8a5695cc Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Fri, 7 Feb 2025 19:55:16 +0100
Subject: [PATCH 4/5] Test sanitization

---
 lib/galaxy/selenium/has_driver.py             |  3 ++
 test/functional/tools/html_output.xml         | 25 +++++++++
 test/functional/tools/sample_tool_conf.xml    |  1 +
 .../test_allowlist_sanitization.py            | 52 +++++++++++++++++++
 4 files changed, 81 insertions(+)
 create mode 100644 test/functional/tools/html_output.xml
 create mode 100644 test/integration_selenium/test_allowlist_sanitization.py

diff --git a/lib/galaxy/selenium/has_driver.py b/lib/galaxy/selenium/has_driver.py
index 9fae270ca7b..d0ab5ed98ca 100644
--- a/lib/galaxy/selenium/has_driver.py
+++ b/lib/galaxy/selenium/has_driver.py
@@ -116,6 +116,9 @@ class HasDriver:
     def element_absent(self, selector_template: Target) -> bool:
         return len(self.find_elements(selector_template)) == 0
 
+    def switch_to_frame(self, name: str = "frame"):
+        return self._wait_on(ec.frame_to_be_available_and_switch_to_it((By.NAME, name)))
+
     def wait_for_xpath(self, xpath: str, **kwds) -> WebElement:
         element = self._wait_on(
             ec.presence_of_element_located((By.XPATH, xpath)), f"XPATH selector [{xpath}] to become present", **kwds
diff --git a/test/functional/tools/html_output.xml b/test/functional/tools/html_output.xml
new file mode 100644
index 00000000000..7558bca44ec
--- /dev/null
+++ b/test/functional/tools/html_output.xml
@@ -0,0 +1,25 @@
+<tool id="html_output" name="html_output" version="1.0.0">
+    <command><![CDATA[
+cp '$html_file' '$output'
+    ]]></command>
+    <configfiles>
+        <configfile name="html_file"><![CDATA[
+<!DOCTYPE html>
+<html lang="en">
+<head>
+    <meta charset="UTF-8">
+    <meta name="viewport" content="width=device-width, initial-scale=1.0">
+    <title data-description="hello-world">Hello World</title>
+</head>
+<body>
+    <main>
+       <h1>Hello, World!</h1>
+    </main>
+</body>
+</html>
+        ]]></configfile>
+    </configfiles>
+    <outputs>
+        <data name="output" format="html" />
+    </outputs>
+</tool>
diff --git a/test/functional/tools/sample_tool_conf.xml b/test/functional/tools/sample_tool_conf.xml
index bb9d7568f60..3e0b6fb3729 100644
--- a/test/functional/tools/sample_tool_conf.xml
+++ b/test/functional/tools/sample_tool_conf.xml
@@ -243,6 +243,7 @@
 
   <tool file="multiple_versions_changes_v01.xml" />
   <tool file="multiple_versions_changes_v02.xml" />
+  <tool file="html_output.xml" />
 
   <tool file="interactivetool_simple.xml" />
   <tool file="interactivetool_two_entry_points.xml" />
diff --git a/test/integration_selenium/test_allowlist_sanitization.py b/test/integration_selenium/test_allowlist_sanitization.py
new file mode 100644
index 00000000000..109862158cb
--- /dev/null
+++ b/test/integration_selenium/test_allowlist_sanitization.py
@@ -0,0 +1,52 @@
+from .framework import (
+    selenium_test,
+    SeleniumIntegrationTestCase,
+)
+
+
+class TestAllowListSanitization(SeleniumIntegrationTestCase):
+    run_as_admin = True
+    axe_skip = True  # skip testing iframe contents
+
+    @classmethod
+    def handle_galaxy_config_kwds(cls, config):
+        super().handle_galaxy_config_kwds(config)
+        config["sanitize_all_html"] = True
+
+    def run_html_output(self):
+        history_id = self.dataset_populator.new_history()
+        run_response = self.dataset_populator.run_tool("html_output", {}, history_id=history_id)
+        hda_id = run_response["outputs"][0]["id"]
+        self.dataset_populator.wait_for_dataset(history_id, dataset_id=hda_id)
+        return hda_id
+
+    @selenium_test
+    def test_html_output_sanitized(self):
+        self.login()
+        hda_id = self.run_html_output()
+        self.get(f"datasets/{hda_id}/preview")
+        self.wait_for_selector_visible("[data-description='sanitization warning']")
+        self.assert_selector_absent("[data-description='allowlist link']")
+        self.screenshot("sanitization warning")
+        assert self.switch_to_frame()
+        self.assert_selector_absent("[data-description='hello-world']")
+
+    @selenium_test
+    def test_html_output_sanitized_admin(self):
+        self.admin_login()
+        hda_id = self.run_html_output()
+        self.get(f"datasets/{hda_id}/preview")
+        self.wait_for_selector_visible("[data-description='sanitization warning']")
+        self.wait_for_selector_visible("[data-description='allowlist link']")
+        self.screenshot("sanitization warning admin")
+        try:
+            self._put(
+                "/api/sanitize_allow?tool_id=html_output", data={"params": {"tool_id": "html_output"}}, admin=True
+            ).raise_for_status()
+            self.driver.refresh()
+            self.assert_selector_absent("[data-description='sanitization warning']")
+            self.assert_selector_absent("[data-description='allowlist link']")
+            assert self.switch_to_frame()
+            self.wait_for_selector("[data-description='hello-world']")
+        finally:
+            self._delete("/api/sanitize_allow?tool_id=html_output", admin=True)

From 12dc5cd1cd296c277612533a1f955178c4a8557e Mon Sep 17 00:00:00 2001
From: mvdbeek <m.vandenbeek@gmail.com>
Date: Fri, 7 Feb 2025 20:04:12 +0100
Subject: [PATCH 5/5] Tweak warning message display

---
 client/src/entry/analysis/modules/CenterFrame.vue | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/client/src/entry/analysis/modules/CenterFrame.vue b/client/src/entry/analysis/modules/CenterFrame.vue
index 4d59a53eb26..7159891d883 100644
--- a/client/src/entry/analysis/modules/CenterFrame.vue
+++ b/client/src/entry/analysis/modules/CenterFrame.vue
@@ -78,10 +78,11 @@ function onLoad(ev: Event) {
     <div class="h-100">
         <Alert v-if="sanitizedMessage" :dismissible="true" variant="warning" data-description="sanitization warning">
             {{ sanitizedMessage }}
-            <p v-if="isAdmin && sanitizedToolId">
+            <span v-if="isAdmin && sanitizedToolId">
+                <br />
                 <router-link data-description="allowlist link" to="/admin/sanitize_allow">Review Allowlist</router-link>
                 if outputs of {{ sanitizedToolId }} are trusted and should be shown as HTML.
-            </p>
+            </span>
         </Alert>
         <LoadingSpan v-if="isLoading">Loading ...</LoadingSpan>
         <iframe