Repository navigation
Fix 500 from the audit logger on a request body it cannot parse - #73226
Merged
Merged
Conversation
action_logging reads the request body to record it in the audit log, and parsed it with no error handling. Whether that mattered depended on the route it is attached to: where the route declares a body, FastAPI parses and validates it first, so a bad body is a 4xx and the dependency never runs. Where the route declares no body -- the deletes, the favourite toggles, the backfill pause/cancel actions -- FastAPI never touches the body, so this dependency was the only thing that parsed it, and the error escaped as a 500 for a request whose endpoint does not read a body at all. The action was lost with it: a DELETE that answered 500 left the variable in place. A body this dependency cannot read is now recorded as an access without a body. It runs before the route so an access is logged whether or not the route succeeds, which also means it must never be the reason a request fails. Returning 400 instead would put validation of a field the API does not declare inside the audit logger. The catch is broad, matching what FastAPI does around the same call. json.loads raises JSONDecodeError for malformed input, UnicodeDecodeError for invalid UTF-8, a bare ValueError for an integer over int_max_str_digits, and RecursionError for deeply nested input. Three earlier fixes to this line each guarded a value json.loads returned -- an empty body (apache#49035), a list (apache#62354), then non-dict bodies again (apache#72683) -- and enumerating types is how each one missed the next shape. requires_access_backfill parses the same body to find a dag_id and suppressed only JSONDecodeError, so it re-raised on the others. A body it cannot read names no Dag, which is the state it already handles, so it now suppresses them the same way. This is reachable only when backfill_id is absent or unparsable, since a valid one is authorized from the path without reading the body. The test enumerates the routes from the app and sends every input class that makes json.loads raise, rather than naming the endpoint of the latest report. It requires an audit row per route as well as a non-500, so a route rejected by an earlier dependency cannot pass while proving nothing.
kaxil
requested review from
bugraoz93,
choo121600,
ephraimbuddy,
henry3260,
jason810496,
pierrejeambrun,
rawwar and
shubhamraj-git
as code owners
September 15, 2026 22:27
eladkal
approved these changes
Sep 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #72683, which guarded the value
json.loadsreturned but not the parse itself. Closes the class behind #49035 and #62354.action_loggingreads the request body to record it in the audit log, and parsed it with no error handling. Whether that mattered depended on the route: where the route declares a body, FastAPI parses and validates it first, so a bad body is a 4xx and this dependency never runs. Where the route declares no body -- the deletes, the favourite toggles, the backfill pause/cancel actions -- FastAPI never touches the body, so this was the only parse of it and the error escaped as a 500. The action was lost with it.Against a local api-server, before:
After, for each input class that makes
json.loadsraise:Design rationale
A body this dependency cannot read is recorded as an access without a body, not rejected. It runs before the route so an access is logged whether or not the route succeeds -- the ordering is deliberate, per the discussion on #64641 -- which also means it must never be the reason a request fails. Returning 400 instead would put validation of a field the API does not declare inside the audit logger.
The catch is broad, matching what FastAPI does around the same call.
json.loadsraisesJSONDecodeErrorfor malformed input,UnicodeDecodeErrorfor invalid UTF-8, a bareValueErrorfor an integer overint_max_str_digits, andRecursionErrorfor deeply nested input. Enumerating types is how this line collected three earlier fixes that each missed the next shape, so it now matches FastAPI's own chain, which ends inexcept Exception.requires_access_backfillparses the same body and suppressed onlyJSONDecodeError, so it re-raised on the others. A body it cannot read names no Dag, which is the state it already handles, so it now suppresses them the same way -- no authorization outcome changes. Reachable only whenbackfill_idis absent or unparsable, since a valid one is authorized from the path without reading the body.The audit row keeps naming the resource. With no readable body, the masking branches fell back to an empty dict, so a variable or connection delete recorded
{"method": "DELETE"}and lost the only record of what it destroyed --Loghas no column for it. Visible in one log, old row then new:Gotchas
The test enumerates the bodyless routes from the app and sends every input class above, rather than naming the endpoint of the latest report. It requires an audit row per route as well as a non-500, because a route rejected by an earlier dependency would otherwise pass while proving nothing --
PUT /parseDagFile/{file_token}does exactly that with a placeholder token, so it is exempted explicitly rather than silently.