fix: restrict Airflow DAG callbacks to the internal-only gateway path - #123
Open
fredj wants to merge 3 commits into
Open
fix: restrict Airflow DAG callbacks to the internal-only gateway path#123fredj wants to merge 3 commits into
fredj wants to merge 3 commits into
Conversation
dag_success/dag_failure for staging and process were public endpoints with no ownership check on integrity_link_id, letting any IMPORT-role user delete or tamper with another user's dataset. Move them under /internal, which the gateway already restricts to ADMINISTRATOR. Airflow reaches them via BACKEND_INTERNAL_URL directly and is unaffected by the path change.
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate compatibility issues remain, along with a regression-test gap.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Restricts Airflow ingestion callbacks to administrator-only /internal routes.
Changes:
- Updates staging and process callback URLs.
- Registers callback handlers under
/internal. - Updates scheduled process DAG callback paths.
File summaries
| File | Summary and review findings |
|---|---|
apps/elt/dags/process-dag-generator.py |
Uses internal process callback URLs. Nit: Add a regression test verifying generated callback URLs use /internal/ingestion/process/... (2 votes). |
apps/backend/src/api/routes/ingestion/staging.py |
Moves staging callbacks to internal routes. Moderate: Existing queued or running DAGs may retain obsolete callback URLs and fail cleanup or metadata updates (1 vote). |
apps/backend/src/api/routes/ingestion/process.py |
Moves process callbacks to internal routes. Moderate: Existing queued or running DAGs may retain obsolete callback URLs and fail cleanup or metadata updates (1 vote). |
apps/backend/src/api/internal.py |
Registers internal ingestion routers. Moderate: Regenerate the Angular client or exclude these routes from the schema to remove stale public callback wrappers (3 votes). |
Review details
Suppressed comments (2)
apps/backend/src/api/routes/ingestion/process.py:311
- Removing the old callback route invalidates URLs already stored in queued or running Airflow DAG runs: the executor saves these URLs in the run
conf, and the DAG later POSTs the saved value. A deployment with an in-flight process run will therefore get a 404 and skip cleanup/metadata updates; drain and retrigger active runs, or provide a temporary independently authenticated compatibility path before removing the old route.
@internal_router.post("/dag_success")
apps/backend/src/api/routes/ingestion/staging.py:707
- Removing the old callback route invalidates URLs already stored in queued or running Airflow DAG runs: the executor saves these URLs in the run
conf, and the DAG later POSTs the saved value. A deployment with an in-flight staging run will therefore get a 404 and skip temp-file/table cleanup; drain and retrigger active runs, or provide a temporary independently authenticated compatibility path before removing the old route.
@internal_router.post("/dag_success")
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
The /internal Airflow-only callbacks and file-read route were still appearing in openapi.json, so the generated Angular client kept wrappers for endpoints only Airflow ever calls. Mark them include_in_schema=False and regenerate the client.
Existing tests only called _build_callback_url with literal paths and never exercised create_dag(), so a revert to the public /ingestion/process/dag_* endpoints would go unnoticed. Assert the generated TriggerDagRunOperator conf uses /internal/... URLs.
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.
dag_success/dag_failure for staging and process were public endpoints with no ownership check on integrity_link_id, letting any IMPORT-role user delete or tamper with another user's dataset. Move them under /internal, which the gateway already restricts to ADMINISTRATOR. Airflow reaches them via BACKEND_INTERNAL_URL directly and is unaffected by the path change.