Fix team name authorization bypass in edgeworker#64556
Fix team name authorization bypass in edgeworker#64556gopidesupavan wants to merge 2 commits intoapache:mainfrom
Conversation
boring-cyborg
bot
added
area:providers
provider:edge
|
@jscheffl found by codex security.. dont have much depth on edge worker.. probably please can you decide is it valid or not. at first instance for me it looks valid. |
|
from this commit: codex flagged. gopidesupavan@ee6d8dd |
gopidesupavan
force-pushed
the
fix-team_name-authorization-bypass-in-edgeworker
branch
from
8fc48f0 to
9daa2f0
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses a multi-team authorization bypass in the Edge Worker “fetch job” API by no longer trusting the client-provided team_name and instead selecting jobs based on the persisted worker record.
Changes:
- Load the
EdgeWorkerModelfrom the DB injobs.fetch()and return 404 when the worker is unknown. - Filter queued jobs by
worker.team_name(when set) instead ofbody.team_name. - Update unit tests to create worker records and assert
body.team_nameis ignored for job selection.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| providers/edge3/src/airflow/providers/edge3/worker_api/routes/jobs.py | Fetch now resolves the worker from DB, returns 404 if missing, and uses persisted worker.team_name for filtering. |
| providers/edge3/tests/unit/edge3/worker_api/routes/test_jobs.py | Tests now create EdgeWorkerModel rows and validate fetch behavior with persisted team settings. |
| if worker.team_name is not None: | ||
| query = query.where(EdgeJobModel.team_name == worker.team_name) |
There was a problem hiding this comment.
The team isolation now relies on worker.team_name, but that value is still writable by the worker via the worker registration/heartbeat endpoints (e.g. worker_api/routes/worker.py assigns worker.team_name = body.team_name). A client holding the shared JWT secret can therefore change its persisted team and fetch jobs for another team, so the authorization bypass is not fully addressed.
Consider making team_name server-controlled/immutable (e.g. reject changes when a worker already exists, or pre-provision workers with a fixed team and disallow team_name in the request body), and add validation tests for this behavior.
| worker = session.scalar(select(EdgeWorkerModel).where(EdgeWorkerModel.worker_name == worker_name)) | ||
| if not worker: | ||
| raise HTTPException(status.HTTP_404_NOT_FOUND, "Worker not found") | ||
|
|
There was a problem hiding this comment.
fetch() now raises an HTTP 404 when the worker is unknown, but the route’s documented responses= only includes 400/403. Please update the OpenAPI exception docs for this endpoint to include 404 as well, so clients can see this behavior in the API schema.
| worker = session.scalar(select(EdgeWorkerModel).where(EdgeWorkerModel.worker_name == worker_name)) | ||
| if not worker: | ||
| raise HTTPException(status.HTTP_404_NOT_FOUND, "Worker not found") | ||
|
|
There was a problem hiding this comment.
New behavior: fetch() raises HTTPException(404) when the worker name is not found. The unit tests currently only cover the happy path; please add a regression test asserting the 404 for an unknown worker.
| @@ -136,6 +138,11 @@ def test_state(self, mock_stats_incr, session: Session): | |||
|
|
|||
| def test_fetch_filters_by_team_name(self, session: Session): | |||
There was a problem hiding this comment.
The test name test_fetch_filters_by_team_name is now a bit misleading since the filter is based on the persisted worker’s team (and explicitly ignores body.team_name). Renaming it to something like test_fetch_filters_by_worker_team_name would make the intent clearer and reduce confusion when reading failures.
| def test_fetch_filters_by_team_name(self, session: Session): | |
| def test_fetch_filters_by_worker_team_name(self, session: Session): |
why:
The fetch API trusted client-supplied team_name, so any worker with the shared token could impersonate another team and fetch cross-team jobs. This broke multi-team isolation and created an authorization bypass.
what:
The fetch route now resolves the worker from the database, returns 404 for unknown workers, and uses persisted worker.team_name for job selection instead of body.team_name.
Was generative AI tooling used to co-author this PR?
{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.