diff --git a/.vscode/settings.json b/.vscode/settings.json index c0e55c7550f..a04f616b296 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -5,5 +5,15 @@ "gulp.autoDetect": "off", "npm.autoDetect": "off", "typescript.tsdk": "node_modules/typescript/lib", - "cssVariables.lookupFiles": ["**/*.d.css", "node_modules/@mantine/core/*.css"] + "typescript.tsserver.maxTsServerMemory": 8192, + "cssVariables.lookupFiles": ["**/*.d.css", "node_modules/@mantine/core/*.css"], + "files.watcherExclude": { + "**/.claude/worktrees/**": true, + "**/node_modules/**": true, + "**/.cpcache/**": true, + "**/target/**": true + }, + "search.exclude": { + "**/.claude/worktrees": true + } } diff --git a/src/metabase/embedding_rest/api/preview_embed.clj b/src/metabase/embedding_rest/api/preview_embed.clj index ec8435950f6..7854c279ae7 100644 --- a/src/metabase/embedding_rest/api/preview_embed.clj +++ b/src/metabase/embedding_rest/api/preview_embed.clj @@ -15,6 +15,7 @@ [metabase.embedding.jwt :as embed] [metabase.embedding.validation :as embedding.validation] [metabase.parameters.schema :as parameters.schema] + [metabase.query-processor.middleware.permissions :as qp.perms] [metabase.query-processor.pivot :as qp.pivot] [metabase.request.core :as request] [metabase.tiles.api :as api.tiles] @@ -69,9 +70,12 @@ [:token string?] [:param-key string?]]] (let [unsigned-token (check-and-unsign token) - card (api.embed.common/card-for-unsigned-token - unsigned-token - :embedding-params (embed/get-in-unsigned-token-or-throw unsigned-token [:_embedding_params]))] + ;; resolving param values needs the Card's real query, so skip the query stripping + ;; in [[metabase.public-sharing-rest.api/remove-card-non-public-columns]] + card (binding [qp.perms/*param-values-query* true] + (api.embed.common/card-for-unsigned-token + unsigned-token + :embedding-params (embed/get-in-unsigned-token-or-throw unsigned-token [:_embedding_params])))] (api.embed.common/card-param-values {:unsigned-token unsigned-token :card card :param-key param-key}))) @@ -87,9 +91,12 @@ [:param-key ms/NonBlankString]] {:keys [value]} :- [:map [:value :string]]] (let [unsigned-token (check-and-unsign token) - card (api.embed.common/card-for-unsigned-token - unsigned-token - :embedding-params (embed/get-in-unsigned-token-or-throw unsigned-token [:_embedding_params]))] + ;; resolving param values needs the Card's real query, so skip the query stripping + ;; in [[metabase.public-sharing-rest.api/remove-card-non-public-columns]] + card (binding [qp.perms/*param-values-query* true] + (api.embed.common/card-for-unsigned-token + unsigned-token + :embedding-params (embed/get-in-unsigned-token-or-throw unsigned-token [:_embedding_params])))] (api.embed.common/card-param-remapped-value {:unsigned-token unsigned-token :card card :param-key param-key diff --git a/src/metabase/public_sharing_rest/api.clj b/src/metabase/public_sharing_rest/api.clj index bd6b5ca8825..b59997e87eb 100644 --- a/src/metabase/public_sharing_rest/api.clj +++ b/src/metabase/public_sharing_rest/api.clj @@ -1,6 +1,7 @@ (ns metabase.public-sharing-rest.api "Metabase API endpoints for viewing publicly-accessible Cards and Dashboards." (:require + [clojure.string :as str] [hiccup.core :as hiccup] [medley.core :as m] [metabase.actions.core :as actions] @@ -12,6 +13,8 @@ [metabase.dashboards.schema :as dashboards.schema] [metabase.documents.prose-mirror :as prose-mirror] [metabase.events.core :as events] + [metabase.lib.core :as lib] + [metabase.lib.metadata :as lib.metadata] [metabase.lib.schema.info :as lib.schema.info] [metabase.models.interface :as mi] [metabase.parameters.dashboard :as parameters.dashboard] @@ -61,9 +64,31 @@ [card] (assoc card :parameters (qp.card/combined-parameters-and-template-tags card))) +(defn- blank-dataset-query + "Replace the contents of `query` with a blank query of the same type, so the query contents themselves (SQL, + filters, aggregations, etc.) are never exposed to the general public. Native queries keep their parameter template + tags so the frontend can still derive parameters from them. MBQL queries keep only their source table or source + card, plus a placeholder `:count` aggregation per original aggregation, so the frontend can still tell MBQL and + native queries apart (e.g. to use the pivot endpoints) and resolve `:aggregation` column refs." + [query] + (if (lib/native? query) + (let [text (->> (lib/template-tags query) + (filter queries/parameter-template-tag?) + (map #(str "{{" (:name %) "}}")) + (str/join " "))] + (lib/with-native-query query (if (str/blank? text) "-" text))) + (if-let [source (or (some->> (lib/primary-source-table-id query) (lib.metadata/table query)) + (some->> (lib/primary-source-card-id query) (lib.metadata/card query)))] + (reduce lib/aggregate + (lib/query query source) + (repeatedly (count (lib/aggregations query -1)) lib/count)) + (lib/native-query query "-")))) + (defn remove-card-non-public-columns "Remove everything from public `card` that shouldn't be visible to the general public. + The `:dataset_query` is replaced with a blank query of the same type via [[blank-dataset-query]]. + This function is used by both OSS (for public cards) and EE (for cards in public documents) to ensure consistent filtering of sensitive fields across all public sharing endpoints." [card] @@ -73,8 +98,11 @@ (mi/instance :model/Card (-> card - (select-keys [:id :name :description :display :visualization_settings :parameters :entity_id :dataset_query]) - (update :dataset_query select-keys [:lib/metadata :lib/type :database :stages]))))) + (select-keys [:id :name :description :display :visualization_settings :parameters :param_fields :entity_id + :dataset_query]) + (update :dataset_query (fn [query] + (cond-> query + (seq query) blank-dataset-query))))))) (defn public-card "Return a public Card matching key-value `conditions`, removing all columns that should not be visible to the general @@ -84,9 +112,9 @@ (-> (api/check-404 (apply t2/select-one [:model/Card :id :dataset_query :description :display :name :parameters :visualization_settings :card_schema] :archived false, conditions)) - remove-card-non-public-columns combine-parameters-and-template-tags - (t2/hydrate :param_fields)))) + (t2/hydrate :param_fields) + remove-card-non-public-columns))) (defn- card-with-uuid [uuid] (public-card :public_uuid uuid)) diff --git a/src/metabase/queries/core.clj b/src/metabase/queries/core.clj index d0a690187b3..4b4d82c2381 100644 --- a/src/metabase/queries/core.clj +++ b/src/metabase/queries/core.clj @@ -37,6 +37,7 @@ maybe-unverify! model-supports-implicit-actions? model? + parameter-template-tag? sole-dashboard-id starting-card-schema-version update-card! diff --git a/src/metabase/queries/models/card.clj b/src/metabase/queries/models/card.clj index 9977b93a372..75adcf1ba57 100644 --- a/src/metabase/queries/models/card.clj +++ b/src/metabase/queries/models/card.clj @@ -339,6 +339,16 @@ ;;; NOTE: this should mirror `getTemplateTagParameters` in frontend/src/metabase-lib/parameters/utils/template-tags.ts ;;; If this function moves you should update the comment that links to this one (#40013) ;;; +(mu/defn parameter-template-tag? :- :boolean + "Whether a parameter is created for this template tag, as opposed to tags that splice content into the query itself, + like snippets, card references, and tables." + [{tag-type :type, widget-type :widget-type} :- [:maybe ::lib.schema.template-tag/template-tag]] + (boolean + (and tag-type + (or (contains? lib.schema.template-tag/raw-value-template-tag-types tag-type) + (= tag-type :temporal-unit) + (and (= tag-type :dimension) widget-type (not= widget-type :none)))))) + ;;; TODO -- does this belong HERE or in the `parameters` module? (mu/defn template-tag-parameters :- ::parameters.schema/parameters "Transforms native query's `template-tags` into `parameters`. @@ -346,10 +356,7 @@ should always be there. Apparently lots of e2e tests are sloppy about this so this is included as a convenience." [card :- [:maybe ::queries.schema/card]] (for [{tag-type :type, widget-type :widget-type, :as tag} (some-> card :dataset_query not-empty lib/all-template-tags) - :when (and tag-type - (or (contains? lib.schema.template-tag/raw-value-template-tag-types tag-type) - (= tag-type :temporal-unit) - (and (= tag-type :dimension) widget-type (not= widget-type :none))))] + :when (parameter-template-tag? tag)] {:id (:id tag) :type (or widget-type (case tag-type :temporal-unit :temporal-unit diff --git a/test/metabase/embedding_rest/api/embed_test.clj b/test/metabase/embedding_rest/api/embed_test.clj index 0896866b00a..664991aa2b5 100644 --- a/test/metabase/embedding_rest/api/embed_test.clj +++ b/test/metabase/embedding_rest/api/embed_test.clj @@ -151,7 +151,9 @@ :display "table" :visualization_settings {} :dataset_query {:lib/type "mbql/query" - :stages [{:lib/type "mbql.stage/mbql"}]} + :stages [{:lib/type "mbql.stage/mbql" + :source-table int? + :aggregation [["count" {:lib/uuid string?}]]}]} :parameters [] :param_fields {}}) @@ -171,6 +173,19 @@ (is (=? successful-card-info (client/client :get 200 (card-url card))))))) +(deftest embed-card-strips-dataset-query-test + (testing "GET /api/embed/card/:token replaces the Card's query with a blank native query" + (with-embedding-enabled-and-new-secret-key! + (with-temp-card [card {:enable_embedding true + :dataset_query (mt/native-query {:query "SELECT id FROM venues"})}] + (let [{:keys [dataset_query]} (client/client :get 200 (card-url card))] + (is (= {:lib/type "mbql/query" + :database (mt/id) + :stages [{:lib/type "mbql.stage/native" + :native "-" + :template-tags []}]} + dataset_query))))))) + (deftest we-should-fail-when-attempting-to-use-an-expired-token (with-embedding-enabled-and-new-secret-key! (with-temp-card [card {:enable_embedding true}] @@ -572,6 +587,18 @@ (is (=? successful-dashboard-info (client/client :get 200 (dashboard-url (:entity_id dash) dash))))))) +(deftest embed-dashboard-strips-dataset-query-test + (testing "GET /api/embed/dashboard/:token replaces each Card's query with a blank query" + (with-embedding-enabled-and-new-secret-key! + (with-temp-dashcard [dashcard {:dash {:enable_embedding true}}] + (let [response (client/client :get 200 (dashboard-url (:dashboard_id dashcard)))] + (is (=? {:lib/type "mbql/query" + :database (mt/id) + :stages [{:lib/type "mbql.stage/mbql" + :source-table (mt/id :venues) + :aggregation [["count" {:lib/uuid string?}]]}]} + (-> response :dashcards first :card :dataset_query)))))))) + (deftest bad-dashboard-id-fails (with-embedding-enabled-and-new-secret-key! (let [dashboard-url (str "embed/dashboard/" (sign {:resource {:dashboard "8"} :params {}}))] diff --git a/test/metabase/embedding_rest/api/preview_embed_test.clj b/test/metabase/embedding_rest/api/preview_embed_test.clj index 1b7c52990c2..d5d278a0dce 100644 --- a/test/metabase/embedding_rest/api/preview_embed_test.clj +++ b/test/metabase/embedding_rest/api/preview_embed_test.clj @@ -4,10 +4,14 @@ metabase.test.data/run-mbql-query {:namespaces [metabase.embedding-rest.api.preview-embed-test]}}}}}} (:require [buddy.sign.jwt :as jwt] + [clojure.set :as set] [clojure.test :refer :all] [metabase.dashboards-rest.api-test :as api.dashboard-test] [metabase.embedding-rest.api.embed-test :as embed-test] [metabase.embedding-rest.api.preview-embed :as api.preview-embed] + [metabase.lib.core :as lib] + [metabase.lib.metadata :as lib.metadata] + [metabase.queries-rest.api.card-test :as api.card-test] [metabase.query-processor.pivot.test-util :as api.pivots] [metabase.test :as mt] [metabase.tiles.api-test :as tiles.api-test] @@ -556,6 +560,69 @@ (format "preview_embed/card/%s/params/%s/values" signed-token "_STATIC_CATEGORY_")))))))))) +(deftest card-params-values-field-filter-test + (testing "GET /api/preview_embed/card/:token/params/:param-key/values with a field filter parameter" + (embed-test/with-embedding-enabled-and-new-secret-key! + (api.card-test/with-card-param-values-fixtures [{:keys [field-filter-card param-keys]}] + (let [embedding-params (zipmap (map (comp keyword :slug) (:parameters field-filter-card)) + (repeat "enabled")) + signed-token (embed-test/card-token field-filter-card + {:_embedding_params embedding-params}) + response (mt/user-http-request :crowberto :get 200 + (format "preview_embed/card/%s/params/%s/values" + signed-token (:field-values param-keys)))] + (is (false? (:has_more_values response))) + (is (set/subset? #{["20th Century Cafe"] ["33 Taps"]} + (-> response :values set)))))))) + +(deftest card-params-values-remapped-fields-test + (testing "preview embed card values/remapping endpoints work for parameters mapped to remapped fields" + (embed-test/with-embedding-enabled-and-new-secret-key! + (mt/with-column-remappings [orders.quantity {5 "N5"} + orders.product_id products.title] + (mt/with-temp [:model/Card card + (let [mp (mt/metadata-provider)] + {:dataset_query + (-> (lib/native-query mp "SELECT * FROM ORDERS JOIN PEOPLE ON ORDERS.USER_ID = PEOPLE.ID WHERE {{quantity}} AND {{product_id_fk}} AND {{user_id_pk}}") + (lib/with-template-tags + {"quantity" {:id "quantity" + :name "quantity" + :display-name "Internal" + :type :dimension + :widget-type :number/= + :dimension (lib/ref (lib.metadata/field mp (mt/id :orders :quantity)))} + "product_id_fk" {:id "product_id_fk" + :name "product_id_fk" + :display-name "FK" + :type :dimension + :widget-type :id + :dimension (lib/ref (lib.metadata/field mp (mt/id :orders :product_id)))} + "user_id_pk" {:id "user_id_pk" + :name "user_id_pk" + :display-name "PK->Name" + :type :dimension + :widget-type :id + :dimension (lib/ref (lib.metadata/field mp (mt/id :people :id)))}})) + :parameters [{:id "quantity", :name "Internal", :slug "quantity", :type "number/=" + :target ["dimension" ["template-tag" "quantity"]]} + {:id "product_id_fk", :name "FK", :slug "product_id_fk", :type "id" + :target ["dimension" ["template-tag" "product_id_fk"]]} + {:id "user_id_pk", :name "PK->Name", :slug "user_id_pk", :type "id" + :target ["dimension" ["template-tag" "user_id_pk"]]}]})] + (let [token (embed-test/card-token card {:_embedding_params {:quantity "enabled" + :product_id_fk "enabled" + :user_id_pk "enabled"}})] + (testing "values for internally-remapped param" + (is (map? (mt/user-http-request :crowberto :get 200 + (format "preview_embed/card/%s/params/quantity/values" token))))) + (testing "values for FK-remapped param" + (is (map? (mt/user-http-request :crowberto :get 200 + (format "preview_embed/card/%s/params/product_id_fk/values" token))))) + (testing "remapping for PK param" + (is (some? (mt/user-http-request :crowberto :get 200 + (format "preview_embed/card/%s/params/user_id_pk/remapping" token) + :value "1")))))))))) + (deftest params-with-static-list-test (testing "embedding with parameter that has source is a static list" (with-embedding-enabled-and-new-secret-key! diff --git a/test/metabase/public_sharing_rest/api_test.clj b/test/metabase/public_sharing_rest/api_test.clj index 348cc9ce09d..99d5a542f4d 100644 --- a/test/metabase/public_sharing_rest/api_test.clj +++ b/test/metabase/public_sharing_rest/api_test.clj @@ -144,6 +144,62 @@ (is (= "Not found." (client/client :get 404 (str "public/card/" uuid)))))))))) +(deftest fetch-card-strips-dataset-query-test + (testing "GET /api/public/card/:uuid replaces the Card's query with a blank query so its contents are not exposed" + (mt/with-temporary-setting-values [enable-public-sharing true] + (with-temp-public-card [{uuid :public_uuid}] + (let [{:keys [dataset_query]} (client/client :get 200 (str "public/card/" uuid))] + (is (=? {:lib/type "mbql/query" + :database (mt/id) + :stages [{:lib/type "mbql.stage/mbql" + :source-table (mt/id :venues) + :aggregation [["count" {:lib/uuid string?}]]}]} + dataset_query))))))) + +(deftest fetch-card-strips-native-query-keeps-parameter-tags-test + (testing "GET /api/public/card/:uuid strips the native query text but keeps parameter template tags" + (mt/with-temporary-setting-values [enable-public-sharing true] + (mt/with-temp [:model/NativeQuerySnippet snippet {:name "greeting" :content "'hello'"}] + (with-temp-public-card + [{uuid :public_uuid} + (let [mp (mt/metadata-provider)] + {:dataset_query + (-> (lib/native-query mp "SELECT {{snippet: greeting}} FROM venues WHERE {{price}}") + (lib/with-template-tags + {"price" {:id "_PRICE_" + :name "price" + :display-name "Price" + :type :dimension + :dimension (lib/ref (lib.metadata/field mp (mt/id :venues :price))) + :widget-type :category} + "snippet: greeting" {:type :snippet + :name "snippet: greeting" + :id (str (random-uuid)) + :snippet-name "greeting" + :display-name "Snippet: Greeting" + :snippet-id (:id snippet)}}))})] + (let [{:keys [dataset_query]} (client/client :get 200 (str "public/card/" uuid))] + (is (=? {:lib/type "mbql/query" + :database (mt/id) + :stages [{:lib/type "mbql.stage/native" + :native "{{price}}" + :template-tags [{:name "price" + :type "dimension" + :widget-type "category"}]}]} + dataset_query)))))))) + +(deftest fetch-dashboard-strips-dataset-query-test + (testing "GET /api/public/dashboard/:uuid replaces each Card's query with a blank query so its contents are not exposed" + (mt/with-temporary-setting-values [enable-public-sharing true] + (with-temp-public-dashboard-and-card [dash _card] + (let [response (client/client :get 200 (str "public/dashboard/" (:public_uuid dash)))] + (is (=? {:lib/type "mbql/query" + :database (mt/id) + :stages [{:lib/type "mbql.stage/mbql" + :source-table (mt/id :venues) + :aggregation [["count" {:lib/uuid string?}]]}]} + (-> response :dashcards first :card :dataset_query)))))))) + (deftest public-queries-are-counted-test (testing "GET /api/public/card/:uuid/query counts as a public query" (mt/with-temporary-setting-values [enable-public-sharing true] diff --git a/test/metabase/queries_rest/api/card_test.clj b/test/metabase/queries_rest/api/card_test.clj index cdbfc820835..c540dd9aa48 100644 --- a/test/metabase/queries_rest/api/card_test.clj +++ b/test/metabase/queries_rest/api/card_test.clj @@ -3700,6 +3700,14 @@ (is (set/subset? #{["Barney's Beanery"] ["bigmista's barbecue"]} (-> response :values set))))))))) +(deftest param-fields-hydrated-test + (testing "GET /api/card/:id hydrates :param_fields with the fields referenced by the card's template tags" + (with-card-param-values-fixtures [{:keys [field-filter-card]}] + (is (=? {:name_param_id [{:id (mt/id :venues :name) + :table_id (mt/id :venues) + :display_name "Name"}]} + (:param_fields (mt/user-http-request :rasta :get 200 (format "card/%d" (:id field-filter-card))))))))) + (deftest param-fields-excluded-without-view-data-permission-test (testing "param_fields should not include fields for tables where the user lacks view-data permission" (with-card-param-values-fixtures [{:keys [field-filter-card]}]