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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
