You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "pierrejeambrun (via GitHub)" <gi...@apache.org> on 2025/04/09 16:40:12 UTC

[PR] Fix default base value [airflow]

pierrejeambrun opened a new pull request, #49013:
URL: https://github.com/apache/airflow/pull/49013

   closes: https://github.com/apache/airflow/issues/48825
   
   This need additional work:
   - Adjust tests
   - Make sure outside of breeze and with a url prefix things are working as expected.


-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix default base value [airflow]

Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #49013:
URL: https://github.com/apache/airflow/pull/49013#discussion_r2035753262


##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -329,14 +329,23 @@ def _requires_access(
         raise HTTPException(status.HTTP_403_FORBIDDEN, "Forbidden")
 
 
-def is_safe_url(target_url: str) -> bool:
+def is_safe_url(target_url: str, request: Request | None = None) -> bool:
     """
     Check that the URL is safe.
 
     Needs to belong to the same domain as base_url, use HTTP or HTTPS (no JavaScript/data schemes),
     is a valid normalized path.
     """
-    base_url = conf.get("api", "base_url")
+    base_url: str | None = conf.get("api", "base_url")

Review Comment:
   Even with your change, it still has a default of empty string :) 



-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix default base value [airflow]

Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #49013:
URL: https://github.com/apache/airflow/pull/49013#discussion_r2037038234


##########
airflow-core/tests/unit/api_fastapi/core_api/test_security.py:
##########
@@ -120,6 +120,7 @@ async def test_requires_access_dag_unauthorized(self, mock_get_auth_manager):
             ("https://server_base_url.com/prefix", True),
             ("/prefix/some_other", True),
             ("prefix/some_other", True),
+            ("https://requesting_server_base_url.com/prefix2", True),  # safe in regards to the request url

Review Comment:
   Thanks for adding this



-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix default base value [airflow]

Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #49013:
URL: https://github.com/apache/airflow/pull/49013#discussion_r2035752556


##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -329,14 +329,23 @@ def _requires_access(
         raise HTTPException(status.HTTP_403_FORBIDDEN, "Forbidden")
 
 
-def is_safe_url(target_url: str) -> bool:
+def is_safe_url(target_url: str, request: Request | None = None) -> bool:
     """
     Check that the URL is safe.
 
     Needs to belong to the same domain as base_url, use HTTP or HTTPS (no JavaScript/data schemes),
     is a valid normalized path.
     """
-    base_url = conf.get("api", "base_url")
+    base_url: str | None = conf.get("api", "base_url")

Review Comment:
   The way the config is defined it's impossible for this to ever actually be None btw -- it's got a default value, so the only way of overriding it would be to an empty string



-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix default base value [airflow]

Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #49013:
URL: https://github.com/apache/airflow/pull/49013#issuecomment-2791251721

   I just fixed / added a few tests. I was able to verify the dagrun link for triggered dag run too:
   ![Screenshot 2025-04-10 at 02 24 27](https://github.com/user-attachments/assets/91268d21-1581-4631-afb6-bbb7d7d246d9)
   


-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Automatically set API base_url to a good value so UI is reachable locally [airflow]

Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #49013:
URL: https://github.com/apache/airflow/pull/49013#issuecomment-2793415562

   This introduced a bug when running Airflow with Breeze, it redirects the user to `http://localhost:8080/` which does not exist


-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix default base value [airflow]

Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #49013:
URL: https://github.com/apache/airflow/pull/49013#discussion_r2035754416


##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -329,14 +329,23 @@ def _requires_access(
         raise HTTPException(status.HTTP_403_FORBIDDEN, "Forbidden")
 
 
-def is_safe_url(target_url: str) -> bool:
+def is_safe_url(target_url: str, request: Request | None = None) -> bool:
     """
     Check that the URL is safe.
 
     Needs to belong to the same domain as base_url, use HTTP or HTTPS (no JavaScript/data schemes),
     is a valid normalized path.
     """
-    base_url = conf.get("api", "base_url")
+    base_url: str | None = conf.get("api", "base_url")

Review Comment:
   yes but if it's empty, we then take it from the `referer` header of the request. Which might be missing...
   
   But i agree this is confusing and need to be cleaned so it's clearer.



-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix default base value [airflow]

Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun closed pull request #49013: Fix default base value
URL: https://github.com/apache/airflow/pull/49013


-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix default base value [airflow]

Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #49013:
URL: https://github.com/apache/airflow/pull/49013#issuecomment-2791298715

   A couple more tests need fixing and we should be good to go. 
   


-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Automatically set API base_url to a good value so UI is reachable locally [airflow]

Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun merged PR #49013:
URL: https://github.com/apache/airflow/pull/49013


-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix default base value [airflow]

Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #49013:
URL: https://github.com/apache/airflow/pull/49013#discussion_r2035754416


##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -329,14 +329,23 @@ def _requires_access(
         raise HTTPException(status.HTTP_403_FORBIDDEN, "Forbidden")
 
 
-def is_safe_url(target_url: str) -> bool:
+def is_safe_url(target_url: str, request: Request | None = None) -> bool:
     """
     Check that the URL is safe.
 
     Needs to belong to the same domain as base_url, use HTTP or HTTPS (no JavaScript/data schemes),
     is a valid normalized path.
     """
-    base_url = conf.get("api", "base_url")
+    base_url: str | None = conf.get("api", "base_url")

Review Comment:
   yes but if it's empty, we then take it from the `referer` header of the request. Which might be missing...
   
   But i agree this is confusing and need to be cleaned.



-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Fix default base value [airflow]

Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #49013:
URL: https://github.com/apache/airflow/pull/49013#issuecomment-2791163680

   I pushed a couple of changes -- (not tests yet, sorry) -- this was enough to get it working locally under any name I could reach in the browser, localhost, 127.0.0.1, and `airflow.breeze.orb.local`


-- 
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-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org