Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize DagRun.run_id and allow flexibility #32293

Merged
merged 1 commit into from
Jul 2, 2023

Conversation

ephraimbuddy
Copy link
Contributor

This commit sanitizes the DagRun.run_id parameter by introducing a configurable option. Users now have the ability to select a specific run_id pattern for their runs, ensuring stricter control over the values used. This update does not impact the default run_id generation performed by the scheduler for scheduled DAG runs or for Dag runs triggered without modifying the run_id parameter in the run configuration page. The configuration flexibility empowers users to align the run_id pattern with their specific requirements.

@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Jun 30, 2023
@ephraimbuddy ephraimbuddy force-pushed the sanitize-run-id branch 3 times, most recently from 736bffd to bf7d84b Compare July 1, 2023 00:56
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

LGTM

@potiuk
Copy link
Member

potiuk commented Jul 1, 2023

The error should be fixed by #32307 . please reabase @ephraimbuddy

This commit sanitizes the DagRun.run_id parameter by introducing a configurable option.
Users now have the ability to select a specific run_id pattern for their runs,
ensuring stricter control over the values used. This update does not impact the default run_id
generation performed by the scheduler for scheduled DAG runs or for Dag runs triggered without
modifying the run_id parameter in the run configuration page.
The configuration flexibility empowers users to align the run_id pattern with their specific requirements.
@ephraimbuddy ephraimbuddy added this to the Airlfow 2.6.3 milestone Jul 2, 2023
@ephraimbuddy ephraimbuddy merged commit 05bd90f into apache:main Jul 2, 2023
@ephraimbuddy ephraimbuddy deleted the sanitize-run-id branch July 2, 2023 11:31
@ephraimbuddy ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Jul 6, 2023
ephraimbuddy added a commit that referenced this pull request Jul 6, 2023
This commit sanitizes the DagRun.run_id parameter by introducing a configurable option.
Users now have the ability to select a specific run_id pattern for their runs,
ensuring stricter control over the values used. This update does not impact the default run_id
generation performed by the scheduler for scheduled DAG runs or for Dag runs triggered without
modifying the run_id parameter in the run configuration page.
The configuration flexibility empowers users to align the run_id pattern with their specific requirements.

(cherry picked from commit 05bd90f)
@rickyzhang82
Copy link

@ephraimbuddy

It is unclear to me how DAG RUN ID could exploit illegal file access, given the limited information from CVE-2023-22887.

But rather than fixing the root cause, we impose a default regex pattern check allowed_run_id_pattern = ^[A-Za-z0-9_.~:+-]+$.

How can we guarantee that the string from this pattern won't generate any malicious code? Shouldn't we address the root cause instead? We don't fix the SQL injection by limiting the string from users with regex. We validate the SQL string with prepared statements. Regex itself is not a validation but a restriction.

ephraimbuddy added a commit to astronomer/airflow that referenced this pull request Jan 18, 2024
While working on sanitizing the run_id(apache#32293), I replaced spaces with +
which is not an ideal way to quote a URL. This led to issues when users
use spaces in their DAG run_id.
This commit fixes the issue by using urllib.parse.quote to properly quote
the URL.
ephraimbuddy added a commit that referenced this pull request Jan 18, 2024
While working on sanitizing the run_id(#32293), I replaced spaces with +
which is not an ideal way to quote a URL. This led to issues when users
use spaces in their DAG run_id.
This commit fixes the issue by using urllib.parse.quote to properly quote
the URL.
jedcunningham pushed a commit that referenced this pull request Feb 9, 2024
While working on sanitizing the run_id(#32293), I replaced spaces with +
which is not an ideal way to quote a URL. This led to issues when users
use spaces in their DAG run_id.
This commit fixes the issue by using urllib.parse.quote to properly quote
the URL.

(cherry picked from commit ec8fab5)
ephraimbuddy added a commit that referenced this pull request Feb 22, 2024
While working on sanitizing the run_id(#32293), I replaced spaces with +
which is not an ideal way to quote a URL. This led to issues when users
use spaces in their DAG run_id.
This commit fixes the issue by using urllib.parse.quote to properly quote
the URL.

(cherry picked from commit ec8fab5)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:webserver Webserver related Issues type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants