Skip to content

Bump starlette to 1.3.1 for bounded form parsing - #73302

Merged
potiuk merged 1 commit into
apache:mainfrom
amoghrajesh:fix-starlette-lowest-bound
Sep 18, 2026
Merged

potiuk merged 1 commit into
apache:mainfrom
amoghrajesh:fix-starlette-lowest-bound

Conversation

@amoghrajesh

Copy link
Copy Markdown
Contributor

Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)

What

Fix for CI https://github.com/apache/airflow/actions/runs/35265542917

test_login.py::TestCreateTokenUnparsableBody::test_form_parser_error_keeps_its_own_detail
assert 422 == 400

airflow-core declared starlette>=1.0.1, so lowest-direct resolution installed 1.0.1, a version whose application/x-www-form-urlencoded parser enforces no limits at all.

Current behaviour

In Starlette 1.0.1, only the multipart parser enforces max_fields. The urlencoded FormParser has no field-count or field-size bound, and Request._get_form does not wrap it in the MultiPartException to HTTPException(400) translation that the multipart branch gets.

parse_login_body relies on that translation: it only converts ClientDisconnect, and deliberately lets Starlette's own 400 ("Too many fields...") propagate rather than flattening it into the generic message. Below Starlette 1.3.1 that reliance does not hold: a 2000-field body parses cleanly, username and password come back None, and LoginBody validation answers 422.

Beyond the test, this means the declared floor claimed support for a configuration in which the unauthenticated POST /auth/token route parses a urlencoded body with no field-count or field-size bound.

Changes of Note

1.3.1 is the exact boundary, established by bisecting the published wheels:

starlette urlencoded max_fields maps to 400
1.0.1 – 1.3.0 no no
1.3.1+ yes yes

Starlette's 1.3.1 notes: "Enforce max_fields and max_part_size in FormParser" and "Enforce FormParser limits in parser callbacks".


  • Read the Pull Request Guidelines for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
  • For fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
  • When adding dependency, check compliance with the ASF 3rd Party License Policy.
  • For significant user-facing changes create newsfragment: {pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.

@amoghrajesh amoghrajesh self-assigned this Sep 18, 2026
@amoghrajesh amoghrajesh added the full tests needed We need to run full set of tests for this PR to merge label Sep 18, 2026
@amoghrajesh amoghrajesh reopened this Sep 18, 2026
@potiuk

potiuk commented Sep 18, 2026

Copy link
Copy Markdown
Member

Independently reproduced this and landed on the same fix, so here's a second data point on the boundary.

Ran the failing test against the published wheels either side of it:

starlette test_form_parser_error_keeps_its_own_detail
1.3.0 assert 422 == 400
1.3.1 passes

So 1.3.1 is exactly right, not off by one. The mechanism matches the description: through 1.3.0, Request._get_form builds FormParser(headers, stream) with no limits and no MultiPartException -> HTTPException(400) translation on the urlencoded branch, while the multipart branch has had both all along. 1.3.1 adds them (Kludex/starlette#3329).

Two things that might be worth adding to the description, since both bear on how risky the bump is:

It doesn't drag fastapi along. Resolving the two direct constraints on their own with --resolution lowest-direct gives fastapi==0.133.0, starlette==1.3.1 — and the failing run resolved fastapi==0.133.0 with starlette==1.0.1, so starlette is the only thing that moves. Worth stating explicitly, since raising a floor that sits under a capped dependency can quietly push the cap.

It narrows the tested range rather than widening it. main's uv.lock already resolves starlette to 1.6.0, so regular CI has been green well above the new floor all along. The bump only stops the nightly job exercising 1.0.1–1.3.0, a range nothing else covers.

One note on verification: core:LowestDeps:14:3.10:API...CLI — the job that actually failed in run 35265542917 — is still pending here at time of writing. That's the one to watch before merge.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

@potiuk
potiuk merged commit 7911e76 into apache:main Sep 18, 2026
174 of 179 checks passed
deepujain pushed a commit to deepujain/airflow that referenced this pull request Sep 20, 2026
deepujain pushed a commit to deepujain/airflow that referenced this pull request Sep 20, 2026
deepujain pushed a commit to deepujain/airflow that referenced this pull request Sep 20, 2026
deepujain pushed a commit to deepujain/airflow that referenced this pull request Sep 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

full tests needed We need to run full set of tests for this PR to merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants