You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "kaxil (via GitHub)" <gi...@apache.org> on 2026/09/16 21:59:37 UTC
[PR] Fix 500 from POST `/auth/token` on a request body it cannot parse [airflow]
kaxil opened a new pull request, #73258:
URL: https://github.com/apache/airflow/pull/73258
`POST /auth/token` answers 500 for any request body it cannot parse. The endpoint is unauthenticated, so this is reachable without credentials.
`parse_login_body` caught only pydantic's `ValidationError`, but the two lines inside its `try` raise plenty else. Against a local api-server, every one of these was a 500:
| body | raised |
|---|---|
| `{` | `JSONDecodeError` |
| `` (empty) | `JSONDecodeError` |
| `{"username": "\xff\xfe"}` | `UnicodeDecodeError` |
| a number over `int_max_str_digits` | bare `ValueError` |
| `[` x 20000 | `RecursionError` |
| `[1, 2]` / `5` / `null` | `TypeError` from `LoginBody(**body)` |
`POST /auth/token/cli` takes the same body and handles all of them, because it declares `body: LoginBody` and FastAPI validates it natively. `/auth/token` instead takes its body through `Depends(parse_login_body)`, so FastAPI parses no body for it (`body_field` is `None`) and that dependency is the only parse. After this change the two routes agree:
| body | `/auth/token` before | after | `/auth/token/cli` |
|---|---|---|---|
| malformed, empty | 500 | 422 | 422 |
| invalid UTF-8, oversized int, deeply nested | 500 | 400 | 400 |
| JSON array, scalar, null | 500 | 422 | 422 |
## Design rationale
**The status codes are FastAPI's, not ones picked here.** Its own chain around `request.json()` maps `JSONDecodeError` to a 422 `json_invalid` and everything else to `HTTPException(400, "There was an error parsing the body")`. Copying that is what makes the table above line up, and it is why the test asserts the two routes agree rather than naming numbers: the assertion then tracks whatever FastAPI does, instead of a constant copied out of it.
**The form branch translates `ClientDisconnect` only, not every error.** Reading either body can raise it when a caller announces a `Content-Length` and closes the socket early, and on the form path that escaped as a 500 too. It is not caught with a blanket `except Exception` because starlette reports its own form-parser limits as a 400 already carrying a useful detail (`"Too many fields. Maximum number of fields is 1000."`), which a blanket catch would replace with the generic message. There is a test for that.
**A `MemoryError` while reading the body is now reported as a client error.** That follows from mirroring FastAPI, which makes the same trade at the same point, and it is the honest cost: a server-side failure that used to surface as a 5xx now does not.
**The two new 422s carry a structured `detail`** matching the `HTTPValidationError` schema the route already advertises, rather than the bare string the pre-existing `ValidationError` arm passes. That arm is left alone; changing its output is not part of fixing a 500.
## Known issues
`POST /auth/token/cli` still answers 500 for an invalid-UTF-8 body sent with **no** `Content-Type` header. FastAPI then leaves the body as raw bytes, and its own 422 handler hands them to `jsonable_encoder`, whose bytes encoder calls `.decode()` and raises. That is untouched here and wants its own fix; the route in this PR is unaffected, because its content-type dependency defaults to JSON when the header is absent.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix 500 from POST `/auth/token` on a request body it cannot parse [airflow]
Posted by "seanmuth (via GitHub)" <gi...@apache.org>.
seanmuth commented on PR #73258:
URL: https://github.com/apache/airflow/pull/73258#issuecomment-5782761860
The `test_form_parser_error_keeps_its_own_detail` test added here fails under the LowestDeps test matrix: `assert 422 == 400` (example run: https://github.com/apache/airflow/actions/runs/35251697939/job/105310413857).
The fix assumes Starlette's own form-parser always raises with status 400 for a "too many fields" body and lets that response through untouched. That assumption doesn't hold on the oldest pinned Starlette version tested by `core:LowestDeps` -- there, the same guard returns 422 instead. Since this merged, it's been failing that job (and marking other PRs' checks unstable) on every run since 2026-09-17, with no tracking issue yet.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix 500 from POST `/auth/token` on a request body it cannot parse [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil merged PR #73258:
URL: https://github.com/apache/airflow/pull/73258
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix 500 from POST `/auth/token` on a request body it cannot parse [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil commented on PR #73258:
URL: https://github.com/apache/airflow/pull/73258#issuecomment-5784530231
Thanks for flagging. This was real but was fixed the next morning by #73302, which raised the floor to `starlette>=1.3.1`. The run you linked is from 2026-09-17 and resolved Starlette 1.0.1. In that version, the url-encoded `FormParser` has no `max_fields` limit (only `MultiPartParser` does), so the 2000-field body parses fine and fails `LoginBody` validation with 422. Starlette 1.3.1 added the limit that this test covers. `core:LowestDeps` API tests pass on current `main` (for example, run 35773153416), so a PR still showing this failure just needs a rebase.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org