codeant-ai-for-open-source[bot] commented on code in PR #33924:
URL: https://github.com/apache/superset/pull/33924#discussion_r3607131939
##########
superset/jinja_context.py:
##########
@@ -349,22 +423,32 @@ def filter_values(
def _escape_value(self, val: Any) -> Any:
"""Return a dialect-quoted form of ``val`` suitable for direct SQL
interpolation. When no dialect is configured the value is returned
- unchanged so callers see the raw value as before. Strings are
- passed through SQLAlchemy's ``String`` literal processor (with the
- surrounding quotes stripped, mirroring ``url_param``). Lists are
- processed element-wise; non-string members are left as-is.
+ unchanged so callers see the raw value as before.
+
+ Strings are rendered through the dialect compiler's
+ ``render_literal_value`` (with the surrounding quotes stripped),
+ which applies dialect-specific escaping beyond quote doubling; in
+ particular, MySQL/MariaDB treat the backslash as an escape
+ character, so backslashes are doubled there to prevent a trailing
+ ``\\'`` from re-opening the string literal. Dialects whose escaping
+ mode cannot be introspected without a live connection err on the
+ side of over-escaping, which can distort a backslash-containing
+ value but can never widen the query.
+
+ Lists are processed element-wise and dict values recursively, so
+ strings nested inside JSON structures are also escaped; dict keys
+ are left untouched since they are used for member lookups, not
+ interpolation. Non-string leaf values are left as-is.
"""
if not self.dialect:
return val
if isinstance(val, str):
- return
String().literal_processor(dialect=self.dialect)(value=val)[1:-1]
+ compiler = self.dialect.statement_compiler(self.dialect, None)
+ return compiler.render_literal_value(val, String())[1:-1]
Review Comment:
**Suggestion:** This escaping path assumes every rendered literal is wrapped
by exactly one leading and trailing quote, then blindly strips the first and
last character. That breaks on dialect outputs that add string
prefixes/alternate literal forms, producing malformed escaped values and
potentially invalid SQL interpolation. Strip quotes only when they are actually
present as a matching pair and preserve any dialect-specific prefix handling.
[security]
<details>
<summary><b>Severity Level:</b> Critical 🚨</summary>
```mdx
- ❌ MSSQL URL-param templates can render malformed string literals.
- ❌ Guest attributes on MSSQL may produce invalid interpolated SQL.
- ⚠️ Dialects with literal prefixes bypass intended escaping semantics.
```
</details>
<details>
<summary><b>Steps of Reproduction âś… </b></summary>
```mdx
1. Configure a Microsoft SQL Server database in Superset, which uses
`MssqlEngineSpec`
(`superset/db_engine_specs/mssql.py:52-88`) and creates a `Database` whose
`get_dialect()`
returns SQLAlchemy’s MSSQL dialect; when building the Jinja context for this
datasource,
`JinjaTemplateProcessor.set_context()` at
`superset/jinja_context.py:987-999` constructs
an `ExtraCache` with `dialect=self._database.get_dialect()`.
2. Create a chart or virtual dataset on this MSSQL connection that uses SQL
templating
with a string macro such as the documented URL-parameter example `WHERE
country = '{{
url_param("country") }}'`
(`docs/docs/using-superset/sql-templating.mdx:10-16`); when the
chart runs, the template engine invokes `ExtraCache.url_param()` at
`superset/jinja_context.py:265-311`, which calls
`self._escape_value(result)` whenever
`escape_result=True` and a dialect is configured.
3. Inside `_escape_value()` at `superset/jinja_context.py:423-452`, the
MSSQL dialect’s
compiler is instantiated (`compiler =
self.dialect.statement_compiler(self.dialect,
None)`) and `compiler.render_literal_value(val, String())` is called; for
dialects like
MSSQL (and PostgreSQL in certain configurations) this method can return
prefixed literals
such as `N'...'` or `E'...'` rather than a bare `'...'`.
4. The current implementation blindly returns
`compiler.render_literal_value(...)[1:-1]`,
stripping only the first and last characters without checking for prefixes
or matching
quotes; for prefixed outputs this drops the prefix and the closing quote
instead of just
the surrounding quotes, so the value handed back to the template—used by
`url_param()`,
`get_guest_user_attribute()` (`superset/jinja_context.py:313-380`), or as
`escaped_val` in
`get_filters()` (`superset/jinja_context.py:574-577`)—is a malformed
fragment (for example
`''value'`) that can yield invalid or mis-escaped SQL when interpolated into
queries on
those dialects.
```
</details>
[](https://app.codeant.ai/fix-in-ide?tool=cursor&prompt_id=02f8c64856dd4a45a44b3156ddbe96b0&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=02f8c64856dd4a45a44b3156ddbe96b0&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/jinja_context.py
**Line:** 445:447
**Comment:**
*Security: This escaping path assumes every rendered literal is wrapped
by exactly one leading and trailing quote, then blindly strips the first and
last character. That breaks on dialect outputs that add string
prefixes/alternate literal forms, producing malformed escaped values and
potentially invalid SQL interpolation. Strip quotes only when they are actually
present as a matching pair and preserve any dialect-specific prefix handling.
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%2F33924&comment_hash=7b1f5fb6a8b5526d787a14d86e07483aba949f57fee2b27c877deaeaba83ea72&reaction=like'>đź‘Ť</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F33924&comment_hash=7b1f5fb6a8b5526d787a14d86e07483aba949f57fee2b27c877deaeaba83ea72&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]
