pierrejeambrun commented on code in PR #53377:
URL: https://github.com/apache/airflow/pull/53377#discussion_r2210057628
##########
airflow-core/src/airflow/ui/src/hooks/usePluginTabs.tsx:
##########
@@ -36,9 +36,11 @@ export const usePluginTabs = (destination: string):
Array<TabPlugin> => {
// Get external views with the specified destination and ensure they have
url_route
const externalViews =
pluginData?.plugins
- .flatMap((plugin) => plugin.external_views)
- .filter((view: ExternalViewResponse) => view.destination === destination
&& Boolean(view.url_route)) ??
- [];
+ .flatMap((plugin) => [...plugin.external_views, ...plugin.react_apps])
+ .filter(
+ (view: ExternalViewResponse | ReactAppResponse) =>
Review Comment:
Maybe also a rename to emphasize the fact that this is an API type.
```suggestion
(view: NavItemReponse) =>
```
##########
airflow-core/src/airflow/ui/src/pages/PluginView.tsx:
##########
@@ -0,0 +1,91 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import { Box } from "@chakra-ui/react";
+import { useTranslation } from "react-i18next";
+import { useParams } from "react-router-dom";
+
+import { usePluginServiceGetPlugins } from "openapi/queries";
+import { ProgressBar } from "src/components/ui";
+
+import { ErrorPage } from "./Error";
+import { Iframe } from "./Iframe";
+import { ReactPlugin } from "./ReactPlugin";
+
+export const PluginView = () => {
+ const { t: translate } = useTranslation();
+ const { page } = useParams();
+ const { data: pluginData, isLoading } = usePluginServiceGetPlugins();
+
+ // Check for external_views first
+ const externalView =
+ page === "legacy-fab-views"
+ ? {
+ destination: "nav" as const,
+ href: "/pluginsv2/",
+ name: translate("nav.legacyFabViews"),
+ url_route: "legacy-fab-views",
+ }
+ : pluginData?.plugins
+ .flatMap((plugin) => plugin.external_views)
+ .find((view) => (view.url_route ?? view.name.toLowerCase().replace("
", "-")) === page);
+
+ // Check for react_apps if no external view found
+ const reactApp = pluginData?.plugins
+ .flatMap((plugin) => plugin.react_apps)
+ .find((view) => (view.url_route ?? view.name.toLowerCase().replace(" ",
"-")) === page);
+
+ if (isLoading) {
+ return (
+ <Box flexGrow={1}>
+ <ProgressBar />
+ </Box>
+ );
+ }
+
+ // If external view is found, render Iframe component
+ if (externalView) {
+ return (
+ <Box
+ flexGrow={1}
+ height="100%"
+ m={-2} // Compensate for parent padding
+ minHeight={0}
+ >
+ <Iframe externalView={externalView} sandbox="allow-scripts
allow-forms" />
Review Comment:
We don't have 'same-origin' here, meaning that the embedded Iframe can't be
from airflow domain itself, is that expected ? I don't think it is.
##########
airflow-core/src/airflow/ui/src/pages/PluginView.tsx:
##########
Review Comment:
Maybe `ExternalViewIframe` or anything else `PluginView` is too generic.
##########
airflow-core/src/airflow/ui/src/router.tsx:
##########
@@ -60,13 +60,13 @@ import { XCom } from "src/pages/XCom";
import { client } from "./queryClient";
-const iframeRoute = {
+const pluginRoute = {
// The following iframe sandbox setting is intentionally less restrictive.
// This is considered safe because the framed content originates from the
Plugins,
// which is part of the deployment of Airflow and trusted as per our
security policy.
//
https://airflow.apache.org/docs/apache-airflow/stable/security/security_model.html
// They are not user provided plugins.
Review Comment:
This comment is not relevant anymore and should be removed or moved to the
appropriate place if we come back to a allow-same-origin + allow-script sandbox
somewhere.
##########
airflow-core/src/airflow/ui/src/utils/sanitizeUrl.ts:
##########
@@ -0,0 +1,84 @@
+/*!
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+type UrlType = "bundle" | "iframe";
+
+/**
+ * Sanitizes URLs for safe use in iframe src attributes or dynamic imports
+ * Prevents XSS by validating protocol and removing dangerous content
+ */
+export const sanitizeUrl = (url: string, type: UrlType): string => {
+ try {
+ // Parse the URL to validate its structure
+ const urlObj = new URL(url);
+
+ // Only allow http, https, and relative URLs
+ if (urlObj.protocol !== "http:" && urlObj.protocol !== "https:" &&
urlObj.protocol !== "") {
+ throw new Error("Invalid protocol");
Review Comment:
Interesting. One of the hypothesis I had was that, none of the external
views or react apps are user provided values.
Those are made by plugins developer and deployed by deployment manager, so I
figured we could trust such values.
WDYT, is that needed in some scenarios ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@airflow.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
