You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "potiuk (via GitHub)" <gi...@apache.org> on 2026/05/07 01:25:01 UTC
[PR] Fail closed in _collect_teams_to_check on body parse failure [airflow]
potiuk opened a new pull request, #66504:
URL: https://github.com/apache/airflow/pull/66504
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.
Reported in apache/tooling-agents#23 (ASVS L1 finding F-008).
---
##### Was generative AI tooling used to co-author this PR?
- [X] Yes — Claude Opus 4.7 (1M context)
Generated-by: Claude Opus 4.7 (1M context) following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden _collect_teams_to_check / requires_access_backfill against malformed bodies [airflow]
Posted by "vatsrahul1001 (via GitHub)" <gi...@apache.org>.
vatsrahul1001 commented on PR #66504:
URL: https://github.com/apache/airflow/pull/66504#issuecomment-4479953946
@potiuk can you address comments?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden _collect_teams_to_check / requires_access_backfill against malformed bodies [airflow]
Posted by "vatsrahul1001 (via GitHub)" <gi...@apache.org>.
vatsrahul1001 commented on PR #66504:
URL: https://github.com/apache/airflow/pull/66504#issuecomment-4474367922
Ready for maintainer review
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden _collect_teams_to_check / requires_access_backfill against malformed bodies [airflow]
Posted by "vatsrahul1001 (via GitHub)" <gi...@apache.org>.
vatsrahul1001 merged PR #66504:
URL: https://github.com/apache/airflow/pull/66504
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden _collect_teams_to_check / requires_access_backfill against malformed bodies [airflow]
Posted by "amoghrajesh (via GitHub)" <gi...@apache.org>.
amoghrajesh commented on code in PR #66504:
URL: https://github.com/apache/airflow/pull/66504#discussion_r3263723594
##########
airflow-core/tests/unit/api_fastapi/core_api/test_security.py:
##########
@@ -587,6 +587,37 @@ async def test_requires_access_connection_post_invalid_team_returns_400(
assert exc_info.value.status_code == 400
assert "nonexistent" in exc_info.value.detail
+ @pytest.mark.db_test
+ @pytest.mark.parametrize("method", ["POST", "PUT"])
+ @patch.object(Team, "get_name_if_exists")
+ @patch.object(Connection, "get_team_name")
+ @patch("airflow.api_fastapi.core_api.security.get_auth_manager")
+ async def test_requires_access_connection_body_parse_failure_fails_closed(
+ self, mock_get_auth_manager, mock_get_team_name, mock_get_name_if_exists, method
+ ):
+ """If the request body cannot be parsed, the auth callback must still run with team=None."""
+ from json import JSONDecodeError
Review Comment:
Can we move this to top level pls?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden _collect_teams_to_check / requires_access_backfill against malformed bodies [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #66504:
URL: https://github.com/apache/airflow/pull/66504#issuecomment-4486604043
Addressed all three review points in f0d28ed81c:
@jason810496 — agreed, switched to raising `HTTP_400_BAD_REQUEST` directly on `JSONDecodeError` instead of falling through to a `team=None` auth call. Updated the existing parse-failure test (renamed `..._fails_closed` → `..._returns_400`) to assert the 400 and that `is_authorized_connection` is never called.
@amoghrajesh —
- Moved `from json import JSONDecodeError` to the top of `test_security.py`.
- On commit 2 not being in the diff: you were right, the non-string rejection was in a second commit on my local branch but only the first one made it into the original push — sorry about that. It's now in this update, with parametrised tests covering integer / list / dict / bool inputs for both \`team_name\` (via \`requires_access_connection\`) and \`dag_id\` (via \`requires_access_backfill\`). Both raise \`400\` before any authz callback or DB lookup runs.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden _collect_teams_to_check / requires_access_backfill against malformed bodies [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #66504:
URL: https://github.com/apache/airflow/pull/66504#issuecomment-4487483439
@amoghrajesh — yes, commit 2 (`f0d28ed81c`, "Reject malformed bodies in core_api authz helpers with 400") is the FINDING-060 changes. They're visible in the current diff:
- `_collect_teams_to_check` — `if raw is not None and not isinstance(raw, str): raise HTTPException(400, "'team_name' must be a string")` (security.py L777-782).
- `requires_access_backfill` — `if dag_id is not None and not isinstance(dag_id, str): raise HTTPException(400, "'dag_id' must be a string")` (security.py L344-349).
Plus parametrised tests for both in `test_security.py` (`test_requires_access_connection_non_string_team_name_returns_400` and `test_requires_access_backfill_non_string_dag_id_returns_400`).
If GitHub's diff UI is hiding them, refreshing against PR HEAD `f0d28ed81c4feb42e6759bb2c83f1a15bef20985` should show them.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden _collect_teams_to_check / requires_access_backfill against malformed bodies [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on PR #66504:
URL: https://github.com/apache/airflow/pull/66504#issuecomment-4472300682
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
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden _collect_teams_to_check / requires_access_backfill against malformed bodies [airflow]
Posted by "jason810496 (via GitHub)" <gi...@apache.org>.
jason810496 commented on code in PR #66504:
URL: https://github.com/apache/airflow/pull/66504#discussion_r3257331375
##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -757,8 +757,12 @@ async def _collect_teams_to_check(
if method != "POST":
teams.add(get_existing_team(resource_id) if resource_id else None)
if method in ("POST", "PUT"):
- with suppress(JSONDecodeError):
+ try:
raw = (await request.json()).get("team_name")
+ except JSONDecodeError:
+ # Fail closed: ensure auth callback still runs when team can't be determined.
+ teams.add(None)
Review Comment:
Would it be better to directly raise `HTTP_400_BAD_REQUEST` if encounter `JSONDecodeError` (early return to avoid the further bypass)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden _collect_teams_to_check / requires_access_backfill against malformed bodies [airflow]
Posted by "potiuk (via GitHub)" <gi...@apache.org>.
potiuk commented on code in PR #66504:
URL: https://github.com/apache/airflow/pull/66504#discussion_r3266058397
##########
airflow-core/src/airflow/api_fastapi/core_api/security.py:
##########
@@ -757,8 +757,12 @@ async def _collect_teams_to_check(
if method != "POST":
teams.add(get_existing_team(resource_id) if resource_id else None)
if method in ("POST", "PUT"):
- with suppress(JSONDecodeError):
+ try:
raw = (await request.json()).get("team_name")
+ except JSONDecodeError:
+ # Fail closed: ensure auth callback still runs when team can't be determined.
+ teams.add(None)
Review Comment:
Yep — went with the stricter "raise 400" approach. In `f0d28ed81c`, `_collect_teams_to_check` now does:
```python
try:
body = await request.json()
except JSONDecodeError:
raise HTTPException(
status_code=status.HTTP_400_BAD_REQUEST,
detail="Request body is not valid JSON",
)
```
No more `suppress`, no chance of the auth callback running with an empty set. Note the analogous `with suppress(JSONDecodeError):` in `requires_access_backfill` is pre-existing and intentionally left alone in this PR — happy to tighten it in a follow-up if you'd like a consistent treatment across both helpers.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
##########
airflow-core/tests/unit/api_fastapi/core_api/test_security.py:
##########
@@ -587,6 +587,37 @@ async def test_requires_access_connection_post_invalid_team_returns_400(
assert exc_info.value.status_code == 400
assert "nonexistent" in exc_info.value.detail
+ @pytest.mark.db_test
+ @pytest.mark.parametrize("method", ["POST", "PUT"])
+ @patch.object(Team, "get_name_if_exists")
+ @patch.object(Connection, "get_team_name")
+ @patch("airflow.api_fastapi.core_api.security.get_auth_manager")
+ async def test_requires_access_connection_body_parse_failure_fails_closed(
+ self, mock_get_auth_manager, mock_get_team_name, mock_get_name_if_exists, method
+ ):
+ """If the request body cannot be parsed, the auth callback must still run with team=None."""
+ from json import JSONDecodeError
Review Comment:
Done in `f0d28ed81c` — `from json import JSONDecodeError` is now at module top (`test_security.py:19`); the inline import inside the test body was removed when the test was renamed to `test_requires_access_connection_body_parse_failure_returns_400`.
---
Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden _collect_teams_to_check / requires_access_backfill against malformed bodies [airflow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #66504:
URL: https://github.com/apache/airflow/pull/66504#issuecomment-4488342882
### Backport successfully created: v3-2-test
Note: As of [Merging PRs targeted for Airflow 3.X](https://github.com/apache/airflow/blob/main/dev/README_AIRFLOW3_DEV.md#merging-prs-targeted-for-airflow-3x)
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](https://apache-airflow.slack.com/archives/C03G9H97MM2) Slack channel.
<table>
<tr>
<th>Status</th>
<th>Branch</th>
<th>Result</th>
</tr>
<tr>
<td>✅</td>
<td>v3-2-test</td>
<td><a href="https://github.com/apache/airflow/pull/67182"><img src="https://img.shields.io/badge/PR-67182-blue" alt="PR Link"></a></td>
</tr>
</table>
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Harden _collect_teams_to_check / requires_access_backfill against malformed bodies [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #66504:
URL: https://github.com/apache/airflow/pull/66504#issuecomment-4488545796
We need this too I believe for the newly introduced `requires_access_event_log`
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org