korbit-ai[bot] commented on code in PR #32610:
URL: https://github.com/apache/superset/pull/32610#discussion_r1990450419
##########
superset/db_engine_specs/clickhouse.py:
##########
@@ -414,3 +417,15 @@ def _mutate_label(label: str) -> str:
:return: Conditionally mutated label
"""
return f"{label}_{md5_sha_from_str(label)[:6]}"
+
+ @classmethod
+ def adjust_engine_params(
+ cls,
+ uri: URL,
+ connect_args: dict[str, Any],
+ catalog: str | None = None,
+ schema: str | None = None,
+ ) -> tuple[URL, dict[str, Any]]:
+ if schema:
+ uri = uri.set(database=parse.quote(schema, safe=""))
Review Comment:
It's true that other `engine_specs` like Hive might not verify their schema
values, but I believe it would still be good practice to include the validation
for ClickHouse in this case. It could help prevent potential SQL injection or
path traversal attacks in the future. Even better, we could potentially update
all `engine_specs` to include this validation. I agree with your concern though
- should we implement this only for ClickHouse or consider it for all
`engine_specs`?
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
