[Pushkal-Gupta]

POST /api/v2/pools and POST /api/v2/variables accept a blank name, so you can create a pool called "" or a variable keyed "". Worse, PATCH /api/v2/pools/{pool_name} with {"name": ""} renames an existing pool to the empty string and answers 200.

What's wrong

Neither request model requires a character in the name, only a maximum length:

# datamodels/variables.py
key: str = Field(max_length=ID_LEN)
# datamodels/pools.py
pool: str = Field(alias="name", max_length=256)

So "", " ", "\t" and "\n" all pass validation and reach the database. You can address the row afterwards because these routes are declared /{pool_name:path} and /{variable_key:path}, and Starlette compiles a :path converter to (?P<name>.*), which matches an empty segment. A plain /{name} compiles to [^/]+ and does not, which is why only these two resources show it.

The rename is the part that bothers me more. patch_pool already tries to stop a body name that disagrees with the URI:

if patch_body.name 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")

It tests truthiness, so "" is read as "no name supplied", skips the comparison and carries on into the update. PATCH /api/v2/pools/realpool with {"name": "", "slots": 2, "include_deferred": false} returns 200 today and leaves the pool named "". patch_variable spells the same guard patch_body.key != variable_key, so variables were never affected by it. The two routes just wrote the check differently.

The fix

Require one non-whitespace character in the two request bodies, and compare the pool name against None instead of truthiness.

The pattern is the one already used on asset partition keys, added in #69453 for the same reason:

# datamodels/assets.py
partition_key: Annotated[str, StringConstraints(pattern=r"\S", max_length=ID_LEN)] | None = None

\S and not something stricter because names here really do contain slashes, which is why the routes use :path at all and what the existing POOL3_NAME = "pool3/with_slashes" covers. ConnectionBody.connection_id uses ^[\w.-]+$, which would reject those. And pattern rather than strip_whitespace so the stored name stays exactly what was sent.

Blank names now get 422, and the blank rename gets the 400 the guard always intended. Reads are untouched, so a deployment that already picked up a blank-named pool or variable through this gap can still list and delete it.

I left PoolPatchBody alone deliberately. Constraining an optional field makes datamodel-codegen lift it into a RootModel, and airflowctl's PoolsOperations.update builds its URL as f"pools/{pool_body.pool}", which then comes out as pools/root='pool_name' and fails two of its tests. Fixing the route guard deals with the rename directly and leaves the generated client alone. That f-string is fragile for the same reason, and Slots is already a RootModel, so something will run into it eventually, but that is a separate change.

Four of the eight files are regenerated rather than hand-edited: the OpenAPI spec, the UI schemas.gen.ts and the airflowctl datamodels each pick up the same two pattern entries from their hooks. types.gen.ts does not change, since the TypeScript types never carried the existing maxLength either.

Tests

Eleven cases: "", " ", " ", "\t" and "\n" against both POST routes, plus one for the rename that checks both the 400 and that the pool still has its name. All eleven fail without this change, and the rename one fails as assert 200 == 400 with the pool left called "".

test_pools.py and test_variables.py go from 177 to 188 passed, airflow-ctl stays at 439, ruff and mypy are clean, and the three generator hooks are stable on a second run.

---

Was generative AI tooling used to co-author this PR?

• Yes — Claude Code (Opus 5)

[fat-catTW]

Thanks for contribution!
Could we add tests for the PATCH and bulk endpoints that also use PoolBody/VariableBody?

[Pushkal-Gupta]

Nice catch, thanks. The bulk endpoints were affected and untested, and chasing it turned up a third route I had missed.

Added 14 tests:

TestBulkPools.test_bulk_rejects_blank_name and TestBulkVariables.test_bulk_rejects_blank_key, each over "", " " and "\t" and over both the create and update actions. Validation happens on the whole BulkBody, so one bad entity rejects the request with a 422 and none of the actions run. Without the constraint these return 200 and the blank entity is created.

TestPatchVariable.test_patch_variable_rejects_blank_key. I had not noticed that patch_variable takes the full VariableBody rather than the partial, so it was affected too, and it was the worst of the three: the test seeds a variable with a blank key, and before the change PATCH /variables/ with {"key": "", ...} matched the equally blank :path parameter and updated the row. It now returns 422 and the value is untouched. The parameters stop at "" and " " there because the body key has to match the uri parameter to get past the mismatch check, and httpx will not put a raw tab in a url.

On PATCH /pools/{pool_name}: that one goes through PoolPatchBody, where name is optional, and I left it unconstrained on purpose. Constraining an optional field makes datamodel-codegen lift it into a RootModel, which breaks airflow-ctl, where operations.py builds its url from f"pools/{pool_body.pool}" and would request pools/root='mypool'. Two airflow-ctl tests fail that way. So the blank name is rejected in the route instead, by comparing against None rather than testing for truthiness, and test_patch_pool_rejects_blank_name_instead_of_renaming covers it. Happy to revisit if you would rather see the constraint on the model and the codegen handled some other way.

test_pools.py and test_variables.py are at 202 passed, up from 188. All 14 new tests fail without the change, the bulk ones as assert 200 == 422.

[fat-catTW]

Thanks for the change!
Look great!

[github-actions]

Backport successfully created: v3-4-test

Note: As of Merging PRs targeted for Airflow 3.X
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.

In matter of doubt please ask in #release-management Slack channel.

Status	Branch	Result
✅	v3-4-test