Uh oh!
There was an error while loading. Please reload this page.
Harden _collect_teams_to_check / requires_access_backfill against malformed bodies - #66504
Conversation
bd9562b to
96db2b2CompareExtends apache#66504 with explicit type validation. The authorization dependencies in `_collect_teams_to_check` and `requires_access_backfill` read `team_name` / `dag_id` from the raw JSON body before Pydantic validation runs on the actual endpoint handler. If a body contains a non-string value (list, dict, integer, …) those values would otherwise flow into `Team.get_name_if_exists` / the authz callback / the existence lookup, producing undefined behaviour or type-confused authz decisions. Raise 400 on a non-string `team_name` / `dag_id` before any auth check runs. Tests parametrised on integer / list / dict / bool inputs assert the 400 + that the authz callback is never consulted. Reported by the L3 ASVS sweep at apache/tooling-agents#23 (FINDING-060).
For POST/PUT in multi-team mode, the helper used `with suppress(JSONDecodeError)` around `await request.json()`. If the body was unparseable, the suppress swallowed the exception, `teams.add(raw)` never ran, and the calling `requires_access_*` dependency iterated over an empty set — silently skipping the authorization callback entirely. Today this is unreachable because every POST/PUT route in core_api uses a Pydantic body model, so FastAPI returns 422 before the auth dependency runs. But the pattern would silently bypass team-scoped authz if a future route used a raw `Request` instead. Replace the bare suppress with an explicit try/except that adds `None` to `teams` on parse failure, so the auth callback always runs at least once.
939cd75 to
5aa93cfComparepotiuk
commented
May 17, 2026
I'd love to get this one merged — and would love it in 3.2.2 if it's not too late. cc @vatsrahul1001 (3.2.2 RM) Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting |
vatsrahul1001
commented
May 18, 2026
Ready for maintainer review |
jason810496
left a comment
There was a problem hiding this comment.
Not sure would it be better to early return to avoid the further bypass.
Uh oh!
There was an error while loading. Please reload this page.
vatsrahul1001
commented
May 18, 2026
@potiuk can you address comments? |
amoghrajesh
left a comment
There was a problem hiding this comment.
Commit 2: Reject non-string dag_id / team_name from raw body before authz (FINDING-060 from L3 sweep)
Is this in this PR? I cannot see in the diff
Uh oh!
There was an error while loading. Please reload this page.
Builds on the previous fail-closed change in _collect_teams_to_check. Two follow-ups from review: * On JSONDecodeError, raise HTTP 400 directly instead of falling through to a team=None auth call — clearer failure mode and removes any ambiguity about whether authz ran. * Reject non-string `team_name` (in _collect_teams_to_check) and non-string `dag_id` (in requires_access_backfill) from the raw body with HTTP 400 before any authz decision or DB lookup. Without this, a list / dict / int / bool would flow into Team.get_name_if_exists, requires_access_dag, or the existence lookup with undefined behaviour or type-confused authz decisions. Both helpers still read the raw body before Pydantic body validation runs on the endpoint handler, so this is defense-in-depth: every current POST/PUT route uses a Pydantic body model and FastAPI returns 422 before the auth dependency runs on a malformed body. Tests: existing parse-failure test renamed and updated to assert 400; new parametrised tests cover integer / list / dict / bool inputs for both team_name and dag_id.
potiuk
commented
May 19, 2026
Addressed all three review points in f0d28ed: @jason810496 — agreed, switched to raising
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting |
potiuk
commented
May 19, 2026
@amoghrajesh — yes, commit 2 (
Plus parametrised tests for both in If GitHub's diff UI is hiding them, refreshing against PR HEAD Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting |
Uh oh!
There was an error while loading. Please reload this page.
Backport successfully created: v3-2-testNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
|
We need this too I believe for the newly introduced `requires_access_event_log, I'll open a PR for it. edit: We don't, that's only required for bodies. Path params are already validated beforehand. |
… against malformed bodies (#66504) (#67182) * Fail closed in _collect_teams_to_check on body parse failure For POST/PUT in multi-team mode, the helper used `with suppress(JSONDecodeError)` around `await request.json()`. If the body was unparseable, the suppress swallowed the exception, `teams.add(raw)` never ran, and the calling `requires_access_*` dependency iterated over an empty set — silently skipping the authorization callback entirely. Today this is unreachable because every POST/PUT route in core_api uses a Pydantic body model, so FastAPI returns 422 before the auth dependency runs. But the pattern would silently bypass team-scoped authz if a future route used a raw `Request` instead. Replace the bare suppress with an explicit try/except that adds `None` to `teams` on parse failure, so the auth callback always runs at least once. * Reject malformed bodies in core_api authz helpers with 400 Builds on the previous fail-closed change in _collect_teams_to_check. Two follow-ups from review: * On JSONDecodeError, raise HTTP 400 directly instead of falling through to a team=None auth call — clearer failure mode and removes any ambiguity about whether authz ran. * Reject non-string `team_name` (in _collect_teams_to_check) and non-string `dag_id` (in requires_access_backfill) from the raw body with HTTP 400 before any authz decision or DB lookup. Without this, a list / dict / int / bool would flow into Team.get_name_if_exists, requires_access_dag, or the existence lookup with undefined behaviour or type-confused authz decisions. Both helpers still read the raw body before Pydantic body validation runs on the endpoint handler, so this is defense-in-depth: every current POST/PUT route uses a Pydantic body model and FastAPI returns 422 before the auth dependency runs on a malformed body. Tests: existing parse-failure test renamed and updated to assert 400; new parametrised tests cover integer / list / dict / bool inputs for both team_name and dag_id. (cherry picked from commit 448f846) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: Rahul Vats <43964496+vatsrahul1001@users.noreply.github.com>
… against malformed bodies (#66504) (#67182) * Fail closed in _collect_teams_to_check on body parse failure For POST/PUT in multi-team mode, the helper used `with suppress(JSONDecodeError)` around `await request.json()`. If the body was unparseable, the suppress swallowed the exception, `teams.add(raw)` never ran, and the calling `requires_access_*` dependency iterated over an empty set — silently skipping the authorization callback entirely. Today this is unreachable because every POST/PUT route in core_api uses a Pydantic body model, so FastAPI returns 422 before the auth dependency runs. But the pattern would silently bypass team-scoped authz if a future route used a raw `Request` instead. Replace the bare suppress with an explicit try/except that adds `None` to `teams` on parse failure, so the auth callback always runs at least once. * Reject malformed bodies in core_api authz helpers with 400 Builds on the previous fail-closed change in _collect_teams_to_check. Two follow-ups from review: * On JSONDecodeError, raise HTTP 400 directly instead of falling through to a team=None auth call — clearer failure mode and removes any ambiguity about whether authz ran. * Reject non-string `team_name` (in _collect_teams_to_check) and non-string `dag_id` (in requires_access_backfill) from the raw body with HTTP 400 before any authz decision or DB lookup. Without this, a list / dict / int / bool would flow into Team.get_name_if_exists, requires_access_dag, or the existence lookup with undefined behaviour or type-confused authz decisions. Both helpers still read the raw body before Pydantic body validation runs on the endpoint handler, so this is defense-in-depth: every current POST/PUT route uses a Pydantic body model and FastAPI returns 422 before the auth dependency runs on a malformed body. Tests: existing parse-failure test renamed and updated to assert 400; new parametrised tests cover integer / list / dict / bool inputs for both team_name and dag_id. (cherry picked from commit 448f846) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: Rahul Vats <43964496+vatsrahul1001@users.noreply.github.com>
… against malformed bodies (#66504) (#67182) * Fail closed in _collect_teams_to_check on body parse failure For POST/PUT in multi-team mode, the helper used `with suppress(JSONDecodeError)` around `await request.json()`. If the body was unparseable, the suppress swallowed the exception, `teams.add(raw)` never ran, and the calling `requires_access_*` dependency iterated over an empty set — silently skipping the authorization callback entirely. Today this is unreachable because every POST/PUT route in core_api uses a Pydantic body model, so FastAPI returns 422 before the auth dependency runs. But the pattern would silently bypass team-scoped authz if a future route used a raw `Request` instead. Replace the bare suppress with an explicit try/except that adds `None` to `teams` on parse failure, so the auth callback always runs at least once. * Reject malformed bodies in core_api authz helpers with 400 Builds on the previous fail-closed change in _collect_teams_to_check. Two follow-ups from review: * On JSONDecodeError, raise HTTP 400 directly instead of falling through to a team=None auth call — clearer failure mode and removes any ambiguity about whether authz ran. * Reject non-string `team_name` (in _collect_teams_to_check) and non-string `dag_id` (in requires_access_backfill) from the raw body with HTTP 400 before any authz decision or DB lookup. Without this, a list / dict / int / bool would flow into Team.get_name_if_exists, requires_access_dag, or the existence lookup with undefined behaviour or type-confused authz decisions. Both helpers still read the raw body before Pydantic body validation runs on the endpoint handler, so this is defense-in-depth: every current POST/PUT route uses a Pydantic body model and FastAPI returns 422 before the auth dependency runs on a malformed body. Tests: existing parse-failure test renamed and updated to assert 400; new parametrised tests cover integer / list / dict / bool inputs for both team_name and dag_id. (cherry picked from commit 448f846) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: Rahul Vats <43964496+vatsrahul1001@users.noreply.github.com>
… against malformed bodies (#66504) (#67182) * Fail closed in _collect_teams_to_check on body parse failure For POST/PUT in multi-team mode, the helper used `with suppress(JSONDecodeError)` around `await request.json()`. If the body was unparseable, the suppress swallowed the exception, `teams.add(raw)` never ran, and the calling `requires_access_*` dependency iterated over an empty set — silently skipping the authorization callback entirely. Today this is unreachable because every POST/PUT route in core_api uses a Pydantic body model, so FastAPI returns 422 before the auth dependency runs. But the pattern would silently bypass team-scoped authz if a future route used a raw `Request` instead. Replace the bare suppress with an explicit try/except that adds `None` to `teams` on parse failure, so the auth callback always runs at least once. * Reject malformed bodies in core_api authz helpers with 400 Builds on the previous fail-closed change in _collect_teams_to_check. Two follow-ups from review: * On JSONDecodeError, raise HTTP 400 directly instead of falling through to a team=None auth call — clearer failure mode and removes any ambiguity about whether authz ran. * Reject non-string `team_name` (in _collect_teams_to_check) and non-string `dag_id` (in requires_access_backfill) from the raw body with HTTP 400 before any authz decision or DB lookup. Without this, a list / dict / int / bool would flow into Team.get_name_if_exists, requires_access_dag, or the existence lookup with undefined behaviour or type-confused authz decisions. Both helpers still read the raw body before Pydantic body validation runs on the endpoint handler, so this is defense-in-depth: every current POST/PUT route uses a Pydantic body model and FastAPI returns 422 before the auth dependency runs on a malformed body. Tests: existing parse-failure test renamed and updated to assert 400; new parametrised tests cover integer / list / dict / bool inputs for both team_name and dag_id. (cherry picked from commit 448f846) Co-authored-by: Jarek Potiuk <jarek@potiuk.com> Co-authored-by: Rahul Vats <43964496+vatsrahul1001@users.noreply.github.com>
The authorization helpers in
core_api/security.pyread fields directly from the raw request JSON body before Pydantic body-validation runs on the actual endpoint handler. Two related defects that share the same fix pattern:Commit 1: Fail closed on JSON parse failure (FINDING-008 from L1 sweep)
For POST/PUT in multi-team mode,
_collect_teams_to_checkusedwith suppress(JSONDecodeError)aroundawait request.json(). If the body was unparseable, the suppress swallowed the exception,teams.add(raw)never ran, and the callingrequires_access_*dependency iterated over an empty set — silently skipping the authorization callback entirely.Replace the bare
suppresswith an explicittry/exceptthat addsNonetoteamson parse failure, so the auth callback always runs at least once.Commit 2: Reject non-string
dag_id/team_namefrom raw body before authz (FINDING-060 from L3 sweep)_collect_teams_to_checkreadsteam_name, andrequires_access_backfillreadsdag_id, from the raw JSON body before Pydantic validation runs on the actual endpoint handler. If a body contains a non-string value (list, dict, integer, …) those values would otherwise flow intoTeam.get_name_if_exists/ the authz callback / the existence lookup, producing undefined behaviour or type-confused authz decisions.Raise
400 Bad Requeston a non-stringteam_name/dag_idbefore any auth check runs. Tests parametrised on integer / list / dict / bool inputs assert the 400 + that the authz callback is never consulted.Reachability today
Both defects are unreachable in current
core_apiroutes because every POST/PUT route uses a Pydantic body model, so FastAPI returns 422 before the auth dependency runs on a malformed body. The patterns would silently bypass team-scoped authz if a future route used a rawRequestinstead — both fixes are defense-in-depth, not exploitable today.Reported by
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Opus 4.7 (1M context) following the guidelines