pierrejeambrun commented on code in PR #52795:
URL: https://github.com/apache/airflow/pull/52795#discussion_r2184773935


##########
airflow-core/src/airflow/ui/src/pages/Iframe.tsx:
##########
@@ -49,12 +49,38 @@ export const Iframe = ({ sandbox = "allow-same-origin 
allow-forms" }: { readonly
     return <ErrorPage />;
   }
 
+  // Build the href URL with context parameters if the view has a destination
+  let src = iframeView.href;
+
+  if (iframeView.destination !== undefined && iframeView.destination !== 
"nav") {
+    // Check if the href contains placeholders that need to be replaced
+    if (dagId !== undefined) {
+      src = src.replaceAll("{dag_id}", dagId);
+    }
+    if (runId !== undefined) {
+      src = src.replaceAll("{run_id}", runId);
+    }
+    if (taskId !== undefined) {
+      src = src.replaceAll("{task_id}", taskId);

Review Comment:
   In the backend doc we have `{DAG_ID}, {RUN_ID}, etc...` as a pattern. Do we 
want to replace it with lower case `{dag_id}, {run_id}, etc...`? 



##########
airflow-core/src/airflow/ui/src/router.tsx:
##########
@@ -208,6 +257,15 @@ export const routerConfig = [
           { element: <TaskOverview />, index: true },
           { element: <TaskInstances />, path: "task_instances" },
           { element: <Events />, path: "events" },
+          {
+            // 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.
+            element: <Iframe sandbox="allow-scripts allow-same-origin 
allow-forms" />,
+            path: "plugin/:page",
+          },

Review Comment:
   Maybe extract this into a really small fn. "getPluginIframeRoute" or 
something, in the same file.
   
   Just so the comment, and how to construct the iframe component remains in 
one place and not copy pasted everywhere. I'm scared of someone updating in one 
place and not in other occurrences. (That doens't cost much to do and would 
help preventing mistakes in the future)



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to