-
Notifications
You must be signed in to change notification settings - Fork 17.6k
fix: prevent SQL parameterization of duplicate_key_handling in MySQL bulk_load_custom #62865
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |
|
|
||
| import json | ||
| import logging | ||
| from typing import TYPE_CHECKING, Any, Union | ||
| from typing import TYPE_CHECKING, Any, Literal, Union | ||
| from urllib.parse import quote_plus, urlencode | ||
|
|
||
| from airflow.providers.common.compat.sdk import AirflowOptionalProviderFeatureException | ||
|
|
@@ -318,7 +318,11 @@ | |
| return token, port | ||
|
|
||
| def bulk_load_custom( | ||
| self, table: str, tmp_file: str, duplicate_key_handling: str = "IGNORE", extra_options: str = "" | ||
| self, | ||
| table: str, | ||
| tmp_file: str, | ||
| duplicate_key_handling: Literal["IGNORE", "REPLACE", ""] = "IGNORE", | ||
| extra_options: str = "", | ||
| ) -> None: | ||
| """ | ||
| Load local data from a file into the database in a more configurable way. | ||
|
|
@@ -339,11 +343,31 @@ | |
|
|
||
| .. seealso:: https://dev.mysql.com/doc/refman/8.0/en/load-data.html | ||
| """ | ||
| _VALID_DUPLICATE_KEY_HANDLING = {"IGNORE", "REPLACE", ""} | ||
| if duplicate_key_handling not in _VALID_DUPLICATE_KEY_HANDLING: | ||
| raise ValueError( | ||
| f"Invalid duplicate_key_handling: {duplicate_key_handling!r}. " | ||
| f"Must be one of {_VALID_DUPLICATE_KEY_HANDLING}." | ||
| ) | ||
|
|
||
| import re | ||
|
|
||
| if extra_options and not re.match(r"^[A-Z @=',;()\w\s.*/-]+$", extra_options): | ||
| raise ValueError( | ||
| f"Invalid extra_options: {extra_options!r}. " | ||
| "Only alphanumeric characters, spaces, and common SQL clauses are allowed." | ||
| ) | ||
|
|
||
| conn = self.get_conn() | ||
| cursor = conn.cursor() | ||
|
|
||
| sql_statement = f"LOAD DATA LOCAL INFILE %s %s INTO TABLE `{table}` %s" | ||
| parameters = (tmp_file, duplicate_key_handling, extra_options) | ||
| # duplicate_key_handling and extra_options are SQL keywords (e.g. IGNORE, REPLACE) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we please also verify if these options folow the literals allowed ? And also ideally add it as MyPy Literal type in the definiion - including table anad extra_options? Those are interpolated directly so sql injection protection here would be very handy (not strictly necessary - but this is potentially a bag of worms.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about extra_options? |
||
| # and must be interpolated into the statement, not passed as query parameters, | ||
| # because parameterized values get quoted as strings which produces invalid SQL. | ||
| sql_statement = ( | ||
| f"LOAD DATA LOCAL INFILE %s {duplicate_key_handling} INTO TABLE `{table}` {extra_options}" | ||
| ) | ||
| parameters = (tmp_file,) | ||
| cursor.execute( | ||
| sql_statement, | ||
| parameters, | ||
|
|
||
Check warning
Code scanning / CodeQL
Overly permissive regular expression range Medium