From 0a455d9a684c831cb43c539fdf7d03d590496be0 Mon Sep 17 00:00:00 2001
From: dpsutton <dan@dpsutton.com>
Date: Fri, 25 Oct 2024 11:17:29 -0500
Subject: [PATCH] Fix csp directives for embed previews (#49155)
* Fix csp directives for embed previews
We set content security directives to allow for iframes on
dashboards. This list did not include 'self' so we can't actually host
an iframe pointing at our, well, self.
Embed previews work by just embedding an iframe with the dashboard and
this breaks if we don't allow iframes from our self.
* e2e test
---------
Co-authored-by: Aleksandr Lesnenko <alxnddr@gmail.com>
---
e2e/support/config.js | 1 +
.../embedding-reproductions.cy.spec.js | 36 +++++++++++++++++++
src/metabase/server/middleware/security.clj | 2 +-
.../server/middleware/security_test.clj | 14 +++++---
4 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/e2e/support/config.js b/e2e/support/config.js
index 9581cb1d9d5..b0aa9cf230e 100644
--- a/e2e/support/config.js
+++ b/e2e/support/config.js
@@ -181,6 +181,7 @@ const mainConfig = {
},
}
: {}),
+ experimentalCspAllowList: ["frame-src"],
projectId: "ywjy9z",
numTestsKeptInMemory: process.env["CI"] ? 1 : 50,
reporter: "cypress-multi-reporters",
diff --git a/e2e/test/scenarios/embedding/embedding-reproductions.cy.spec.js b/e2e/test/scenarios/embedding/embedding-reproductions.cy.spec.js
index f7a9a029d4f..b22725565ec 100644
--- a/e2e/test/scenarios/embedding/embedding-reproductions.cy.spec.js
+++ b/e2e/test/scenarios/embedding/embedding-reproductions.cy.spec.js
@@ -2,6 +2,7 @@ import { SAMPLE_DATABASE } from "e2e/support/cypress_sample_database";
import {
addOrUpdateDashboardCard,
createNativeQuestion,
+ createQuestionAndDashboard,
describeEE,
filterWidget,
getDashboardCard,
@@ -987,3 +988,38 @@ describe("issue 40660", () => {
});
});
});
+
+describe("issue 49142", () => {
+ const questionDetails = {
+ name: "Products",
+ query: { "source-table": PRODUCTS_ID, limit: 2 },
+ };
+
+ const dashboardDetails = {
+ name: "Embeddable dashboard",
+ enable_embedding: true,
+ };
+
+ beforeEach(() => {
+ restore();
+ cy.signInAsAdmin();
+
+ createQuestionAndDashboard({
+ questionDetails,
+ dashboardDetails,
+ }).then(({ body: { dashboard_id } }) => {
+ visitDashboard(dashboard_id);
+ });
+ });
+
+ it("embedding preview should be always working", () => {
+ openStaticEmbeddingModal({
+ activeTab: "lookAndFeel",
+ previewMode: "preview",
+ });
+ cy.findByTestId("embed-preview-iframe")
+ .its("0.contentDocument.body")
+ .should("be.visible")
+ .and("contain", "Embeddable dashboard");
+ });
+});
diff --git a/src/metabase/server/middleware/security.clj b/src/metabase/server/middleware/security.clj
index 0ca4576a8a1..a25752168ae 100644
--- a/src/metabase/server/middleware/security.clj
+++ b/src/metabase/server/middleware/security.clj
@@ -104,7 +104,7 @@
(->> (str/split hosts-string #"[ ,\s\r\n]+")
(remove str/blank?)
(mapcat add-wildcard-entries)
- vec))
+ (into ["'self'"])))
(def ^{:doc "Parse the string of allowed iframe hosts, adding wildcard prefixes as needed."}
parse-allowed-iframe-hosts
diff --git a/test/metabase/server/middleware/security_test.clj b/test/metabase/server/middleware/security_test.clj
index 132b63bed07..f537825845b 100644
--- a/test/metabase/server/middleware/security_test.clj
+++ b/test/metabase/server/middleware/security_test.clj
@@ -60,10 +60,13 @@
(deftest csp-header-iframe-hosts-tests
(testing "Allowed iframe hosts setting is used in the CSP frame-src directive."
(tu/with-temporary-setting-values [public-settings/allowed-iframe-hosts "https://www.wikipedia.org, https://www.metabase.com https://clojure.org"]
- (is (= (str "frame-src https://wikipedia.org https://*.wikipedia.org https://www.wikipedia.org "
+ (is (= (str "frame-src 'self' https://wikipedia.org https://*.wikipedia.org https://www.wikipedia.org "
"https://metabase.com https://*.metabase.com https://www.metabase.com "
"https://clojure.org https://*.clojure.org")
- (csp-directive "frame-src"))))))
+ (csp-directive "frame-src")))))
+ (testing "Includes 'self' so embed previews work (#49142)"
+ (let [hosts (-> (csp-directive "frame-src") (str/split #"\s+") set)]
+ (is (contains? hosts "'self'") "frame-src hosts does not include 'self'"))))
(deftest xframeoptions-header-tests
(mt/with-premium-features #{:embedding}
@@ -232,7 +235,8 @@
(testing "The allowed iframe hosts parse in the expected way."
(let [default-hosts @#'public-settings/default-allowed-iframe-hosts]
(testing "The defaults hosts parse correctly"
- (is (= ["youtube.com"
+ (is (= ["'self'"
+ "youtube.com"
"*.youtube.com"
"youtu.be"
"*.youtu.be"
@@ -275,7 +279,7 @@
"*.x.com"]
(mw.security/parse-allowed-iframe-hosts default-hosts))))
(testing "Additional hosts a user may configure will parse correctly as well"
- (is (= ["localhost"
+ (is (= ["'self'" "localhost"
"http://localhost:8000"
"my.domain.local:9876"
"*"
@@ -286,5 +290,5 @@
"www.mysite.cool.com"]
(mw.security/parse-allowed-iframe-hosts "localhost, http://localhost:8000, my.domain.local:9876, *, www.mysite.com/, www.mysite.cool.com"))))
(testing "invalid hosts are not included"
- (is (= []
+ (is (= ["'self'"]
(mw.security/parse-allowed-iframe-hosts "asdf/wasd/:8000 */localhost:*")))))))
--
GitLab