codeant-ai-for-open-source[bot] commented on code in PR #38777:
URL: https://github.com/apache/superset/pull/38777#discussion_r2988054034
##########
docs/admin_docs/configuration/sql-templating.mdx:
##########
@@ -324,6 +335,19 @@ cache hit in the future and Superset can retrieve cached
data.
The `{{ url_param('custom_variable') }}` macro lets you define arbitrary URL
parameters and reference them in your SQL code.
+:::warning
+`url_param()` returns the raw URL query-string value **without escaping or
sanitization**. If the rendered value is placed directly into a SQL string
literal (e.g. `WHERE col = '{{ url_param("x") }}'`), a user who controls the
URL can break out of the string context and inject arbitrary SQL.
Review Comment:
**Suggestion:** The new warning says `url_param()` always returns raw
unescaped input, but the implementation escapes values by default in form-data
contexts (`escape_result=True`) and only returns raw values in specific paths
(for example direct request args). This incorrect contract can lead readers to
apply extra manual escaping on already-escaped values, causing double-escaping
and incorrect query results. Clarify that input is untrusted and
context-dependent rather than universally raw. [logic error]
<details>
<summary><b>Severity Level:</b> Major ⚠️</summary>
```mdx
- ⚠️ SQL templating docs misstate `url_param()` output behavior.
- ⚠️ Users may double-escape values in chart SQL templates.
- ⚠️ Query filters can return incorrect string-matching results.
```
</details>
```suggestion
`url_param()` should always be treated as untrusted input. Depending on
context it may be returned raw (for example direct request args) or SQL-escaped
using the active dialect; if escaping is disabled or values are concatenated
into SQL literals/fragments, a user who controls the URL can break out of
string context and inject arbitrary SQL.
```
<details>
<summary><b>Steps of Reproduction ✅ </b></summary>
```mdx
1. Follow the real template execution path:
`JinjaTemplateProcessor.set_context()` binds
`url_param` into Jinja context at `superset/jinja_context.py:832`, so SQL
templates call
`ExtraCache.url_param()`.
2. Trigger request-args path: in `ExtraCache.url_param()` at
`superset/jinja_context.py:288-290`, when `request.args` has the key, value
is returned
immediately with no escaping.
3. Trigger form-data path: same method at
`superset/jinja_context.py:291-297` reads
`form_data["url_params"]` and escapes by default when `escape_result=True`
and dialect
exists.
4. Verify behavior from tests: `tests/unit_tests/jinja_context_test.py:309`
expects
escaped `"O''Brien"` by default, while `:332` expects raw `"O'Brien"` only
when
`escape_result=False`.
5. Compare to PR doc line
`docs/admin_docs/configuration/sql-templating.mdx:339`, which
claims universally raw output; this is inaccurate contract wording and can
lead users to
add extra manual escaping on already-escaped values.
```
</details>
<details>
<summary><b>Prompt for AI Agent 🤖 </b></summary>
```mdx
This is a comment left during a code review.
**Path:** docs/admin_docs/configuration/sql-templating.mdx
**Line:** 339:339
**Comment:**
*Logic Error: The new warning says `url_param()` always returns raw
unescaped input, but the implementation escapes values by default in form-data
contexts (`escape_result=True`) and only returns raw values in specific paths
(for example direct request args). This incorrect contract can lead readers to
apply extra manual escaping on already-escaped values, causing double-escaping
and incorrect query results. Clarify that input is untrusted and
context-dependent rather than universally raw.
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.
```
</details>
<a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38777&comment_hash=a38fb08ff929ef9bd8d018f6b3c36d430c3ca19e4989c339097bd5db489c5f9a&reaction=like'>👍</a>
| <a
href='https://app.codeant.ai/feedback?pr_url=https%3A%2F%2Fgithub.com%2Fapache%2Fsuperset%2Fpull%2F38777&comment_hash=a38fb08ff929ef9bd8d018f6b3c36d430c3ca19e4989c339097bd5db489c5f9a&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]
