Pushkal-Gupta opened a new pull request, #74896:
URL: https://github.com/apache/airflow/pull/74896
`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:
```python
# 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:
```python
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:
```python
# 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?
- [X] Yes (please specify the tool below)
--
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]
