This is an automated email from the ASF dual-hosted git repository.
bbovenzi pushed a commit to branch v3-4-test
in repository https://gitbox.apache.org/repos/asf/airflow.git
The following commit(s) were added to refs/heads/v3-4-test by this push:
new 63504d29972 [v3-4-test] Reject blank pool names and variable keys in
the REST API (#74896) (#75258)
63504d29972 is described below
commit 63504d299729f4ded251fea026859fbe0137c2ba
Author: github-actions[bot]
<41898282+github-actions[bot]@users.noreply.github.com>
AuthorDate: Wed Oct 21 09:10:29 2026 -0400
[v3-4-test] Reject blank pool names and variable keys in the REST API
(#74896) (#75258)
* Reject blank pool names and variable keys in the REST API
* Add newsfragment
* Add blank name and key tests for the PATCH and bulk endpoints
(cherry picked from commit ca6499d83b8e5b8c3d14cc768745c6b6b4fd49a5)
Co-authored-by: Pushkal Gupta
<[email protected]>
---
airflow-core/newsfragments/74896.bugfix.rst | 1 +
.../api_fastapi/core_api/datamodels/pools.py | 6 ++-
.../api_fastapi/core_api/datamodels/variables.py | 7 +++-
.../core_api/openapi/v2-rest-api-generated.yaml | 2 +
.../api_fastapi/core_api/routes/public/pools.py | 4 +-
.../airflow/ui/openapi-gen/requests/schemas.gen.ts | 2 +
.../core_api/routes/public/test_pools.py | 48 ++++++++++++++++++++++
.../core_api/routes/public/test_variables.py | 44 +++++++++++++++++++-
.../src/airflowctl/api/datamodels/generated.py | 4 +-
9 files changed, 110 insertions(+), 8 deletions(-)
diff --git a/airflow-core/newsfragments/74896.bugfix.rst
b/airflow-core/newsfragments/74896.bugfix.rst
new file mode 100644
index 00000000000..0ea1420f44e
--- /dev/null
+++ b/airflow-core/newsfragments/74896.bugfix.rst
@@ -0,0 +1 @@
+The REST API now rejects a pool name or variable key that is empty or only
whitespace. ``POST /api/v2/pools`` and ``POST /api/v2/variables`` return 422
for such a value, and ``PATCH /api/v2/pools/{pool_name}`` returns 400 instead
of renaming the pool to the empty string. Pools and variables that already have
a blank name can still be read and deleted.
diff --git a/airflow-core/src/airflow/api_fastapi/core_api/datamodels/pools.py
b/airflow-core/src/airflow/api_fastapi/core_api/datamodels/pools.py
index 8fef6a094ba..d6021a85a7c 100644
--- a/airflow-core/src/airflow/api_fastapi/core_api/datamodels/pools.py
+++ b/airflow-core/src/airflow/api_fastapi/core_api/datamodels/pools.py
@@ -20,7 +20,7 @@ from __future__ import annotations
from collections.abc import Callable
from typing import Annotated
-from pydantic import BeforeValidator, Field, model_validator
+from pydantic import BeforeValidator, Field, StringConstraints, model_validator
from airflow.api_fastapi.core_api.base import BaseModel, StrictBaseModel
from airflow.configuration import conf
@@ -98,7 +98,9 @@ class PoolPatchBody(StrictBaseModel):
class PoolBody(BasePool, StrictBaseModel):
"""Pool serializer for post bodies."""
- pool: str = Field(alias="name", max_length=256)
+ # pattern (not strip_whitespace) so the name is stored exactly as sent,
and `\S` rather
+ # than a stricter charset because `/{pool_name:path}` deliberately lets
names contain slashes.
+ pool: Annotated[str, StringConstraints(pattern=r"\S", max_length=256)] =
Field(alias="name")
description: str | None = None
include_deferred: bool = False
team_name: str | None = Field(max_length=50, default=None)
diff --git
a/airflow-core/src/airflow/api_fastapi/core_api/datamodels/variables.py
b/airflow-core/src/airflow/api_fastapi/core_api/datamodels/variables.py
index 16c98c8588a..87e2db81767 100644
--- a/airflow-core/src/airflow/api_fastapi/core_api/datamodels/variables.py
+++ b/airflow-core/src/airflow/api_fastapi/core_api/datamodels/variables.py
@@ -19,8 +19,9 @@ from __future__ import annotations
import json
from collections.abc import Iterable
+from typing import Annotated
-from pydantic import Field, JsonValue, field_validator, model_validator
+from pydantic import Field, JsonValue, StringConstraints, field_validator,
model_validator
from airflow._shared.secrets_masker import redact
from airflow.api_fastapi.core_api.base import BaseModel, StrictBaseModel,
make_partial_model
@@ -56,7 +57,9 @@ class VariableResponse(BaseModel):
class VariableBody(StrictBaseModel):
"""Variable serializer for bodies."""
- key: str = Field(max_length=ID_LEN)
+ # pattern (not strip_whitespace) so the key is stored exactly as sent, and
`\S` rather than a
+ # stricter charset because `/{variable_key:path}` deliberately lets keys
contain slashes.
+ key: Annotated[str, StringConstraints(pattern=r"\S", max_length=ID_LEN)]
value: JsonValue = Field(serialization_alias="val")
description: str | None = Field(default=None)
team_name: str | None = Field(max_length=50, default=None)
diff --git
a/airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml
b/airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml
index 640faa0942a..d7f5e852357 100644
---
a/airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml
+++
b/airflow-core/src/airflow/api_fastapi/core_api/openapi/v2-rest-api-generated.yaml
@@ -17295,6 +17295,7 @@ components:
name:
type: string
maxLength: 256
+ pattern: \S
title: Name
slots:
type: integer
@@ -18913,6 +18914,7 @@ components:
key:
type: string
maxLength: 250
+ pattern: \S
title: Key
value:
$ref: '#/components/schemas/JsonValue'
diff --git
a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/pools.py
b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/pools.py
index 5b8e2098a37..551c1ecbe6a 100644
--- a/airflow-core/src/airflow/api_fastapi/core_api/routes/public/pools.py
+++ b/airflow-core/src/airflow/api_fastapi/core_api/routes/public/pools.py
@@ -144,7 +144,9 @@ def patch_pool(
update_mask: list[str] | None = Query(None),
) -> PoolResponse:
"""Update a Pool."""
- if patch_body.name and patch_body.name != pool_name:
+ # Tested against None, not truthiness: a blank name is a mismatch like any
other, and
+ # treating it as "no name supplied" let it fall through and rename the
pool to "".
+ if patch_body.name is not None and patch_body.name != pool_name:
raise HTTPException(
status.HTTP_400_BAD_REQUEST,
"Invalid body, pool name from request body doesn't match uri
parameter",
diff --git a/airflow-core/src/airflow/ui/openapi-gen/requests/schemas.gen.ts
b/airflow-core/src/airflow/ui/openapi-gen/requests/schemas.gen.ts
index 2eb5f596b63..e3e6d461263 100644
--- a/airflow-core/src/airflow/ui/openapi-gen/requests/schemas.gen.ts
+++ b/airflow-core/src/airflow/ui/openapi-gen/requests/schemas.gen.ts
@@ -7124,6 +7124,7 @@ export const $PoolBody = {
name: {
type: 'string',
maxLength: 256,
+ pattern: '\\S',
title: 'Name'
},
slots: {
@@ -9614,6 +9615,7 @@ export const $VariableBody = {
key: {
type: 'string',
maxLength: 250,
+ pattern: '\\S',
title: 'Key'
},
value: {
diff --git
a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py
b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py
index 70744d6680e..c1477b88948 100644
--- a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py
+++ b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_pools.py
@@ -486,6 +486,20 @@ class TestPatchPool(TestPoolsEndpoint):
in response.json()["detail"][0]["msg"]
)
+ def test_patch_pool_rejects_blank_name_instead_of_renaming(self,
test_client, session):
+ # The guard tested the body name for truthiness, so a blank one was
read as "no name
+ # supplied", skipped the mismatch check and renamed the pool to ""
instead.
+ self.create_pools()
+ response = test_client.patch(
+ f"/pools/{POOL1_NAME}",
+ json={"name": "", "slots": POOL1_SLOT, "include_deferred":
POOL1_INCLUDE_DEFERRED},
+ )
+ assert response.status_code == 400
+ assert response.json()["detail"] == (
+ "Invalid body, pool name from request body doesn't match uri
parameter"
+ )
+ assert session.scalar(select(Pool.pool).where(Pool.pool ==
POOL1_NAME)) == POOL1_NAME
+
class TestPostPool(TestPoolsEndpoint):
@conf_vars({("core", "multi_team"): "True"})
@@ -595,6 +609,16 @@ class TestPostPool(TestPoolsEndpoint):
)
assert response.status_code == 422
+ @pytest.mark.parametrize("name", ["", " ", " ", "\t", "\n"])
+ def test_post_pool_rejects_blank_name(self, test_client, session, name):
+ n_pools = session.scalar(select(func.count()).select_from(Pool))
+ response = test_client.post(
+ "/pools",
+ json={"name": name, "slots": 1, "include_deferred": False},
+ )
+ assert response.status_code == 422
+ assert session.scalar(select(func.count()).select_from(Pool)) ==
n_pools
+
@conf_vars({("core", "multi_team"): "False"})
def test_post_pool_rejects_team_name_when_multi_team_disabled(self,
test_client):
response = test_client.post(
@@ -1325,6 +1349,30 @@ class TestBulkPools(TestPoolsEndpoint):
expected_error_names = {err["input"]["name"] for err in detail}
assert sorted(expected_error_names) == ["pool_2", "pool_3"]
+ @pytest.mark.parametrize("action", ["create", "update"])
+ @pytest.mark.parametrize("name", ["", " ", "\t"])
+ def test_bulk_rejects_blank_name(self, test_client, session, action, name):
+ # The bulk endpoint validates its entities as PoolBody, so it picks up
the same
+ # constraint as POST. One bad entity rejects the whole request before
any action runs.
+ self.create_pools()
+ n_pools = session.scalar(select(func.count()).select_from(Pool))
+ response = test_client.patch(
+ "/pools",
+ json={
+ "actions": [
+ {
+ "action": action,
+ "entities": [
+ {"name": POOL1_NAME, "slots": 1,
"include_deferred": False},
+ {"name": name, "slots": 1, "include_deferred":
False},
+ ],
+ }
+ ]
+ },
+ )
+ assert response.status_code == 422
+ assert session.scalar(select(func.count()).select_from(Pool)) ==
n_pools
+
@conf_vars({("core", "multi_team"): "True"})
def test_bulk_create_overwrite_preserves_unset_team_name(self,
test_client, session):
"""A bulk create+overwrite that omits ``team_name`` must NOT reset an
existing pool's
diff --git
a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_variables.py
b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_variables.py
index 9a61130e3c1..29c1f4a87d5 100644
---
a/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_variables.py
+++
b/airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_variables.py
@@ -22,7 +22,7 @@ from unittest import mock
from unittest.mock import ANY
import pytest
-from sqlalchemy import select
+from sqlalchemy import func, select
from sqlalchemy.orm import Session
from airflow.api_fastapi.core_api.datamodels.common import BulkBody
@@ -616,6 +616,18 @@ class TestPatchVariable(TestVariableEndpoint):
in response.json()["detail"][0]["msg"]
)
+ # Only values a uri can carry: the body key has to match the uri parameter
to get past the
+ # mismatch check, and httpx refuses to put a raw tab or newline in a url.
+ @pytest.mark.parametrize("key", ["", " "])
+ def test_patch_variable_rejects_blank_key(self, test_client, session, key):
+ # PATCH takes the full VariableBody, so it picks up the same
constraint as POST. A blank
+ # key matched the equally blank `:path` uri parameter, so the update
went ahead.
+ Variable.set(key=key, value="old value", session=session)
+ session.commit()
+ response = test_client.patch(f"/variables/{key}", json={"key": key,
"value": "new value"})
+ assert response.status_code == 422
+ assert Variable.get(key, session=session) == "old value"
+
@pytest.mark.enable_redact
def test_patch_with_update_mask_description_only(self, test_client,
session):
"""PATCH with update_mask=['description'] should only update
description, keeping value unchanged."""
@@ -838,6 +850,12 @@ class TestPostVariable(TestVariableEndpoint):
in response.json()["detail"][0]["msg"]
)
+ @pytest.mark.parametrize("key", ["", " ", " ", "\t", "\n"])
+ def test_post_variable_rejects_blank_key(self, test_client, session, key):
+ response = test_client.post("/variables", json={"key": key, "value":
"some value"})
+ assert response.status_code == 422
+ assert session.scalar(select(Variable).filter_by(key=key)) is None
+
@pytest.mark.parametrize(
"body",
[
@@ -1601,6 +1619,30 @@ class TestBulkVariables(TestVariableEndpoint):
expected_error_keys = {err["input"]["key"] for err in detail}
assert sorted(expected_error_keys) == ["var_2", "var_3"]
+ @pytest.mark.parametrize("action", ["create", "update"])
+ @pytest.mark.parametrize("key", ["", " ", "\t"])
+ def test_bulk_rejects_blank_key(self, test_client, session, action, key):
+ # The bulk endpoint validates its entities as VariableBody, so one
blank key rejects the
+ # whole request before any action runs.
+ self.create_variables()
+ n_variables =
session.scalar(select(func.count()).select_from(Variable))
+ response = test_client.patch(
+ "/variables",
+ json={
+ "actions": [
+ {
+ "action": action,
+ "entities": [
+ {"key": TEST_VARIABLE_KEY, "value": "some value"},
+ {"key": key, "value": "some value"},
+ ],
+ }
+ ]
+ },
+ )
+ assert response.status_code == 422
+ assert session.scalar(select(func.count()).select_from(Variable)) ==
n_variables
+
@pytest.mark.parametrize("num_variables", [1, 25])
def test_bulk_delete_resolves_existence_in_single_query(self, session,
num_variables):
"""Bulk delete looks up all targeted variables in one query, not one
per key (no N+1)."""
diff --git a/airflow-ctl/src/airflowctl/api/datamodels/generated.py
b/airflow-ctl/src/airflowctl/api/datamodels/generated.py
index e192f3a1d86..4777eb0a3d2 100644
--- a/airflow-ctl/src/airflowctl/api/datamodels/generated.py
+++ b/airflow-ctl/src/airflowctl/api/datamodels/generated.py
@@ -1170,7 +1170,7 @@ class PoolBody(BaseModel):
model_config = ConfigDict(
extra="forbid",
)
- name: Annotated[str, Field(max_length=256, title="Name")]
+ name: Annotated[str, Field(max_length=256, pattern="\\S", title="Name")]
slots: Annotated[int, Field(description="Number of slots. Use -1 for
unlimited.", ge=-1, title="Slots")]
description: Annotated[str | None, Field(title="Description")] = None
include_deferred: Annotated[bool | None, Field(title="Include Deferred")]
= False
@@ -1542,7 +1542,7 @@ class VariableBody(BaseModel):
model_config = ConfigDict(
extra="forbid",
)
- key: Annotated[str, Field(max_length=250, title="Key")]
+ key: Annotated[str, Field(max_length=250, pattern="\\S", title="Key")]
value: JsonValue
description: Annotated[str | None, Field(title="Description")] = None
team_name: Annotated[TeamName | None, Field(title="Team Name")] = None
