Repository navigation
Fix 500 from POST /auth/token on a request body it cannot parse - #73258
Conversation
15c9b3a to
264f351
Compare
|
The 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 Drafted-by: Claude Sonnet 5 (no human review before posting) |
|
@seanmuth Thanks for flagging. This was real but was fixed the next morning by #73302, which raised the floor to |
POST /auth/tokenanswers 500 for any request body it cannot parse. The endpoint is unauthenticated, so this is reachable without credentials.parse_login_bodycaught only pydantic'sValidationError, but the two lines inside itstryraise plenty else. Against a local api-server, every one of these was a 500:{JSONDecodeErrorJSONDecodeError{"username": "\xff\xfe"}UnicodeDecodeErrorint_max_str_digitsValueError[x 20000RecursionError[1, 2]/5/nullTypeErrorfromLoginBody(**body)POST /auth/token/clitakes the same body and handles all of them, because it declaresbody: LoginBodyand FastAPI validates it natively./auth/tokeninstead takes its body throughDepends(parse_login_body), so FastAPI parses no body for it (body_fieldisNone) and that dependency is the only parse. After this change the two routes agree:/auth/tokenbefore/auth/token/cliDesign rationale
The status codes are FastAPI's, not ones picked here. Its own chain around
request.json()mapsJSONDecodeErrorto a 422json_invalidand everything else toHTTPException(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
ClientDisconnectonly, not every error. Reading either body can raise it when a caller announces aContent-Lengthand closes the socket early, and on the form path that escaped as a 500 too. It is not caught with a blanketexcept Exceptionbecause 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
MemoryErrorwhile 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
detailmatching theHTTPValidationErrorschema the route already advertises, rather than the bare string the pre-existingValidationErrorarm passes. That arm is left alone; changing its output is not part of fixing a 500.Known issues
POST /auth/token/clistill answers 500 for an invalid-UTF-8 body sent with noContent-Typeheader. FastAPI then leaves the body as raw bytes, and its own 422 handler hands them tojsonable_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.