Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions airflow-core/src/airflow/api_fastapi/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
from __future__ import annotations

import logging
import os
from contextlib import AsyncExitStack, asynccontextmanager
from typing import TYPE_CHECKING, cast
from urllib.parse import urlsplit
Expand All @@ -41,10 +40,9 @@
if TYPE_CHECKING:
from airflow.api_fastapi.auth.managers.base_auth_manager import BaseAuthManager

API_BASE_URL = conf.get("api", "base_url")
if API_BASE_URL and not API_BASE_URL.endswith("/"):
API_BASE_URL = conf.get("api", "base_url", fallback="")
if not API_BASE_URL or not API_BASE_URL.endswith("/"):
API_BASE_URL += "/"
os.environ["AIRFLOW__API__BASE_URL"] = API_BASE_URL
API_ROOT_PATH = urlsplit(API_BASE_URL).path

# Define the full path on which the potential auth manager fastapi is mounted
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ def create_token_all_admins() -> LoginResponse:
)
def login_all_admins() -> RedirectResponse:
"""Login the user with no credentials."""
response = RedirectResponse(url=conf.get("api", "base_url"))
response = RedirectResponse(url=conf.get("api", "base_url", fallback="/"))
secure = conf.has_option("api", "ssl_cert")
response.set_cookie(
COOKIE_NAME_JWT_TOKEN,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,7 @@ def get_fastapi_app(self) -> FastAPI | None:
def webapp(request: Request, rest_of_path: str):
return templates.TemplateResponse(
"/index.html",
{"request": request, "backend_server_base_url": conf.get("api", "base_url")},
{"request": request, "backend_server_base_url": str(request.base_url)},
media_type="text/html",
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,13 @@ export const routerConfig = [
path: "/",
},
];
const baseUrl = document.querySelector("base")?.href ?? "http://localhost:8080/";

const baseHref = document.querySelector("head>base")?.getAttribute("href");
const baseUrl =
baseHref !== null && baseHref !== undefined && baseHref !== ""
? baseHref
: `${globalThis.location.origin}/`;

const basename = new URL(`${baseUrl}auth`).pathname;

export const router = createBrowserRouter(routerConfig, { basename });
3 changes: 1 addition & 2 deletions airflow-core/src/airflow/api_fastapi/core_api/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
from airflow.api_fastapi.auth.tokens import get_signing_key
from airflow.api_fastapi.core_api.init_dagbag import get_dag_bag
from airflow.api_fastapi.core_api.middleware import FlaskExceptionsMiddleware
from airflow.configuration import conf
from airflow.exceptions import AirflowException
from airflow.settings import AIRFLOW_PATH

Expand Down Expand Up @@ -79,7 +78,7 @@ def init_views(app: FastAPI) -> None:
def webapp(request: Request, rest_of_path: str):
return templates.TemplateResponse(
"/index.html",
{"request": request, "backend_server_base_url": conf.get("api", "base_url")},
{"request": request, "backend_server_base_url": str(request.base_url)},
media_type="text/html",
)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,12 @@ def login(request: Request, next: None | str = None) -> RedirectResponse:
"""Redirect to the login URL depending on the AuthManager configured."""
login_url = request.app.state.auth_manager.get_url_login()

if next and not is_safe_url(next):
if next and not is_safe_url(next, request=request):
raise HTTPException(status_code=400, detail="Invalid or unsafe next URL")

if next:
login_url += f"?next={next}"

return RedirectResponse(login_url)


Expand Down
31 changes: 22 additions & 9 deletions airflow-core/src/airflow/api_fastapi/core_api/security.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@

from pathlib import Path
from typing import TYPE_CHECKING, Annotated, Callable
from urllib.parse import urljoin, urlparse
from urllib.parse import ParseResult, urljoin, urlparse

from fastapi import Depends, HTTPException, Request, status
from fastapi.security import OAuth2PasswordBearer
Expand Down Expand Up @@ -329,21 +329,34 @@ 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")
parsed_bases: tuple[tuple[str, ParseResult], ...] = ()

parsed_base = urlparse(base_url)
parsed_target = urlparse(urljoin(base_url, target_url)) # Resolves relative URLs
# Check if the target URL matches either the configured base URL, or the URL used to make the request
if request is not None:
url = str(request.base_url)
parsed_bases += ((url, urlparse(url)),)
if base_url := conf.get("api", "base_url", fallback=None):
parsed_bases += ((base_url, urlparse(base_url)),)

target_path = Path(parsed_target.path).resolve()
if not parsed_bases:
# Can't enforce any security check.
return True

if target_path and parsed_base.path and not target_path.is_relative_to(parsed_base.path):
return False
for base_url, parsed_base in parsed_bases:
parsed_target = urlparse(urljoin(base_url, target_url)) # Resolves relative URLs

return parsed_target.scheme in {"http", "https"} and parsed_target.netloc == parsed_base.netloc
target_path = Path(parsed_target.path).resolve()

if target_path and parsed_base.path and not target_path.is_relative_to(parsed_base.path):
continue

if parsed_target.scheme in {"http", "https"} and parsed_target.netloc == parsed_base.netloc:
return True
return False
4 changes: 2 additions & 2 deletions airflow-core/src/airflow/config_templates/config.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1308,8 +1308,8 @@ api:
should contain the API server endpoint.
version_added: ~
type: string
example: ~
default: "http://localhost:8080"
example: "https://my-airflow.company.com"
default: ~
host:
description: |
The ip specified when starting the api server
Expand Down
2 changes: 1 addition & 1 deletion airflow-core/src/airflow/models/taskinstance.py
Original file line number Diff line number Diff line change
Expand Up @@ -1915,7 +1915,7 @@ def generate_command(
def log_url(self) -> str:
"""Log URL for TaskInstance."""
run_id = quote(self.run_id)
base_url = conf.get_mandatory_value("api", "BASE_URL")
base_url = conf.get("api", "base_url", fallback="http://localhost:8080/")
map_index = f"/mapped/{self.map_index}" if self.map_index >= 0 else ""
try_number = f"?try_number={self.try_number}" if self.try_number > 0 else ""
_log_uri = f"{base_url}dags/{self.dag_id}/runs/{run_id}/tasks/{self.task_id}{map_index}{try_number}"
Expand Down
8 changes: 7 additions & 1 deletion airflow-core/src/airflow/ui/src/main.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,13 @@ axios.interceptors.response.use(
const params = new URLSearchParams();

params.set("next", globalThis.location.href);
const baseUrl = document.querySelector("head>base")?.getAttribute("href") ?? "";

const baseHref = document.querySelector("head>base")?.getAttribute("href");
const baseUrl =
baseHref !== null && baseHref !== undefined && baseHref !== ""
? baseHref
: `${globalThis.location.origin}/`;

const loginPath = new URL("api/v2/auth/login", baseUrl).pathname;

globalThis.location.replace(`${loginPath}?${params.toString()}`);
Expand Down
2 changes: 1 addition & 1 deletion airflow-core/src/airflow/utils/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -205,7 +205,7 @@ def build_airflow_dagrun_url(dag_id: str, run_id: str) -> str:
For example:
http://localhost:8080/dags/hi/runs/manual__2025-02-23T18:27:39.051358+00:00_RZa1at4Q
"""
baseurl = conf.get("api", "base_url")
baseurl = conf.get("api", "base_url", fallback="/")
return urljoin(baseurl, f"dags/{dag_id}/runs/{run_id}")


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
# under the License.
from __future__ import annotations

from unittest.mock import MagicMock
from unittest.mock import MagicMock, patch

import pytest

Expand Down Expand Up @@ -45,7 +45,8 @@ class TestGetLogin(TestAuthEndpoint):
{"next": "http://localhost:8080", "other_param": "something_else"},
],
)
def test_should_respond_307(self, test_client, params):
@patch("airflow.api_fastapi.core_api.routes.public.auth.is_safe_url", return_value=True)
def test_should_respond_307(self, mock_is_safe_url, test_client, params):
response = test_client.get("/auth/login", follow_redirects=False, params=params)

assert response.status_code == 307
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment thread
pierrejeambrun marked this conversation as resolved.
# Relative path, will go up one level escaping the prefix folder
("some_other", False),
("./some_other", False),
Expand All @@ -135,4 +136,6 @@ async def test_requires_access_dag_unauthorized(self, mock_get_auth_manager):
)
@conf_vars({("api", "base_url"): "https://server_base_url.com/prefix"})
def test_is_safe_url(self, url, expected_is_safe):
assert is_safe_url(url) == expected_is_safe
request = Mock()
request.base_url = "https://requesting_server_base_url.com/prefix2"
assert is_safe_url(url, request=request) == expected_is_safe
2 changes: 1 addition & 1 deletion airflow-core/tests/unit/utils/test_helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ def test_merge_dicts_recursive_right_only(self):
assert merged == {"a": 1, "r": {"b": 0, "c": 3}}

def test_build_airflow_dagrun_url(self):
expected_url = "http://localhost:8080/dags/somedag/runs/abc123"
expected_url = "/dags/somedag/runs/abc123"
assert build_airflow_dagrun_url(dag_id="somedag", run_id="abc123") == expected_url

@pytest.mark.parametrize(
Expand Down
1 change: 0 additions & 1 deletion dev/breeze/src/airflow_breeze/params/shell_params.py
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,6 @@ def env_variables_for_docker_commands(self) -> dict[str, str]:
_set_var(_env, "AIRFLOW_IMAGE_KUBERNETES", self.airflow_image_kubernetes)
_set_var(_env, "AIRFLOW_VERSION", self.airflow_version)
_set_var(_env, "AIRFLOW__API_AUTH__JWT_SECRET", b64encode(os.urandom(16)).decode("utf-8"))
_set_var(_env, "AIRFLOW__API__BASE_URL", f"http://localhost:{WEB_HOST_PORT}")
_set_var(_env, "AIRFLOW__CELERY__BROKER_URL", self.airflow_celery_broker_url)
_set_var(_env, "AIRFLOW__CORE__AUTH_MANAGER", self.auth_manager_path)
_set_var(_env, "AIRFLOW__CORE__EXECUTOR", self.executor)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ def avp_facade(self):

@cached_property
def apiserver_endpoint(self) -> str:
return conf.get("api", "base_url")
return conf.get("api", "base_url", fallback="/")

def deserialize_user(self, token: dict[str, Any]) -> AwsAuthManagerUser:
return AwsAuthManagerUser(user_id=token.pop("sub"), **token)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ def login_callback(request: Request):
username=saml_auth.get_nameid(),
email=attributes["email"][0] if "email" in attributes else None,
)
url = conf.get("api", "base_url")
url = conf.get("api", "base_url", fallback="/")
token = get_auth_manager().generate_jwt(user)
response = RedirectResponse(url=url, status_code=303)

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -689,7 +689,7 @@ def test_filter_authorized_dag_ids(self, method, user, auth_manager, test_user,

def test_get_url_login(self, auth_manager):
result = auth_manager.get_url_login()
assert result == f"http://localhost:8080{AUTH_MANAGER_FASTAPI_APP_PREFIX}/login"
assert result == f"{AUTH_MANAGER_FASTAPI_APP_PREFIX}/login"

def test_get_cli_commands_return_cli_commands(self, auth_manager):
assert len(auth_manager.get_cli_commands()) > 0
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ def init_flask_resources(self) -> None:

@cached_property
def apiserver_endpoint(self) -> str:
return conf.get("api", "base_url")
return conf.get("api", "base_url", fallback="/")

@staticmethod
def get_cli_commands() -> list[CLICommand]:
Expand Down
2 changes: 1 addition & 1 deletion providers/fab/src/airflow/providers/fab/www/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ def index(self):

return response
else:
return redirect(conf.get("api", "base_url"), code=302)
return redirect(conf.get("api", "base_url", fallback="/"), code=302)


def show_traceback(error):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -618,11 +618,11 @@ class TestSecurityManager:

def test_get_url_login(self, auth_manager):
result = auth_manager.get_url_login()
assert result == f"http://localhost:8080{AUTH_MANAGER_FASTAPI_APP_PREFIX}/login/"
assert result == f"{AUTH_MANAGER_FASTAPI_APP_PREFIX}/login/"

def test_get_url_logout(self, auth_manager):
result = auth_manager.get_url_logout()
assert result == f"http://localhost:8080{AUTH_MANAGER_FASTAPI_APP_PREFIX}/logout/"
assert result == f"{AUTH_MANAGER_FASTAPI_APP_PREFIX}/logout/"

@mock.patch.object(FabAuthManager, "_is_authorized", return_value=True)
def test_get_extra_menu_items(self, _, auth_manager_with_appbuilder, flask_app):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ def test_extra_operator_link(self, mock_xcom_get_one, dag_maker):

link = task.operator_extra_links[0].get_link(operator=task, ti_key=ti.key)

base_url = conf.get_mandatory_value("api", "base_url").lower()
base_url = conf.get("api", "base_url", fallback="/").lower()
expected_url = f"{base_url}dags/{TRIGGERED_DAG_ID}/runs/test_run_id"
assert link == expected_url, f"Expected {expected_url}, but got {link}"

Expand Down