codeant-ai-for-open-source[bot] commented on code in PR #42095:
URL: https://github.com/apache/superset/pull/42095#discussion_r3596836050
##########
superset/models/helpers.py:
##########
@@ -749,6 +749,109 @@ def modified(self) -> Markup:
_NO_BYPASS: frozenset[type] = frozenset()
+def _make_metric_line_comments_safe( # noqa: C901
+ expression: str,
+) -> tuple[str, bool]:
+ """Convert SQL line comments without regenerating otherwise valid SQL."""
+ result: list[str] = []
+ index = 0
+ quote: str | None = None
+ dollar_quote: str | None = None
+ backslash_escapes = False
+ in_block_comment = False
+ while index < len(expression):
+ if in_block_comment:
+ end = expression.find("*/", index)
+ if end < 0:
+ return expression, False
+ result.append(expression[index : end + 2])
+ index = end + 2
+ in_block_comment = False
+ continue
+ if dollar_quote:
+ end = expression.find(dollar_quote, index)
+ if end < 0:
+ return expression, False
+ result.append(expression[index : end + len(dollar_quote)])
+ index = end + len(dollar_quote)
+ dollar_quote = None
+ continue
+ if quote:
+ character = expression[index]
+ result.append(character)
+ index += 1
+ if backslash_escapes and character == "\\" and index <
len(expression):
+ result.append(expression[index])
+ index += 1
+ elif character == quote:
+ if index < len(expression) and expression[index] == quote:
+ result.append(expression[index])
+ index += 1
+ else:
+ quote = None
+ backslash_escapes = False
+ continue
+
+ if expression.startswith("/*", index):
+ in_block_comment = True
+ continue
+ if expression.startswith("--", index):
+ line_end = len(expression)
+ for separator in ("\n", "\r"):
+ separator_index = expression.find(separator, index + 2)
+ if separator_index >= 0:
+ line_end = min(line_end, separator_index)
+ contents = expression[index + 2 : line_end]
+ if "*/" in contents:
+ return expression, False
+ result.append(f"/*{contents} */")
+ index = line_end
+ continue
+ if expression[index] in {"'", '"'}:
+ quote = expression[index]
+ backslash_escapes = (
+ quote == "'"
+ and index > 0
+ and expression[index - 1] in {"E", "e"}
+ and (index == 1 or not expression[index - 2].isalnum())
+ )
+ result.append(quote)
+ index += 1
+ continue
+ if expression[index] == "$":
+ match = re.match(r"\$[A-Za-z_][A-Za-z0-9_]*\$|\$\$",
expression[index:])
+ if match:
+ dollar_quote = match.group(0)
+ result.append(dollar_quote)
+ index += len(dollar_quote)
+ continue
+ result.append(expression[index])
+ index += 1
+ if quote or dollar_quote or in_block_comment:
+ return expression, False
+ return "".join(result), True
+
+
+def _normalize_custom_sql_metric(
+ expression: str,
+ engine: str,
+ normalizer: Callable[[str], str],
+) -> tuple[str, bool]:
+ """Normalize a metric and report whether its source is safe to preserve."""
+ expression = normalizer(expression)
+ if engine not in {"postgresql", "redshift"}:
+ return expression, False
Review Comment:
**Suggestion:** The PostgreSQL-family guard omits the supported `postgres`
backend alias, so alias-based Postgres connections skip the Postgres-specific
safe-source path and fall back to generic clause sanitization behavior. This
can reintroduce SQL regeneration differences for comment-bearing expressions on
valid Postgres connections. Include the alias in this engine check (or key off
the engine spec instead of a hard-coded string set). [api mismatch]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ❌ Postgres metrics on 'postgres://' skip comment-preserving safe path.
- ⚠️ DATE_TRUNC unit normalization inconsistent across Postgres backend
variants.
- ⚠️ Alias Postgres connections can rehit grouping mismatch bug.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Configure a Database using a SQLAlchemy URI with the "postgres" scheme
(for example
"postgres://user:pass@host/db"); the Database.backend property at
"superset/models/core.py:93-95" returns the backend name "postgres" based on
the URL.
2. Attach a SqlaTable and a SqlMetric to this Database, following the
pattern in
"superset/connectors/sqla/models.py:1230-25" where SqlMetric.get_sqla_col()
reads
metric.expression and then calls `_normalize_custom_sql_metric(expression,
self.table.database.backend,
self.table.database.db_engine_spec.normalize_custom_sql_metric)`.
3. Use a metric expression matching the tested Postgres DATE_TRUNC patterns
but with a
trailing line comment, for example "DATE_TRUNC('QUARTER', created_at) --
trailing", as in
"superset/tests/integration_tests/sqla_models_tests.py:80-15", and compile
the metric via
`metric.get_sqla_col()` on the alias-backed Database.
4. When `_normalize_custom_sql_metric()` executes in
"superset/models/helpers.py:141-145",
the engine argument is "postgres", so the guard at lines 842-843 (`if engine
not in
{"postgresql", "redshift"}: return expression, False`) short-circuits before
the
PostgreSQL-family comment-safe path; saved metrics on "postgres://" backends
never run
`_make_metric_line_comments_safe()` and adhoc metrics processed via
`_process_sql_expression()` likewise skip the `source_is_safe` restoration,
causing
alias-based Postgres connections to use generic sanitization semantics and
potentially
reintroducing the DATE_TRUNC grouping mismatch described in SC-112048.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=5e4de6699b50425ea333818f89c63bdb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=5e4de6699b50425ea333818f89c63bdb&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 842:843
**Comment:**
*Api Mismatch: The PostgreSQL-family guard omits the supported
`postgres` backend alias, so alias-based Postgres connections skip the
Postgres-specific safe-source path and fall back to generic clause sanitization
behavior. This can reintroduce SQL regeneration differences for comment-bearing
expressions on valid Postgres connections. Include the alias in this engine
check (or key off the engine spec instead of a hard-coded string set).
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42095&comment_hash=cd0ae770e8f4356fddf55379012cb30801915bd545d82a996f0ae48a2163d8e3&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42095&comment_hash=cd0ae770e8f4356fddf55379012cb30801915bd545d82a996f0ae48a2163d8e3&reaction=dislike'>👎</a>
##########
superset/models/helpers.py:
##########
@@ -749,6 +749,109 @@ def modified(self) -> Markup:
_NO_BYPASS: frozenset[type] = frozenset()
+def _make_metric_line_comments_safe( # noqa: C901
+ expression: str,
+) -> tuple[str, bool]:
+ """Convert SQL line comments without regenerating otherwise valid SQL."""
+ result: list[str] = []
+ index = 0
+ quote: str | None = None
+ dollar_quote: str | None = None
+ backslash_escapes = False
+ in_block_comment = False
+ while index < len(expression):
+ if in_block_comment:
+ end = expression.find("*/", index)
+ if end < 0:
+ return expression, False
+ result.append(expression[index : end + 2])
+ index = end + 2
+ in_block_comment = False
+ continue
+ if dollar_quote:
+ end = expression.find(dollar_quote, index)
+ if end < 0:
+ return expression, False
+ result.append(expression[index : end + len(dollar_quote)])
+ index = end + len(dollar_quote)
+ dollar_quote = None
+ continue
+ if quote:
+ character = expression[index]
+ result.append(character)
+ index += 1
+ if backslash_escapes and character == "\\" and index <
len(expression):
+ result.append(expression[index])
+ index += 1
+ elif character == quote:
+ if index < len(expression) and expression[index] == quote:
+ result.append(expression[index])
+ index += 1
+ else:
+ quote = None
+ backslash_escapes = False
+ continue
+
+ if expression.startswith("/*", index):
+ in_block_comment = True
+ continue
+ if expression.startswith("--", index):
+ line_end = len(expression)
+ for separator in ("\n", "\r"):
+ separator_index = expression.find(separator, index + 2)
+ if separator_index >= 0:
+ line_end = min(line_end, separator_index)
+ contents = expression[index + 2 : line_end]
+ if "*/" in contents:
+ return expression, False
+ result.append(f"/*{contents} */")
+ index = line_end
+ continue
+ if expression[index] in {"'", '"'}:
+ quote = expression[index]
+ backslash_escapes = (
+ quote == "'"
+ and index > 0
+ and expression[index - 1] in {"E", "e"}
+ and (index == 1 or not expression[index - 2].isalnum())
+ )
+ result.append(quote)
+ index += 1
+ continue
+ if expression[index] == "$":
+ match = re.match(r"\$[A-Za-z_][A-Za-z0-9_]*\$|\$\$",
expression[index:])
+ if match:
+ dollar_quote = match.group(0)
+ result.append(dollar_quote)
+ index += len(dollar_quote)
+ continue
+ result.append(expression[index])
+ index += 1
+ if quote or dollar_quote or in_block_comment:
+ return expression, False
+ return "".join(result), True
+
+
+def _normalize_custom_sql_metric(
+ expression: str,
+ engine: str,
+ normalizer: Callable[[str], str],
+) -> tuple[str, bool]:
+ """Normalize a metric and report whether its source is safe to preserve."""
+ expression = normalizer(expression)
+ if engine not in {"postgresql", "redshift"}:
+ return expression, False
+
+ expression, source_is_safe = _make_metric_line_comments_safe(expression)
+ if source_is_safe:
+ return expression, True
+
+ try:
+ return sanitize_clause(expression, engine), False
+ except QueryClauseValidationException:
+ return expression, False
Review Comment:
**Suggestion:** This fallback swallows clause-validation failures and
returns the original expression, which can propagate unnormalized/unsafe SQL to
callers that reuse this helper without re-validating (for example saved-metric
compilation paths). Instead of silently returning raw SQL here, propagate a
validation error or return a sanitized-safe fallback so all call sites get
consistent safety guarantees. [security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ Saved Postgres metrics can bypass clause validation and sanitization.
- ⚠️ Invalid or unsafe metric SQL reaches query construction unchanged.
- ⚠️ Inconsistent behavior versus adhoc metric validation path.
```
</details>
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Create a PostgreSQL or Redshift Database (for example with
"postgresql://" as in
"superset/tests/integration_tests/sqla_models_tests.py:80-15") and attach a
SqlaTable and
SqlMetric, so that SqlMetric.get_sqla_col() in
"superset/connectors/sqla/models.py:1230-25" is used to compile the saved
metric.
2. Define the saved metric.expression as a clause that is invalid for the
SQL parser, such
as a multi-statement or otherwise malformed fragment, so that
`sanitize_clause(expression,
engine)` in "superset/sql/parse.py:2124-39" would raise
QueryClauseValidationException
when invoked.
3. Call `metric.get_sqla_col()`; its implementation at
"superset/connectors/sqla/models.py:1230-25" optionally applies a Jinja
template processor
and then calls `_normalize_custom_sql_metric(expression,
self.table.database.backend,
self.table.database.db_engine_spec.normalize_custom_sql_metric)` before
wrapping the
result in `literal_column(expression)`.
4. Inside `_normalize_custom_sql_metric()` at
"superset/models/helpers.py:141-153", after
normalizing DATE_TRUNC units, the block at lines 849-852 (`try: return
sanitize_clause(expression, engine), False except
QueryClauseValidationException: return
expression, False`) catches any clause-validation failure from
`sanitize_clause` and
silently returns the original, unvalidated expression; since
SqlMetric.get_sqla_col()
ignores the boolean flag and performs no further validation, the raw invalid
or
potentially unsafe SQL is propagated into query construction, whereas adhoc
metric paths
using `_process_sql_expression()` re-run `sanitize_clause` and raise
QueryObjectValidationError, creating inconsistent safety guarantees across
call sites.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=85fada5cabc84f5a93fc3e7da45dfab8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
[](https://app.codeant.ai/fix-in-ide?tool=vscode-claude&prompt_id=85fada5cabc84f5a93fc3e7da45dfab8&service=github&base_url=https%3A%2F%2Fgithub.com&org=apache&repo=apache%2Fsuperset)
*(Use Cmd/Ctrl + Click for best experience)*
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** superset/models/helpers.py
**Line:** 849:852
**Comment:**
*Security: This fallback swallows clause-validation failures and
returns the original expression, which can propagate unnormalized/unsafe SQL to
callers that reuse this helper without re-validating (for example saved-metric
compilation paths). Instead of silently returning raw SQL here, propagate a
validation error or return a sanitized-safe fallback so all call sites get
consistent safety guarantees.
Validate the correctness of the flagged issue. If correct, How can I resolve
this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask
user if the user wants to fix the rest of the comments as well. if said yes,
then fetch all the comments validate the correctness and implement a minimal fix
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42095&comment_hash=b7b2919ec793d6d5b1d8b21b387f7f4b1b93d77d8052cb67fa8fa944d82ed5c9&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F42095&comment_hash=b7b2919ec793d6d5b1d8b21b387f7f4b1b93d77d8052cb67fa8fa944d82ed5c9&reaction=dislike'>👎</a>
--
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]
