Skip to content

Fix 500 for non-dict JSON bodies on variable/connection endpoints - #72683

Merged
pierrejeambrun merged 2 commits into
apache:mainfrom
FrankYang0529:airflow-action-logging-non-dict-body
Sep 10, 2026
Merged

pierrejeambrun merged 2 commits into
apache:mainfrom
FrankYang0529:airflow-action-logging-non-dict-body

Conversation

@FrankYang0529

Copy link
Copy Markdown
Member

Why

  • action_logging is a FastAPI dependency, and dependencies are solved before Pydantic validates the request body. It sees whatever json.loads produced, so a list, string or number reaches it as valid JSON.
  • Two of its branches read that body as a mapping via request_body.items() with no type guard, so POST /api/v2/variables and POST /api/v2/connections raise AttributeError and return 500 instead of the 422 the OpenAPI spec declares.
  • action_logging commits its audit row before the endpoint runs, so an attempt is recorded even when the request is later rejected. The crash happens before that commit, so on main these requests leave no audit row at all.

How

  • Assign the parsed body to request_body only when it is a dict, so the variable keeps its {} initial value for every other shape.

Verification

  • Unit test: uv run --frozen --project airflow-core pytest airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_connections.py airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_variables.py
  • Integration test:
  1. Start breeze
breeze shell --backend sqlite
  1. Start an API server
export AIRFLOW__CORE__LOAD_EXAMPLES=False
airflow db migrate
airflow api-server --port 8080 &
until curl -sf -o /dev/null http://localhost:8080/api/v2/version; do sleep 2; done
  1. Get a token
TOKEN=$(curl -s -X POST http://localhost:8080/auth/token \
  -H 'Content-Type: application/json' \
  -d '{"username": "admin", "password": "admin"}' \
  | python -c 'import sys, json; print(json.load(sys.stdin)["access_token"])')
  1. POST a non-dict JSON body to both endpoints
for path in variables connections; do
  for body in '[{"key": "a"}]' '"a string"' '42'; do
    echo -n "POST /$path <- $body => "
    curl -s -o /tmp/body.json -w 'HTTP %{http_code}\n' \
      -X POST "http://localhost:8080/api/v2/$path" \
      -H "Authorization: Bearer $TOKEN" -H 'Content-Type: application/json' -d "$body"
    head -c 200 /tmp/body.json; echo
  done
done

Before this PR, all six requests return HTTP 500 with a bare Internal Server Error.

With this PR they return the 422 the OpenAPI spec declares:

POST /variables <- [{"key": "a"}] => HTTP 422
{"detail":[{"type":"model_attributes_type","loc":["body"],
"msg":"Input should be a valid dictionary or object to extract fields from","input":[{"key":"a"}]}]}
  1. Confirm valid bodies are unaffected
curl -s -w '\nHTTP %{http_code}\n' -X POST http://localhost:8080/api/v2/variables \
  -H "Authorization: Bearer $TOKEN" -H 'Content-Type: application/json' \
  -d '{"key": "demo", "value": "s3cr3t", "description": "d"}'

curl -s -w '\nHTTP %{http_code}\n' -X POST http://localhost:8080/api/v2/connections \
  -H "Authorization: Bearer $TOKEN" -H 'Content-Type: application/json' \
  -d '{"connection_id": "demo", "conn_type": "http", "password": "p4ss", "extra": "{\"token\": \"abc\"}"}'
  1. Check the audit log
curl -s "http://localhost:8080/api/v2/eventLogs?limit=100" -H "Authorization: Bearer $TOKEN" \
  | python -c '
import sys, json
for r in json.load(sys.stdin)["event_logs"]:
    print("id=%-3s event=%-16s owner=%-6s extra=%s" % (r["event_log_id"], r["event"], r["owner"], r["extra"]))

Before this PR, the six rejected requests leave no audit rows at all — action_logging raises before reaching its session.commit(), so the record of the attempt is lost.

With this PR every attempt is recorded:

id=1   event=cli_api_server   owner=root   extra={"host_name": "...", "full_command": "..."}
id=2   event=post_variable    owner=admin  extra={"method": "POST"}
id=3   event=post_variable    owner=admin  extra={"method": "POST"}
id=4   event=post_variable    owner=admin  extra={"method": "POST"}
id=5   event=post_connection  owner=admin  extra={"method": "POST"}
id=6   event=post_connection  owner=admin  extra={"method": "POST"}
id=7   event=post_connection  owner=admin  extra={"method": "POST"}
id=8   event=post_variable    owner=admin  extra={"key": "demo", "value": "***", "description": "d", "method": "POST"}
id=9   event=post_connection  owner=admin  extra={"connection_id": "demo", "conn_type": "http", "password": "***", "extra": {"token": "***"}, "method": "POST"}

Was generative AI tooling used to co-author this PR?
  • Yes - Claude Code

  • 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.

@boring-cyborg boring-cyborg Bot added the area:API Airflow's REST/HTTP API label Sep 8, 2026
@FrankYang0529
FrankYang0529 marked this pull request as ready for review September 8, 2026 10:46

@Andrushika Andrushika left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, one nitpick left. Thanks!

Comment thread airflow-core/src/airflow/api_fastapi/logging/decorators.py Outdated
Signed-off-by: PoAn Yang <payang@apache.org>
Signed-off-by: PoAn Yang <payang@apache.org>
@FrankYang0529
FrankYang0529 force-pushed the airflow-action-logging-non-dict-body branch from 915f1ef to 60e0f60 Compare September 10, 2026 01:17

@pierrejeambrun pierrejeambrun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, THanks

@pierrejeambrun pierrejeambrun added this to the Airflow 3.3.2 milestone Sep 10, 2026
@pierrejeambrun
pierrejeambrun merged commit b134148 into apache:main Sep 10, 2026
79 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

Backport successfully created: v3-3-test

Note: As of Merging PRs targeted for Airflow 3.X
the committer who merges the PR is responsible for backporting the PRs that are bug fixes (generally speaking) to the maintenance branches.

In matter of doubt please ask in #release-management Slack channel.

Status Branch Result
✅ v3-3-test PR Link

@FrankYang0529
FrankYang0529 deleted the airflow-action-logging-non-dict-body branch September 10, 2026 10:36
github-actions Bot pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Sep 10, 2026
…ndpoints (apache#72683)

* Fix 500 for non-dict JSON bodies on variable/connection endpoints

Signed-off-by: PoAn Yang <payang@apache.org>

* Simplify the comment

Signed-off-by: PoAn Yang <payang@apache.org>

---------
(cherry picked from commit b134148)

Co-authored-by: PoAn Yang <payang@apache.org>
Signed-off-by: PoAn Yang <payang@apache.org>
aws-airflow-bot pushed a commit to aws-mwaa/upstream-to-airflow that referenced this pull request Sep 10, 2026
…ndpoints (apache#72683)

* Fix 500 for non-dict JSON bodies on variable/connection endpoints

Signed-off-by: PoAn Yang <payang@apache.org>

* Simplify the comment

Signed-off-by: PoAn Yang <payang@apache.org>

---------
(cherry picked from commit b134148)

Co-authored-by: PoAn Yang <payang@apache.org>
Signed-off-by: PoAn Yang <payang@apache.org>
pierrejeambrun pushed a commit that referenced this pull request Sep 10, 2026
…ndpoints (#72683) (#72878)

* Fix 500 for non-dict JSON bodies on variable/connection endpoints



* Simplify the comment



---------
(cherry picked from commit b134148)

Signed-off-by: PoAn Yang <payang@apache.org>
Co-authored-by: PoAn Yang <payang@apache.org>
imrichardwu pushed a commit to imrichardwu/airflow that referenced this pull request Sep 11, 2026
…ache#72683)

* Fix 500 for non-dict JSON bodies on variable/connection endpoints

Signed-off-by: PoAn Yang <payang@apache.org>

* Simplify the comment

Signed-off-by: PoAn Yang <payang@apache.org>

---------

Signed-off-by: PoAn Yang <payang@apache.org>
xvega pushed a commit to xvega/airflow that referenced this pull request Sep 13, 2026
…ache#72683)

* Fix 500 for non-dict JSON bodies on variable/connection endpoints

Signed-off-by: PoAn Yang <payang@apache.org>

* Simplify the comment

Signed-off-by: PoAn Yang <payang@apache.org>

---------

Signed-off-by: PoAn Yang <payang@apache.org>
kaxil added a commit to astronomer/airflow that referenced this pull request Sep 15, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants